Post: Reject at parse-time reversed attributes in direct notation with bad values.

This is an optimization that trades rejecting inputs earlier at the
cost of expressive error messages.  It should be possible to recover
the error messages, however.

This will reject input like `[:db/{add,retract} v :attribute/_reversed NOT-AN-ENTITY]`.

db/src/db.rs

@@ -2085,7 +2085,7 @@ mod tests {
 
         // Verify that we can explode map notation with nested maps, even if the inner map would be
         // dangling, if we give a :db/id explicitly.
-        assert_transact!(conn, "[{:test/dangling {:db/id \"t\" :test/many 11}}]");
+        assert_transact!(conn, "[{:test/dangling {:db/id \"t\" :test/many 12}}]");
     }
 
     #[test]
@@ -2191,21 +2191,35 @@ mod tests {
         assert_matches!(tempids(&report),
                         "{}");
 
-        // Verify that we can't explode direct reverse notation with nested value maps.
-        assert_transact!(conn,
-                         "[[:db/add \"t\" :test/_dangling {:test/many 11}]]",
-                         Err("not yet implemented: Cannot explode map notation value in :attr/_reversed notation for attribute 444"));
+    }
+
+    #[test]
+    fn test_explode_reversed_notation_errors() {
+        let mut conn = TestConn::default();
+
+        // Start by installing a few attributes.
+        assert_transact!(conn, "[[:db/add 111 :db/ident :test/many]
+                                 [:db/add 111 :db/valueType :db.type/long]
+                                 [:db/add 111 :db/cardinality :db.cardinality/many]
+                                 [:db/add 222 :db/ident :test/component]
+                                 [:db/add 222 :db/isComponent true]
+                                 [:db/add 222 :db/valueType :db.type/ref]
+                                 [:db/add 333 :db/ident :test/unique]
+                                 [:db/add 333 :db/unique :db.unique/identity]
+                                 [:db/add 333 :db/index true]
+                                 [:db/add 333 :db/valueType :db.type/long]
+                                 [:db/add 444 :db/ident :test/dangling]
+                                 [:db/add 444 :db/valueType :db.type/ref]]");
+
+        // `tx-parser` should fail to parse direct reverse notation with nested value maps and
+        // nested value vectors, so we only test things that "get through" to the map notation
+        // dynamic processor here.
 
         // Verify that we can't explode reverse notation in map notation with nested value maps.
         assert_transact!(conn,
-                         "[{:test/_dangling {:test/many 11}}]",
+                         "[{:test/_dangling {:test/many 14}}]",
                          Err("not yet implemented: Cannot explode map notation value in :attr/_reversed notation for attribute 444"));
 
-        // Verify that we can't explode direct reverse notation with nested value vectors.
-        assert_transact!(conn,
-                         "[[:db/add \"t\" :test/_dangling [:test/many]]]",
-                         Err("not yet implemented: Cannot explode vector value in :attr/_reversed notation for attribute 444"));
-
         // Verify that we can't explode reverse notation in map notation with nested value vectors.
         assert_transact!(conn,
                          "[{:test/_dangling [:test/many]}]",

parser-utils/src/value_and_span.rs

@@ -452,6 +452,26 @@ pub fn namespaced_keyword<'a>() -> Expected<FnParser<Stream<'a>, fn(Stream<'a>)
     parser(namespaced_keyword_ as fn(Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>>).expected("namespaced_keyword")
 }
 
+pub fn forward_keyword_<'a>(input: Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>> {
+    satisfy_map(|v: &'a edn::ValueAndSpan| v.inner.as_namespaced_keyword().and_then(|k| if k.is_forward() { Some(k) } else { None }))
+        .parse_lazy(input)
+        .into()
+}
+
+pub fn forward_keyword<'a>() -> Expected<FnParser<Stream<'a>, fn(Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>>>> {
+    parser(forward_keyword_ as fn(Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>>).expected("forward_keyword")
+}
+
+pub fn backward_keyword_<'a>(input: Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>> {
+    satisfy_map(|v: &'a edn::ValueAndSpan| v.inner.as_namespaced_keyword().and_then(|k| if k.is_backward() { Some(k) } else { None }))
+        .parse_lazy(input)
+        .into()
+}
+
+pub fn backward_keyword<'a>() -> Expected<FnParser<Stream<'a>, fn(Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>>>> {
+    parser(backward_keyword_ as fn(Stream<'a>) -> ParseResult<&'a edn::NamespacedKeyword, Stream<'a>>).expected("backward_keyword")
+}
+
 /// 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`

tx-parser/src/lib.rs

@@ -45,6 +45,8 @@ use mentat_parser_utils::{ResultParser};
 use mentat_parser_utils::value_and_span::{
     Item,
     OfExactlyParsing,
+    backward_keyword,
+    forward_keyword,
     integer,
     list,
     map,
@@ -57,12 +59,25 @@ pub use errors::*;
 
 pub struct Tx<'a>(std::marker::PhantomData<&'a ()>);
 
+// Accepts entid, :attribute/forward, and :attribute/_backward.
 def_parser!(Tx, entid, Entid, {
     integer()
         .map(|x| Entid::Entid(x))
         .or(namespaced_keyword().map(|x| Entid::Ident(x.clone())))
 });
 
+// Accepts entid and :attribute/forward.
+def_parser!(Tx, forward_entid, Entid, {
+    integer()
+        .map(|x| Entid::Entid(x))
+        .or(forward_keyword().map(|x| Entid::Ident(x.clone())))
+});
+
+// Accepts only :attribute/_backward.
+def_parser!(Tx, backward_entid, Entid, {
+    backward_keyword().map(|x| Entid::Ident(x.to_reversed()))
+});
+
 def_matches_plain_symbol!(Tx, literal_lookup_ref, "lookup-ref");
 
 def_parser!(Tx, lookup_ref, LookupRef, {
@@ -106,11 +121,16 @@ def_matches_namespaced_keyword!(Tx, literal_db_retract, "db", "retract");
 
 def_parser!(Tx, add_or_retract, Entity, {
     vector().of_exactly(
+        // TODO: This commits as soon as it sees :db/{add,retract}, but could use an improved error message.
         (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)| {
+          try((Tx::entid_or_lookup_ref_or_temp_id(),
+               Tx::forward_entid(),
+               Tx::atom_or_lookup_ref_or_vector()))
+          .or(try((Tx::atom_or_lookup_ref_or_vector(),
+                   Tx::backward_entid(),
+                   Tx::entid_or_lookup_ref_or_temp_id()))
+              .map(|(v, a, e)| (e, a, v))))
+            .map(|(op, (e, a, v))| {
                 Entity::AddOrRetract {
                     op: op,
                     e: e,

tx-parser/tests/parser.rs

@@ -85,4 +85,22 @@ fn test_entities() {
                ]);
 }
 
+#[test]
+fn test_reverse_notation_illegal_nested_values() {
+    // Verify that we refuse to parse direct reverse notation with nested value maps or vectors.
+
+    let input = "[[:db/add 100 :test/_dangling {:test/many 13}]]";
+    let edn = parse::value(input).expect("to parse test input");
+    let result = Tx::parse(&edn);
+    // TODO: it would be much better to assert details about the error (here and below), but right
+    // now the error message isn't clear that the given value isn't valid for the backward attribute
+    // :test/_dangling.
+    assert!(result.is_err());
+
+    let input = "[[:db/add 100 :test/_dangling [:test/many 13]]]";
+    let edn = parse::value(input).expect("to parse test input");
+    let result = Tx::parse(&edn);
+    assert!(result.is_err());
+}
+
 // TODO: test error handling in select cases.