Rebase against rust branch.

* Remove `WhereNotClause` and populate `NotJoin` with `WhereClause`.
* Fix validation for `not` and `not-join`, removing tests that were invalid.
* Address rustification comments.

parser-utils/src/lib.rs

@@ -38,8 +38,20 @@ pub type ResultParser<O, I> = Expected<FnParser<I, fn(I) -> ParseResult<O, I>>>;
 #[macro_export]
 macro_rules! assert_parses_to {
     ( $parser: expr, $input: expr, $expected: expr ) => {{
-        let mut par = $parser();
-        let result = par.parse($input.with_spans().into_atom_stream()).map(|x| x.0); // TODO: check remainder of stream.
+        let par = $parser();
+        let result = par.skip(eof()).parse($input.with_spans().into_atom_stream()).map(|x| x.0);
+        assert_eq!(result, Ok($expected));
+    }}
+}
+
+/// `assert_edn_parses_to!` simplifies some of the boilerplate around running a parser function
+/// against string input and expecting a certain result.
+#[macro_export]
+macro_rules! assert_edn_parses_to {
+    ( $parser: expr, $input: expr, $expected: expr ) => {{
+        let par = $parser();
+        let input = edn::parse::value($input).expect("to be able to parse input as EDN");
+        let result = par.skip(eof()).parse(input.into_atom_stream()).map(|x| x.0);
         assert_eq!(result, Ok($expected));
     }}
 }

query-algebrizer/src/clauses/mod.rs

@@ -56,11 +56,15 @@ use types::{
 };
 
 mod or;
+mod not;
 mod pattern;
 mod predicate;
 mod resolve;
 
-use validate::validate_or_join;
+use validate::{
+    validate_not_join,
+    validate_or_join,
+};
 
 // We do this a lot for errors.
 trait RcCloned<T> {
@@ -586,6 +590,9 @@ impl ConjoiningClauses {
                 //?;
                 //self.apply_or_join(schema, o)
             },
+            WhereClause::NotJoin(n) => {
+                validate_not_join(&n)
+            },
             _ => unimplemented!(),
         }
     }

query-algebrizer/src/errors.rs

@@ -45,6 +45,12 @@ error_chain! {
             description("non-matching variables in 'or' clause")
             display("non-matching variables in 'or' clause")
         }
+
+        NonMatchingVariablesInNotClause {
+            // TODO: flesh out.
+            description("non-matching variables in 'not' clause")
+            display("non-matching variables in 'not' clause")
+        }
     }
 }
 

query-algebrizer/src/validate.rs

@@ -13,6 +13,7 @@ use std::collections::BTreeSet;
 use mentat_query::{
     ContainsVariables,
     OrJoin,
+    NotJoin,
     Variable,
     UnifyVars,
 };
@@ -74,6 +75,23 @@ pub fn validate_or_join(or_join: &OrJoin) -> Result<()> {
     }
 }
 
+pub fn validate_not_join(not_join: &NotJoin) -> Result<()> {
+    // Grab our mentioned variables and ensure that the rules are followed.
+    match not_join.unify_vars {
+        UnifyVars::Implicit => {
+            Ok(())
+        },
+        UnifyVars::Explicit(ref vars) => {
+            // The joined vars must each appear somewhere in the clauses mentioned variables.
+            let var_set: BTreeSet<Variable> = vars.iter().cloned().collect();
+            if !var_set.is_subset(&not_join.collect_mentioned_variables()) {
+                bail!(ErrorKind::NonMatchingVariablesInNotClause);
+            }
+            Ok(())
+        },
+    }
+}
+
 #[cfg(test)]
 mod tests {
     extern crate mentat_core;
@@ -96,7 +114,10 @@ mod tests {
 
     use clauses::ident;
 
-    use super::validate_or_join;
+    use super::{
+        validate_not_join,
+        validate_or_join,
+    };
 
     fn value_ident(ns: &str, name: &str) -> PatternValuePlace {
         PatternValuePlace::IdentOrKeyword(::std::rc::Rc::new(NamespacedKeyword::new(ns, name)))
@@ -229,4 +250,125 @@ mod tests {
             _ => panic!(),
         };
     }
+
+
+    /// Tests that the top-level form is a valid `not`, returning the clauses.
+    fn valid_not_join(parsed: FindQuery, expected_unify: UnifyVars) -> Vec<WhereClause> {
+        // Filter out all the clauses that are not `not`s.
+        let mut nots = parsed.where_clauses.into_iter().filter(|x| match x {
+            &WhereClause::NotJoin(_) => true,
+            _ => false,
+        });
+
+        // There should be only one not clause.
+        let clause = nots.next().unwrap();
+        assert_eq!(None, nots.next());
+
+        match clause {
+            WhereClause::NotJoin(not_join) => {
+                // It's valid: the variables are the same in each branch.
+                assert_eq!((), validate_not_join(&not_join).unwrap());
+                assert_eq!(expected_unify, not_join.unify_vars);
+                not_join.clauses
+            },
+            _ => panic!(),
+        }
+    }
+
+    /// Test that a `not` is valid if it is implicit.
+    #[test]
+    fn test_success_not() {
+        let query = r#"[:find ?name
+                        :where [?id :artist/name ?name]
+                            (not [?id :artist/country :country/CA]
+                                 [?id :artist/country :country/GB])]"#;
+        let parsed = parse_find_string(query).expect("expected successful parse");
+        let clauses = valid_not_join(parsed, UnifyVars::Implicit);
+
+        // Check each part of the body
+        let mut parts = clauses.into_iter();
+        match (parts.next(), parts.next(), parts.next()) {
+            (Some(clause1), Some(clause2), None) => {
+                assert_eq!(
+                    clause1,
+                    WhereClause::Pattern(Pattern {
+                        source: None,
+                        entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?id")),
+                        attribute: ident("artist", "country"),
+                        value: value_ident("country", "CA"),
+                        tx: PatternNonValuePlace::Placeholder,
+                    }));
+                assert_eq!(
+                    clause2,
+                    WhereClause::Pattern(Pattern {
+                        source: None,
+                        entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?id")),
+                        attribute: ident("artist", "country"),
+                        value: value_ident("country", "GB"),
+                        tx: PatternNonValuePlace::Placeholder,
+                    }));
+            },
+            _ => panic!(),
+        };
+    }
+
+    #[test]
+    fn test_success_not_join() {
+        let query = r#"[:find ?artist
+                        :where [?artist :artist/name]
+                               (not-join [?artist]
+                                   [?release :release/artists ?artist]
+                                   [?release :release/year 1970])]"#;
+        let parsed = parse_find_string(query).expect("expected successful parse");
+        let clauses = valid_not_join(parsed, UnifyVars::Explicit(vec![Variable::from_valid_name("?artist")]));
+
+        // Let's do some detailed parse checks.
+        let mut parts = clauses.into_iter();
+        match (parts.next(), parts.next(), parts.next()) {
+            (Some(clause1), Some(clause2), None) => {
+                assert_eq!(
+                    clause1,
+                    WhereClause::Pattern(Pattern {
+                        source: None,
+                        entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?release")),
+                        attribute: ident("release", "artists"),
+                        value: PatternValuePlace::Variable(Variable::from_valid_name("?artist")),
+                        tx: PatternNonValuePlace::Placeholder,
+                    }));
+                assert_eq!(
+                    clause2,
+                    WhereClause::Pattern(Pattern {
+                        source: None,
+                        entity: PatternNonValuePlace::Variable(Variable::from_valid_name("?release")),
+                        attribute: ident("release", "year"),
+                        value: PatternValuePlace::EntidOrInteger(1970),
+                        tx: PatternNonValuePlace::Placeholder,
+                    }));
+            },
+            _ => panic!(),
+        };
+    }
+    
+    /// Test that a `not-join` that does not use the joining var fails to validate.
+    #[test]
+    fn test_invalid_explicit_not_join_non_matching_join_vars() {
+        let query = r#"[:find ?artist
+                        :where [?artist :artist/name]
+                               (not-join [?artist]
+                                   [?release :release/artists "Pink Floyd"]
+                                   [?release :release/year 1970])]"#;
+        let parsed = parse_find_string(query).expect("expected successful parse");
+        let mut nots = parsed.where_clauses.iter().filter(|&x| match *x {
+            WhereClause::NotJoin(_) => true,
+            _ => false,
+        });
+
+        let clause = nots.next().unwrap().clone();
+        assert_eq!(None, nots.next());
+
+        match clause {
+            WhereClause::NotJoin(not_join) => assert!(validate_not_join(&not_join).is_err()),
+            _ => panic!(),
+        }
+    }    
 }

query-parser/src/parse.rs

@@ -42,6 +42,7 @@ use self::mentat_query::{
     FromValue,
     OrJoin,
     OrWhereClause,
+    NotJoin,
     Pattern,
     PatternNonValuePlace,
     PatternValuePlace,
@@ -135,6 +136,10 @@ def_matches_plain_symbol!(Where, or, "or");
 
 def_matches_plain_symbol!(Where, or_join, "or-join");
 
+def_matches_plain_symbol!(Where, not, "not");
+
+def_matches_plain_symbol!(Where, not_join, "not-join");
+
 def_parser!(Where, rule_vars, Vec<Variable>, {
     seq()
         .of_exactly(many1(Query::variable()))
@@ -182,6 +187,33 @@ def_parser!(Where, or_join_clause, WhereClause, {
             }))
 });
 
+def_parser!(Where, not_clause, WhereClause, {
+    seq()
+        .of_exactly(Where::not()
+            .with(many1(Where::clause()))
+            .map(|clauses| {
+                WhereClause::NotJoin(
+                    NotJoin {
+                        unify_vars: UnifyVars::Implicit,
+                        clauses: clauses,
+                    })
+            }))
+});
+
+def_parser!(Where, not_join_clause, WhereClause, {
+    seq()
+        .of_exactly(Where::not_join()
+            .with(Where::rule_vars())
+            .and(many1(Where::clause()))
+            .map(|(vars, clauses)| {
+                WhereClause::NotJoin(
+                    NotJoin {
+                        unify_vars: UnifyVars::Explicit(vars),
+                        clauses: clauses,
+                    })
+            }))
+});
+
 /// A vector containing just a parenthesized filter expression.
 def_parser!(Where, pred, WhereClause, {
     // Accept either a nested list or a nested vector here:
@@ -246,6 +278,8 @@ def_parser!(Where, clause, WhereClause, {
             // We don't yet handle source vars.
             try(Where::or_join_clause()),
             try(Where::or_clause()),
+            try(Where::not_join_clause()),
+            try(Where::not_clause()),
 
             try(Where::pred()),
     ])
@@ -548,6 +582,49 @@ mod test {
                               }));
     }
 
+    #[test]
+    fn test_not() {
+        let e = edn::PlainSymbol::new("?e");
+        let a = edn::PlainSymbol::new("?a");
+        let v = edn::PlainSymbol::new("?v");
+
+        assert_edn_parses_to!(Where::not_clause,
+                              "(not [?e ?a ?v])",
+                              WhereClause::NotJoin(
+                              NotJoin {
+                                  unify_vars: UnifyVars::Implicit,
+                                  clauses: vec![
+                                      WhereClause::Pattern(Pattern {
+                                          source: None,
+                                          entity: PatternNonValuePlace::Variable(variable(e)),
+                                          attribute: PatternNonValuePlace::Variable(variable(a)),
+                                          value: PatternValuePlace::Variable(variable(v)),
+                                          tx: PatternNonValuePlace::Placeholder,
+                                      })],
+                              }));
+    }
+
+    #[test]
+    fn test_not_join() {
+        let e = edn::PlainSymbol::new("?e");
+        let a = edn::PlainSymbol::new("?a");
+        let v = edn::PlainSymbol::new("?v");
+        
+        assert_edn_parses_to!(Where::not_join_clause,
+                              "(not-join [?e] [?e ?a ?v])",
+                              WhereClause::NotJoin(
+                              NotJoin {
+                                  unify_vars: UnifyVars::Explicit(vec![variable(e.clone())]),
+                                  clauses: vec![WhereClause::Pattern(Pattern {
+                                          source: None,
+                                          entity: PatternNonValuePlace::Variable(variable(e)),
+                                          attribute: PatternNonValuePlace::Variable(variable(a)),
+                                          value: PatternValuePlace::Variable(variable(v)),
+                                          tx: PatternNonValuePlace::Placeholder,
+                                      })],
+                              }));
+    }
+
     #[test]
     fn test_find_sp_variable() {
         let sym = edn::PlainSymbol::new("?x");

query/src/lib.rs

@@ -570,11 +570,16 @@ pub struct OrJoin {
     pub clauses: Vec<OrWhereClause>,
 }
 
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct NotJoin {
+    pub unify_vars: UnifyVars,
+    pub clauses: Vec<WhereClause>,
+}
+
 #[allow(dead_code)]
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub enum WhereClause {
-    Not,
-    NotJoin,
+    NotJoin(NotJoin),
     OrJoin(OrJoin),
     Pred(Predicate),
     WhereFn,
@@ -628,8 +633,7 @@ impl ContainsVariables for WhereClause {
             &OrJoin(ref o)  => o.accumulate_mentioned_variables(acc),
             &Pred(ref p)    => p.accumulate_mentioned_variables(acc),
             &Pattern(ref p) => p.accumulate_mentioned_variables(acc),
-            &Not            => (),
-            &NotJoin        => (),
+            &NotJoin(ref n) => n.accumulate_mentioned_variables(acc),
             &WhereFn        => (),
             &RuleExpr       => (),
         }
@@ -654,6 +658,14 @@ impl ContainsVariables for OrJoin {
     }
 }
 
+impl ContainsVariables for NotJoin {
+    fn accumulate_mentioned_variables(&self, acc: &mut BTreeSet<Variable>) {
+        for clause in &self.clauses {
+            clause.accumulate_mentioned_variables(acc);
+        }
+    }
+}
+
 impl ContainsVariables for Predicate {
     fn accumulate_mentioned_variables(&self, acc: &mut BTreeSet<Variable>) {
         for arg in &self.args {