Address review comments rnewman.

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

query-algebrizer/src/clauses/mod.rs

@@ -62,8 +62,8 @@ mod predicate;
 mod resolve;
 
 use validate::{
-    validate_or_join,
     validate_not_join,
+    validate_or_join,
 };
 
 // We do this a lot for errors.

query-algebrizer/src/validate.rs

@@ -79,37 +79,14 @@ 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 => {
-            if not_join.clauses.len() < 2 {
             Ok(())
-            } else {
-                let mut clauses = not_join.clauses.iter();
-                let template = clauses.next().unwrap().collect_mentioned_variables();
-                for clause in clauses {
-                    if template != clause.collect_mentioned_variables() {
-                        bail!(ErrorKind::NonMatchingVariablesInNotClause);
-                    }
-                }
-                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();
-            for var in &var_set {
+            if !var_set.is_subset(&not_join.collect_mentioned_variables()) {
-                if !&not_join.clauses.iter().any(|clause| clause.collect_mentioned_variables().contains(&var))  {
                 bail!(ErrorKind::NonMatchingVariablesInNotClause);
             }
-            }
-
-            // any other mentioned variables must also unify
-            let mut clauses = not_join.clauses.iter();
-            let template: BTreeSet<Variable> = clauses.next().unwrap().collect_mentioned_variables().difference(&var_set).cloned().collect();
-            for clause in clauses {
-                let mentioned_vars = clause.collect_mentioned_variables().difference(&var_set).cloned().collect();
-                if template != mentioned_vars {
-                    bail!(ErrorKind::NonMatchingVariablesInOrClause);
-                }
-            }
             Ok(())
         },
     }
@@ -125,7 +102,6 @@ mod tests {
         FindQuery,
         NamespacedKeyword,
         OrWhereClause,
-        WhereNotClause,
         Pattern,
         PatternNonValuePlace,
         PatternValuePlace,
@@ -139,8 +115,8 @@ mod tests {
     use clauses::ident;
 
     use super::{
-        validate_or_join,
         validate_not_join,
+        validate_or_join,
     };
 
     fn value_ident(ns: &str, name: &str) -> PatternValuePlace {
@@ -277,15 +253,15 @@ mod tests {
 
 
     /// Tests that the top-level form is a valid `not`, returning the clauses.
-    fn valid_not_join(parsed: FindQuery, expected_unify: UnifyVars) -> Vec<WhereNotClause> {
+    fn valid_not_join(parsed: FindQuery, expected_unify: UnifyVars) -> Vec<WhereClause> {
-        // filter all the not clauses
+        // Filter out all the clauses that are not `not`s.
-        let mut nots = parsed.where_clauses.iter().filter(|&x| match *x {
+        let mut nots = parsed.where_clauses.into_iter().filter(|x| match x {
-            WhereClause::NotJoin(_) => true,
+            &WhereClause::NotJoin(_) => true,
             _ => false,
         });
 
-        // there should be only one not clause
+        // There should be only one not clause.
-        let clause = nots.next().unwrap().clone();
+        let clause = nots.next().unwrap();
         assert_eq!(None, nots.next());
 
         match clause {
@@ -299,7 +275,7 @@ mod tests {
         }
     }
 
-    /// Test that a `not` is valid if all of its arms refer to the same variables.
+    /// Test that a `not` is valid if it is implicit.
     #[test]
     fn test_success_not() {
         let query = r#"[:find ?name
@@ -315,54 +291,27 @@ mod tests {
             (Some(clause1), Some(clause2), None) => {
                 assert_eq!(
                     clause1,
-                    WhereNotClause::Clause(
                     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,
-                    WhereNotClause::Clause(
                     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 that a `not` with differing variable sets in each body part will fail to validate.
-    #[test]
-    fn test_invalid_implicit_not() {
-        let query = r#"[:find ?name
-                        :where [?id :artist/name ?name]
-                            (not [?id :artist/country :country/CA]
-                                 [?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!(),
-        }
-    }
-
     #[test]
     fn test_success_not_join() {
         let query = r#"[:find ?artist
@@ -373,30 +322,28 @@ mod tests {
         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'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,
-                    WhereNotClause::Clause(
                     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,
-                    WhereNotClause::Clause(
                     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!(),
         };
@@ -424,27 +371,4 @@ mod tests {
             _ => panic!(),
         }
     }    
-    
-    /// Test that a `not-join` that does not match any vars declared inside it fails to validate.
-    #[test]
-    fn test_invalid_explicit_not_join_non_matching_internal_vars() {
-        let query = r#"[:find ?artist
-                        :where [?artist :artist/name]
-                               (not-join [?artist]
-                                   [?release :release/artists ?artist]
-                                   [?year :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

@@ -43,7 +43,6 @@ use self::mentat_query::{
     OrJoin,
     OrWhereClause,
     NotJoin,
-    WhereNotClause,
     Pattern,
     PatternNonValuePlace,
     PatternValuePlace,
@@ -188,19 +187,11 @@ def_parser!(Where, or_join_clause, WhereClause, {
             }))
 });
 
-def_value_parser_fn!(Where, not_pattern_clause, WhereNotClause, input, {
-    Where::clause().map(|clause| WhereNotClause::Clause(clause)).parse_stream(input)
-});
-
-def_value_parser_fn!(Where, where_not_clause, WhereNotClause, input, {
-    choice([Where::not_pattern_clause()]).parse_stream(input)
-});
-
 def_value_parser_fn!(Where, not_clause, WhereClause, input, {
     satisfy_map(|x: edn::Value| {
         seq(x).and_then(|items| {
             let mut p = Where::not()
-                        .with(many1(Where::where_not_clause()))
+                        .with(many1(Where::clause()))
                         .skip(eof())
                         .map(|clauses| {
                             WhereClause::NotJoin(
@@ -220,7 +211,7 @@ def_value_parser_fn!(Where, not_join_clause, WhereClause, input, {
         seq(x).and_then(|items| {
             let mut p = Where::not_join()
                         .with(Where::rule_vars())
-                        .and(many1(Where::where_not_clause()))
+                        .and(many1(Where::clause()))
                         .skip(eof())
                         .map(|(vars, clauses)| {
                             WhereClause::NotJoin(
@@ -618,14 +609,14 @@ mod test {
                           WhereClause::NotJoin(
                               NotJoin {
                                   unify_vars: UnifyVars::Implicit,
-                                  clauses: vec![WhereNotClause::Clause(
+                                  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,
-                                      }))],
+                                      })],
                               }));
     }
 
@@ -645,14 +636,13 @@ mod test {
                           WhereClause::NotJoin(
                               NotJoin {
                                   unify_vars: UnifyVars::Explicit(vec![variable(e.clone())]),
-                                  clauses: vec![WhereNotClause::Clause(
+                                  clauses: vec![WhereClause::Pattern(Pattern {
-                                      WhereClause::Pattern(Pattern {
                                           source: None,
                                           entity: PatternNonValuePlace::Variable(variable(e)),
                                           attribute: PatternNonValuePlace::Variable(variable(a)),
                                           value: PatternValuePlace::Variable(variable(v)),
                                           tx: PatternNonValuePlace::Placeholder,
-                                      }))],
+                                      })],
                               }));
     }
 

query/src/lib.rs

@@ -570,24 +570,10 @@ pub struct OrJoin {
     pub clauses: Vec<OrWhereClause>,
 }
 
-#[derive(Clone, Debug, Eq, PartialEq)]
-pub enum WhereNotClause {
-    Clause(WhereClause),
-}
-
-impl WhereNotClause {
-    pub fn is_pattern_or_patterns(&self) -> bool {
-        match self {
-            &WhereNotClause::Clause(WhereClause::Pattern(_)) => true,
-            _ => false,
-        }
-    }
-}
-
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct NotJoin {
     pub unify_vars: UnifyVars,
-    pub clauses: Vec<WhereNotClause>,
+    pub clauses: Vec<WhereClause>,
 }
 
 #[allow(dead_code)]
@@ -680,15 +666,6 @@ impl ContainsVariables for NotJoin {
     }
 }
 
-impl ContainsVariables for WhereNotClause {
-    fn accumulate_mentioned_variables(&self, acc: &mut BTreeSet<Variable>) {
-        use WhereNotClause::*;
-        match self {
-            &Clause(ref clause) => clause.accumulate_mentioned_variables(acc),
-        }
-    }
-}
-
 impl ContainsVariables for Predicate {
     fn accumulate_mentioned_variables(&self, acc: &mut BTreeSet<Variable>) {
         for arg in &self.args {