Remove Changeset

This is a long overdue refactoring. I'm going to do a similar
refactoring for `InsertValues` as part of #1106, but this shares a lot
of groundwork for that and is much smaller in scope. `Changeset` has
always been entirely redundant with `QueryFragment`, but we had to keep
it as a separate trait for two places where the implementation changed:

- `Eq` doesn't qualify the column name
- tuples skip commas when `is_noop` returned true (for `Changeset` this
  was only the case for `None` or a tuple where all elements were `None`)

The first one is easy to solve, we just return a type other than `Eq` in
`AsChangeset` (there was very little reason to use `Eq` in the first
place. We weren't able to re-use any of its code.)

The second one is a bit trickier, since we need to replicate the
`is_noop` method in `QueryFragment`. I'm fine with doing this, since
`InsertValues` has the same method (and tuples implement `walk_ast`
identically for `InsertValues` and `Changeset`), so it makes sense to
share them.

Originally I added an explicit `noop` method to `AstPass` which we would
call. However, it felt weird that the impl for `()` which literally just
does `Ok(())` didn't call `noop`. At that point I realized I could just
generalize this to "did a method that generates SQL get called?" This
works fine for updates. I'm not 100% sure if it'll work for inserts, but
it's worth a shot.

I should note that the semantics of the new `is_noop` are slightly
different than the one on `InsertValues`, which explicitly states that
an empty array will return `false`. This will make it return `true` when
we switch inserts to use this. Since we already have to explicitly
handle empty arrays long before we start checking `is_noop`, I think
that's fine.

diesel/src/expression/operators.rs

@@ -64,7 +64,7 @@ macro_rules! __diesel_operator_body {
         #[derive(Debug, Clone, Copy, QueryId)]
         #[doc(hidden)]
         pub struct $name<$($ty_param,)+> {
-            $($field_name: $ty_param,)+
+            $(pub(crate) $field_name: $ty_param,)+
         }
 
         impl<$($ty_param,)+> $name<$($ty_param,)+> {
@@ -356,42 +356,8 @@ diesel_postfix_operator!(Desc, " DESC", ());
 
 diesel_prefix_operator!(Not, "NOT ");
 
-use backend::Backend;
 use insertable::{ColumnInsertValue, Insertable};
 use query_source::Column;
-use query_builder::*;
-use result::QueryResult;
-use super::AppearsOnTable;
-
-impl<T, U, DB> Changeset<DB> for Eq<T, U>
-where
-    DB: Backend,
-    T: Column,
-    U: AppearsOnTable<T::Table> + QueryFragment<DB>,
-{
-    fn is_noop(&self) -> bool {
-        false
-    }
-
-    fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> {
-        try!(out.push_identifier(T::NAME));
-        out.push_sql(" = ");
-        QueryFragment::walk_ast(&self.right, out)
-    }
-}
-
-impl<T, U> AsChangeset for Eq<T, U>
-where
-    T: Column,
-    U: AppearsOnTable<T::Table>,
-{
-    type Target = T::Table;
-    type Changeset = Self;
-
-    fn as_changeset(self) -> Self {
-        self
-    }
-}
 
 impl<T, U> Insertable<T::Table> for Eq<T, U>
 where

diesel/src/pg/upsert/on_conflict_actions.rs

@@ -34,11 +34,11 @@ impl<T> DoUpdate<T> {
 
 impl<T> QueryFragment<Pg> for DoUpdate<T>
 where
-    T: Changeset<Pg>,
+    T: QueryFragment<Pg>,
 {
     fn walk_ast(&self, mut out: AstPass<Pg>) -> QueryResult<()> {
         out.unsafe_to_cache_prepared();
-        if self.changeset.is_noop() {
+        if self.changeset.is_noop()? {
             out.push_sql(" DO NOTHING");
         } else {
             out.push_sql(" DO UPDATE SET ");

diesel/src/query_builder/ast_pass.rs

@@ -70,6 +70,16 @@ where
         }
     }
 
+    /// Does running this AST pass have any effect?
+    ///
+    /// The result will be set to `false` if any method that generates SQL
+    /// is called.
+    pub(crate) fn is_noop(result: &'a mut bool) -> Self {
+        AstPass {
+            internals: AstPassInternals::IsNoop(result),
+        }
+    }
+
     /// Call this method whenever you pass an instance of `AstPass` by value.
     ///
     /// Effectively copies `self`, with a narrower lifetime. When passing a
@@ -96,6 +106,7 @@ where
                 let f_with_shorter_lifetime = unsafe { mem::transmute(&mut **f) };
                 DebugBinds(f_with_shorter_lifetime)
             }
+            IsNoop(ref mut result) => IsNoop(&mut **result),
         };
         AstPass { internals }
     }
@@ -152,8 +163,10 @@ where
     /// # fn main() {}
     /// ```
     pub fn push_sql(&mut self, sql: &str) {
-        if let AstPassInternals::ToSql(ref mut builder) = self.internals {
-            builder.push_sql(sql);
+        match self.internals {
+            AstPassInternals::ToSql(ref mut builder) => builder.push_sql(sql),
+            AstPassInternals::IsNoop(ref mut result) => **result = false,
+            _ => {}
         }
     }
 
@@ -162,8 +175,10 @@ where
     /// The identifier will be quoted using the rules specific to the backend
     /// the query is being constructed for.
     pub fn push_identifier(&mut self, identifier: &str) -> QueryResult<()> {
-        if let AstPassInternals::ToSql(ref mut builder) = self.internals {
-            builder.push_identifier(identifier)?;
+        match self.internals {
+            AstPassInternals::ToSql(ref mut builder) => builder.push_identifier(identifier)?,
+            AstPassInternals::IsNoop(ref mut result) => **result = false,
+            _ => {}
         }
         Ok(())
     }
@@ -188,7 +203,8 @@ where
             DebugBinds(ref mut f) => {
                 f.entry(bind);
             }
-            _ => {} // noop
+            IsNoop(ref mut result) => **result = false,
+            _ => {}
         }
         Ok(())
     }
@@ -238,4 +254,5 @@ where
     },
     IsSafeToCachePrepared(&'a mut bool),
     DebugBinds(&'a mut fmt::DebugList<'a, 'a>),
+    IsNoop(&'a mut bool),
 }

diesel/src/query_builder/mod.rs

@@ -43,8 +43,8 @@ pub use self::query_id::QueryId;
 pub use self::select_statement::{BoxedSelectStatement, SelectStatement};
 pub use self::sql_query::SqlQuery;
 #[doc(inline)]
-pub use self::update_statement::{AsChangeset, Changeset, IncompleteUpdateStatement,
-                                 IntoUpdateTarget, UpdateStatement, UpdateTarget};
+pub use self::update_statement::{AsChangeset, IncompleteUpdateStatement, IntoUpdateTarget,
+                                 UpdateStatement, UpdateTarget};
 
 use std::error::Error;
 
@@ -164,6 +164,14 @@ pub trait QueryFragment<DB: Backend> {
         self.walk_ast(AstPass::is_safe_to_cache_prepared(&mut result))?;
         Ok(result)
     }
+
+    #[doc(hidden)]
+    /// Does walking this AST have any effect?
+    fn is_noop(&self) -> QueryResult<bool> {
+        let mut result = true;
+        self.walk_ast(AstPass::is_noop(&mut result))?;
+        Ok(result)
+    }
 }
 
 impl<T: ?Sized, DB> QueryFragment<DB> for Box<T>
@@ -192,6 +200,19 @@ impl<DB: Backend> QueryFragment<DB> for () {
     }
 }
 
+impl<T, DB> QueryFragment<DB> for Option<T>
+where
+    DB: Backend,
+    T: QueryFragment<DB>,
+{
+    fn walk_ast(&self, out: AstPass<DB>) -> QueryResult<()> {
+        match *self {
+            Some(ref c) => c.walk_ast(out),
+            None => Ok(()),
+        }
+    }
+}
+
 /// Types that can be converted into a complete, typed SQL query.
 ///
 /// This is used internally to automatically add the right select clause when

diesel/src/query_builder/update_statement/changeset.rs

@@ -1,6 +1,8 @@
 use backend::Backend;
-use query_builder::AstPass;
-use query_source::QuerySource;
+use expression::AppearsOnTable;
+use expression::operators::Eq;
+use query_builder::*;
+use query_source::{Column, QuerySource};
 use result::QueryResult;
 
 /// Types which can be passed to
@@ -29,17 +31,6 @@ pub trait AsChangeset {
     fn as_changeset(self) -> Self::Changeset;
 }
 
-#[doc(hidden)]
-pub trait Changeset<DB: Backend> {
-    /// Does this changeset actually include any changes?
-    fn is_noop(&self) -> bool;
-
-    /// See [`QueryFragment#walk_ast`]
-    ///
-    /// [`QueryFragment#walk_ast`]: trait.QueryFragment.html#tymethod.walk_ast
-    fn walk_ast(&self, pass: AstPass<DB>) -> QueryResult<()>;
-}
-
 impl<T: AsChangeset> AsChangeset for Option<T> {
     type Target = T::Target;
     type Changeset = Option<T::Changeset>;
@@ -49,15 +40,37 @@ impl<T: AsChangeset> AsChangeset for Option<T> {
     }
 }
 
-impl<T: Changeset<DB>, DB: Backend> Changeset<DB> for Option<T> {
-    fn is_noop(&self) -> bool {
-        self.is_none()
-    }
+impl<Left, Right> AsChangeset for Eq<Left, Right>
+where
+    Left: Column,
+    Right: AppearsOnTable<Left::Table>,
+{
+    type Target = Left::Table;
+    type Changeset = Assign<Left, Right>;
 
-    fn walk_ast(&self, out: AstPass<DB>) -> QueryResult<()> {
-        match *self {
-            Some(ref c) => c.walk_ast(out),
-            None => Ok(()),
+    fn as_changeset(self) -> Self::Changeset {
+        Assign {
+            _column: self.left,
+            expr: self.right,
         }
     }
 }
+
+#[derive(Debug, Clone, Copy)]
+pub struct Assign<Col, Expr> {
+    _column: Col,
+    expr: Expr,
+}
+
+impl<T, U, DB> QueryFragment<DB> for Assign<T, U>
+where
+    DB: Backend,
+    T: Column,
+    U: QueryFragment<DB>,
+{
+    fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> {
+        try!(out.push_identifier(T::NAME));
+        out.push_sql(" = ");
+        QueryFragment::walk_ast(&self.expr, out)
+    }
+}

diesel/src/query_builder/update_statement/mod.rs

@@ -1,7 +1,7 @@
 pub mod changeset;
 pub mod target;
 
-pub use self::changeset::{AsChangeset, Changeset};
+pub use self::changeset::AsChangeset;
 pub use self::target::{IntoUpdateTarget, UpdateTarget};
 
 use backend::Backend;
@@ -172,11 +172,11 @@ where
     T: Table,
     T::FromClause: QueryFragment<DB>,
     U: QueryFragment<DB>,
-    V: changeset::Changeset<DB>,
+    V: QueryFragment<DB>,
     Ret: QueryFragment<DB>,
 {
     fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> {
-        if self.values.is_noop() {
+        if self.values.is_noop()? {
             return Err(QueryBuilderError(
                 "There are no changes to save. This query cannot be built".into(),
             ));

diesel/src/type_impls/tuples.rs

@@ -73,12 +73,17 @@ macro_rules! tuple_impls {
             }
 
             impl<$($T: QueryFragment<DB>),+, DB: Backend> QueryFragment<DB> for ($($T,)+) {
+                #[allow(unused_assignments)]
                 fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> {
+                    let mut needs_comma = false;
                     $(
-                        if $idx != 0 {
-                            out.push_sql(", ");
+                        if !self.$idx.is_noop()? {
+                            if needs_comma {
+                                out.push_sql(", ");
+                            }
+                            self.$idx.walk_ast(out.reborrow())?;
+                            needs_comma = true;
                         }
-                        self.$idx.walk_ast(out.reborrow())?;
                     )+
                     Ok(())
                 }
@@ -198,31 +203,6 @@ macro_rules! tuple_impls {
                 }
             }
 
-            impl<DB, $($T,)+> Changeset<DB> for ($($T,)+) where
-                DB: Backend,
-                $($T: Changeset<DB>,)+
-            {
-                fn is_noop(&self) -> bool {
-                    $(self.$idx.is_noop() &&)+ true
-                }
-
-                #[allow(unused_assignments)]
-                fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> {
-                    let mut needs_comma = false;
-                    $(
-                        let noop_element = self.$idx.is_noop();
-                        if !noop_element {
-                            if needs_comma {
-                                out.push_sql(", ");
-                            }
-                            self.$idx.walk_ast(out.reborrow())?;
-                            needs_comma = true;
-                        }
-                    )+
-                    Ok(())
-                }
-            }
-
             impl<$($T,)+ Parent> BelongsTo<Parent> for ($($T,)+) where
                 A: BelongsTo<Parent>,
             {