Improve parsing of nested edn::ValueAndSpan streams. r=rnewman (#393)

* Pre: Expose more in edn.

* Pre: Make it easier to work with ValueAndSpan.

with_spans() is a temporary hack, needed only because I don't care to
parse the bootstrap assertions from text right now.

* Part 1a: Add `value_and_span` for parsing nested `edn::ValueAndSpan` instances.

I wasn't able to abstract over `edn::Value` and `edn::ValueAndSpan`;
there are multiple obstacles.  I chose to roll with
`edn::ValueAndSpan` since it exposes the additional span information
that we will want to form good error messages in the future.

* Part 1b: Add keyword_map() parsing an `edn::Value::Vector` into an `edn::Value::map`.

* Part 1c: Add `Log`/`.log(...)` for logging parser progress.

This is a terrible hack, but it sure helps to debug complicated nested
parsers.  I don't even know what a principled approach would look
like; since our parser combinators are so frequently expressed in
code, it's hard to imagine a data-driven interpreter that can help
debug things.

* Part 2: Use `value_and_span` apparatus in tx-parser/.

I break an abstraction boundary by returning a value column
`edn::ValueAndSpan` rather than just an `edn::Value`.  That is, the
transaction processor shouldn't care where the `edn::Value` it is
processing arose -- even we care to track that information we should
bake it into the `Entity` type.  We do this because we need to
dynamically parse the value column to support nested maps, and parsing
requires a full `edn::ValueAndSpan`.  Alternately, we could cheat and
fake the spans when parsing nested maps, but that's potentially
expensive.

* Part 3: Use `value_and_span` apparatus in query-parser/.

* Part 4: Use `value_and_span` apparatus in root crate.

* Review comment: Make Span and SpanPosition Copy.

* Review comment: nits.

* 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.

* Review comment: Extract and use def_matches_* macros.

* Review comment: .map() as late as possible.

db/src/bootstrap.rs

@@ -273,6 +273,6 @@ pub fn bootstrap_entities() -> Vec<Entity> {
 
     // Failure here is a coding error (since the inputs are fixed), not a runtime error.
     // TODO: represent these bootstrap data errors rather than just panicing.
-    let bootstrap_entities: Vec<Entity> = mentat_tx_parser::Tx::parse(&[bootstrap_assertions][..]).unwrap();
+    let bootstrap_entities: Vec<Entity> = mentat_tx_parser::Tx::parse(bootstrap_assertions.with_spans()).unwrap();
     return bootstrap_entities;
 }

db/src/db.rs

@@ -1125,8 +1125,8 @@ mod tests {
 
         fn transact<I>(&mut self, transaction: I) -> Result<TxReport> where I: Borrow<str> {
             // Failure to parse the transaction is a coding error, so we unwrap.
-            let assertions = edn::parse::value(transaction.borrow()).expect(format!("to be able to parse {} into EDN", transaction.borrow()).as_str()).without_spans();
+            let assertions = edn::parse::value(transaction.borrow()).expect(format!("to be able to parse {} into EDN", transaction.borrow()).as_str());
-            let entities: Vec<_> = mentat_tx_parser::Tx::parse(&[assertions.clone()][..]).expect(format!("to be able to parse {} into entities", assertions).as_str());
+            let entities: Vec<_> = mentat_tx_parser::Tx::parse(assertions.clone()).expect(format!("to be able to parse {} into entities", assertions).as_str());
 
             let details = {
                 // The block scopes the borrow of self.sqlite.

db/src/tx.rs

@@ -254,13 +254,13 @@ impl<'conn, 'a> Tx<'conn, 'a> {
 
                     let v = match v {
                         entmod::AtomOrLookupRefOrVectorOrMapNotation::Atom(v) => {
-                            if attribute.value_type == ValueType::Ref && v.is_text() {
+                            if attribute.value_type == ValueType::Ref && v.inner.is_text() {
-                                Either::Right(LookupRefOrTempId::TempId(temp_ids.intern(v.as_text().cloned().map(TempId::External).unwrap())))
+                                Either::Right(LookupRefOrTempId::TempId(temp_ids.intern(v.inner.as_text().cloned().map(TempId::External).unwrap())))
                             } else {
                                 // Here is where we do schema-aware typechecking: we either assert that
                                 // the given value is in the attribute's value set, or (in limited
                                 // cases) coerce the value into the attribute's value set.
-                                let typed_value: TypedValue = self.schema.to_typed_value(&v, &attribute)?;
+                                let typed_value: TypedValue = self.schema.to_typed_value(&v.without_spans(), &attribute)?;
                                 Either::Left(typed_value)
                             }
                         },

edn/src/edn.rustpeg

@@ -32,7 +32,7 @@ pub nil -> ValueAndSpan =
     start:#position "nil" end:#position {
         ValueAndSpan {
             inner: SpannedValue::Nil,
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -40,7 +40,7 @@ pub nan -> ValueAndSpan =
     start:#position "#f" whitespace+ "NaN" end:#position {
         ValueAndSpan {
             inner: SpannedValue::Float(OrderedFloat(NAN)),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -48,7 +48,7 @@ pub infinity -> ValueAndSpan =
     start:#position "#f" whitespace+ s:$(sign) "Infinity" end:#position {
         ValueAndSpan {
             inner: SpannedValue::Float(OrderedFloat(if s == "+" { INFINITY } else { NEG_INFINITY })),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -56,13 +56,13 @@ pub boolean -> ValueAndSpan =
     start:#position "true" end:#position {
         ValueAndSpan {
             inner: SpannedValue::Boolean(true),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     } /
     start:#position "false" end:#position {
         ValueAndSpan {
             inner: SpannedValue::Boolean(false),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -77,7 +77,7 @@ pub bigint -> ValueAndSpan =
     start:#position b:$( sign? digit+ ) "N" end:#position {
         ValueAndSpan {
             inner: SpannedValue::BigInteger(b.parse::<BigInt>().unwrap()),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -85,7 +85,7 @@ pub octalinteger -> ValueAndSpan =
     start:#position "0" i:$( octaldigit+ ) end:#position {
         ValueAndSpan {
             inner: SpannedValue::Integer(i64::from_str_radix(i, 8).unwrap()),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -93,7 +93,7 @@ pub hexinteger -> ValueAndSpan =
     start:#position "0x" i:$( hex+ ) end:#position {
         ValueAndSpan {
             inner: SpannedValue::Integer(i64::from_str_radix(i, 16).unwrap()),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -102,7 +102,7 @@ pub basedinteger -> ValueAndSpan =
     start:#position b:$( validbase ) "r" i:$( alphanumeric+ ) end:#position {
         ValueAndSpan {
             inner: SpannedValue::Integer(i64::from_str_radix(i, b.parse::<u32>().unwrap()).unwrap()),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -110,7 +110,7 @@ pub integer -> ValueAndSpan =
     start:#position i:$( sign? digit+ ) end:#position {
         ValueAndSpan {
             inner: SpannedValue::Integer(i.parse::<i64>().unwrap()),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -124,7 +124,7 @@ pub float -> ValueAndSpan =
     start:#position f:$( frac_exp / exp / frac ) end:#position {
         ValueAndSpan {
             inner: SpannedValue::Float(OrderedFloat(f.parse::<f64>().unwrap())),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -138,7 +138,7 @@ pub text -> ValueAndSpan =
     start:#position "\"" t:$( char* ) "\"" end:#position {
         ValueAndSpan {
             inner: SpannedValue::Text(t.to_string()),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -164,7 +164,7 @@ pub symbol -> ValueAndSpan =
     end:#position {
         ValueAndSpan {
             inner: SpannedValue::from_symbol(ns, n),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -176,7 +176,7 @@ pub keyword -> ValueAndSpan =
     end:#position {
         ValueAndSpan {
             inner: SpannedValue::from_keyword(ns, n),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -184,7 +184,7 @@ pub list -> ValueAndSpan =
     start:#position "(" __ v:(value)* __ ")" end:#position {
         ValueAndSpan {
             inner: SpannedValue::List(LinkedList::from_iter(v)),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -192,7 +192,7 @@ pub vector -> ValueAndSpan =
     start:#position "[" __ v:(value)* __ "]" end:#position {
         ValueAndSpan {
             inner: SpannedValue::Vector(v),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -200,7 +200,7 @@ pub set -> ValueAndSpan =
     start:#position "#{" __ v:(value)* __ "}" end:#position {
         ValueAndSpan {
             inner: SpannedValue::Set(BTreeSet::from_iter(v)),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 
@@ -213,7 +213,7 @@ pub map -> ValueAndSpan =
     start:#position "{" __ v:(pair)* __ "}" end:#position {
         ValueAndSpan {
             inner: SpannedValue::Map(BTreeMap::from_iter(v)),
-            span: Span(start, end)
+            span: Span::new(start, end)
         }
     }
 

edn/src/lib.rs

@@ -26,5 +26,5 @@ pub mod parse {
 pub use num::BigInt;
 pub use ordered_float::OrderedFloat;
 pub use parse::ParseError;
-pub use types::Value;
+pub use types::{Span, SpannedValue, Value, ValueAndSpan};
 pub use symbols::{Keyword, NamespacedKeyword, PlainSymbol, NamespacedSymbol};

edn/src/types.rs

@@ -66,8 +66,14 @@ pub enum SpannedValue {
 }
 
 /// Span represents the current offset (start, end) into the input string.
-#[derive(PartialEq, Eq, Hash, Clone, Debug)]
+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
-pub struct Span(pub usize, pub usize);
+pub struct Span(pub u32, pub u32);
+
+impl Span {
+    pub fn new(start: usize, end: usize) -> Span {
+        Span(start as u32, end as u32)
+    }
+}
 
 /// A wrapper type around `SpannedValue` and `Span`, representing some EDN value
 /// and the parsing offset (start, end) in the original EDN string.
@@ -77,6 +83,40 @@ pub struct ValueAndSpan {
     pub span: Span,
 }
 
+impl ValueAndSpan {
+    pub fn new<I>(spanned_value: SpannedValue, span: I) -> ValueAndSpan where I: Into<Option<Span>> {
+        ValueAndSpan {
+            inner: spanned_value,
+            span: span.into().unwrap_or(Span(0, 0)), // TODO: consider if this has implications.
+        }
+    }
+
+    pub fn into_atom(self) -> Option<ValueAndSpan> {
+        if self.inner.is_atom() {
+            Some(self)
+        } else {
+            None
+        }
+    }
+
+    pub fn into_text(self) -> Option<String> {
+        self.inner.into_text()
+    }
+}
+
+impl Value {
+    /// For debug use only!
+    ///
+    /// But right now, it's used in the bootstrapper.  We'll fix that soon.
+    pub fn with_spans(self) -> ValueAndSpan {
+        let s = self.to_pretty(120).unwrap();
+        use ::parse;
+        let with_spans = parse::value(&s).unwrap();
+        assert_eq!(self, with_spans.clone().without_spans());
+        with_spans
+    }
+}
+
 impl From<SpannedValue> for Value {
     fn from(src: SpannedValue) -> Value {
         match src {

parser-utils/src/lib.rs

@@ -11,8 +11,20 @@
 extern crate combine;
 extern crate edn;
 
-use combine::ParseResult;
+use combine::{
-use combine::combinator::{Expected, FnParser};
+    ParseResult,
+};
+use combine::combinator::{
+    Expected,
+    FnParser,
+};
+
+pub mod log;
+pub mod value_and_span;
+
+pub use log::{
+    LogParsing,
+};
 
 /// A type definition for a function parser that either parses an `O` from an input stream of type
 /// `I`, or fails with an "expected" failure.
@@ -25,10 +37,10 @@ pub type ResultParser<O, I> = Expected<FnParser<I, fn(I) -> ParseResult<O, I>>>;
 /// parser function against input and expecting a certain result.
 #[macro_export]
 macro_rules! assert_parses_to {
-    ( $parser: path, $input: expr, $expected: expr ) => {{
+    ( $parser: expr, $input: expr, $expected: expr ) => {{
         let mut par = $parser();
-        let result = par.parse(&$input[..]);
+        let result = par.parse($input.with_spans().into_atom_stream()).map(|x| x.0); // TODO: check remainder of stream.
-        assert_eq!(result, Ok(($expected, &[][..])));
+        assert_eq!(result, Ok($expected));
     }}
 }
 
@@ -82,6 +94,20 @@ macro_rules! def_parser_fn {
     }
 }
 
+#[macro_export]
+macro_rules! def_parser {
+    ( $parser: ident, $name: ident, $result_type: ty, $body: block ) => {
+        impl $parser {
+            fn $name() -> ResultParser<$result_type, $crate::value_and_span::Stream> {
+                fn inner(input: $crate::value_and_span::Stream) -> ParseResult<$result_type, $crate::value_and_span::Stream> {
+                    $body.parse_lazy(input).into()
+                }
+                parser(inner as fn($crate::value_and_span::Stream) -> ParseResult<$result_type, $crate::value_and_span::Stream>).expected(stringify!($name))
+            }
+        }
+    }
+}
+
 /// `def_value_parser_fn` is a short-cut to `def_parser_fn` with the input type
 /// being `edn::Value`.
 #[macro_export]

parser-utils/src/log.rs

@@ -0,0 +1,87 @@
+// Copyright 2016 Mozilla
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not use
+// this file except in compliance with the License. You may obtain a copy of the
+// License at http://www.apache.org/licenses/LICENSE-2.0
+// Unless required by applicable law or agreed to in writing, software distributed
+// under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
+// CONDITIONS OF ANY KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations under the License.
+
+use std::io::Write;
+
+use combine::{
+    ParseError,
+    Parser,
+    ParseResult,
+    Stream,
+};
+
+/// println!, but to stderr.
+///
+/// Doesn't pollute stdout, which is useful when running tests under Emacs, which parses the output
+/// of the test suite to format errors and can get confused when user output is interleaved into the
+/// stdout stream.
+///
+/// Cribbed from http://stackoverflow.com/a/27590832.
+macro_rules! println_stderr(
+    ($($arg:tt)*) => { {
+        let r = writeln!(&mut ::std::io::stderr(), $($arg)*);
+        r.expect("failed printing to stderr");
+    } }
+);
+
+#[derive(Clone)]
+pub struct Log<P, T>(P, T)
+    where P: Parser,
+          T: ::std::fmt::Debug;
+
+impl<I, P, T> Parser for Log<P, T>
+    where I: Stream,
+          I::Item: ::std::fmt::Debug,
+          P: Parser<Input = I>,
+          P::Output: ::std::fmt::Debug,
+          T: ::std::fmt::Debug,
+{
+    type Input = I;
+    type Output = P::Output;
+
+    fn parse_stream(&mut self, input: I) -> ParseResult<Self::Output, I> {
+        let head = input.clone().uncons();
+        let result = self.0.parse_stream(input.clone());
+        match result {
+            Ok((ref value, _)) => println_stderr!("{:?}: [{:?} ...] => Ok({:?})", self.1, head.ok(), value),
+            Err(_) => println_stderr!("{:?}: [{:?} ...] => Err(_)", self.1, head.ok()),
+        }
+        result
+    }
+
+    fn add_error(&mut self, errors: &mut ParseError<Self::Input>) {
+        self.0.add_error(errors);
+    }
+}
+
+#[inline(always)]
+pub fn log<P, T>(p: P, msg: T) -> Log<P, T>
+    where P: Parser,
+          T: ::std::fmt::Debug,
+{
+    Log(p, msg)
+}
+
+/// We need a trait to define `Parser.log` and have it live outside of the `combine` crate.
+pub trait LogParsing: Parser + Sized {
+    fn log<T>(self, msg: T) -> Log<Self, T>
+        where Self: Sized,
+              T: ::std::fmt::Debug;
+}
+
+impl<P> LogParsing for P
+    where P: Parser,
+{
+    fn log<T>(self, msg: T) -> Log<Self, T>
+        where T: ::std::fmt::Debug,
+    {
+        log(self, msg)
+    }
+}

parser-utils/src/value_and_span.rs

@@ -0,0 +1,461 @@
+// Copyright 2016 Mozilla
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not use
+// this file except in compliance with the License. You may obtain a copy of the
+// License at http://www.apache.org/licenses/LICENSE-2.0
+// Unless required by applicable law or agreed to in writing, software distributed
+// under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
+// CONDITIONS OF ANY KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations under the License.
+
+use std;
+use std::fmt::{
+    Debug,
+    Display,
+    Formatter,
+};
+use std::cmp::Ordering;
+
+use combine::{
+    ConsumedResult,
+    ParseError,
+    Parser,
+    ParseResult,
+    StreamOnce,
+    many,
+    many1,
+    parser,
+    satisfy,
+    satisfy_map,
+};
+use combine::primitives; // To not shadow Error.
+use combine::primitives::{
+    Consumed,
+    FastResult,
+};
+use combine::combinator::{
+    Expected,
+    FnParser,
+};
+
+use edn;
+
+/// A wrapper to let us order `edn::Span` in whatever way is appropriate for parsing with `combine`.
+#[derive(Clone, Copy, Debug)]
+pub struct SpanPosition(edn::Span);
+
+impl Display for SpanPosition {
+    fn fmt(&self, f: &mut Formatter) -> ::std::fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
+impl PartialEq for SpanPosition {
+    fn eq(&self, other: &Self) -> bool {
+        self.cmp(other) == Ordering::Equal
+    }
+}
+
+impl Eq for SpanPosition { }
+
+impl PartialOrd for SpanPosition {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl Ord for SpanPosition {
+    fn cmp(&self, other: &Self) -> Ordering {
+        (self.0).0.cmp(&(other.0).0)
+    }
+}
+
+/// An iterator specifically for iterating `edn::ValueAndSpan` instances in various ways.
+///
+/// Enumerating each iteration type allows us to have a single `combine::Stream` implementation
+/// yielding `ValueAndSpan` items, which allows us to yield uniform `combine::ParseError` types from
+/// disparate parsers.
+#[derive(Clone)]
+pub enum IntoIter {
+    Empty(std::iter::Empty<edn::ValueAndSpan>),
+    Atom(std::iter::Once<edn::ValueAndSpan>),
+    Vector(std::vec::IntoIter<edn::ValueAndSpan>),
+    List(std::collections::linked_list::IntoIter<edn::ValueAndSpan>),
+    /// Iterates via a single `flat_map` [k1, v1, k2, v2, ...].
+    Map(std::vec::IntoIter<edn::ValueAndSpan>),
+    // TODO: Support Set and Map more naturally.  This is significantly more work because the
+    // existing BTreeSet and BTreeMap iterators do not implement Clone, and implementing Clone for
+    // them is involved.  Since we don't really need to parse sets and maps at this time, this will
+    // do for now.
+}
+
+impl Iterator for IntoIter {
+    type Item = edn::ValueAndSpan;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        match *self {
+            IntoIter::Empty(ref mut i) => i.next(),
+            IntoIter::Atom(ref mut i) => i.next(),
+            IntoIter::Vector(ref mut i) => i.next(),
+            IntoIter::List(ref mut i) => i.next(),
+            IntoIter::Map(ref mut i) => i.next(),
+        }
+    }
+}
+
+/// A single `combine::Stream` implementation iterating `edn::ValueAndSpan` instances.  Equivalent
+/// to `combine::IteratorStream` as produced by `combine::from_iter`, but specialized to
+/// `edn::ValueAndSpan`.
+#[derive(Clone)]
+pub struct Stream(IntoIter, SpanPosition);
+
+/// Things specific to parsing with `combine` and our `Stream` that need a trait to live outside of
+/// the `edn` crate.
+pub trait Item: Clone + PartialEq + Sized {
+    /// Position could be specialized to `SpanPosition`.
+    type Position: Clone + Ord + std::fmt::Display;
+
+    /// A slight generalization of `combine::Positioner` that allows to set the position based on
+    /// the `edn::ValueAndSpan` being iterated.
+    fn start(&self) -> Self::Position;
+    fn update_position(&self, &mut Self::Position);
+
+    fn into_child_stream_iter(self) -> IntoIter;
+    fn into_child_stream(self) -> Stream;
+    fn into_atom_stream_iter(self) -> IntoIter;
+    fn into_atom_stream(self) -> Stream;
+}
+
+impl Item for edn::ValueAndSpan {
+    type Position = SpanPosition;
+
+    fn start(&self) -> Self::Position {
+        SpanPosition(self.span.clone())
+    }
+
+    fn update_position(&self, position: &mut Self::Position) {
+        *position = SpanPosition(self.span.clone())
+    }
+
+    fn into_child_stream_iter(self) -> IntoIter {
+        match self.inner {
+            edn::SpannedValue::Vector(values) => IntoIter::Vector(values.into_iter()),
+            edn::SpannedValue::List(values) => IntoIter::List(values.into_iter()),
+            // Parsing pairs with `combine` is tricky; parsing sequences is easy.
+            edn::SpannedValue::Map(map) => IntoIter::Map(map.into_iter().flat_map(|(a, v)| std::iter::once(a).chain(std::iter::once(v))).collect::<Vec<_>>().into_iter()),
+            _ => IntoIter::Empty(std::iter::empty()),
+        }
+    }
+
+    fn into_child_stream(self) -> Stream {
+        let span = self.span.clone();
+        Stream(self.into_child_stream_iter(), SpanPosition(span))
+    }
+
+    fn into_atom_stream_iter(self) -> IntoIter {
+        IntoIter::Atom(std::iter::once(self))
+    }
+
+    fn into_atom_stream(self) -> Stream {
+        let span = self.span.clone();
+        Stream(self.into_atom_stream_iter(), SpanPosition(span))
+    }
+}
+
+/// `OfExactly` and `of_exactly` allow us to express nested parsers naturally.
+///
+/// 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`
+/// must accept the resulting input `Stream`.  This compatibility allows us to lift errors from the
+/// nested parser to the outer parser, which is part of what has made parsing `&'a [edn::Value]`
+/// difficult.
+#[derive(Clone)]
+pub struct OfExactly<P, N>(P, N);
+
+impl<P, N, O> Parser for OfExactly<P, N>
+    where P: Parser<Input=Stream, Output=edn::ValueAndSpan>,
+          N: Parser<Input=Stream, Output=O>,
+{
+    type Input = P::Input;
+    type Output = O;
+    #[inline]
+    fn parse_lazy(&mut self, input: Self::Input) -> ConsumedResult<Self::Output, Self::Input> {
+        use self::FastResult::*;
+
+        match self.0.parse_lazy(input) {
+            ConsumedOk((outer_value, outer_input)) => {
+                match self.1.parse_lazy(outer_value.into_child_stream()) {
+                    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.
+                    EmptyErr(e) | ConsumedErr(e) => ConsumedErr(e),
+                }
+            },
+            EmptyOk((outer_value, outer_input)) => {
+                match self.1.parse_lazy(outer_value.into_child_stream()) {
+                    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),
+                }
+            },
+            ConsumedErr(e) => ConsumedErr(e),
+            EmptyErr(e) => EmptyErr(e),
+        }
+    }
+
+    fn add_error(&mut self, errors: &mut ParseError<Self::Input>) {
+        self.0.add_error(errors);
+    }
+}
+
+#[inline(always)]
+pub fn of_exactly<P, N, O>(p: P, n: N) -> OfExactly<P, N>
+    where P: Parser<Input=Stream, Output=edn::ValueAndSpan>,
+          N: Parser<Input=Stream, Output=O>,
+{
+    OfExactly(p, n)
+}
+
+/// We need a trait to define `Parser.of` and have it live outside of the `combine` crate.
+pub trait OfExactlyParsing: Parser + Sized {
+    fn of_exactly<N, O>(self, n: N) -> OfExactly<Self, N>
+        where Self: Sized,
+              N: Parser<Input = Self::Input, Output=O>;
+}
+
+impl<P> OfExactlyParsing for P
+    where P: Parser<Input=Stream, Output=edn::ValueAndSpan>
+{
+    fn of_exactly<N, O>(self, n: N) -> OfExactly<P, N>
+        where N: Parser<Input = Self::Input, Output=O>
+    {
+        of_exactly(self, n)
+    }
+}
+
+/// Equivalent to `combine::IteratorStream`.
+impl StreamOnce for Stream
+{
+    type Item = edn::ValueAndSpan;
+    type Range = edn::ValueAndSpan;
+    type Position = SpanPosition;
+
+    #[inline]
+    fn uncons(&mut self) -> std::result::Result<Self::Item, primitives::Error<Self::Item, Self::Item>> {
+        match self.0.next() {
+            Some(x) => {
+                x.update_position(&mut self.1);
+                Ok(x)
+            },
+            None => Err(primitives::Error::end_of_input()),
+        }
+    }
+
+    #[inline(always)]
+    fn position(&self) -> Self::Position {
+        self.1.clone()
+    }
+}
+
+/// Shorthands, just enough to convert the `mentat_db` crate for now.  Written using `Box` for now:
+/// it's simple and we can address allocation issues if and when they surface.
+pub fn vector() -> Box<Parser<Input=Stream, Output=edn::ValueAndSpan>> {
+    satisfy(|v: edn::ValueAndSpan| v.inner.is_vector()).boxed()
+}
+
+pub fn list() -> Box<Parser<Input=Stream, Output=edn::ValueAndSpan>> {
+    satisfy(|v: edn::ValueAndSpan| v.inner.is_list()).boxed()
+}
+
+pub fn map() -> Box<Parser<Input=Stream, Output=edn::ValueAndSpan>> {
+    satisfy(|v: edn::ValueAndSpan| v.inner.is_map()).boxed()
+}
+
+pub fn seq() -> Box<Parser<Input=Stream, Output=edn::ValueAndSpan>> {
+    satisfy(|v: edn::ValueAndSpan| v.inner.is_list() || v.inner.is_vector()).boxed()
+}
+
+pub fn integer() -> Box<Parser<Input=Stream, Output=i64>> {
+    satisfy_map(|v: edn::ValueAndSpan| v.inner.as_integer()).boxed()
+}
+
+pub fn namespaced_keyword() -> Box<Parser<Input=Stream, Output=edn::NamespacedKeyword>> {
+    satisfy_map(|v: edn::ValueAndSpan| v.inner.as_namespaced_keyword().cloned()).boxed()
+}
+
+/// Like `combine::token()`, but compare an `edn::Value` to an `edn::ValueAndSpan`.
+pub fn value(value: edn::Value) -> Box<Parser<Input=Stream, Output=edn::ValueAndSpan>> {
+    // TODO: make this comparison faster.  Right now, we drop all the spans; if we walked the value
+    // trees together, we could avoid creating garbage.
+    satisfy(move |v: edn::ValueAndSpan| value == v.inner.into()).boxed()
+}
+
+fn keyword_map_(input: Stream) -> ParseResult<edn::ValueAndSpan, Stream>
+{
+    // One run is a keyword followed by one or more non-keywords.
+    let run = (satisfy(|v: edn::ValueAndSpan| v.inner.is_keyword()),
+               many1(satisfy(|v: edn::ValueAndSpan| !v.inner.is_keyword()))
+               .map(|vs: Vec<edn::ValueAndSpan>| {
+                   // TODO: extract "spanning".
+                   let beg = vs.first().unwrap().span.0;
+                   let end = vs.last().unwrap().span.1;
+                   edn::ValueAndSpan {
+                       inner: edn::SpannedValue::Vector(vs),
+                       span: edn::Span(beg, end),
+                   }
+               }));
+
+    let mut runs = vector().of_exactly(many::<Vec<_>, _>(run));
+
+    let (data, input) = try!(runs.parse_lazy(input).into());
+
+    let mut m: std::collections::BTreeMap<edn::ValueAndSpan, edn::ValueAndSpan> = std::collections::BTreeMap::default();
+    for (k, vs) in data {
+        if m.insert(k, vs).is_some() {
+            // TODO: improve this message.
+            return Err(Consumed::Empty(ParseError::from_errors(input.into_inner().position(), Vec::new())))
+        }
+    }
+
+    let map = edn::ValueAndSpan {
+        inner: edn::SpannedValue::Map(m),
+        span: edn::Span(0, 0), // TODO: fix this.
+    };
+
+    Ok((map, input))
+}
+
+/// Turn a vector of keywords and non-keyword values into a map.  As an example, turn
+/// ```edn
+/// [:keyword1 value1 value2 ... :keyword2 value3 value4 ...]
+/// ```
+/// into
+/// ```edn
+/// {:keyword1 [value1 value2 ...] :keyword2 [value3 value4 ...]}
+/// ```.
+pub fn keyword_map() -> Expected<FnParser<Stream, fn(Stream) -> ParseResult<edn::ValueAndSpan, Stream>>>
+{
+    // The `as` work arounds https://github.com/rust-lang/rust/issues/20178.
+    parser(keyword_map_ as fn(Stream) -> ParseResult<edn::ValueAndSpan, Stream>).expected("keyword map")
+}
+
+/// Generate a `satisfy` expression that matches a `PlainSymbol` value with the given name.
+///
+/// We do this rather than using `combine::token` so that we don't need to allocate a new `String`
+/// inside a `PlainSymbol` inside a `SpannedValue` inside a `ValueAndSpan` just to match input.
+#[macro_export]
+macro_rules! def_matches_plain_symbol {
+    ( $parser: ident, $name: ident, $input: expr ) => {
+        def_parser!($parser, $name, edn::ValueAndSpan, {
+            satisfy(|v: edn::ValueAndSpan| {
+                match v.inner {
+                    edn::SpannedValue::PlainSymbol(ref s) => s.0.as_str() == $input,
+                    _ => false,
+                }
+            })
+        });
+    }
+}
+
+/// Generate a `satisfy` expression that matches a `Keyword` value with the given name.
+///
+/// We do this rather than using `combine::token` to save allocations.
+#[macro_export]
+macro_rules! def_matches_keyword {
+    ( $parser: ident, $name: ident, $input: expr ) => {
+        def_parser!($parser, $name, edn::ValueAndSpan, {
+            satisfy(|v: edn::ValueAndSpan| {
+                match v.inner {
+                    edn::SpannedValue::Keyword(ref s) => s.0.as_str() == $input,
+                    _ => false,
+                }
+            })
+        });
+    }
+}
+
+/// Generate a `satisfy` expression that matches a `NamespacedKeyword` value with the given
+/// namespace and name.
+///
+/// We do this rather than using `combine::token` to save allocations.
+#[macro_export]
+macro_rules! def_matches_namespaced_keyword {
+    ( $parser: ident, $name: ident, $input_namespace: expr, $input_name: expr ) => {
+        def_parser!($parser, $name, edn::ValueAndSpan, {
+            satisfy(|v: edn::ValueAndSpan| {
+                match v.inner {
+                    edn::SpannedValue::NamespacedKeyword(ref s) => s.namespace.as_str() == $input_namespace && s.name.as_str() == $input_name,
+                    _ => false,
+                }
+            })
+        });
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use combine::{eof};
+    use super::*;
+
+    /// Take a string `input` and a string `expected` and ensure that `input` parses to an
+    /// `edn::Value` keyword map equivalent to the `edn::Value` that `expected` parses to.
+    macro_rules! assert_keyword_map_eq {
+        ( $input: expr, $expected: expr ) => {{
+            let input = edn::parse::value($input).expect("to be able to parse input EDN");
+            let expected = $expected.map(|e| {
+                edn::parse::value(e).expect("to be able to parse expected EDN").without_spans()
+            });
+            let mut par = keyword_map().map(|x| x.without_spans()).skip(eof());
+            let result = par.parse(input.into_atom_stream()).map(|x| x.0);
+            assert_eq!(result.ok(), expected);
+        }}
+    }
+
+    #[test]
+    fn test_keyword_map() {
+        assert_keyword_map_eq!(
+            "[:foo 1 2 3 :bar 4]",
+            Some("{:foo [1 2 3] :bar [4]}"));
+
+        // Trailing keywords aren't allowed.
+        assert_keyword_map_eq!(
+            "[:foo]",
+            None);
+        assert_keyword_map_eq!(
+            "[:foo 2 :bar]",
+            None);
+
+        // Duplicate keywords aren't allowed.
+        assert_keyword_map_eq!(
+            "[:foo 2 :foo 1]",
+            None);
+
+        // Starting with anything but a keyword isn't allowed.
+        assert_keyword_map_eq!(
+            "[2 :foo 1]",
+            None);
+
+        // Consecutive keywords aren't allowed.
+        assert_keyword_map_eq!(
+            "[:foo :bar 1]",
+            None);
+
+        // Empty lists return an empty map.
+        assert_keyword_map_eq!(
+            "[]",
+            Some("{}"));
+    }
+}

query-parser/src/find.rs

@@ -1,261 +0,0 @@
-// Copyright 2016 Mozilla
-//
-// Licensed under the Apache License, Version 2.0 (the "License"); you may not use
-// this file except in compliance with the License. You may obtain a copy of the
-// License at http://www.apache.org/licenses/LICENSE-2.0
-// Unless required by applicable law or agreed to in writing, software distributed
-// under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
-// CONDITIONS OF ANY KIND, either express or implied. See the License for the
-// specific language governing permissions and limitations under the License.
-
-/// ! This module defines the interface and implementation for parsing an EDN
-/// ! input into a structured Datalog query.
-/// !
-/// ! The query types are defined in the `query` crate, because they
-/// ! are shared between the parser (EDN -> query), the translator
-/// ! (query -> SQL), and the executor (query, SQL -> running code).
-/// !
-/// ! The query input can be in two forms: a 'flat' human-oriented
-/// ! sequence:
-/// !
-/// ! ```clojure
-/// ! [:find ?y :in $ ?x :where [?x :foaf/knows ?y]]
-/// ! ```
-/// !
-/// ! or a more programmatically generable map:
-/// !
-/// ! ```clojure
-/// ! {:find [?y]
-/// !  :in [$]
-/// !  :where [[?x :foaf/knows ?y]]}
-/// ! ```
-/// !
-/// ! We parse by expanding the array format into four parts, treating them as the four
-/// ! parts of the map.
-
-extern crate edn;
-extern crate mentat_parser_utils;
-extern crate mentat_query;
-
-use std::collections::BTreeMap;
-
-use self::mentat_query::{
-    FindQuery,
-    FnArg,
-    FromValue,
-    Predicate,
-    PredicateFn,
-    SrcVar,
-    Variable,
-};
-
-use self::mentat_parser_utils::ValueParseError;
-
-use super::parse::{
-    ErrorKind,
-    QueryParseResult,
-    Result,
-    clause_seq_to_patterns,
-};
-
-use super::util::vec_to_keyword_map;
-
-/// If the provided slice of EDN values are all variables as
-/// defined by `value_to_variable`, return a `Vec` of `Variable`s.
-/// Otherwise, return the unrecognized Value in a `NotAVariableError`.
-fn values_to_variables(vals: &[edn::Value]) -> Result<Vec<Variable>> {
-    let mut out: Vec<Variable> = Vec::with_capacity(vals.len());
-    for v in vals {
-        if let Some(var) = Variable::from_value(v) {
-            out.push(var);
-            continue;
-        }
-        bail!(ErrorKind::NotAVariableError(v.clone()));
-    }
-    return Ok(out);
-}
-
-#[allow(unused_variables)]
-fn parse_find_parts(find: &[edn::Value],
-                    ins: Option<&[edn::Value]>,
-                    with: Option<&[edn::Value]>,
-                    wheres: &[edn::Value])
-                    -> QueryParseResult {
-    // :find must be an array of plain var symbols (?foo), pull expressions, and aggregates.
-    // For now we only support variables and the annotations necessary to declare which
-    // flavor of :find we want:
-    //     ?x ?y ?z       = FindRel
-    //     [?x ...]       = FindColl
-    //     ?x .           = FindScalar
-    //     [?x ?y ?z]     = FindTuple
-    //
-    // :in must be an array of sources ($), rules (%), and vars (?). For now we only support the
-    // default source. :in can be omitted, in which case the default is equivalent to `:in $`.
-    // TODO: process `ins`.
-    let source = SrcVar::DefaultSrc;
-
-    // :with is an array of variables. This is simple, so we don't use a parser.
-    let with_vars = if let Some(vals) = with {
-        values_to_variables(vals)?
-    } else {
-        vec![]
-    };
-
-    // :wheres is a whole datastructure.
-    let where_clauses = clause_seq_to_patterns(wheres)?;
-
-    super::parse::find_seq_to_find_spec(find)
-        .map(|spec| {
-            FindQuery {
-                find_spec: spec,
-                default_source: source,
-                with: with_vars,
-                in_vars: vec![],       // TODO
-                in_sources: vec![],    // TODO
-                where_clauses: where_clauses,
-            }
-        })
-}
-
-fn parse_find_map(map: BTreeMap<edn::Keyword, Vec<edn::Value>>) -> QueryParseResult {
-    // Eagerly awaiting `const fn`.
-    let kw_find = edn::Keyword::new("find");
-    let kw_in = edn::Keyword::new("in");
-    let kw_with = edn::Keyword::new("with");
-    let kw_where = edn::Keyword::new("where");
-
-    // Oh, if only we had `guard`.
-    if let Some(find) = map.get(&kw_find) {
-        if let Some(wheres) = map.get(&kw_where) {
-            parse_find_parts(find,
-                             map.get(&kw_in).map(|x| x.as_slice()),
-                             map.get(&kw_with).map(|x| x.as_slice()),
-                             wheres)
-        } else {
-            bail!(ErrorKind::MissingFieldError(kw_where))
-        }
-    } else {
-        bail!(ErrorKind::MissingFieldError(kw_find))
-    }
-}
-
-fn parse_find_edn_map(map: BTreeMap<edn::Value, edn::Value>) -> QueryParseResult {
-    // Every key must be a Keyword. Every value must be a Vec.
-    let mut m = BTreeMap::new();
-
-    if map.is_empty() {
-        return parse_find_map(m);
-    }
-
-    for (k, v) in map {
-        if let edn::Value::Keyword(kw) = k {
-            if let edn::Value::Vector(vec) = v {
-                m.insert(kw, vec);
-                continue;
-            } else {
-                bail!(ErrorKind::InvalidInputError(v))
-            }
-        } else {
-            bail!(ErrorKind::InvalidInputError(k))
-        }
-    }
-
-    parse_find_map(m)
-}
-
-pub fn parse_find_string(string: &str) -> QueryParseResult {
-    let expr = edn::parse::value(string)?;
-    parse_find(expr.without_spans())
-}
-
-pub fn parse_find(expr: edn::Value) -> QueryParseResult {
-    // No `match` because scoping and use of `expr` in error handling is nuts.
-    if let edn::Value::Map(m) = expr {
-        return parse_find_edn_map(m);
-    }
-    if let edn::Value::Vector(ref v) = expr {
-        if let Some(m) = vec_to_keyword_map(v) {
-            return parse_find_map(m);
-        }
-    }
-    bail!(ErrorKind::InvalidInputError(expr))
-}
-
-#[cfg(test)]
-mod test_parse {
-    extern crate edn;
-
-    use std::rc::Rc;
-
-    use self::edn::{NamespacedKeyword, PlainSymbol};
-    use self::edn::types::Value;
-    use super::mentat_query::{
-        Element,
-        FindSpec,
-        Pattern,
-        PatternNonValuePlace,
-        PatternValuePlace,
-        SrcVar,
-        Variable,
-        WhereClause,
-    };
-    use super::*;
-
-    // TODO: when #224 lands, fix to_keyword to be variadic.
-    #[test]
-    fn test_parse_find() {
-        let truncated_input = edn::Value::Vector(vec![Value::from_keyword(None, "find")]);
-        assert!(parse_find(truncated_input).is_err());
-
-        let input =
-            edn::Value::Vector(vec![Value::from_keyword(None, "find"),
-                                    Value::from_symbol(None, "?x"),
-                                    Value::from_symbol(None, "?y"),
-                                    Value::from_keyword(None, "where"),
-                                    edn::Value::Vector(vec![Value::from_symbol(None, "?x"),
-                                                            Value::from_keyword("foo", "bar"),
-                                                            Value::from_symbol(None, "?y")])]);
-
-        let parsed = parse_find(input).unwrap();
-        if let FindSpec::FindRel(elems) = parsed.find_spec {
-            assert_eq!(2, elems.len());
-            assert_eq!(vec![Element::Variable(Variable::from_valid_name("?x")),
-                            Element::Variable(Variable::from_valid_name("?y"))],
-                       elems);
-        } else {
-            panic!("Expected FindRel.");
-        }
-
-        assert_eq!(SrcVar::DefaultSrc, parsed.default_source);
-        assert_eq!(parsed.where_clauses,
-                   vec![
-                   WhereClause::Pattern(Pattern {
-                       source: None,
-                       entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?x")),
-                       attribute: PatternNonValuePlace::Ident(Rc::new(NamespacedKeyword::new("foo", "bar"))),
-                       value: PatternValuePlace::Variable(Variable::from_valid_name("?y")),
-                       tx: PatternNonValuePlace::Placeholder,
-                   })]);
-    }
-
-    #[test]
-    fn test_parse_predicate() {
-        let input = "[:find ?x :where [?x :foo/bar ?y] [[< ?y 10]]]";
-        let parsed = parse_find_string(input).unwrap();
-        assert_eq!(parsed.where_clauses,
-                   vec![
-                      WhereClause::Pattern(Pattern {
-                          source: None,
-                          entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?x")),
-                          attribute: PatternNonValuePlace::Ident(Rc::new(NamespacedKeyword::new("foo", "bar"))),
-                          value: PatternValuePlace::Variable(Variable::from_valid_name("?y")),
-                          tx: PatternNonValuePlace::Placeholder,
-                      }),
-                      WhereClause::Pred(Predicate {
-                          operator: PlainSymbol::new("<"),
-                          args: vec![FnArg::Variable(Variable::from_valid_name("?y")),
-                                     FnArg::EntidOrInteger(10)],
-                      }),
-                  ]);
-    }
-}

query-parser/src/lib.rs

@@ -21,19 +21,12 @@ extern crate edn;
 #[macro_use]
 extern crate mentat_parser_utils;
 
-mod util;
 mod parse;
-pub mod find;
-
-pub use find::{
-    parse_find,
-    parse_find_string,
-};
 
 pub use parse::{
     Error,
     ErrorKind,
-    QueryParseResult,
     Result,
     ResultExt,
+    parse_find_string,
 };

query-parser/src/parse.rs

@@ -13,14 +13,27 @@ extern crate edn;
 extern crate mentat_parser_utils;
 extern crate mentat_query;
 
-use self::combine::{eof, many, many1, optional, parser, satisfy_map, Parser, ParseResult, Stream};
+use std; // To refer to std::result::Result.
-use self::combine::combinator::{choice, try};
+
+use self::combine::{eof, many, many1, optional, parser, satisfy, satisfy_map, Parser, ParseResult, Stream};
+use self::combine::combinator::{choice, or, try};
 
 use self::mentat_parser_utils::{
     ResultParser,
     ValueParseError,
 };
 
+use self::mentat_parser_utils::value_and_span::Stream as ValueStream;
+use self::mentat_parser_utils::value_and_span::{
+    Item,
+    OfExactlyParsing,
+    keyword_map,
+    list,
+    map,
+    seq,
+    vector,
+};
+
 use self::mentat_query::{
     Element,
     FindQuery,
@@ -50,17 +63,17 @@ error_chain! {
     }
 
     errors {
-        NotAVariableError(value: edn::Value) {
+        NotAVariableError(value: edn::ValueAndSpan) {
             description("not a variable")
             display("not a variable: '{}'", value)
         }
 
-        FindParseError(e: ValueParseError) {
+        FindParseError(e: combine::ParseError<ValueStream>) {
             description(":find parse error")
             display(":find parse error")
         }
 
-        WhereParseError(e: ValueParseError) {
+        WhereParseError(e: combine::ParseError<ValueStream>) {
             description(":where parse error")
             display(":where parse error")
         }
@@ -83,179 +96,127 @@ error_chain! {
     }
 }
 
-pub type WhereParseResult = Result<Vec<WhereClause>>;
+pub struct Query;
-pub type FindParseResult = Result<FindSpec>;
-pub type QueryParseResult = Result<FindQuery>;
 
-pub struct Query<I>(::std::marker::PhantomData<fn(I) -> I>);
+def_parser!(Query, variable, Variable, {
+    satisfy_map(Variable::from_value)
+});
 
-impl<I> Query<I>
+def_parser!(Query, source_var, SrcVar, {
-    where I: Stream<Item = edn::Value>
+    satisfy_map(SrcVar::from_value)
-{
+});
-    fn to_parsed_value<T>(r: ParseResult<T, I>) -> Option<T> {
-        r.ok().map(|x| x.0)
-    }
-}
 
 // TODO: interning.
-def_value_satisfy_parser_fn!(Query, variable, Variable, Variable::from_value);
+def_parser!(Query, predicate_fn, PredicateFn, {
-def_value_satisfy_parser_fn!(Query, source_var, SrcVar, SrcVar::from_value);
+    satisfy_map(PredicateFn::from_value)
-def_value_satisfy_parser_fn!(Query, predicate_fn, PredicateFn, PredicateFn::from_value);
-def_value_satisfy_parser_fn!(Query, fn_arg, FnArg, FnArg::from_value);
-
-pub struct Where<I>(::std::marker::PhantomData<fn(I) -> I>);
-
-def_value_satisfy_parser_fn!(Where,
-                             pattern_value_place,
-                             PatternValuePlace,
-                             PatternValuePlace::from_value);
-def_value_satisfy_parser_fn!(Where,
-                             pattern_non_value_place,
-                             PatternNonValuePlace,
-                             PatternNonValuePlace::from_value);
-
-fn seq<T: Into<Option<edn::Value>>>(x: T) -> Option<Vec<edn::Value>> {
-    match x.into() {
-        Some(edn::Value::List(items)) => Some(items.into_iter().collect()),
-        Some(edn::Value::Vector(items)) => Some(items),
-        _ => None,
-    }
-}
-
-/// Take a vector Value containing one vector Value, and return the `Vec` inside the inner vector.
-/// Also accepts an inner list, returning it as a `Vec`.
-fn unwrap_nested(x: edn::Value) -> Option<Vec<edn::Value>> {
-    match x {
-        edn::Value::Vector(mut v) => {
-            seq(v.pop())
-        }
-        _ => None,
-    }
-}
-
-def_value_parser_fn!(Where, and, (), input, {
-    matches_plain_symbol!("and", input)
 });
 
-def_value_parser_fn!(Where, or, (), input, {
+def_parser!(Query, fn_arg, FnArg, {
-    matches_plain_symbol!("or", input)
+    satisfy_map(FnArg::from_value)
 });
 
-def_value_parser_fn!(Where, or_join, (), input, {
+def_parser!(Query, arguments, Vec<FnArg>, {
-    matches_plain_symbol!("or-join", input)
+    (many::<Vec<FnArg>, _>(Query::fn_arg()))
 });
 
-def_value_parser_fn!(Where, rule_vars, Vec<Variable>, input, {
+pub struct Where;
-    satisfy_map(|x: edn::Value| {
+
-        seq(x).and_then(|items| {
+def_parser!(Where, pattern_value_place, PatternValuePlace,  {
-            let mut p = many1(Query::variable()).skip(eof());
+    satisfy_map(PatternValuePlace::from_value)
-            Query::to_parsed_value(p.parse_lazy(&items[..]).into())
-        })}).parse_stream(input)
 });
 
-def_value_parser_fn!(Where, or_pattern_clause, OrWhereClause, input, {
+def_parser!(Where, pattern_non_value_place, PatternNonValuePlace, {
-    Where::clause().map(|clause| OrWhereClause::Clause(clause)).parse_stream(input)
+    satisfy_map(PatternNonValuePlace::from_value)
 });
 
-def_value_parser_fn!(Where, or_and_clause, OrWhereClause, input, {
+def_matches_plain_symbol!(Where, and, "and");
-    satisfy_map(|x: edn::Value| {
+
-        seq(x).and_then(|items| {
+def_matches_plain_symbol!(Where, or, "or");
-            let mut p = Where::and()
+
+def_matches_plain_symbol!(Where, or_join, "or-join");
+
+def_parser!(Where, rule_vars, Vec<Variable>, {
+    seq()
+        .of_exactly(many1(Query::variable()))
+});
+
+def_parser!(Where, or_pattern_clause, OrWhereClause, {
+    Where::clause().map(|clause| OrWhereClause::Clause(clause))
+});
+
+def_parser!(Where, or_and_clause, OrWhereClause, {
+    seq()
+        .of_exactly(Where::and()
             .with(many1(Where::clause()))
-                        .skip(eof())
+            .map(OrWhereClause::And))
-                        .map(OrWhereClause::And);
-            let r: ParseResult<OrWhereClause, _> = p.parse_lazy(&items[..]).into();
-            Query::to_parsed_value(r)
-        })
-    }).parse_stream(input)
 });
 
-def_value_parser_fn!(Where, or_where_clause, OrWhereClause, input, {
+def_parser!(Where, or_where_clause, OrWhereClause, {
-    choice([Where::or_pattern_clause(), Where::or_and_clause()]).parse_stream(input)
+    choice([Where::or_pattern_clause(), Where::or_and_clause()])
 });
 
-def_value_parser_fn!(Where, or_clause, WhereClause, input, {
+def_parser!(Where, or_clause, WhereClause, {
-    satisfy_map(|x: edn::Value| {
+    seq()
-        seq(x).and_then(|items| {
+        .of_exactly(Where::or()
-            let mut p = Where::or()
             .with(many1(Where::or_where_clause()))
-                        .skip(eof())
             .map(|clauses| {
                 WhereClause::OrJoin(
                     OrJoin {
                         unify_vars: UnifyVars::Implicit,
                         clauses: clauses,
                     })
-                        });
+            }))
-            let r: ParseResult<WhereClause, _> = p.parse_lazy(&items[..]).into();
-            Query::to_parsed_value(r)
-        })
-    }).parse_stream(input)
 });
 
-def_value_parser_fn!(Where, or_join_clause, WhereClause, input, {
+def_parser!(Where, or_join_clause, WhereClause, {
-    satisfy_map(|x: edn::Value| {
+    seq()
-        seq(x).and_then(|items| {
+        .of_exactly(Where::or_join()
-            let mut p = Where::or_join()
             .with(Where::rule_vars())
             .and(many1(Where::or_where_clause()))
-                        .skip(eof())
             .map(|(vars, clauses)| {
                 WhereClause::OrJoin(
                     OrJoin {
                         unify_vars: UnifyVars::Explicit(vars),
                         clauses: clauses,
                     })
-                        });
+            }))
-            let r: ParseResult<WhereClause, _> = p.parse_lazy(&items[..]).into();
-            Query::to_parsed_value(r)
-        })
-    }).parse_stream(input)
 });
 
 /// A vector containing just a parenthesized filter expression.
-def_value_parser_fn!(Where, pred, WhereClause, input, {
+def_parser!(Where, pred, WhereClause, {
-    satisfy_map(|x: edn::Value| {
+    // Accept either a nested list or a nested vector here:
-        // Accept either a list or a vector here:
     // `[(foo ?x ?y)]` or `[[foo ?x ?y]]`
-        unwrap_nested(x).and_then(|items| {
+    vector()
-            let mut p = (Query::predicate_fn(), Query::arguments())
+        .of_exactly(seq()
-                        .skip(eof())
+            .of_exactly((Query::predicate_fn(), Query::arguments())
                 .map(|(f, args)| {
                     WhereClause::Pred(
                         Predicate {
                             operator: f.0,
                             args: args,
                         })
-            });
+                })))
-            let r: ParseResult<WhereClause, _> = p.parse_lazy(&items[..]).into();
-            Query::to_parsed_value(r)
-        })
-    }).parse_stream(input)
 });
 
-def_value_parser_fn!(Where, pattern, WhereClause, input, {
+def_parser!(Where, pattern, WhereClause, {
-    satisfy_map(|x: edn::Value| {
+    vector()
-        if let edn::Value::Vector(y) = x {
+        .of_exactly(
             // While *technically* Datomic allows you to have a query like:
             // [:find … :where [[?x]]]
             // We don't -- we require at least e, a.
-            let mut p = (optional(Query::source_var()),              // src
+            (optional(Query::source_var()),              // src
              Where::pattern_non_value_place(),           // e
              Where::pattern_non_value_place(),           // a
              optional(Where::pattern_value_place()),     // v
              optional(Where::pattern_non_value_place())) // tx
-                    .skip(eof())
+                .and_then(|(src, e, a, v, tx)| {
-                    .map(|(src, e, a, v, tx)| {
                     let v = v.unwrap_or(PatternValuePlace::Placeholder);
                     let tx = tx.unwrap_or(PatternNonValuePlace::Placeholder);
 
                     // Pattern::new takes care of reversal of reversed
                     // attributes: [?x :foo/_bar ?y] turns into
                     // [?y :foo/bar ?x].
-                        Pattern::new(src, e, a, v, tx).map(WhereClause::Pattern)
+                    //
-                    });
-
                     // This is a bit messy: the inner conversion to a Pattern can
                     // fail if the input is something like
                     //
@@ -269,135 +230,148 @@ def_value_parser_fn!(Where, pattern, WhereClause, input, {
                     // [23.4 :foo/reversed ?x]
                     // ```
                     //
-            // is nonsense. That leaves us with nested optionals; we unwrap them here.
+                    // is nonsense. That leaves us with a nested optional, which we unwrap here.
-            let r: ParseResult<Option<WhereClause>, _> = p.parse_lazy(&y[..]).into();
+                    Pattern::new(src, e, a, v, tx)
-            let v: Option<Option<WhereClause>> = Query::to_parsed_value(r);
+                        .map(WhereClause::Pattern)
-            v.unwrap_or(None)
+                        .ok_or(combine::primitives::Error::Expected("pattern".into()))
-        } else {
+                }))
-            None
-        }
-    }).parse_stream(input)
 });
 
-def_value_parser_fn!(Query, arguments, Vec<FnArg>, input, {
+def_parser!(Where, clause, WhereClause, {
-    (many::<Vec<FnArg>, _>(Query::fn_arg()))
+    choice([try(Where::pattern()),
-    .skip(eof())
-    .parse_stream(input)
-});
-
-def_value_parser_fn!(Where, clause, WhereClause, input, {
-    choice([Where::pattern(),
-            Where::pred(),
             // It's either
             //   (or-join [vars] clauses…)
             // or
             //   (or clauses…)
             // We don't yet handle source vars.
-            Where::or_join_clause(),
+            try(Where::or_join_clause()),
-            Where::or_clause(),
+            try(Where::or_clause()),
-    ]).parse_stream(input)
+
+            try(Where::pred()),
+    ])
 });
 
-def_value_parser_fn!(Where, clauses, Vec<WhereClause>, input, {
+def_parser!(Where, clauses, Vec<WhereClause>, {
     // Right now we only support patterns and predicates. See #239 for more.
     (many1::<Vec<WhereClause>, _>(Where::clause()))
-    .skip(eof())
-    .parse_stream(input)
 });
 
-pub struct Find<I>(::std::marker::PhantomData<fn(I) -> I>);
+pub struct Find;
 
-def_value_parser_fn!(Find, period, (), input, {
+def_matches_plain_symbol!(Find, period, ".");
-    matches_plain_symbol!(".", input)
-});
 
-def_value_parser_fn!(Find, ellipsis, (), input, {
+def_matches_plain_symbol!(Find, ellipsis, "...");
-    matches_plain_symbol!("...", input)
-});
 
-def_value_parser_fn!(Find, find_scalar, FindSpec, input, {
+def_parser!(Find, find_scalar, FindSpec, {
     Query::variable()
         .skip(Find::period())
         .skip(eof())
         .map(|var| FindSpec::FindScalar(Element::Variable(var)))
-        .parse_stream(input)
 });
 
-def_value_parser_fn!(Find, find_coll, FindSpec, input, {
+def_parser!(Find, find_coll, FindSpec, {
-    satisfy_unwrap!(edn::Value::Vector, y, {
+    vector()
-            let mut p = Query::variable()
+        .of_exactly(Query::variable()
-                        .skip(Find::ellipsis())
+            .skip(Find::ellipsis()))
-                        .skip(eof())
+            .map(|var| FindSpec::FindColl(Element::Variable(var)))
-                        .map(|var| FindSpec::FindColl(Element::Variable(var)));
-            let r: ParseResult<FindSpec, _> = p.parse_lazy(&y[..]).into();
-            Query::to_parsed_value(r)
-        })
-        .parse_stream(input)
 });
 
-def_value_parser_fn!(Find, elements, Vec<Element>, input, {
+def_parser!(Find, elements, Vec<Element>, {
-    many1::<Vec<Variable>, _>(Query::variable()).skip(eof())
+    many1::<Vec<Element>, _>(Query::variable().map(Element::Variable))
-        .map(|vars| {
-            vars.into_iter()
-                .map(Element::Variable)
-                .collect()
-        })
-        .parse_stream(input)
 });
 
-def_value_parser_fn!(Find, find_rel, FindSpec, input, {
+def_parser!(Find, find_rel, FindSpec, {
-    Find::elements().map(FindSpec::FindRel).parse_stream(input)
+    Find::elements().map(FindSpec::FindRel)
 });
 
-def_value_parser_fn!(Find, find_tuple, FindSpec, input, {
+def_parser!(Find, find_tuple, FindSpec, {
-    satisfy_unwrap!(edn::Value::Vector, y, {
+    vector()
-        let r: ParseResult<FindSpec, _> =
+        .of_exactly(Find::elements().map(FindSpec::FindTuple))
-            Find::elements().map(FindSpec::FindTuple).parse_lazy(&y[..]).into();
-        Query::to_parsed_value(r)
-    })
-    .parse_stream(input)
 });
 
-def_value_parser_fn!(Find, find, FindSpec, input, {
+/// Parse a stream of values into one of four find specs.
-    // Any one of the four specs might apply, so we combine them with `choice`.
+///
-    // Our parsers consume input, so we need to wrap them in `try` so that they
+/// `:find` must be an array of plain var symbols (?foo), pull expressions, and aggregates.  For now
-    // operate independently.
+/// we only support variables and the annotations necessary to declare which flavor of :find we
-    choice::<[&mut Parser<Input = I, Output = FindSpec>; 4], _>
+/// want:
+///
+///
+///     `?x ?y ?z  `     = FindRel
+///     `[?x ...]  `     = FindColl
+///     `?x .      `     = FindScalar
+///     `[?x ?y ?z]`     = FindTuple
+def_parser!(Find, spec, FindSpec, {
+    // Any one of the four specs might apply, so we combine them with `choice`.  Our parsers consume
+    // input, so we need to wrap them in `try` so that they operate independently.
+    choice::<[&mut Parser<Input = _, Output = FindSpec>; 4], _>
         ([&mut try(Find::find_scalar()),
           &mut try(Find::find_coll()),
           &mut try(Find::find_tuple()),
           &mut try(Find::find_rel())])
-        .parse_stream(input)
 });
 
-// Parse a sequence of values into one of four find specs.
+def_matches_keyword!(Find, literal_find, "find");
-//
+
-// `:find` must be an array of plain var symbols (?foo), pull expressions, and aggregates.
+def_matches_keyword!(Find, literal_with, "with");
-// For now we only support variables and the annotations necessary to declare which
+
-// flavor of :find we want:
+def_matches_keyword!(Find, literal_where, "where");
-//
+
-//
+/// Express something close to a builder pattern for a `FindQuery`.
-//     `?x ?y ?z  `     = FindRel
+enum FindQueryPart {
-//     `[?x ...]  `     = FindColl
+    FindSpec(FindSpec),
-//     `?x .      `     = FindScalar
+    With(Vec<Variable>),
-//     `[?x ?y ?z]`     = FindTuple
+    WhereClauses(Vec<WhereClause>),
-//
-#[allow(dead_code)]
-pub fn find_seq_to_find_spec(find: &[edn::Value]) -> FindParseResult {
-    Find::find()
-        .parse(find)
-        .map(|x| x.0)
-        .map_err::<ValueParseError, _>(|e| e.translate_position(find).into())
-        .map_err(|e| Error::from_kind(ErrorKind::FindParseError(e)))
 }
 
-#[allow(dead_code)]
+/// This is awkward, but will do for now.  We use `keyword_map()` to optionally accept vector find
-pub fn clause_seq_to_patterns(clauses: &[edn::Value]) -> WhereParseResult {
+/// queries, then we use `FindQueryPart` to collect parts that have heterogeneous types; and then we
-    Where::clauses()
+/// construct a `FindQuery` from them.
-        .parse(clauses)
+def_parser!(Find, query, FindQuery, {
+    let p_find_spec = Find::literal_find()
+        .with(vector().of_exactly(Find::spec().map(FindQueryPart::FindSpec)));
+
+    let p_with_vars = Find::literal_with()
+        .with(vector().of_exactly(many(Query::variable()).map(FindQueryPart::With)));
+
+    let p_where_clauses = Find::literal_where()
+        .with(vector().of_exactly(Where::clauses().map(FindQueryPart::WhereClauses))).expected(":where clauses");
+
+    (or(map(), keyword_map()))
+        .of_exactly(many(choice::<[&mut Parser<Input = ValueStream, Output = FindQueryPart>; 3], _>([
+            &mut try(p_find_spec),
+            &mut try(p_with_vars),
+            &mut try(p_where_clauses),
+        ])))
+        .and_then(|parts: Vec<FindQueryPart>| -> std::result::Result<FindQuery, combine::primitives::Error<edn::ValueAndSpan, edn::ValueAndSpan>>  {
+            let mut find_spec = None;
+            let mut with_vars = None;
+            let mut where_clauses = None;
+
+            for part in parts {
+                match part {
+                    FindQueryPart::FindSpec(x) => find_spec = Some(x),
+                    FindQueryPart::With(x) => with_vars = Some(x),
+                    FindQueryPart::WhereClauses(x) => where_clauses = Some(x),
+                }
+            }
+
+            Ok(FindQuery {
+                find_spec: find_spec.clone().ok_or(combine::primitives::Error::Unexpected("expected :find".into()))?,
+                default_source: SrcVar::DefaultSrc,
+                with: with_vars.unwrap_or(vec![]),
+                in_vars: vec![],       // TODO
+                in_sources: vec![],    // TODO
+                where_clauses: where_clauses.ok_or(combine::primitives::Error::Unexpected("expected :where".into()))?,
+            })
+        })
+});
+
+pub fn parse_find_string(string: &str) -> Result<FindQuery> {
+    let expr = edn::parse::value(string)?;
+    Find::query()
+        .parse(expr.into_atom_stream())
         .map(|x| x.0)
-        .map_err::<ValueParseError, _>(|e| e.translate_position(clauses).into())
+        .map_err(|e| Error::from_kind(ErrorKind::FindParseError(e)))
-        .map_err(|e| Error::from_kind(ErrorKind::WhereParseError(e)))
 }
 
 #[cfg(test)]
@@ -441,10 +415,10 @@ mod test {
         let a = edn::NamespacedKeyword::new("foo", "bar");
         let v = OrderedFloat(99.9);
         let tx = edn::PlainSymbol::new("?tx");
-        let input = [edn::Value::Vector(vec!(edn::Value::PlainSymbol(e.clone()),
+        let input = edn::Value::Vector(vec!(edn::Value::PlainSymbol(e.clone()),
                                             edn::Value::NamespacedKeyword(a.clone()),
                                             edn::Value::Float(v.clone()),
-                 edn::Value::PlainSymbol(tx.clone())))];
+                                            edn::Value::PlainSymbol(tx.clone())));
         assert_parses_to!(Where::pattern, input, WhereClause::Pattern(Pattern {
             source: None,
             entity: PatternNonValuePlace::Placeholder,
@@ -461,11 +435,11 @@ mod test {
         let a = edn::PlainSymbol::new("?a");
         let v = edn::PlainSymbol::new("?v");
         let tx = edn::PlainSymbol::new("?tx");
-        let input = [edn::Value::Vector(vec!(edn::Value::PlainSymbol(s.clone()),
+        let input = edn::Value::Vector(vec!(edn::Value::PlainSymbol(s.clone()),
                  edn::Value::PlainSymbol(e.clone()),
                  edn::Value::PlainSymbol(a.clone()),
                  edn::Value::PlainSymbol(v.clone()),
-                 edn::Value::PlainSymbol(tx.clone())))];
+                 edn::Value::PlainSymbol(tx.clone())));
         assert_parses_to!(Where::pattern, input, WhereClause::Pattern(Pattern {
             source: Some(SrcVar::NamedSrc("x".to_string())),
             entity: PatternNonValuePlace::Variable(variable(e)),
@@ -481,13 +455,13 @@ mod test {
         let a = edn::NamespacedKeyword::new("foo", "_bar");
         let v = OrderedFloat(99.9);
         let tx = edn::PlainSymbol::new("?tx");
-        let input = [edn::Value::Vector(vec!(edn::Value::PlainSymbol(e.clone()),
+        let input = edn::Value::Vector(vec!(edn::Value::PlainSymbol(e.clone()),
                  edn::Value::NamespacedKeyword(a.clone()),
                  edn::Value::Float(v.clone()),
-                 edn::Value::PlainSymbol(tx.clone())))];
+                 edn::Value::PlainSymbol(tx.clone())));
 
         let mut par = Where::pattern();
-        let result = par.parse(&input[..]);
+        let result = par.parse(input.with_spans().into_atom_stream());
         assert!(matches!(result, Err(_)), "Expected a parse error.");
     }
 
@@ -497,10 +471,10 @@ mod test {
         let a = edn::NamespacedKeyword::new("foo", "_bar");
         let v = edn::PlainSymbol::new("?v");
         let tx = edn::PlainSymbol::new("?tx");
-        let input = [edn::Value::Vector(vec!(edn::Value::PlainSymbol(e.clone()),
+        let input = edn::Value::Vector(vec!(edn::Value::PlainSymbol(e.clone()),
                  edn::Value::NamespacedKeyword(a.clone()),
                  edn::Value::PlainSymbol(v.clone()),
-                 edn::Value::PlainSymbol(tx.clone())))];
+                 edn::Value::PlainSymbol(tx.clone())));
 
         // Note that the attribute is no longer reversed, and the entity and value have
         // switched places.
@@ -516,7 +490,7 @@ mod test {
     #[test]
     fn test_rule_vars() {
         let e = edn::PlainSymbol::new("?e");
-        let input = [edn::Value::Vector(vec![edn::Value::PlainSymbol(e.clone())])];
+        let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(e.clone())]);
         assert_parses_to!(Where::rule_vars, input,
                           vec![variable(e.clone())]);
     }
@@ -527,11 +501,11 @@ mod test {
         let e = edn::PlainSymbol::new("?e");
         let a = edn::PlainSymbol::new("?a");
         let v = edn::PlainSymbol::new("?v");
-        let input = [edn::Value::List(
+        let input = edn::Value::List(
             vec![edn::Value::PlainSymbol(oj),
                  edn::Value::Vector(vec![edn::Value::PlainSymbol(e.clone()),
                                          edn::Value::PlainSymbol(a.clone()),
-                                         edn::Value::PlainSymbol(v.clone())])].into_iter().collect())];
+                                         edn::Value::PlainSymbol(v.clone())])].into_iter().collect());
         assert_parses_to!(Where::or_clause, input,
                           WhereClause::OrJoin(
                               OrJoin {
@@ -553,12 +527,12 @@ mod test {
         let e = edn::PlainSymbol::new("?e");
         let a = edn::PlainSymbol::new("?a");
         let v = edn::PlainSymbol::new("?v");
-        let input = [edn::Value::List(
+        let input = edn::Value::List(
             vec![edn::Value::PlainSymbol(oj),
                  edn::Value::Vector(vec![edn::Value::PlainSymbol(e.clone())]),
                  edn::Value::Vector(vec![edn::Value::PlainSymbol(e.clone()),
                                          edn::Value::PlainSymbol(a.clone()),
-                                         edn::Value::PlainSymbol(v.clone())])].into_iter().collect())];
+                                         edn::Value::PlainSymbol(v.clone())])].into_iter().collect());
         assert_parses_to!(Where::or_join_clause, input,
                           WhereClause::OrJoin(
                               OrJoin {
@@ -577,16 +551,16 @@ mod test {
     #[test]
     fn test_find_sp_variable() {
         let sym = edn::PlainSymbol::new("?x");
-        let input = [edn::Value::PlainSymbol(sym.clone())];
+        let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(sym.clone())]);
-        assert_parses_to!(Query::variable, input, variable(sym));
+        assert_parses_to!(|| vector().of_exactly(Query::variable()), input, variable(sym));
     }
 
     #[test]
     fn test_find_scalar() {
         let sym = edn::PlainSymbol::new("?x");
         let period = edn::PlainSymbol::new(".");
-        let input = [edn::Value::PlainSymbol(sym.clone()), edn::Value::PlainSymbol(period.clone())];
+        let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(sym.clone()), edn::Value::PlainSymbol(period.clone())]);
-        assert_parses_to!(Find::find_scalar,
+        assert_parses_to!(|| vector().of_exactly(Find::find_scalar()),
                           input,
                           FindSpec::FindScalar(Element::Variable(variable(sym))));
     }
@@ -595,8 +569,8 @@ mod test {
     fn test_find_coll() {
         let sym = edn::PlainSymbol::new("?x");
         let period = edn::PlainSymbol::new("...");
-        let input = [edn::Value::Vector(vec![edn::Value::PlainSymbol(sym.clone()),
+        let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(sym.clone()),
-                                             edn::Value::PlainSymbol(period.clone())])];
+                                            edn::Value::PlainSymbol(period.clone())]);
         assert_parses_to!(Find::find_coll,
                           input,
                           FindSpec::FindColl(Element::Variable(variable(sym))));
@@ -606,8 +580,8 @@ mod test {
     fn test_find_rel() {
         let vx = edn::PlainSymbol::new("?x");
         let vy = edn::PlainSymbol::new("?y");
-        let input = [edn::Value::PlainSymbol(vx.clone()), edn::Value::PlainSymbol(vy.clone())];
+        let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(vx.clone()), edn::Value::PlainSymbol(vy.clone())]);
-        assert_parses_to!(Find::find_rel,
+        assert_parses_to!(|| vector().of_exactly(Find::find_rel()),
                           input,
                           FindSpec::FindRel(vec![Element::Variable(variable(vx)),
                                                  Element::Variable(variable(vy))]));
@@ -617,37 +591,11 @@ mod test {
     fn test_find_tuple() {
         let vx = edn::PlainSymbol::new("?x");
         let vy = edn::PlainSymbol::new("?y");
-        let input = [edn::Value::Vector(vec![edn::Value::PlainSymbol(vx.clone()),
+        let input = edn::Value::Vector(vec![edn::Value::PlainSymbol(vx.clone()),
-                                             edn::Value::PlainSymbol(vy.clone())])];
+                                             edn::Value::PlainSymbol(vy.clone())]);
         assert_parses_to!(Find::find_tuple,
                           input,
                           FindSpec::FindTuple(vec![Element::Variable(variable(vx)),
                                                    Element::Variable(variable(vy))]));
     }
-
-    #[test]
-    fn test_find_processing() {
-        let vx = edn::PlainSymbol::new("?x");
-        let vy = edn::PlainSymbol::new("?y");
-        let ellipsis = edn::PlainSymbol::new("...");
-        let period = edn::PlainSymbol::new(".");
-
-        let scalar = [edn::Value::PlainSymbol(vx.clone()), edn::Value::PlainSymbol(period.clone())];
-        let tuple = [edn::Value::Vector(vec![edn::Value::PlainSymbol(vx.clone()),
-                                             edn::Value::PlainSymbol(vy.clone())])];
-        let coll = [edn::Value::Vector(vec![edn::Value::PlainSymbol(vx.clone()),
-                                            edn::Value::PlainSymbol(ellipsis.clone())])];
-        let rel = [edn::Value::PlainSymbol(vx.clone()), edn::Value::PlainSymbol(vy.clone())];
-
-        assert_eq!(FindSpec::FindScalar(Element::Variable(variable(vx.clone()))),
-                   find_seq_to_find_spec(&scalar).unwrap());
-        assert_eq!(FindSpec::FindTuple(vec![Element::Variable(variable(vx.clone())),
-                                            Element::Variable(variable(vy.clone()))]),
-                   find_seq_to_find_spec(&tuple).unwrap());
-        assert_eq!(FindSpec::FindColl(Element::Variable(variable(vx.clone()))),
-                   find_seq_to_find_spec(&coll).unwrap());
-        assert_eq!(FindSpec::FindRel(vec![Element::Variable(variable(vx.clone())),
-                                          Element::Variable(variable(vy.clone()))]),
-                   find_seq_to_find_spec(&rel).unwrap());
-    }
 }

query-parser/src/util.rs

@@ -1,151 +0,0 @@
-// Copyright 2016 Mozilla
-//
-// Licensed under the Apache License, Version 2.0 (the "License"); you may not use
-// this file except in compliance with the License. You may obtain a copy of the
-// License at http://www.apache.org/licenses/LICENSE-2.0
-// Unless required by applicable law or agreed to in writing, software distributed
-// under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
-// CONDITIONS OF ANY KIND, either express or implied. See the License for the
-// specific language governing permissions and limitations under the License.
-
-extern crate edn;
-
-use std::collections::BTreeMap;
-
-/// Take a slice of EDN values, as would be extracted from an
-/// `edn::Value::Vector`, and turn it into a map.
-///
-/// The slice must consist of subsequences of an initial plain
-/// keyword, followed by one or more non-plain-keyword values.
-///
-/// The plain keywords are used as keys into the resulting map.
-/// The values are accumulated into vectors.
-///
-/// Invalid input causes this function to return `None`.
-///
-/// TODO: this function can be generalized to take an arbitrary
-/// destructuring/break function, yielding a map with a custom
-/// key type and splitting in the right places.
-pub fn vec_to_keyword_map(vec: &[edn::Value]) -> Option<BTreeMap<edn::Keyword, Vec<edn::Value>>> {
-    let mut m = BTreeMap::new();
-
-    if vec.is_empty() {
-        return Some(m);
-    }
-
-    if vec.len() == 1 {
-        return None;
-    }
-
-    // Turn something like
-    //
-    //   `[:foo 1 2 3 :bar 4 5 6]`
-    //
-    // into
-    //
-    //   `Some((:foo, [1 2 3]))`
-    fn step(slice: &[edn::Value]) -> Option<(edn::Keyword, Vec<edn::Value>)> {
-        // [:foo 1 2 3 :bar] is invalid: nothing follows `:bar`.
-        if slice.len() < 2 {
-            return None;
-        }
-
-        // The first item must be a keyword.
-        if let edn::Value::Keyword(ref k) = slice[0] {
-
-            // The second can't be: [:foo :bar 1 2 3] is invalid.
-            if slice[1].is_keyword() {
-                return None;
-            }
-
-            // Accumulate items until we reach the next keyword.
-            let mut acc = Vec::new();
-            for v in &slice[1..] {
-                if v.is_keyword() {
-                    break;
-                }
-                acc.push(v.clone());
-            }
-            return Some((k.clone(), acc));
-        }
-
-        None
-    }
-
-    let mut bits = vec;
-    while !bits.is_empty() {
-        match step(bits) {
-            Some((k, v)) => {
-                bits = &bits[(v.len() + 1)..];
-
-                // Duplicate keys aren't allowed.
-                if m.contains_key(&k) {
-                    return None;
-                }
-                m.insert(k, v);
-            },
-            None => return None,
-        }
-    }
-    return Some(m);
-}
-
-#[test]
-fn test_vec_to_keyword_map() {
-    let foo = edn::symbols::Keyword("foo".to_string());
-    let bar = edn::symbols::Keyword("bar".to_string());
-    let baz = edn::symbols::Keyword("baz".to_string());
-
-    // [:foo 1 2 3 :bar 4]
-    let input = vec!(edn::Value::Keyword(foo.clone()),
-                     edn::Value::Integer(1),
-                     edn::Value::Integer(2),
-                     edn::Value::Integer(3),
-                     edn::Value::Keyword(bar.clone()),
-                     edn::Value::Integer(4));
-
-    let m = vec_to_keyword_map(&input).unwrap();
-
-    assert!(m.contains_key(&foo));
-    assert!(m.contains_key(&bar));
-    assert!(!m.contains_key(&baz));
-
-    let onetwothree = vec!(edn::Value::Integer(1),
-                           edn::Value::Integer(2),
-                           edn::Value::Integer(3));
-    let four = vec!(edn::Value::Integer(4));
-
-    assert_eq!(m.get(&foo).unwrap(), &onetwothree);
-    assert_eq!(m.get(&bar).unwrap(), &four);
-
-    // Trailing keywords aren't allowed.
-    assert_eq!(None,
-               vec_to_keyword_map(&vec!(edn::Value::Keyword(foo.clone()))));
-    assert_eq!(None,
-               vec_to_keyword_map(&vec!(edn::Value::Keyword(foo.clone()),
-                                        edn::Value::Integer(2),
-                                        edn::Value::Keyword(bar.clone()))));
-
-    // Duplicate keywords aren't allowed.
-    assert_eq!(None,
-               vec_to_keyword_map(&vec!(edn::Value::Keyword(foo.clone()),
-                                        edn::Value::Integer(2),
-                                        edn::Value::Keyword(foo.clone()),
-                                        edn::Value::Integer(1))));
-
-    // Starting with anything but a keyword isn't allowed.
-    assert_eq!(None,
-               vec_to_keyword_map(&vec!(edn::Value::Integer(2),
-                                        edn::Value::Keyword(foo.clone()),
-                                        edn::Value::Integer(1))));
-
-    // Consecutive keywords aren't allowed.
-    assert_eq!(None,
-               vec_to_keyword_map(&vec!(edn::Value::Keyword(foo.clone()),
-                                        edn::Value::Keyword(bar.clone()),
-                                        edn::Value::Integer(1))));
-
-    // Empty lists return an empty map.
-    assert_eq!(BTreeMap::new(), vec_to_keyword_map(&vec!()).unwrap());
-}
-

query-parser/tests/find_tests.rs

@@ -97,7 +97,7 @@ fn can_parse_simple_or() {
 #[test]
 fn can_parse_unit_or_join() {
     let s = "[:find ?x . :where (or-join [?x] [?x _ 15])]";
-    let p = parse_find_string(s).unwrap();
+    let p = parse_find_string(s).expect("to be able to parse find");
 
     assert_eq!(p.find_spec,
                FindSpec::FindScalar(Element::Variable(Variable::from_valid_name("?x"))));

query/src/lib.rs

@@ -67,15 +67,15 @@ impl Variable {
 }
 
 pub trait FromValue<T> {
-    fn from_value(v: &edn::Value) -> Option<T>;
+    fn from_value(v: edn::ValueAndSpan) -> Option<T>;
 }
 
 /// If the provided EDN value is a PlainSymbol beginning with '?', return
 /// it wrapped in a Variable. If not, return None.
 /// TODO: intern strings. #398.
 impl FromValue<Variable> for Variable {
-    fn from_value(v: &edn::Value) -> Option<Variable> {
+    fn from_value(v: edn::ValueAndSpan) -> Option<Variable> {
-        if let edn::Value::PlainSymbol(ref s) = *v {
+        if let edn::SpannedValue::PlainSymbol(ref s) = v.inner {
             Variable::from_symbol(s)
         } else {
             None
@@ -112,8 +112,8 @@ impl fmt::Debug for Variable {
 pub struct PredicateFn(pub PlainSymbol);
 
 impl FromValue<PredicateFn> for PredicateFn {
-    fn from_value(v: &edn::Value) -> Option<PredicateFn> {
+    fn from_value(v: edn::ValueAndSpan) -> Option<PredicateFn> {
-        if let edn::Value::PlainSymbol(ref s) = *v {
+        if let edn::SpannedValue::PlainSymbol(ref s) = v.inner {
             PredicateFn::from_symbol(s)
         } else {
             None
@@ -135,8 +135,8 @@ pub enum SrcVar {
 }
 
 impl FromValue<SrcVar> for SrcVar {
-    fn from_value(v: &edn::Value) -> Option<SrcVar> {
+    fn from_value(v: edn::ValueAndSpan) -> Option<SrcVar> {
-        if let edn::Value::PlainSymbol(ref s) = *v {
+        if let edn::SpannedValue::PlainSymbol(ref s) = v.inner {
             SrcVar::from_symbol(s)
         } else {
             None
@@ -184,16 +184,17 @@ pub enum FnArg {
 }
 
 impl FromValue<FnArg> for FnArg {
-    fn from_value(v: &edn::Value) -> Option<FnArg> {
+    fn from_value(v: edn::ValueAndSpan) -> Option<FnArg> {
         // TODO: support SrcVars.
-        Variable::from_value(v)
+        Variable::from_value(v.clone()) // TODO: don't clone!
                  .and_then(|v| Some(FnArg::Variable(v)))
-                 .or_else(||
+                 .or_else(|| {
-            match v {
+                          println!("from_value {}", v.inner);
-                &edn::Value::Integer(i) => Some(FnArg::EntidOrInteger(i)),
+            match v.inner {
-                &edn::Value::Float(f) => Some(FnArg::Constant(NonIntegerConstant::Float(f))),
+                edn::SpannedValue::Integer(i) => Some(FnArg::EntidOrInteger(i)),
+                edn::SpannedValue::Float(f) => Some(FnArg::Constant(NonIntegerConstant::Float(f))),
                 _ => unimplemented!(),
-            })
+            }})
     }
 }
 
@@ -234,14 +235,14 @@ impl PatternNonValuePlace {
 }
 
 impl FromValue<PatternNonValuePlace> for PatternNonValuePlace {
-    fn from_value(v: &edn::Value) -> Option<PatternNonValuePlace> {
+    fn from_value(v: edn::ValueAndSpan) -> Option<PatternNonValuePlace> {
-        match v {
+        match v.inner {
-            &edn::Value::Integer(x) => if x >= 0 {
+            edn::SpannedValue::Integer(x) => if x >= 0 {
                 Some(PatternNonValuePlace::Entid(x))
             } else {
                 None
             },
-            &edn::Value::PlainSymbol(ref x) => if x.0.as_str() == "_" {
+            edn::SpannedValue::PlainSymbol(ref x) => if x.0.as_str() == "_" {
                 Some(PatternNonValuePlace::Placeholder)
             } else {
                 if let Some(v) = Variable::from_symbol(x) {
@@ -250,7 +251,7 @@ impl FromValue<PatternNonValuePlace> for PatternNonValuePlace {
                     None
                 }
             },
-            &edn::Value::NamespacedKeyword(ref x) =>
+            edn::SpannedValue::NamespacedKeyword(ref x) =>
                 Some(PatternNonValuePlace::Ident(Rc::new(x.clone()))),
             _ => None,
         }
@@ -276,23 +277,23 @@ pub enum PatternValuePlace {
 }
 
 impl FromValue<PatternValuePlace> for PatternValuePlace {
-    fn from_value(v: &edn::Value) -> Option<PatternValuePlace> {
+    fn from_value(v: edn::ValueAndSpan) -> Option<PatternValuePlace> {
-        match v {
+        match v.inner {
-            &edn::Value::Integer(x) =>
+            edn::SpannedValue::Integer(x) =>
                 Some(PatternValuePlace::EntidOrInteger(x)),
-            &edn::Value::PlainSymbol(ref x) if x.0.as_str() == "_" =>
+            edn::SpannedValue::PlainSymbol(ref x) if x.0.as_str() == "_" =>
                 Some(PatternValuePlace::Placeholder),
-            &edn::Value::PlainSymbol(ref x) =>
+            edn::SpannedValue::PlainSymbol(ref x) =>
                 Variable::from_symbol(x).map(PatternValuePlace::Variable),
-            &edn::Value::NamespacedKeyword(ref x) =>
+            edn::SpannedValue::NamespacedKeyword(ref x) =>
                 Some(PatternValuePlace::IdentOrKeyword(Rc::new(x.clone()))),
-            &edn::Value::Boolean(x) =>
+            edn::SpannedValue::Boolean(x) =>
                 Some(PatternValuePlace::Constant(NonIntegerConstant::Boolean(x))),
-            &edn::Value::Float(x) =>
+            edn::SpannedValue::Float(x) =>
                 Some(PatternValuePlace::Constant(NonIntegerConstant::Float(x))),
-            &edn::Value::BigInteger(ref x) =>
+            edn::SpannedValue::BigInteger(ref x) =>
                 Some(PatternValuePlace::Constant(NonIntegerConstant::BigInteger(x.clone()))),
-            &edn::Value::Text(ref x) =>
+            edn::SpannedValue::Text(ref x) =>
                 // TODO: intern strings. #398.
                 Some(PatternValuePlace::Constant(NonIntegerConstant::Text(Rc::new(x.clone())))),
             _ => None,

src/conn.rs

@@ -129,9 +129,8 @@ impl Conn {
                     sqlite: &mut rusqlite::Connection,
                     transaction: &str) -> Result<TxReport> {
 
-        let assertion_vector = edn::parse::value(transaction)
+        let assertion_vector = edn::parse::value(transaction)?;
-            .map(|x| x.without_spans())?;
+        let entities = mentat_tx_parser::Tx::parse(assertion_vector)?;
-        let entities = mentat_tx_parser::Tx::parse(&[assertion_vector][..])?;
 
         let tx = sqlite.transaction()?;
 
@@ -178,7 +177,6 @@ mod tests {
     use super::*;
 
     extern crate mentat_parser_utils;
-    use self::mentat_parser_utils::ValueParseError;
 
     #[test]
     fn test_transact_errors() {
@@ -203,7 +201,7 @@ mod tests {
         // Bad transaction data: missing leading :db/add.
         let report = conn.transact(&mut sqlite, "[[\"t\" :db/ident :b/keyword]]");
         match report.unwrap_err() {
-            Error(ErrorKind::TxParseError(::mentat_tx_parser::errors::ErrorKind::ParseError(ValueParseError { .. })), _) => { },
+            Error(ErrorKind::TxParseError(::mentat_tx_parser::errors::ErrorKind::ParseError(_)), _) => { },
             x => panic!("expected EDN parse error, got {:?}", x),
         }
 

tx-parser/src/errors.rs

@@ -10,7 +10,8 @@
 
 #![allow(dead_code)]
 
-use mentat_parser_utils::ValueParseError;
+use combine;
+use mentat_parser_utils::value_and_span::Stream;
 
 error_chain! {
     types {
@@ -18,9 +19,9 @@ error_chain! {
     }
 
     errors {
-        ParseError(value_parse_error: ValueParseError) {
+        ParseError(parse_error: combine::ParseError<Stream>) {
             description("error parsing edn values")
-            display("error parsing edn values:\n{}", value_parse_error)
+            display("error parsing edn values:\n{}", parse_error)
         }
 
         DbIdError {

tx-parser/src/lib.rs

@@ -10,8 +10,6 @@
 
 #![allow(dead_code)]
 
-use std::iter::once;
-
 extern crate combine;
 #[macro_use]
 extern crate error_chain;
@@ -22,13 +20,15 @@ extern crate mentat_tx;
 #[macro_use]
 extern crate mentat_parser_utils;
 
-use combine::{eof, many, parser, satisfy_map, token, Parser, ParseResult, Stream};
+use combine::{
-use combine::combinator::{Expected, FnParser};
+    eof,
-use edn::symbols::{
+    many,
-    NamespacedKeyword,
+    parser,
-    PlainSymbol,
+    satisfy,
+    satisfy_map,
+    Parser,
+    ParseResult,
 };
-use edn::types::Value;
 use mentat_tx::entities::{
     AtomOrLookupRefOrVectorOrMapNotation,
     Entid,
@@ -39,97 +39,73 @@ use mentat_tx::entities::{
     OpType,
     TempId,
 };
-use mentat_parser_utils::{ResultParser, ValueParseError};
+use mentat_parser_utils::{ResultParser};
+use mentat_parser_utils::value_and_span::{
+    Item,
+    OfExactlyParsing,
+    integer,
+    list,
+    map,
+    namespaced_keyword,
+    vector,
+};
 
 pub mod errors;
 pub use errors::*;
 
-pub struct Tx<I>(::std::marker::PhantomData<fn(I) -> I>);
+pub struct Tx;
 
-type TxParser<O, I> = Expected<FnParser<I, fn(I) -> ParseResult<O, I>>>;
+def_parser!(Tx, entid, Entid, {
-
+    integer()
-fn fn_parser<O, I>(f: fn(I) -> ParseResult<O, I>, err: &'static str) -> TxParser<O, I>
-    where I: Stream<Item = Value>
-{
-    parser(f).expected(err)
-}
-
-def_value_satisfy_parser_fn!(Tx, integer, i64, Value::as_integer);
-
-fn value_to_namespaced_keyword(val: &Value) -> Option<NamespacedKeyword> {
-    val.as_namespaced_keyword().cloned()
-}
-def_value_satisfy_parser_fn!(Tx, keyword, NamespacedKeyword, value_to_namespaced_keyword);
-
-def_parser_fn!(Tx, entid, Value, Entid, input, {
-    Tx::<I>::integer()
         .map(|x| Entid::Entid(x))
-        .or(Tx::<I>::keyword().map(|x| Entid::Ident(x)))
+        .or(namespaced_keyword().map(|x| Entid::Ident(x)))
-        .parse_lazy(input)
-        .into()
 });
 
-fn value_to_lookup_ref(val: &Value) -> Option<LookupRef> {
+def_matches_plain_symbol!(Tx, literal_lookup_ref, "lookup-ref");
-    val.as_list().and_then(|list| {
-        let vs: Vec<Value> = list.into_iter().cloned().collect();
-        let mut p = token(Value::PlainSymbol(PlainSymbol::new("lookup-ref")))
-            .with((Tx::<&[Value]>::entid(),
-                   Tx::<&[Value]>::atom()))
-            .skip(eof())
-            .map(|(a, v)| LookupRef { a: a, v: v });
-        let r: ParseResult<LookupRef, _> = p.parse_lazy(&vs[..]).into();
-        r.ok().map(|x| x.0)
-    })
-}
-def_value_satisfy_parser_fn!(Tx, lookup_ref, LookupRef, value_to_lookup_ref);
 
-def_parser_fn!(Tx, entid_or_lookup_ref_or_temp_id, Value, EntidOrLookupRefOrTempId, input, {
+def_parser!(Tx, lookup_ref, LookupRef, {
-    Tx::<I>::entid().map(EntidOrLookupRefOrTempId::Entid)
+    list().of_exactly(
-        .or(Tx::<I>::lookup_ref().map(EntidOrLookupRefOrTempId::LookupRef))
+        Tx::literal_lookup_ref()
-        .or(Tx::<I>::temp_id().map(EntidOrLookupRefOrTempId::TempId))
+            .with((Tx::entid(),
-        .parse_lazy(input)
+                   Tx::atom()))
-        .into()
+            .map(|(a, v)| LookupRef { a: a, v: v.without_spans() }))
 });
 
-def_parser_fn!(Tx, temp_id, Value, TempId, input, {
+def_parser!(Tx, entid_or_lookup_ref_or_temp_id, EntidOrLookupRefOrTempId, {
-    satisfy_map(|x: Value| x.into_text().map(TempId::External))
+    Tx::entid().map(EntidOrLookupRefOrTempId::Entid)
-        .parse_stream(input)
+        .or(Tx::lookup_ref().map(EntidOrLookupRefOrTempId::LookupRef))
+        .or(Tx::temp_id().map(EntidOrLookupRefOrTempId::TempId))
 });
 
-def_parser_fn!(Tx, atom, Value, Value, input, {
+def_parser!(Tx, temp_id, TempId, {
-    satisfy_map(|x: Value| x.into_atom())
+    satisfy_map(|x: edn::ValueAndSpan| x.into_text().map(TempId::External))
-        .parse_stream(input)
 });
 
-fn value_to_nested_vector(val: &Value) -> Option<Vec<AtomOrLookupRefOrVectorOrMapNotation>> {
+def_parser!(Tx, atom, edn::ValueAndSpan, {
-    val.as_vector().and_then(|vs| {
+    satisfy_map(|x: edn::ValueAndSpan| x.into_atom())
-        let mut p = many(Tx::<&[Value]>::atom_or_lookup_ref_or_vector()).skip(eof());
-        let r: ParseResult<Vec<AtomOrLookupRefOrVectorOrMapNotation>, _> =  p.parse_lazy(&vs[..]).into();
-        r.map(|x| x.0).ok()
-    })
-}
-def_value_satisfy_parser_fn!(Tx, nested_vector, Vec<AtomOrLookupRefOrVectorOrMapNotation>, value_to_nested_vector);
-
-def_parser_fn!(Tx, atom_or_lookup_ref_or_vector, Value, AtomOrLookupRefOrVectorOrMapNotation, input, {
-    Tx::<I>::lookup_ref().map(AtomOrLookupRefOrVectorOrMapNotation::LookupRef)
-        .or(Tx::<I>::nested_vector().map(AtomOrLookupRefOrVectorOrMapNotation::Vector))
-        .or(Tx::<I>::map_notation().map(AtomOrLookupRefOrVectorOrMapNotation::MapNotation))
-        .or(Tx::<I>::atom().map(AtomOrLookupRefOrVectorOrMapNotation::Atom))
-        .parse_lazy(input)
-        .into()
 });
 
-fn value_to_add_or_retract(val: &Value) -> Option<Entity> {
+def_parser!(Tx, nested_vector, Vec<AtomOrLookupRefOrVectorOrMapNotation>, {
-    val.as_vector().and_then(|vs| {
+    vector().of_exactly(many(Tx::atom_or_lookup_ref_or_vector()))
-        let add = token(Value::NamespacedKeyword(NamespacedKeyword::new("db", "add")))
+});
-            .map(|_| OpType::Add);
+
-        let retract = token(Value::NamespacedKeyword(NamespacedKeyword::new("db", "retract")))
+def_parser!(Tx, atom_or_lookup_ref_or_vector, AtomOrLookupRefOrVectorOrMapNotation, {
-            .map(|_| OpType::Retract);
+    Tx::lookup_ref().map(AtomOrLookupRefOrVectorOrMapNotation::LookupRef)
-        let mut p = (add.or(retract),
+        .or(Tx::nested_vector().map(AtomOrLookupRefOrVectorOrMapNotation::Vector))
-                     Tx::<&[Value]>::entid_or_lookup_ref_or_temp_id(),
+        .or(Tx::map_notation().map(AtomOrLookupRefOrVectorOrMapNotation::MapNotation))
-                     Tx::<&[Value]>::entid(),
+        .or(Tx::atom().map(AtomOrLookupRefOrVectorOrMapNotation::Atom))
-                     Tx::<&[Value]>::atom_or_lookup_ref_or_vector())
+});
-            .skip(eof())
+
+def_matches_namespaced_keyword!(Tx, literal_db_add, "db", "add");
+
+def_matches_namespaced_keyword!(Tx, literal_db_retract, "db", "retract");
+
+def_parser!(Tx, add_or_retract, Entity, {
+    vector().of_exactly(
+        (Tx::literal_db_add().map(|_| OpType::Add).or(Tx::literal_db_retract().map(|_| OpType::Retract)),
+             Tx::entid_or_lookup_ref_or_temp_id(),
+             Tx::entid(),
+             Tx::atom_or_lookup_ref_or_vector())
         .map(|(op, e, a, v)| {
             Entity::AddOrRetract {
                 op: op,
@@ -137,67 +113,40 @@ fn value_to_add_or_retract(val: &Value) -> Option<Entity> {
                 a: a,
                 v: v,
             }
+        }))
 });
-        let r: ParseResult<Entity, _> =  p.parse_lazy(&vs[..]).into();
-        r.map(|x| x.0).ok()
-    })
-}
-def_value_satisfy_parser_fn!(Tx, add_or_retract, Entity, value_to_add_or_retract);
 
-fn value_to_map_notation(val: &Value) -> Option<MapNotation> {
+def_parser!(Tx, map_notation, MapNotation, {
-    val.as_map().cloned().and_then(|map| {
+    map()
-        // Parsing pairs is tricky; parsing sequences is easy.
+        .of_exactly(many((Tx::entid(), Tx::atom_or_lookup_ref_or_vector())))
-        let avseq: Vec<Value> = map.into_iter().flat_map(|(a, v)| once(a).chain(once(v))).collect();
-
-        let av = (Tx::<&[Value]>::entid(),
-                  Tx::<&[Value]>::atom_or_lookup_ref_or_vector())
-            .map(|(a, v)| -> (Entid, AtomOrLookupRefOrVectorOrMapNotation) { (a, v) });
-        let mut p = many(av)
-            .skip(eof())
         .map(|avs: Vec<(Entid, AtomOrLookupRefOrVectorOrMapNotation)>| -> MapNotation {
             avs.into_iter().collect()
-            });
-        let r: ParseResult<MapNotation, _> =  p.parse_lazy(&avseq[..]).into();
-        r.map(|x| x.0).ok()
         })
-}
-def_value_satisfy_parser_fn!(Tx, map_notation, MapNotation, value_to_map_notation);
-
-def_parser_fn!(Tx, entity, Value, Entity, input, {
-    let mut p = Tx::<I>::add_or_retract()
-        .or(Tx::<I>::map_notation().map(Entity::MapNotation));
-    p.parse_stream(input)
 });
 
-fn value_to_entities(val: &Value) -> Option<Vec<Entity>> {
+def_parser!(Tx, entity, Entity, {
-    val.as_vector().and_then(|vs| {
+    Tx::add_or_retract()
-        let mut p = many(Tx::<&[Value]>::entity())
+        .or(Tx::map_notation().map(Entity::MapNotation))
-            .skip(eof());
+});
-        let r: ParseResult<Vec<Entity>, _> = p.parse_lazy(&vs[..]).into();
-        r.ok().map(|x| x.0)
-    })
-}
 
-def_value_satisfy_parser_fn!(Tx, entities, Vec<Entity>, value_to_entities);
+def_parser!(Tx, entities, Vec<Entity>, {
+    vector().of_exactly(many(Tx::entity()))
+});
 
-impl<'a> Tx<&'a [edn::Value]> {
+impl Tx {
-    pub fn parse(input: &'a [edn::Value]) -> std::result::Result<Vec<Entity>, errors::Error> {
+    pub fn parse(input: edn::ValueAndSpan) -> std::result::Result<Vec<Entity>, errors::Error> {
-        Tx::<_>::entities()
+        Tx::entities()
             .skip(eof())
-            .parse(input)
+            .parse(input.into_atom_stream())
             .map(|x| x.0)
-            .map_err::<ValueParseError, _>(|e| e.translate_position(input).into())
             .map_err(|e| Error::from_kind(ErrorKind::ParseError(e)))
     }
-}
 
-impl<'a> Tx<&'a [edn::Value]> {
+    fn parse_entid_or_lookup_ref_or_temp_id(input: edn::ValueAndSpan) -> std::result::Result<EntidOrLookupRefOrTempId, errors::Error> {
-    pub fn parse_entid_or_lookup_ref_or_temp_id(input: &'a [edn::Value]) -> std::result::Result<EntidOrLookupRefOrTempId, errors::Error> {
+        Tx::entid_or_lookup_ref_or_temp_id()
-        Tx::<_>::entid_or_lookup_ref_or_temp_id()
             .skip(eof())
-            .parse(input)
+            .parse(input.into_atom_stream())
             .map(|x| x.0)
-            .map_err::<ValueParseError, _>(|e| e.translate_position(input).into())
             .map_err(|e| Error::from_kind(ErrorKind::ParseError(e)))
     }
 }
@@ -213,7 +162,7 @@ pub fn remove_db_id(map: &mut MapNotation) -> std::result::Result<Option<EntidOr
     let db_id: Option<EntidOrLookupRefOrTempId> = if let Some(id) = map.remove(&db_id_key) {
         match id {
             AtomOrLookupRefOrVectorOrMapNotation::Atom(v) => {
-                let db_id = Tx::<_>::parse_entid_or_lookup_ref_or_temp_id(&[v][..])
+                let db_id = Tx::parse_entid_or_lookup_ref_or_temp_id(v)
                     .chain_err(|| Error::from(ErrorKind::DbIdError))?;
                 Some(db_id)
             },
@@ -233,19 +182,25 @@ pub fn remove_db_id(map: &mut MapNotation) -> std::result::Result<Option<EntidOr
 #[cfg(test)]
 mod tests {
     use super::*;
-    use std::collections::LinkedList;
+
+    use std::collections::BTreeMap;
+
     use combine::Parser;
-    use edn::symbols::NamespacedKeyword;
+    use edn::{
-    use edn::types::Value;
+        NamespacedKeyword,
+        PlainSymbol,
+        Span,
+        SpannedValue,
+        Value,
+        ValueAndSpan,
+    };
     use mentat_tx::entities::{
         Entid,
         EntidOrLookupRefOrTempId,
         Entity,
-        LookupRef,
         OpType,
         AtomOrLookupRefOrVectorOrMapNotation,
     };
-    use std::collections::BTreeMap;
 
     fn kw(namespace: &str, name: &str) -> Value {
         Value::NamespacedKeyword(NamespacedKeyword::new(namespace, name))
@@ -253,108 +208,98 @@ mod tests {
 
     #[test]
     fn test_add() {
-        let input = [Value::Vector(vec![kw("db", "add"),
+        let input = Value::Vector(vec![kw("db", "add"),
                                        kw("test", "entid"),
                                        kw("test", "a"),
-                                        Value::Text("v".into())])];
+                                       Value::Text("v".into())]);
         let mut parser = Tx::entity();
-        let result = parser.parse(&input[..]);
+        let result = parser.parse(input.with_spans().into_atom_stream()).map(|x| x.0);
         assert_eq!(result,
-                   Ok((Entity::AddOrRetract {
+                   Ok(Entity::AddOrRetract {
                        op: OpType::Add,
                        e: EntidOrLookupRefOrTempId::Entid(Entid::Ident(NamespacedKeyword::new("test",
                                                                                               "entid"))),
                        a: Entid::Ident(NamespacedKeyword::new("test", "a")),
-                       v: AtomOrLookupRefOrVectorOrMapNotation::Atom(Value::Text("v".into())),
+                       v: AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("v".into()), Span(29, 32))),
-                   },
+                   }));
-                       &[][..])));
     }
 
     #[test]
     fn test_retract() {
-        let input = [Value::Vector(vec![kw("db", "retract"),
+        let input = Value::Vector(vec![kw("db", "retract"),
                                        Value::Integer(101),
                                        kw("test", "a"),
-                                        Value::Text("v".into())])];
+                                       Value::Text("v".into())]);
         let mut parser = Tx::entity();
-        let result = parser.parse(&input[..]);
+        let result = parser.parse(input.with_spans().into_atom_stream()).map(|x| x.0);
         assert_eq!(result,
-                   Ok((Entity::AddOrRetract {
+                   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())),
+                       v: AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("v".into()), Span(25, 28))),
-                   },
+                   }));
-                       &[][..])));
     }
 
     #[test]
     fn test_lookup_ref() {
-        let mut list = LinkedList::new();
+        let input = Value::Vector(vec![kw("db", "add"),
-        list.push_back(Value::PlainSymbol(PlainSymbol::new("lookup-ref")));
+                                       Value::List(vec![Value::PlainSymbol(PlainSymbol::new("lookup-ref")),
-        list.push_back(kw("test", "a1"));
+                                                        kw("test", "a1"),
-        list.push_back(Value::Text("v1".into()));
+                                                        Value::Text("v1".into())].into_iter().collect()),
-
-        let input = [Value::Vector(vec![kw("db", "add"),
-                                        Value::List(list),
                                        kw("test", "a"),
-                                        Value::Text("v".into())])];
+                                       Value::Text("v".into())]);
         let mut parser = Tx::entity();
-        let result = parser.parse(&input[..]);
+        let result = parser.parse(input.with_spans().into_atom_stream()).map(|x| x.0);
         assert_eq!(result,
-                   Ok((Entity::AddOrRetract {
+                   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())),
+                       v: AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("v".into()), Span(44, 47))),
-                   },
+                   }));
-                       &[][..])));
     }
 
     #[test]
     fn test_nested_vector() {
-        let mut list = LinkedList::new();
+        let input = Value::Vector(vec![kw("db", "add"),
-        list.push_back(Value::PlainSymbol(PlainSymbol::new("lookup-ref")));
+                                       Value::List(vec![Value::PlainSymbol(PlainSymbol::new("lookup-ref")),
-        list.push_back(kw("test", "a1"));
+                                                        kw("test", "a1"),
-        list.push_back(Value::Text("v1".into()));
+                                                        Value::Text("v1".into())].into_iter().collect()),
-
-        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())])])];
+                                       Value::Vector(vec![Value::Text("v1".into()), Value::Text("v2".into())])]);
         let mut parser = Tx::entity();
-        let result = parser.parse(&input[..]);
+        let result = parser.parse(input.with_spans().into_atom_stream()).map(|x| x.0);
         assert_eq!(result,
-                   Ok((Entity::AddOrRetract {
+                   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())),
+                       v: AtomOrLookupRefOrVectorOrMapNotation::Vector(vec![AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("v1".into()), Span(45, 49))),
-                                                                            AtomOrLookupRefOrVectorOrMapNotation::Atom(Value::Text("v2".into()))]),
+                                                                            AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("v2".into()), Span(50, 54)))]),
-                   },
+                   }));
-                       &[][..])));
     }
 
     #[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", "id")), AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::Text("t".to_string()), Span(8, 11))));
-        expected.insert(Entid::Ident(NamespacedKeyword::new("db", "ident")), AtomOrLookupRefOrVectorOrMapNotation::Atom(kw("test", "attribute")));
+        expected.insert(Entid::Ident(NamespacedKeyword::new("db", "ident")), AtomOrLookupRefOrVectorOrMapNotation::Atom(ValueAndSpan::new(SpannedValue::NamespacedKeyword(NamespacedKeyword::new("test", "attribute")), Span(22, 37))));
 
         let mut map: BTreeMap<Value, Value> = 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 input = Value::Map(map.clone());
+
         let mut parser = Tx::entity();
-        let result = parser.parse(&input[..]);
+        let result = parser.parse(input.with_spans().into_atom_stream()).map(|x| x.0);
         assert_eq!(result,
-                   Ok((Entity::MapNotation(expected),
+                   Ok(Entity::MapNotation(expected)));
-                       &[][..])));
     }
 }

tx-parser/tests/parser.rs

@@ -15,7 +15,6 @@ extern crate mentat_tx_parser;
 
 use edn::parse;
 use edn::symbols::NamespacedKeyword;
-use edn::types::Value;
 use mentat_tx::entities::{
     AtomOrLookupRefOrVectorOrMapNotation,
     Entid,
@@ -33,29 +32,28 @@ fn test_entities() {
  [:db/add "tempid" :test/a "v"]
  [:db/retract 102 :test/b "w"]]"#;
 
-    let edn = parse::value(input).unwrap().without_spans();
+    let edn = parse::value(input).expect("to parse test input");
-    let input = [edn];
 
-    let result = Tx::parse(&input[..]);
+    let result = Tx::parse(edn);
     assert_eq!(result.unwrap(),
                vec![
                    Entity::AddOrRetract {
                        op: OpType::Add,
                        e: EntidOrLookupRefOrTempId::Entid(Entid::Entid(101)),
                        a: Entid::Ident(NamespacedKeyword::new("test", "a")),
-                       v: AtomOrLookupRefOrVectorOrMapNotation::Atom(Value::Text("v".into())),
+                       v: AtomOrLookupRefOrVectorOrMapNotation::Atom(edn::ValueAndSpan::new(edn::SpannedValue::Text("v".into()), edn::Span(23, 26))),
                    },
                    Entity::AddOrRetract {
                        op: OpType::Add,
                        e: EntidOrLookupRefOrTempId::TempId(TempId::External("tempid".into())),
                        a: Entid::Ident(NamespacedKeyword::new("test", "a")),
-                       v: AtomOrLookupRefOrVectorOrMapNotation::Atom(Value::Text("v".into())),
+                       v: AtomOrLookupRefOrVectorOrMapNotation::Atom(edn::ValueAndSpan::new(edn::SpannedValue::Text("v".into()), edn::Span(55, 58))),
                    },
                    Entity::AddOrRetract {
                        op: OpType::Retract,
                        e: EntidOrLookupRefOrTempId::Entid(Entid::Entid(102)),
                        a: Entid::Ident(NamespacedKeyword::new("test", "b")),
-                       v: AtomOrLookupRefOrVectorOrMapNotation::Atom(Value::Text("w".into())),
+                       v: AtomOrLookupRefOrVectorOrMapNotation::Atom(edn::ValueAndSpan::new(edn::SpannedValue::Text("w".into()), edn::Span(86, 89))),
                    },
                ]);
 }

tx/src/entities.rs

@@ -15,7 +15,6 @@ extern crate edn;
 use std::collections::BTreeMap;
 use std::fmt;
 
-use self::edn::types::Value;
 use self::edn::symbols::NamespacedKeyword;
 
 /// A tempid, either an external tempid given in a transaction (usually as an `edn::Value::Text`),
@@ -62,14 +61,14 @@ pub struct LookupRef {
     pub a: Entid,
     // In theory we could allow nested lookup-refs.  In practice this would require us to process
     // lookup-refs in multiple phases, like how we resolve tempids, which isn't worth the effort.
-    pub v: Value, // An atom.
+    pub v: edn::Value, // An atom.
 }
 
 pub type MapNotation = BTreeMap<Entid, AtomOrLookupRefOrVectorOrMapNotation>;
 
 #[derive(Clone, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)]
 pub enum AtomOrLookupRefOrVectorOrMapNotation {
-    Atom(Value),
+    Atom(edn::ValueAndSpan),
     LookupRef(LookupRef),
     Vector(Vec<AtomOrLookupRefOrVectorOrMapNotation>),
     MapNotation(MapNotation),