(tx) Replace :db/tx with (transaction-tx) transaction function and broaden support. (#664)

:db/tx (and Datomic's version, :datomic/tx) suffer from the same
ambiguities that [a v] lookup references do -- determining the type of
the result is context sensitive.  (In this case, is :db/tx a reference
to the current transaction ID, or is it a valid keyword?)  This commit
addresses the ambiguity by introducing a notion of a transaction
functions, and provides a little scaffolding for adding more (should
the need arise).  I left the scaffolding in place rather than handling
just (transaction-tx) because I started trying to
implement (transaction-instant) as well, which is more difficult --
see the comments.

It's worth noting that this approach generalizes more or less directly
to ?input variables, since those can be eagerly bound like the
implemented transaction function (transaction-tx).

db/src/db.rs

@@ -1376,7 +1376,7 @@ mod tests {
         let mut conn = TestConn::default();
 
         // Test that txInstant can be asserted.
-        assert_transact!(conn, "[[:db/add :db/tx :db/txInstant #inst \"2017-06-16T00:56:41.257Z\"]
+        assert_transact!(conn, "[[:db/add (transaction-tx) :db/txInstant #inst \"2017-06-16T00:56:41.257Z\"]
                                  [:db/add 100 :db/ident :name/Ivan]
                                  [:db/add 101 :db/ident :name/Petr]]");
         assert_matches!(conn.last_transaction(),
@@ -1385,14 +1385,14 @@ mod tests {
                           [?tx :db/txInstant #inst \"2017-06-16T00:56:41.257Z\" ?tx true]]");
 
         // Test multiple txInstant with different values should fail.
-        assert_transact!(conn, "[[:db/add :db/tx :db/txInstant #inst \"2017-06-16T00:59:11.257Z\"]
+        assert_transact!(conn, "[[:db/add (transaction-tx) :db/txInstant #inst \"2017-06-16T00:59:11.257Z\"]
-                                 [:db/add :db/tx :db/txInstant #inst \"2017-06-16T00:59:11.752Z\"]
+                                 [:db/add (transaction-tx) :db/txInstant #inst \"2017-06-16T00:59:11.752Z\"]
                                  [:db/add 102 :db/ident :name/Vlad]]",
                          Err("conflicting datoms in tx"));
 
         // Test multiple txInstants with the same value.
-        assert_transact!(conn, "[[:db/add :db/tx :db/txInstant #inst \"2017-06-16T00:59:11.257Z\"]
+        assert_transact!(conn, "[[:db/add (transaction-tx) :db/txInstant #inst \"2017-06-16T00:59:11.257Z\"]
-                                 [:db/add :db/tx :db/txInstant #inst \"2017-06-16T00:59:11.257Z\"]
+                                 [:db/add (transaction-tx) :db/txInstant #inst \"2017-06-16T00:59:11.257Z\"]
                                  [:db/add 103 :db/ident :name/Dimitri]
                                  [:db/add 104 :db/ident :name/Anton]]");
         assert_matches!(conn.last_transaction(),
@@ -1400,11 +1400,34 @@ mod tests {
                           [104 :db/ident :name/Anton ?tx true]
                           [?tx :db/txInstant #inst \"2017-06-16T00:59:11.257Z\" ?tx true]]");
 
-        // Test txInstant retraction
+        // We need a few attributes to work with.
-        // Test disabled: retracting a datom that doesn't exist should fail.
+        assert_transact!(conn, "[[:db/add 111 :db/ident :test/str]
-        // assert_transact!(conn, "[[:db/retract :db/tx :db/txInstant #inst \"2017-06-16T00:59:11.257Z\"]
+                                 [:db/add 111 :db/valueType :db.type/string]
-        //                          [:db/add 105 :db/ident :name/Vadim]]",
+                                 [:db/add 222 :db/ident :test/ref]
-        //                  Err("Should fail!"));
+                                 [:db/add 222 :db/valueType :db.type/ref]]");
+
+        // Test that we can assert metadata about the current transaction.
+        assert_transact!(conn, "[[:db/add (transaction-tx) :test/str \"We want metadata!\"]]");
+        assert_matches!(conn.last_transaction(),
+                        "[[?tx :db/txInstant ?ms ?tx true]
+                          [?tx :test/str \"We want metadata!\" ?tx true]]");
+
+        // Test that we can use (transaction-tx) as a value.
+        assert_transact!(conn, "[[:db/add 333 :test/ref (transaction-tx)]]");
+        assert_matches!(conn.last_transaction(),
+                        "[[333 :test/ref ?tx ?tx true]
+                          [?tx :db/txInstant ?ms ?tx true]]");
+
+        // Test that we type-check properly.  In the value position, (transaction-tx) yields a ref;
+        // :db/ident expects a keyword.
+        assert_transact!(conn, "[[:db/add 444 :db/ident (transaction-tx)]]",
+                         Err("not yet implemented: Transaction function transaction-tx produced value of type :db.type/ref but expected type :db.type/keyword"));
+
+        // Test that we can assert metadata about the current transaction.
+        assert_transact!(conn, "[[:db/add (transaction-tx) :test/ref (transaction-tx)]]");
+        assert_matches!(conn.last_transaction(),
+                        "[[?tx :db/txInstant ?ms ?tx true]
+                          [?tx :test/ref ?tx ?tx true]]");
     }
 
     #[test]

db/src/debug.rs

@@ -172,7 +172,7 @@ pub(crate) fn datoms_after<S: Borrow<Schema>>(conn: &rusqlite::Connection, schem
 /// Return the sequence of transactions in the store with transaction ID strictly greater than the
 /// given `tx`, ordered by (tx, e, a, v).
 ///
-/// Each transaction returned includes the [:db/tx :db/txInstant ...] datom.
+/// Each transaction returned includes the [(transaction-tx) :db/txInstant ...] datom.
 pub(crate) fn transactions_after<S: Borrow<Schema>>(conn: &rusqlite::Connection, schema: &S, tx: i64) -> Result<Transactions> {
     let borrowed_schema = schema.borrow();
 

db/src/tx.rs

@@ -282,11 +282,6 @@ impl<'conn, 'a, W> Tx<'conn, 'a, W> where W: TransactWatcher {
                         Ok(Either::Left(e))
                     },
 
-                    // Special case: current tx ID.
-                    entmod::EntidOrLookupRefOrTempId::TempId(TempId::Tx) => {
-                        Ok(Either::Left(self.tx_id))
-                    },
-
                     entmod::EntidOrLookupRefOrTempId::TempId(e) => {
                         Ok(Either::Right(LookupRefOrTempId::TempId(self.intern_temp_id(e))))
                     },
@@ -294,6 +289,13 @@ impl<'conn, 'a, W> Tx<'conn, 'a, W> where W: TransactWatcher {
                     entmod::EntidOrLookupRefOrTempId::LookupRef(ref lookup_ref) => {
                         Ok(Either::Right(LookupRefOrTempId::LookupRef(self.intern_lookup_ref(lookup_ref)?)))
                     },
+
+                    entmod::EntidOrLookupRefOrTempId::TxFunction(ref tx_function) => {
+                        match tx_function.op.0.as_str() {
+                            "transaction-tx" => Ok(Either::Left(self.tx_id)),
+                            unknown @ _ => bail!(ErrorKind::NotYetImplemented(format!("Unknown transaction function {}", unknown))),
+                        }
+                    },
                 }
             }
 
@@ -341,6 +343,13 @@ impl<'conn, 'a, W> Tx<'conn, 'a, W> where W: TransactWatcher {
                             entmod::AtomOrLookupRefOrVectorOrMapNotation::LookupRef(ref lookup_ref) =>
                                 Ok(Either::Right(LookupRefOrTempId::LookupRef(self.intern_lookup_ref(lookup_ref)?))),
 
+                            entmod::AtomOrLookupRefOrVectorOrMapNotation::TxFunction(ref tx_function) => {
+                                match tx_function.op.0.as_str() {
+                                    "transaction-tx" => Ok(Either::Left(KnownEntid(self.tx_id.0))),
+                                    unknown @ _ => bail!(ErrorKind::NotYetImplemented(format!("Unknown transaction function {}", unknown))),
+                                }
+                            },
+
                             entmod::AtomOrLookupRefOrVectorOrMapNotation::Vector(_) =>
                                 bail!(ErrorKind::NotYetImplemented(format!("Cannot explode vector value in :attr/_reversed notation for attribute {}", forward_a))),
 
@@ -412,6 +421,26 @@ impl<'conn, 'a, W> Tx<'conn, 'a, W> where W: TransactWatcher {
                                 Either::Right(LookupRefOrTempId::LookupRef(in_process.intern_lookup_ref(lookup_ref)?))
                             },
 
+                            entmod::AtomOrLookupRefOrVectorOrMapNotation::TxFunction(ref tx_function) => {
+                                let typed_value = match tx_function.op.0.as_str() {
+                                    "transaction-tx" => TypedValue::Ref(self.tx_id),
+                                    unknown @ _ => bail!(ErrorKind::NotYetImplemented(format!("Unknown transaction function {}", unknown))),
+                                };
+
+                                // Here we do schema-aware typechecking: we assert that the computed
+                                // value is in the attribute's value set.  If and when we have
+                                // transaction functions that produce numeric values, we'll have to
+                                // be more careful here, because a function that produces an integer
+                                // value can be used where a double is expected.  See also
+                                // `SchemaTypeChecking.to_typed_value(...)`.
+                                if attribute.value_type != typed_value.value_type() {
+                                    bail!(ErrorKind::NotYetImplemented(format!("Transaction function {} produced value of type {} but expected type {}",
+                                                                               tx_function.op.0.as_str(), typed_value.value_type(), attribute.value_type)));
+                                }
+
+                                Either::Left(typed_value)
+                            },
+
                             entmod::AtomOrLookupRefOrVectorOrMapNotation::Vector(vs) => {
                                 if !attribute.multival {
                                     bail!(ErrorKind::NotYetImplemented(format!("Cannot explode vector value for attribute {} that is not :db.cardinality :db.cardinality/many", a)));
@@ -682,7 +711,7 @@ impl<'conn, 'a, W> Tx<'conn, 'a, W> where W: TransactWatcher {
 
         tx_instant = self.tx_instant.unwrap_or_else(now);
 
-        // Transact [:db/add :db/txInstant tx_instant :db/tx].
+        // Transact [:db/add :db/txInstant tx_instant (transaction-tx)].
         non_fts_one.push((self.tx_id,
                           entids::DB_TX_INSTANT,
                           self.schema.require_attribute_for_entid(entids::DB_TX_INSTANT).unwrap(),

tx-parser/src/lib.rs

@@ -40,6 +40,7 @@ use mentat_tx::entities::{
     MapNotation,
     OpType,
     TempId,
+    TxFunction,
 };
 use mentat_parser_utils::{ResultParser};
 use mentat_parser_utils::value_and_span::{
@@ -89,16 +90,18 @@ def_parser!(Tx, lookup_ref, LookupRef, {
 });
 
 def_parser!(Tx, entid_or_lookup_ref_or_temp_id, EntidOrLookupRefOrTempId, {
-    Tx::db_tx().map(EntidOrLookupRefOrTempId::TempId)
+    Tx::temp_id().map(EntidOrLookupRefOrTempId::TempId)
         .or(Tx::entid().map(EntidOrLookupRefOrTempId::Entid))
-        .or(Tx::lookup_ref().map(EntidOrLookupRefOrTempId::LookupRef))
+        .or(try(Tx::lookup_ref().map(EntidOrLookupRefOrTempId::LookupRef)))
-        .or(Tx::temp_id().map(EntidOrLookupRefOrTempId::TempId))
+        .or(try(Tx::tx_function().map(EntidOrLookupRefOrTempId::TxFunction)))
 });
 
-def_matches_namespaced_keyword!(Tx, literal_db_tx, "db", "tx");
+def_matches_plain_symbol!(Tx, literal_transaction_tx, "transaction-tx");
 
-def_parser!(Tx, db_tx, TempId, {
+def_parser!(Tx, tx_function, TxFunction, {
-    Tx::literal_db_tx().map(|_| TempId::Tx)
+    list().of_exactly(
+        Tx::literal_transaction_tx().map(|_| edn::PlainSymbol::new("transaction-tx"))
+            .map(|op| TxFunction { op: op }))
 });
 
 def_parser!(Tx, temp_id, TempId, {
@@ -114,8 +117,9 @@ def_parser!(Tx, nested_vector, Vec<AtomOrLookupRefOrVectorOrMapNotation>, {
 });
 
 def_parser!(Tx, atom_or_lookup_ref_or_vector, AtomOrLookupRefOrVectorOrMapNotation, {
-    choice::<[&mut Parser<Input = _, Output = AtomOrLookupRefOrVectorOrMapNotation>; 4], _>
+    choice::<[&mut Parser<Input = _, Output = AtomOrLookupRefOrVectorOrMapNotation>; 5], _>
         ([&mut try(Tx::lookup_ref().map(AtomOrLookupRefOrVectorOrMapNotation::LookupRef)),
+          &mut try(Tx::tx_function().map(AtomOrLookupRefOrVectorOrMapNotation::TxFunction)),
           &mut Tx::nested_vector().map(AtomOrLookupRefOrVectorOrMapNotation::Vector),
           &mut Tx::map_notation().map(AtomOrLookupRefOrVectorOrMapNotation::MapNotation),
           &mut Tx::atom().map(|x| x.clone()).map(AtomOrLookupRefOrVectorOrMapNotation::Atom)
@@ -198,6 +202,7 @@ pub fn remove_db_id(map: &mut MapNotation) -> std::result::Result<Option<EntidOr
                 Some(db_id)
             },
             AtomOrLookupRefOrVectorOrMapNotation::LookupRef(_) |
+            AtomOrLookupRefOrVectorOrMapNotation::TxFunction(_) |
             AtomOrLookupRefOrVectorOrMapNotation::Vector(_) |
             AtomOrLookupRefOrVectorOrMapNotation::MapNotation(_) => {
                 bail!(ErrorKind::DbIdError)

tx/src/entities.rs

@@ -23,14 +23,12 @@ use self::edn::symbols::NamespacedKeyword;
 pub enum TempId {
     External(String),
     Internal(i64),
-    Tx, // Special identifier used to refer to the current transaction.
 }
 
 impl TempId {
     pub fn into_external(self) -> Option<String> {
         match self {
             TempId::External(s) => Some(s),
-            TempId::Tx |
             TempId::Internal(_) => None,
         }
     }
@@ -41,7 +39,6 @@ impl fmt::Display for TempId {
         match self {
             &TempId::External(ref s) => write!(f, "{}", s),
             &TempId::Internal(x) => write!(f, "<tempid {}>", x),
-            &TempId::Tx => write!(f, "<Tx>"),
         }
     }
 }
@@ -69,12 +66,30 @@ pub struct LookupRef {
     pub v: edn::Value, // An atom.
 }
 
+/// A "transaction function" that exposes some value determined by the current transaction.  The
+/// prototypical example is the current transaction ID, `(transaction-tx)`.
+///
+/// A natural next step might be to expose the current transaction instant `(transaction-instant)`,
+/// but that's more difficult: the transaction itself can set the transaction instant (with some
+/// restrictions), so the transaction function must be late-binding.  Right now, that's difficult to
+/// arrange in the transactor.
+///
+/// In the future, we might accept arguments; for example, perhaps we might expose `(ancestor
+/// (transaction-tx) n)` to find the n-th ancestor of the current transaction.  If we do accept
+/// arguments, then the special case of `(lookup-ref a v)` should be handled as part of the
+/// generalization.
+#[derive(Clone, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)]
+pub struct TxFunction {
+    pub op: edn::PlainSymbol,
+}
+
 pub type MapNotation = BTreeMap<Entid, AtomOrLookupRefOrVectorOrMapNotation>;
 
 #[derive(Clone, Debug, Eq, Hash, Ord, PartialOrd, PartialEq)]
 pub enum AtomOrLookupRefOrVectorOrMapNotation {
     Atom(edn::ValueAndSpan),
     LookupRef(LookupRef),
+    TxFunction(TxFunction),
     Vector(Vec<AtomOrLookupRefOrVectorOrMapNotation>),
     MapNotation(MapNotation),
 }
@@ -83,6 +98,7 @@ pub enum AtomOrLookupRefOrVectorOrMapNotation {
 pub enum EntidOrLookupRefOrTempId {
     Entid(Entid),
     LookupRef(LookupRef),
+    TxFunction(TxFunction),
     TempId(TempId),
 }