Refactor the query builder to make it easier to add new AST passes

I had initially wanted to move us towards a more of a visitor pattern,
where the `AstVisitor` trait would have most of the methods from
`QueryBuilder`, with `BindCollector` and `fn
unsafe_to_cache_prepared(&mut self)` tacked on. However, I quickly hit a
wall there when I realized that `BindCollector` cannot be made object
safe, which would prevent me from making `QueryFragment` object safe, so
that won't work.

The benefit of using the visitor pattern here would have been that it
would be easier for code *outside* of Diesel to add new AST passes if it
wanted to (within limits, I don't want to go so far as to have
`visit_specific_type` as that would be even more painful than what we
have now). This form doesn't bring that benefit.

However, using an enum instead does make it easier for us to add new
passes within Diesel itself, since we now have a single function which
will apply to all future AST passes for the majority of nodes (most
nodes don't care about any pass other than `to_sql`, they just call
their children).

The plan is to follow this up by moving `to_sql` into the `walk_ast`
method as well, and eliminate `Changeset`, and `InsertValues` since
it'll now be easier to add the new "this is like that other pass, but
slightly different for this one case" type passes.

The signature is a little funky here, since we're passing a mutable
reference to an enum which just wraps a mutable reference. I really only
want the inner `&mut`, since I want implementors to be able to mutate
the `QueryBuilder` or `BindCollector`, but not change the variant. The
reason for the outer `&mut` is because reborrowing doesn't apply to
structs. We can't pass `AstPass<'a>` multiple times, as it's not `Copy`.
When we pass a mutable reference directly, Rust creates a new reference
with a narrowed lifetime. We can't do that here. The reason I kept the
inner `&mut` instead of just having `AstPass` own the value is that we'd
have no way to cleanly get the value being mutated back out of it when
we were done.

Author: sgrif

diesel/src/connection/statement_cache.rs

@@ -137,7 +137,7 @@ impl<DB, Statement> StatementCache<DB, Statement> where
 
         let cache_key = try!(StatementCacheKey::for_source(source, bind_types));
 
-        if !source.is_safe_to_cache_prepared() {
+        if !source.is_safe_to_cache_prepared()? {
             let sql = try!(cache_key.sql(source));
             return prepare_fn(&sql).map(MaybeCached::CannotCache)
         }

diesel/src/expression/array_comparison.rs

@@ -1,7 +1,7 @@
 use backend::Backend;
 use expression::*;
 use expression::helper_types::SqlTypeOf;
-use query_builder::{Query, QueryBuilder, QueryFragment, BuildQueryResult};
+use query_builder::*;
 use result::QueryResult;
 use types::Bool;
 
@@ -79,16 +79,11 @@ impl<T, U, DB> QueryFragment<DB> for In<T, U> where
         Ok(())
     }
 
-    fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        try!(self.left.collect_binds(out));
-        try!(self.values.collect_binds(out));
+    fn walk_ast(&self, pass: &mut AstPass<DB>) -> QueryResult<()> {
+        self.left.walk_ast(pass)?;
+        self.values.walk_ast(pass)?;
         Ok(())
     }
-
-    fn is_safe_to_cache_prepared(&self) -> bool {
-        self.left.is_safe_to_cache_prepared() &&
-            self.values.is_safe_to_cache_prepared()
-    }
 }
 
 impl<T, U, DB> QueryFragment<DB> for NotIn<T, U> where
@@ -108,16 +103,11 @@ impl<T, U, DB> QueryFragment<DB> for NotIn<T, U> where
         Ok(())
     }
 
-    fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        try!(self.left.collect_binds(out));
-        try!(self.values.collect_binds(out));
+    fn walk_ast(&self, pass: &mut AstPass<DB>) -> QueryResult<()> {
+        self.left.walk_ast(pass)?;
+        self.values.walk_ast(pass)?;
         Ok(())
     }
-
-    fn is_safe_to_cache_prepared(&self) -> bool {
-        self.left.is_safe_to_cache_prepared() &&
-            self.values.is_safe_to_cache_prepared()
-    }
 }
 
 impl_query_id!(In<T, U>);
@@ -212,16 +202,16 @@ impl<T, DB> QueryFragment<DB> for Many<T> where
         Ok(())
     }
 
-    fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        for value in &self.0 {
-            try!(value.collect_binds(out));
+    fn walk_ast(&self, pass: &mut AstPass<DB>) -> QueryResult<()> {
+        if let AstPass::IsSafeToCachePrepared(ref mut result) = *pass {
+            **result = false;
+        } else {
+            for value in &self.0 {
+                value.walk_ast(pass)?;
+            }
         }
         Ok(())
     }
-
-    fn is_safe_to_cache_prepared(&self) -> bool {
-        false
-    }
 }
 
 impl_query_id!(noop: Many<T>);
@@ -262,12 +252,8 @@ impl<T, ST, DB> QueryFragment<DB> for Subselect<T, ST> where
         self.values.to_sql(out)
     }
 
-    fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        self.values.collect_binds(out)
-    }
-
-    fn is_safe_to_cache_prepared(&self) -> bool {
-        self.values.is_safe_to_cache_prepared()
+    fn walk_ast(&self, pass: &mut AstPass<DB>) -> QueryResult<()> {
+        self.values.walk_ast(pass)
     }
 }
 

diesel/src/expression/bound.rs

@@ -34,12 +34,11 @@ impl<T, U, DB> QueryFragment<DB> for Bound<T, U> where
         Ok(())
     }
 
-    fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        out.push_bound_value(&self.item)
-    }
-
-    fn is_safe_to_cache_prepared(&self) -> bool {
-        true
+    fn walk_ast(&self, pass: &mut AstPass<DB>) -> QueryResult<()> {
+        if let AstPass::CollectBinds(ref mut out) = *pass {
+            out.push_bound_value(&self.item)?;
+        }
+        Ok(())
     }
 }
 

diesel/src/expression/coerce.rs

@@ -56,12 +56,8 @@ impl<T, ST, DB> QueryFragment<DB> for Coerce<T, ST> where
         self.expr.to_sql(out)
     }
 
-    fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        self.expr.collect_binds(out)
-    }
-
-    fn is_safe_to_cache_prepared(&self) -> bool {
-        self.expr.is_safe_to_cache_prepared()
+    fn walk_ast(&self, pass: &mut AstPass<DB>) -> QueryResult<()> {
+        self.expr.walk_ast(pass)
     }
 }
 

diesel/src/expression/count.rs

@@ -88,13 +88,8 @@ impl<T: QueryFragment<DB>, DB: Backend> QueryFragment<DB> for Count<T> {
         Ok(())
     }
 
-    fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        try!(self.target.collect_binds(out));
-        Ok(())
-    }
-
-    fn is_safe_to_cache_prepared(&self) -> bool {
-        self.target.is_safe_to_cache_prepared()
+    fn walk_ast(&self, pass: &mut AstPass<DB>) -> QueryResult<()> {
+        self.target.walk_ast(pass)
     }
 }
 
@@ -115,13 +110,9 @@ impl<DB: Backend> QueryFragment<DB> for CountStar {
         Ok(())
     }
 
-    fn collect_binds(&self, _out: &mut DB::BindCollector) -> QueryResult<()> {
+    fn walk_ast(&self, _: &mut AstPass<DB>) -> QueryResult<()> {
         Ok(())
     }
-
-    fn is_safe_to_cache_prepared(&self) -> bool {
-        true
-    }
 }
 
 impl_query_id!(CountStar);

diesel/src/expression/exists.rs

@@ -63,14 +63,10 @@ impl<T, DB> QueryFragment<DB> for Exists<T> where
         Ok(())
     }
 
-    fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        try!(self.0.collect_binds(out));
+    fn walk_ast(&self, pass: &mut AstPass<DB>) -> QueryResult<()> {
+        self.0.walk_ast(pass)?;
         Ok(())
     }
-
-    fn is_safe_to_cache_prepared(&self) -> bool {
-        self.0.is_safe_to_cache_prepared()
-    }
 }
 
 impl_query_id!(Exists<T>);

diesel/src/expression/functions/aggregate_folding.rs

@@ -39,14 +39,10 @@ macro_rules! fold_function {
                 Ok(())
             }
 
-            fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-                try!(self.target.collect_binds(out));
+            fn walk_ast(&self, pass: &mut AstPass<DB>) -> QueryResult<()> {
+                self.target.walk_ast(pass)?;
                 Ok(())
             }
-
-            fn is_safe_to_cache_prepared(&self) -> bool {
-                self.target.is_safe_to_cache_prepared()
-            }
         }
 
         impl_query_id!($type_name<T>);

diesel/src/expression/functions/aggregate_ordering.rs

@@ -38,14 +38,10 @@ macro_rules! ord_function {
                 Ok(())
             }
 
-            fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-                try!(self.target.collect_binds(out));
+            fn walk_ast(&self, pass: &mut AstPass<DB>) -> QueryResult<()> {
+                self.target.walk_ast(pass)?;
                 Ok(())
             }
-
-            fn is_safe_to_cache_prepared(&self) -> bool {
-                self.target.is_safe_to_cache_prepared()
-            }
         }
 
         impl_query_id!($type_name<T>);

diesel/src/expression/functions/date_and_time.rs

@@ -23,13 +23,9 @@ impl<DB: Backend> QueryFragment<DB> for now {
         Ok(())
     }
 
-    fn collect_binds(&self, _out: &mut DB::BindCollector) -> QueryResult<()> {
+    fn walk_ast(&self, _: &mut AstPass<DB>) -> QueryResult<()> {
         Ok(())
     }
-
-    fn is_safe_to_cache_prepared(&self) -> bool {
-        true
-    }
 }
 
 impl_query_id!(now);

diesel/src/expression/functions/mod.rs

@@ -47,15 +47,11 @@ macro_rules! sql_function_body {
                 Ok(())
             }
 
-            fn collect_binds(&self, out: &mut DB::BindCollector) -> $crate::result::QueryResult<()> {
-                try!($crate::query_builder::QueryFragment::collect_binds(
-                        &($(&self.$arg_name),*), out));
+            fn walk_ast(&self, pass: &mut $crate::query_builder::AstPass<DB>) -> $crate::result::QueryResult<()> {
+                try!($crate::query_builder::QueryFragment::walk_ast(
+                        &($(&self.$arg_name),*), pass));
                 Ok(())
             }
-
-            fn is_safe_to_cache_prepared(&self) -> bool {
-                ($(&self.$arg_name),*).is_safe_to_cache_prepared()
-            }
         }
 
         impl_query_id!($struct_name<$($arg_name),+>);
@@ -166,13 +162,9 @@ macro_rules! no_arg_sql_function_body {
                 Ok(())
             }
 
-            fn collect_binds(&self, _out: &mut DB::BindCollector) -> $crate::result::QueryResult<()> {
+            fn walk_ast(&self, _: &mut $crate::query_builder::AstPass<DB>) -> QueryResult<()> {
                 Ok(())
             }
-
-            fn is_safe_to_cache_prepared(&self) -> bool {
-                true
-            }
         }
     };
 
@@ -188,13 +180,9 @@ macro_rules! no_arg_sql_function_body {
                 Ok(())
             }
 
-            fn collect_binds(&self, _out: &mut DB::BindCollector) -> $crate::result::QueryResult<()> {
+            fn walk_ast(&self, _: &mut $crate::query_builder::AstPass<DB>) -> QueryResult<()> {
                 Ok(())
             }
-
-            fn is_safe_to_cache_prepared(&self) -> bool {
-                true
-            }
         }
     };
 }