Make AstPass an opaque struct

I'm moving towards merging `to_sql` with `walk_ast`. The code will look
*really* bad if there's a ton of conditionals on which pass it is.
Instead I'd like to have the code look the same as it would if I were
able to make `AstPass` a trait (see 97e5cba for
context on why we can't do that)

In order to enforce that only the methods are used, and nothing is
actually destructuring, I've had to do a bit of funky indirection.

Author: sgrif

diesel/src/expression/array_comparison.rs

@@ -203,12 +203,9 @@ impl<T, DB> QueryFragment<DB> for Many<T> where
     }
 
     fn walk_ast(&self, mut pass: AstPass<DB>) -> QueryResult<()> {
-        if let AstPass::IsSafeToCachePrepared(result) = pass {
-            *result = false;
-        } else {
-            for value in &self.0 {
-                value.walk_ast(pass.reborrow())?;
-            }
+        pass.unsafe_to_cache_prepared();
+        for value in &self.0 {
+            value.walk_ast(pass.reborrow())?;
         }
         Ok(())
     }

diesel/src/expression/bound.rs

@@ -34,10 +34,8 @@ impl<T, U, DB> QueryFragment<DB> for Bound<T, U> where
         Ok(())
     }
 
-    fn walk_ast(&self, pass: AstPass<DB>) -> QueryResult<()> {
-        if let AstPass::CollectBinds(out) = pass {
-            out.push_bound_value(&self.item)?;
-        }
+    fn walk_ast(&self, mut pass: AstPass<DB>) -> QueryResult<()> {
+        pass.push_bind_param(&self.item)?;
         Ok(())
     }
 }

diesel/src/expression/predicates.rs

@@ -252,8 +252,8 @@ impl<T, U, DB> Changeset<DB> for Eq<T, U> where
         QueryFragment::to_sql(&self.right, out)
     }
 
-    fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        QueryFragment::collect_binds(&self.right, out)
+    fn walk_ast(&self, out: AstPass<DB>) -> QueryResult<()> {
+        QueryFragment::walk_ast(&self.right, out)
     }
 }
 

diesel/src/expression/sql_literal.rs

@@ -40,10 +40,8 @@ impl<ST, DB> QueryFragment<DB> for SqlLiteral<ST> where
         Ok(())
     }
 
-    fn walk_ast(&self, pass: AstPass<DB>) -> QueryResult<()> {
-        if let AstPass::IsSafeToCachePrepared(result) = pass {
-            *result = false;
-        }
+    fn walk_ast(&self, mut pass: AstPass<DB>) -> QueryResult<()> {
+        pass.unsafe_to_cache_prepared();
         Ok(())
     }
 }

diesel/src/insertable.rs

@@ -3,7 +3,7 @@ use std::iter;
 use backend::{Backend, SupportsDefaultKeyword};
 use expression::Expression;
 use result::QueryResult;
-use query_builder::{QueryBuilder, BuildQueryResult};
+use query_builder::{QueryBuilder, BuildQueryResult, AstPass};
 use query_source::{Table, Column};
 use types::IntoNullable;
 
@@ -27,7 +27,7 @@ pub trait Insertable<T: Table, DB: Backend> {
 pub trait InsertValues<DB: Backend> {
     fn column_names(&self, out: &mut DB::QueryBuilder) -> BuildQueryResult;
     fn values_clause(&self, out: &mut DB::QueryBuilder) -> BuildQueryResult;
-    fn values_bind_params(&self, out: &mut DB::BindCollector) -> QueryResult<()>;
+    fn walk_ast(&self, out: AstPass<DB>) -> QueryResult<()>;
 }
 
 #[derive(Debug, Copy, Clone)]
@@ -85,9 +85,9 @@ impl<T, DB> InsertValues<DB> for BatchInsertValues<T> where
         Ok(())
     }
 
-    fn values_bind_params(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
+    fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> {
         for values in self.0.clone() {
-            try!(values.values_bind_params(out));
+            values.walk_ast(out.reborrow())?;
         }
         Ok(())
     }

diesel/src/pg/upsert/on_conflict_actions.rs

@@ -163,11 +163,9 @@ impl<T> QueryFragment<Pg> for DoUpdate<T> where
         Ok(())
     }
 
-    fn walk_ast(&self, pass: AstPass<Pg>) -> QueryResult<()> {
-        match pass {
-            AstPass::CollectBinds(out) => self.changeset.collect_binds(out)?,
-            AstPass::IsSafeToCachePrepared(result) => *result = false,
-        }
+    fn walk_ast(&self, mut pass: AstPass<Pg>) -> QueryResult<()> {
+        pass.unsafe_to_cache_prepared();
+        self.changeset.walk_ast(pass)?;
         Ok(())
     }
 }

diesel/src/pg/upsert/on_conflict_clause.rs

@@ -116,10 +116,10 @@ impl<Values, Target, Action> InsertValues<Pg> for OnConflictValues<Values, Targe
         Ok(())
     }
 
-    fn values_bind_params(&self, out: &mut <Pg as Backend>::BindCollector) -> QueryResult<()> {
-        try!(self.values.values_bind_params(out));
-        try!(self.target.collect_binds(out));
-        try!(self.action.collect_binds(out));
+    fn walk_ast(&self, mut out: AstPass<Pg>) -> QueryResult<()> {
+        self.values.walk_ast(out.reborrow())?;
+        self.target.walk_ast(out.reborrow())?;
+        self.action.walk_ast(out.reborrow())?;
         Ok(())
     }
 }

diesel/src/query_builder/ast_pass.rs

@@ -0,0 +1,72 @@
+use backend::Backend;
+use query_builder::BindCollector;
+use result::QueryResult;
+use types::{ToSql, HasSqlType};
+
+#[doc(hidden)]
+#[allow(missing_debug_implementations)]
+pub struct AstPass<'a, DB> where
+    DB: Backend,
+    DB::BindCollector: 'a,
+{
+    internals: AstPassInternals<'a, DB>,
+}
+
+impl<'a, DB> AstPass<'a, DB> where
+    DB: Backend,
+    DB::BindCollector: 'a,
+{
+    pub fn collect_binds(collector: &'a mut DB::BindCollector) -> Self {
+        AstPass {
+            internals: AstPassInternals::CollectBinds(collector),
+        }
+    }
+
+    pub fn is_safe_to_cache_prepared(result: &'a mut bool) -> Self {
+        AstPass {
+            internals: AstPassInternals::IsSafeToCachePrepared(result),
+        }
+    }
+
+    /// Effectively copies `self`, with a narrower lifetime. This method
+    /// matches the semantics of the implicit reborrow that occurs when passing
+    /// a reference by value in Rust.
+    pub fn reborrow(&mut self) -> AstPass<DB> {
+        use self::AstPassInternals::*;
+        let internals = match self.internals {
+            CollectBinds(ref mut collector) => CollectBinds(&mut **collector),
+            IsSafeToCachePrepared(ref mut result) => IsSafeToCachePrepared(&mut **result),
+        };
+        AstPass { internals }
+    }
+
+    pub fn unsafe_to_cache_prepared(&mut self) {
+        if let AstPassInternals::IsSafeToCachePrepared(ref mut result) = self.internals {
+            **result = false
+        }
+    }
+
+    pub fn push_bind_param<T, U>(&mut self, bind: &U) -> QueryResult<()> where
+        DB: HasSqlType<T>,
+        U: ToSql<T, DB>,
+    {
+        if let AstPassInternals::CollectBinds(ref mut out) = self.internals {
+            out.push_bound_value(bind)?;
+        }
+        Ok(())
+    }
+}
+
+#[allow(missing_debug_implementations)]
+/// This is separate from the struct to cause the enum to be opaque, forcing
+/// usage of the methods provided rather than matching on the enum directly.
+/// This essentially mimics the capabilities that would be available if
+/// `AstPass` were a trait.
+enum AstPassInternals<'a, DB> where
+    DB: Backend,
+    DB::BindCollector: 'a,
+{
+    CollectBinds(&'a mut DB::BindCollector),
+    IsSafeToCachePrepared(&'a mut bool),
+}
+

diesel/src/query_builder/insert_statement.rs

@@ -117,17 +117,11 @@ impl<T, U, Op, Ret, DB> QueryFragment<DB> for InsertStatement<T, U, Op, Ret> whe
     }
 
     fn walk_ast(&self, mut pass: AstPass<DB>) -> QueryResult<()> {
-        if let AstPass::IsSafeToCachePrepared(result) = pass {
-            *result = false;
-        } else {
-            let values = self.records.values();
-            self.operator.walk_ast(pass.reborrow())?;
-            self.target.from_clause().walk_ast(pass.reborrow())?;
-            if let AstPass::CollectBinds(ref mut out) = pass {
-                values.values_bind_params(out)?;
-            }
-            self.returning.walk_ast(pass.reborrow())?;
-        }
+        pass.unsafe_to_cache_prepared();
+        self.operator.walk_ast(pass.reborrow())?;
+        self.target.from_clause().walk_ast(pass.reborrow())?;
+        self.records.values().walk_ast(pass.reborrow())?;
+        self.returning.walk_ast(pass.reborrow())?;
         Ok(())
     }
 }

diesel/src/query_builder/mod.rs

@@ -6,6 +6,7 @@ mod query_id;
 #[macro_use]
 mod clause_macro;
 
+mod ast_pass;
 pub mod bind_collector;
 mod delete_statement;
 #[doc(hidden)]
@@ -24,6 +25,7 @@ pub mod where_clause;
 pub mod insert_statement;
 pub mod update_statement;
 
+pub use self::ast_pass::AstPass;
 pub use self::bind_collector::BindCollector;
 pub use self::query_id::QueryId;
 #[doc(hidden)]
@@ -73,32 +75,6 @@ impl<'a, T: Query> Query for &'a T {
     type SqlType = T::SqlType;
 }
 
-#[doc(hidden)]
-#[allow(missing_debug_implementations)]
-pub enum AstPass<'a, DB> where
-    DB: Backend,
-    DB::BindCollector: 'a,
-{
-    CollectBinds(&'a mut DB::BindCollector),
-    IsSafeToCachePrepared(&'a mut bool),
-}
-
-impl<'a, DB> AstPass<'a, DB> where
-    DB: Backend,
-    DB::BindCollector: 'a,
-{
-    /// Effectively copies `self`, with a narrower lifetime. This method
-    /// matches the semantics of the implicit reborrow that occurs when passing
-    /// a reference by value in Rust.
-    pub fn reborrow(&mut self) -> AstPass<DB> {
-        use self::AstPass::*;
-        match *self {
-            CollectBinds(ref mut collector) => CollectBinds(&mut **collector),
-            IsSafeToCachePrepared(ref mut result) => IsSafeToCachePrepared(&mut **result),
-        }
-    }
-}
-
 /// An untyped fragment of SQL. This may be a complete SQL command (such as
 /// an update statement without a `RETURNING` clause), or a subsection (such as
 /// our internal types used to represent a `WHERE` clause). All methods on
@@ -109,12 +85,12 @@ pub trait QueryFragment<DB: Backend> {
     fn walk_ast(&self, pass: AstPass<DB>) -> QueryResult<()>;
 
     fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
-        self.walk_ast(AstPass::CollectBinds(out))
+        self.walk_ast(AstPass::collect_binds(out))
     }
 
     fn is_safe_to_cache_prepared(&self) -> QueryResult<bool> {
         let mut result = true;
-        self.walk_ast(AstPass::IsSafeToCachePrepared(&mut result))?;
+        self.walk_ast(AstPass::is_safe_to_cache_prepared(&mut result))?;
         Ok(result)
     }
 }