From 0165a842ef3f3c82b03b08a17ef02f1c163d890b Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Mon, 3 Apr 2017 11:48:01 -0700 Subject: [PATCH] Review comment: Make `or` be `or_exactly`. I baked the eof checking directly into the parser, rather than using the skip and eof parsers. I also took the time to restore some tests that were mistakenly commented out. --- parser-utils/src/value_and_span.rs | 41 ++++--- query-parser/src/parse.rs | 34 +++--- tx-parser/src/lib.rs | 179 ++++++++++++++--------------- 3 files changed, 126 insertions(+), 128 deletions(-) diff --git a/parser-utils/src/value_and_span.rs b/parser-utils/src/value_and_span.rs index 4863130e..1fa49e56 100644 --- a/parser-utils/src/value_and_span.rs +++ b/parser-utils/src/value_and_span.rs @@ -22,7 +22,6 @@ use combine::{ Parser, ParseResult, StreamOnce, - eof, many, many1, parser, @@ -35,10 +34,8 @@ use combine::primitives::{ FastResult, }; use combine::combinator::{ - Eof, Expected, FnParser, - Skip, }; use edn; @@ -165,9 +162,9 @@ impl Item for edn::ValueAndSpan { } } -/// `Of` and `of` allow us to express nested parsers naturally. +/// `OfExactly` and `of_exactly` allow us to express nested parsers naturally. /// -/// For example, `vector().of(many(list()))` parses a vector-of-lists, like [(1 2) (:a :b) ("test") ()]. +/// For example, `vector().of_exactly(many(list()))` parses a vector-of-lists, like [(1 2) (:a :b) ("test") ()]. /// /// The "outer" parser `P` and the "nested" parser `N` must be compatible: `P` must produce an /// output `edn::ValueAndSpan` which can itself be turned into a stream of child elements; and `N` @@ -175,9 +172,9 @@ impl Item for edn::ValueAndSpan { /// nested parser to the outer parser, which is part of what has made parsing `&'a [edn::Value]` /// difficult. #[derive(Clone)] -pub struct Of(P, N); +pub struct OfExactly(P, N); -impl Parser for Of +impl Parser for OfExactly where P: Parser, N: Parser, { @@ -190,7 +187,12 @@ impl Parser for Of match self.0.parse_lazy(input) { ConsumedOk((outer_value, outer_input)) => { match self.1.parse_lazy(outer_value.into_child_stream()) { - ConsumedOk((inner_value, _)) | EmptyOk((inner_value, _)) => ConsumedOk((inner_value, outer_input)), + ConsumedOk((inner_value, mut inner_input)) | EmptyOk((inner_value, mut inner_input)) => { + match inner_input.uncons() { + Err(ref err) if *err == primitives::Error::end_of_input() => ConsumedOk((inner_value, outer_input)), + _ => EmptyErr(ParseError::empty(inner_input.position())), + } + }, // TODO: Improve the error output to reference the nested value (or span) in // some way. This seems surprisingly difficult to do, so we just surface the // inner error message right now. See also the comment below. @@ -199,7 +201,12 @@ impl Parser for Of }, EmptyOk((outer_value, outer_input)) => { match self.1.parse_lazy(outer_value.into_child_stream()) { - ConsumedOk((inner_value, _)) | EmptyOk((inner_value, _)) => EmptyOk((inner_value, outer_input)), + ConsumedOk((inner_value, mut inner_input)) | EmptyOk((inner_value, mut inner_input)) => { + match inner_input.uncons() { + Err(ref err) if *err == primitives::Error::end_of_input() => EmptyOk((inner_value, outer_input)), + _ => EmptyErr(ParseError::empty(inner_input.position())), + } + }, // TODO: Improve the error output. See the comment above. EmptyErr(e) | ConsumedErr(e) => EmptyErr(e), } @@ -215,27 +222,27 @@ impl Parser for Of } #[inline(always)] -pub fn of(p: P, n: N) -> Of>> +pub fn of_exactly(p: P, n: N) -> OfExactly where P: Parser, N: Parser, { - Of(p, n.skip(eof())) + OfExactly(p, n) } /// We need a trait to define `Parser.of` and have it live outside of the `combine` crate. -pub trait OfParsing: Parser + Sized { - fn of(self, n: N) -> Of>> +pub trait OfExactlyParsing: Parser + Sized { + fn of_exactly(self, n: N) -> OfExactly where Self: Sized, N: Parser; } -impl

OfParsing for P +impl

OfExactlyParsing for P where P: Parser { - fn of(self, n: N) -> Of>> + fn of_exactly(self, n: N) -> OfExactly where N: Parser { - of(self, n) + of_exactly(self, n) } } @@ -311,7 +318,7 @@ fn keyword_map_(input: Stream) -> ParseResult } })); - let mut runs = vector().of(many::, _>(run)); + let mut runs = vector().of_exactly(many::, _>(run)); let (data, input) = try!(runs.parse_lazy(input).into()); diff --git a/query-parser/src/parse.rs b/query-parser/src/parse.rs index 1ba01b98..aedd7624 100644 --- a/query-parser/src/parse.rs +++ b/query-parser/src/parse.rs @@ -26,7 +26,7 @@ use self::mentat_parser_utils::{ use self::mentat_parser_utils::value_and_span::Stream as ValueStream; use self::mentat_parser_utils::value_and_span::{ Item, - OfParsing, + OfExactlyParsing, keyword_map, list, map, @@ -162,7 +162,7 @@ def_parser!(Where, or_join, edn::ValueAndSpan, { def_parser!(Where, rule_vars, Vec, { seq() - .of(many1(Query::variable())) + .of_exactly(many1(Query::variable())) }); def_parser!(Where, or_pattern_clause, OrWhereClause, { @@ -171,7 +171,7 @@ def_parser!(Where, or_pattern_clause, OrWhereClause, { def_parser!(Where, or_and_clause, OrWhereClause, { seq() - .of(Where::and() + .of_exactly(Where::and() .with(many1(Where::clause())) .map(OrWhereClause::And)) }); @@ -182,7 +182,7 @@ def_parser!(Where, or_where_clause, OrWhereClause, { def_parser!(Where, or_clause, WhereClause, { seq() - .of(Where::or() + .of_exactly(Where::or() .with(many1(Where::or_where_clause())) .map(|clauses| { WhereClause::OrJoin( @@ -195,7 +195,7 @@ def_parser!(Where, or_clause, WhereClause, { def_parser!(Where, or_join_clause, WhereClause, { seq() - .of(Where::or_join() + .of_exactly(Where::or_join() .with(Where::rule_vars()) .and(many1(Where::or_where_clause())) .map(|(vars, clauses)| { @@ -212,8 +212,8 @@ def_parser!(Where, pred, WhereClause, { // Accept either a nested list or a nested vector here: // `[(foo ?x ?y)]` or `[[foo ?x ?y]]` vector() - .of(seq() - .of((Query::predicate_fn(), Query::arguments()) + .of_exactly(seq() + .of_exactly((Query::predicate_fn(), Query::arguments()) .map(|(f, args)| { WhereClause::Pred( Predicate { @@ -225,7 +225,7 @@ def_parser!(Where, pred, WhereClause, { def_parser!(Where, pattern, WhereClause, { vector() - .of( + .of_exactly( // While *technically* Datomic allows you to have a query like: // [:find … :where [[?x]]] // We don't -- we require at least e, a. @@ -313,7 +313,7 @@ def_parser!(Find, find_scalar, FindSpec, { def_parser!(Find, find_coll, FindSpec, { vector() - .of(Query::variable() + .of_exactly(Query::variable() .map(|var| FindSpec::FindColl(Element::Variable(var))) .skip(Find::ellipsis())) }); @@ -328,7 +328,7 @@ def_parser!(Find, find_rel, FindSpec, { def_parser!(Find, find_tuple, FindSpec, { vector() - .of(Find::elements().map(FindSpec::FindTuple)) + .of_exactly(Find::elements().map(FindSpec::FindTuple)) }); /// Parse a stream values into one of four find specs. @@ -395,16 +395,16 @@ enum FindQueryPart { /// construct a `FindQuery` from them. def_parser!(Find, query, FindQuery, { let p_find_spec = Find::literal_find() - .with(vector().of(Find::spec().map(FindQueryPart::FindSpec))); + .with(vector().of_exactly(Find::spec().map(FindQueryPart::FindSpec))); let p_with_vars = Find::literal_with() - .with(vector().of(many(Query::variable()).map(FindQueryPart::With))); + .with(vector().of_exactly(many(Query::variable()).map(FindQueryPart::With))); let p_where_clauses = Find::literal_where() - .with(vector().of(Where::clauses().map(FindQueryPart::WhereClauses))).expected(":where clauses"); + .with(vector().of_exactly(Where::clauses().map(FindQueryPart::WhereClauses))).expected(":where clauses"); (or(map(), keyword_map())) - .of(many(choice::<[&mut Parser; 3], _>([ + .of_exactly(many(choice::<[&mut Parser; 3], _>([ &mut try(p_find_spec), &mut try(p_with_vars), &mut try(p_where_clauses), @@ -619,7 +619,7 @@ mod test { fn test_find_sp_variable() { let sym = edn::PlainSymbol::new("?x"); let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(sym.clone())]); - assert_parses_to!(|| vector().of(Query::variable()), input, variable(sym)); + assert_parses_to!(|| vector().of_exactly(Query::variable()), input, variable(sym)); } #[test] @@ -627,7 +627,7 @@ mod test { let sym = edn::PlainSymbol::new("?x"); let period = edn::PlainSymbol::new("."); let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(sym.clone()), edn::Value::PlainSymbol(period.clone())]); - assert_parses_to!(|| vector().of(Find::find_scalar()), + assert_parses_to!(|| vector().of_exactly(Find::find_scalar()), input, FindSpec::FindScalar(Element::Variable(variable(sym)))); } @@ -648,7 +648,7 @@ mod test { let vx = edn::PlainSymbol::new("?x"); let vy = edn::PlainSymbol::new("?y"); let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(vx.clone()), edn::Value::PlainSymbol(vy.clone())]); - assert_parses_to!(|| vector().of(Find::find_rel()), + assert_parses_to!(|| vector().of_exactly(Find::find_rel()), input, FindSpec::FindRel(vec![Element::Variable(variable(vx)), Element::Variable(variable(vy))])); diff --git a/tx-parser/src/lib.rs b/tx-parser/src/lib.rs index 35f148e7..e3c657c4 100644 --- a/tx-parser/src/lib.rs +++ b/tx-parser/src/lib.rs @@ -45,7 +45,7 @@ use mentat_tx::entities::{ use mentat_parser_utils::{ResultParser}; use mentat_parser_utils::value_and_span::{ Item, - OfParsing, + OfExactlyParsing, integer, list, map, @@ -66,8 +66,8 @@ def_parser!(Tx, entid, Entid, { }); def_parser!(Tx, lookup_ref, LookupRef, { - list() - .of(value(edn::Value::PlainSymbol(PlainSymbol::new("lookup-ref"))) + list().of_exactly( + value(edn::Value::PlainSymbol(PlainSymbol::new("lookup-ref"))) .with((Tx::entid(), Tx::atom())) .map(|(a, v)| LookupRef { a: a, v: v.without_spans() })) @@ -88,7 +88,7 @@ def_parser!(Tx, atom, edn::ValueAndSpan, { }); def_parser!(Tx, nested_vector, Vec, { - vector().of(many(Tx::atom_or_lookup_ref_or_vector())) + vector().of_exactly(many(Tx::atom_or_lookup_ref_or_vector())) }); def_parser!(Tx, atom_or_lookup_ref_or_vector, AtomOrLookupRefOrVectorOrMapNotation, { @@ -116,12 +116,12 @@ def_parser!(Tx, add_or_retract, Entity, { } }); - vector().of(p) + vector().of_exactly(p) }); def_parser!(Tx, map_notation, MapNotation, { map() - .of(many((Tx::entid(), Tx::atom_or_lookup_ref_or_vector()))) + .of_exactly(many((Tx::entid(), Tx::atom_or_lookup_ref_or_vector()))) .map(|avs: Vec<(Entid, AtomOrLookupRefOrVectorOrMapNotation)>| -> MapNotation { avs.into_iter().collect() }) @@ -133,7 +133,7 @@ def_parser!(Tx, entity, Entity, { }); def_parser!(Tx, entities, Vec, { - vector().of(many(Tx::entity())) + vector().of_exactly(many(Tx::entity())) }); impl Tx { @@ -185,7 +185,9 @@ pub fn remove_db_id(map: &mut MapNotation) -> std::result::Result Value { Value::NamespacedKeyword(NamespacedKeyword::new(namespace, name)) @@ -213,9 +213,9 @@ mod tests { let input = Value::Vector(vec![kw("db", "add"), kw("test", "entid"), kw("test", "a"), - Value::Text("v".into())]).with_spans(); + Value::Text("v".into())]); let mut parser = Tx::entity(); - let result = parser.parse(input.into_atom_stream()).map(|x| x.0); + let result = parser.parse(input.with_spans().into_atom_stream()).map(|x| x.0); assert_eq!(result, Ok(Entity::AddOrRetract { op: OpType::Add, @@ -226,91 +226,82 @@ mod tests { })); } - // #[test] - // fn test_retract() { - // let input = [Value::Vector(vec![kw("db", "retract"), - // Value::Integer(101), - // kw("test", "a"), - // Value::Text("v".into())])]; - // let mut parser = Tx::entity(); - // let result = parser.parse(&input[..]); - // assert_eq!(result, - // Ok((Entity::AddOrRetract { - // op: OpType::Retract, - // e: EntidOrLookupRefOrTempId::Entid(Entid::Entid(101)), - // a: Entid::Ident(NamespacedKeyword::new("test", "a")), - // v: AtomOrLookupRefOrVectorOrMapNotation::Atom(Value::Text("v".into())), - // }, - // &[][..]))); - // } + #[test] + fn test_retract() { + let input = Value::Vector(vec![kw("db", "retract"), + Value::Integer(101), + kw("test", "a"), + Value::Text("v".into())]); + let mut parser = Tx::entity(); + let result = parser.parse(input.with_spans().into_atom_stream()).map(|x| x.0); + assert_eq!(result, + Ok(Entity::AddOrRetract { + op: OpType::Retract, + e: EntidOrLookupRefOrTempId::Entid(Entid::Entid(101)), + a: Entid::Ident(NamespacedKeyword::new("test", "a")), + v: AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("v".into()), Span(25, 28))), + })); + } - // #[test] - // fn test_lookup_ref() { - // let mut list = LinkedList::new(); - // list.push_back(Value::PlainSymbol(PlainSymbol::new("lookup-ref"))); - // list.push_back(kw("test", "a1")); - // list.push_back(Value::Text("v1".into())); + #[test] + fn test_lookup_ref() { + let input = Value::Vector(vec![kw("db", "add"), + Value::List(vec![Value::PlainSymbol(PlainSymbol::new("lookup-ref")), + kw("test", "a1"), + Value::Text("v1".into())].into_iter().collect()), + kw("test", "a"), + Value::Text("v".into())]); + let mut parser = Tx::entity(); + let result = parser.parse(input.with_spans().into_atom_stream()).map(|x| x.0); + assert_eq!(result, + Ok(Entity::AddOrRetract { + op: OpType::Add, + e: EntidOrLookupRefOrTempId::LookupRef(LookupRef { + a: Entid::Ident(NamespacedKeyword::new("test", "a1")), + v: Value::Text("v1".into()), + }), + a: Entid::Ident(NamespacedKeyword::new("test", "a")), + v: AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("v".into()), Span(44, 47))), + })); + } - // let input = [Value::Vector(vec![kw("db", "add"), - // Value::List(list), - // kw("test", "a"), - // Value::Text("v".into())])]; - // let mut parser = Tx::entity(); - // let result = parser.parse(&input[..]); - // assert_eq!(result, - // Ok((Entity::AddOrRetract { - // op: OpType::Add, - // e: EntidOrLookupRefOrTempId::LookupRef(LookupRef { - // a: Entid::Ident(NamespacedKeyword::new("test", "a1")), - // v: Value::Text("v1".into()), - // }), - // a: Entid::Ident(NamespacedKeyword::new("test", "a")), - // v: AtomOrLookupRefOrVectorOrMapNotation::Atom(Value::Text("v".into())), - // }, - // &[][..]))); - // } + #[test] + fn test_nested_vector() { + let input = Value::Vector(vec![kw("db", "add"), + Value::List(vec![Value::PlainSymbol(PlainSymbol::new("lookup-ref")), + kw("test", "a1"), + Value::Text("v1".into())].into_iter().collect()), + kw("test", "a"), + Value::Vector(vec![Value::Text("v1".into()), Value::Text("v2".into())])]); + let mut parser = Tx::entity(); + let result = parser.parse(input.with_spans().into_atom_stream()).map(|x| x.0); + assert_eq!(result, + Ok(Entity::AddOrRetract { + op: OpType::Add, + e: EntidOrLookupRefOrTempId::LookupRef(LookupRef { + a: Entid::Ident(NamespacedKeyword::new("test", "a1")), + v: Value::Text("v1".into()), + }), + a: Entid::Ident(NamespacedKeyword::new("test", "a")), + v: AtomOrLookupRefOrVectorOrMapNotation::Vector(vec![AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("v1".into()), Span(45, 49))), + AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("v2".into()), Span(50, 54)))]), + })); + } - // #[test] - // fn test_nested_vector() { - // let mut list = LinkedList::new(); - // list.push_back(Value::PlainSymbol(PlainSymbol::new("lookup-ref"))); - // list.push_back(kw("test", "a1")); - // list.push_back(Value::Text("v1".into())); + #[test] + fn test_map_notation() { + let mut expected: MapNotation = BTreeMap::default(); + expected.insert(Entid::Ident(NamespacedKeyword::new("db", "id")), AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("t".to_string()), Span(8, 11)))); + expected.insert(Entid::Ident(NamespacedKeyword::new("db", "ident")), AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::NamespacedKeyword(NamespacedKeyword::new("test", "attribute")), Span(22, 37)))); - // let input = [Value::Vector(vec![kw("db", "add"), - // Value::List(list), - // kw("test", "a"), - // Value::Vector(vec![Value::Text("v1".into()), Value::Text("v2".into())])])]; - // let mut parser = Tx::entity(); - // let result = parser.parse(&input[..]); - // assert_eq!(result, - // Ok((Entity::AddOrRetract { - // op: OpType::Add, - // e: EntidOrLookupRefOrTempId::LookupRef(LookupRef { - // a: Entid::Ident(NamespacedKeyword::new("test", "a1")), - // v: Value::Text("v1".into()), - // }), - // a: Entid::Ident(NamespacedKeyword::new("test", "a")), - // v: AtomOrLookupRefOrVectorOrMapNotation::Vector(vec![AtomOrLookupRefOrVectorOrMapNotation::Atom(Value::Text("v1".into())), - // AtomOrLookupRefOrVectorOrMapNotation::Atom(Value::Text("v2".into()))]), - // }, - // &[][..]))); - // } + let mut map: BTreeMap = BTreeMap::default(); + map.insert(kw("db", "id"), Value::Text("t".to_string())); + map.insert(kw("db", "ident"), kw("test", "attribute")); + let input = Value::Map(map.clone()); - // #[test] - // fn test_map_notation() { - // let mut expected: MapNotation = BTreeMap::default(); - // expected.insert(Entid::Ident(NamespacedKeyword::new("db", "id")), AtomOrLookupRefOrVectorOrMapNotation::Atom(Value::Text("t".to_string()))); - // expected.insert(Entid::Ident(NamespacedKeyword::new("db", "ident")), AtomOrLookupRefOrVectorOrMapNotation::Atom(kw("test", "attribute"))); - - // let mut map: BTreeMap = BTreeMap::default(); - // map.insert(kw("db", "id"), Value::Text("t".to_string())); - // map.insert(kw("db", "ident"), kw("test", "attribute")); - // let input = [Value::Map(map.clone())]; - // let mut parser = Tx::entity(); - // let result = parser.parse(&input[..]); - // assert_eq!(result, - // Ok((Entity::MapNotation(expected), - // &[][..]))); - // } + let mut parser = Tx::entity(); + let result = parser.parse(input.with_spans().into_atom_stream()).map(|x| x.0); + assert_eq!(result, + Ok(Entity::MapNotation(expected))); + } }