Split SelectableExpression into two traits

The change in #709 had the side effect of re-introducing #104. With the design that we have right now, nullability isn't propagating upwards. This puts the issue of "expressions aren't validating that the type of its arguments haven't become nullable, and thus nulls are slipping in where they shouldn't be" at odds with "we can't use complex expressions in filters for joins because the SQL type changed". This semi-resolves the issue by restricting when we care about nullability. Ultimately the only time it really matters is when we're selecting data, as we need to enforce that the result goes into an `Option`. For places where we don't see the bytes in Rust (filter, order, etc), `NULL` is effectively `false`. This change goes back to fully fixing #104, but brings back a small piece of #621. I've changed everything that is a composite expression to only be selectable if the SQL type hasn't changed. This means that you won't be able to do things like `users.left_outer_join(posts).select(posts::id + 1)`, but you will be able to use whatever you want in `filter`. This change is also to support what I think will fix the root of all these issues. The design of "Here's the SQL type on this query source" is just fundamentally not what we need. There is only one case where the type changes, and that is to become null when it is on the right side of a left join, the left side of a right join, or either side of a full join. One of the changes that #709 made was to require that you explicitly call `.nullable()` on a tuple if you wanted to get `Option<(i32, String)>` instead of `(Option<i32>, Option<String>)`. This has worked out fine, and isn't a major ergonomic pain. The common case is just to use the default select clause anyway. So I want to go further down this path. The longer term plan is to remove `SqlTypeForSelect` entirely, and *not* implement `SelectableExpression` for columns on the nullable side of a join. We will then provide these two blanket impls: ```rust impl<Left, Right, T> SelectableExpression<LeftOuterJoin<Left, Right>> for Nullable<T> where T: SelectableExpression<Right>, {} impl<Left, Right, Head, Tail> SelectableExpression<LeftOuterJoin<Left, Right>> for Nullable<Cons<Head, Tail>> where Nullable<Head>: SelectableExpression<LeftOuterJoin<Left, Right>>, Nullable<Tail>: SelectableExpression<LeftOuterJoin<Left, Right>>, {} ``` (Note: Those impls overlap. Providing them as blanket impls would require rust-lang/rust#40097. Providing them as non-blanket impls would require us to mark `Nullable` and possibly `Cons` as `#[fundamental]`) The end result will be that nullability naturally propagates as we want it to. Given `sql_function!(lower, lower_t, (x: Text) -> Text)`, doing `select(lower(posts::name).nullable())` will work. `lower(posts::name)` will fail because `posts::name` doesn't impl `SelectableExpression`. `lower(posts::name.nullable())` will fail because while `SelectableExpression` will be met, the SQL type of the argument isn't what's expected. Putting `.nullable` at the very top level naturally follows SQL's semantics here.
diesel-rs · Mar 3, 2017 · 6dec748 · 6dec748
1 parent 6a6f583
commit 6dec748
Show file tree

Hide file tree

Showing 35 changed files with 254 additions and 176 deletions.
diff --git a/diesel/src/expression/array_comparison.rs b/diesel/src/expression/array_comparison.rs
@@ -52,22 +52,6 @@ impl<T, U> Expression for NotIn<T, U> where
     type SqlType = Bool;
 }
 
-impl<T, U, QS> SelectableExpression<QS> for In<T, U> where
-    In<T, U>: Expression,
-    T: SelectableExpression<QS>,
-    U: SelectableExpression<QS>,
-{
-    type SqlTypeForSelect = Self::SqlType;
-}
-
-impl<T, U, QS> SelectableExpression<QS> for NotIn<T, U> where
-    NotIn<T, U>: Expression,
-    T: SelectableExpression<QS>,
-    U: SelectableExpression<QS>,
-{
-    type SqlTypeForSelect = Self::SqlType;
-}
-
 impl<T, U> NonAggregate for In<T, U> where
     In<T, U>: Expression,
 {
@@ -138,6 +122,8 @@ impl<T, U, DB> QueryFragment<DB> for NotIn<T, U> where
 
 impl_query_id!(In<T, U>);
 impl_query_id!(NotIn<T, U>);
+impl_selectable_expression!(In<T, U>);
+impl_selectable_expression!(NotIn<T, U>);
 
 use std::marker::PhantomData;
 use query_builder::{SelectStatement, BoxedSelectStatement};
@@ -202,12 +188,18 @@ impl<T> MaybeEmpty for Many<T> {
 }
 
 impl<T, QS> SelectableExpression<QS> for Many<T> where
-    Many<T>: Expression,
+    Many<T>: AppearsOnTable<QS>,
     T: SelectableExpression<QS>,
 {
     type SqlTypeForSelect = T::SqlTypeForSelect;
 }
 
+impl<T, QS> AppearsOnTable<QS> for Many<T> where
+    Many<T>: Expression,
+    T: AppearsOnTable<QS>,
+{
+}
+
 impl<T, DB> QueryFragment<DB> for Many<T> where
     DB: Backend,
     T: QueryFragment<DB>,
@@ -252,12 +244,18 @@ impl<T, ST> MaybeEmpty for Subselect<T, ST> {
 }
 
 impl<T, ST, QS> SelectableExpression<QS> for Subselect<T, ST> where
-    Subselect<T, ST>: Expression,
+    Subselect<T, ST>: AppearsOnTable<QS>,
     T: Query,
 {
     type SqlTypeForSelect = ST;
 }
 
+impl<T, ST, QS> AppearsOnTable<QS> for Subselect<T, ST> where
+    Subselect<T, ST>: Expression,
+    T: Query,
+{
+}
+
 impl<T, ST, DB> QueryFragment<DB> for Subselect<T, ST> where
     DB: Backend,
     T: QueryFragment<DB>,

diff --git a/diesel/src/expression/bound.rs b/diesel/src/expression/bound.rs
@@ -4,7 +4,7 @@ use backend::Backend;
 use query_builder::*;
 use result::Error::SerializationError;
 use result::QueryResult;
-use super::{Expression, SelectableExpression, NonAggregate};
+use super::*;
 use types::{HasSqlType, ToSql, IsNull};
 
 #[derive(Debug, Clone, Copy)]
@@ -64,11 +64,16 @@ impl<T: QueryId, U> QueryId for Bound<T, U> {
 }
 
 impl<T, U, QS> SelectableExpression<QS> for Bound<T, U> where
-    Bound<T, U>: Expression,
+    Bound<T, U>: AppearsOnTable<QS>,
 {
     type SqlTypeForSelect = T;
 }
 
+impl<T, U, QS> AppearsOnTable<QS> for Bound<T, U> where
+    Bound<T, U>: Expression,
+{
+}
+
 impl<T, U> NonAggregate for Bound<T, U> where
     Bound<T, U>: Expression,
 {

diff --git a/diesel/src/expression/coerce.rs b/diesel/src/expression/coerce.rs
@@ -1,7 +1,7 @@
 use std::marker::PhantomData;
 
 use backend::Backend;
-use expression::{Expression, SelectableExpression, NonAggregate};
+use expression::*;
 use query_builder::*;
 use result::QueryResult;
 
@@ -44,6 +44,11 @@ impl<T, ST, QS> SelectableExpression<QS> for Coerce<T, ST> where
     type SqlTypeForSelect = Self::SqlType;
 }
 
+impl<T, ST, QS> AppearsOnTable<QS> for Coerce<T, ST> where
+    T: AppearsOnTable<QS>,
+{
+}
+
 impl<T, ST, DB> QueryFragment<DB> for Coerce<T, ST> where
     T: QueryFragment<DB>,
     DB: Backend,

diff --git a/diesel/src/expression/count.rs b/diesel/src/expression/count.rs
@@ -1,7 +1,7 @@
 use backend::Backend;
 use query_builder::*;
 use result::QueryResult;
-use super::{Expression, SelectableExpression};
+use super::Expression;
 use types::BigInt;
 
 /// Creates a SQL `COUNT` expression
@@ -78,10 +78,7 @@ impl<T: QueryFragment<DB>, DB: Backend> QueryFragment<DB> for Count<T> {
 }
 
 impl_query_id!(Count<T>);
-
-impl<T: Expression, QS> SelectableExpression<QS> for Count<T> {
-    type SqlTypeForSelect = BigInt;
-}
+impl_selectable_expression!(Count<T>);
 
 #[derive(Debug, Clone, Copy)]
 #[doc(hidden)]
@@ -106,8 +103,5 @@ impl<DB: Backend> QueryFragment<DB> for CountStar {
     }
 }
 
-impl<QS> SelectableExpression<QS> for CountStar {
-    type SqlTypeForSelect = BigInt;
-}
-
 impl_query_id!(CountStar);
+impl_selectable_expression!(CountStar);
diff --git a/diesel/src/expression/exists.rs b/diesel/src/expression/exists.rs
@@ -1,5 +1,5 @@
 use backend::Backend;
-use expression::{Expression, SelectableExpression, NonAggregate};
+use expression::{Expression, NonAggregate};
 use query_builder::*;
 use result::QueryResult;
 use types::Bool;
@@ -49,12 +49,6 @@ impl<T> Expression for Exists<T> where
     type SqlType = Bool;
 }
 
-impl<T, QS> SelectableExpression<QS> for Exists<T> where
-    Exists<T>: Expression,
-{
-    type SqlTypeForSelect = Bool;
-}
-
 impl<T> NonAggregate for Exists<T> {
 }
 
@@ -80,3 +74,4 @@ impl<T, DB> QueryFragment<DB> for Exists<T> where
 }
 
 impl_query_id!(Exists<T>);
+impl_selectable_expression!(Exists<T>);
diff --git a/diesel/src/expression/functions/aggregate_folding.rs b/diesel/src/expression/functions/aggregate_folding.rs
@@ -1,5 +1,5 @@
 use backend::Backend;
-use expression::{Expression, SelectableExpression};
+use expression::Expression;
 use query_builder::*;
 use result::QueryResult;
 use types::{Foldable, HasSqlType};
@@ -50,13 +50,7 @@ macro_rules! fold_function {
         }
 
         impl_query_id!($type_name<T>);
-
-        impl<T, QS> SelectableExpression<QS> for $type_name<T> where
-            $type_name<T>: Expression,
-            T: SelectableExpression<QS>,
-        {
-            type SqlTypeForSelect = Self::SqlType;
-        }
+        impl_selectable_expression!($type_name<T>);
     }
 }
 

diff --git a/diesel/src/expression/functions/aggregate_ordering.rs b/diesel/src/expression/functions/aggregate_ordering.rs
@@ -1,5 +1,5 @@
 use backend::Backend;
-use expression::{Expression, SelectableExpression};
+use expression::Expression;
 use query_builder::*;
 use result::QueryResult;
 use types::{SqlOrd, HasSqlType, IntoNullable};
@@ -49,13 +49,7 @@ macro_rules! ord_function {
         }
 
         impl_query_id!($type_name<T>);
-
-        impl<T, QS> SelectableExpression<QS> for $type_name<T> where
-            $type_name<T>: Expression,
-            T: SelectableExpression<QS>,
-        {
-            type SqlTypeForSelect = Self::SqlType;
-        }
+        impl_selectable_expression!($type_name<T>);
     }
 }
 

diff --git a/diesel/src/expression/functions/date_and_time.rs b/diesel/src/expression/functions/date_and_time.rs
@@ -1,5 +1,5 @@
 use backend::Backend;
-use expression::{Expression, SelectableExpression, NonAggregate};
+use expression::{Expression, NonAggregate};
 use query_builder::*;
 use result::QueryResult;
 use types::*;
@@ -14,10 +14,6 @@ impl Expression for now {
     type SqlType = Timestamp;
 }
 
-impl<QS> SelectableExpression<QS> for now {
-    type SqlTypeForSelect = Timestamp;
-}
-
 impl NonAggregate for now {
 }
 
@@ -37,6 +33,7 @@ impl<DB: Backend> QueryFragment<DB> for now {
 }
 
 impl_query_id!(now);
+impl_selectable_expression!(now);
 
 operator_allowed!(now, Add, add);
 operator_allowed!(now, Sub, sub);

diff --git a/diesel/src/expression/functions/mod.rs b/diesel/src/expression/functions/mod.rs
@@ -62,12 +62,22 @@ macro_rules! sql_function_body {
 
         #[allow(non_camel_case_types)]
         impl<$($arg_name),*, QS> $crate::expression::SelectableExpression<QS> for $struct_name<$($arg_name),*> where
-            $($arg_name: $crate::expression::SelectableExpression<QS>,)*
-            $struct_name<$($arg_name),*>: $crate::expression::Expression,
+            $($arg_name: $crate::expression::SelectableExpression<
+              QS,
+              SqlTypeForSelect = <$arg_name as $crate::expression::Expression>::SqlType,
+            >,)*
+            $struct_name<$($arg_name),*>: $crate::expression::AppearsOnTable<QS>,
         {
             type SqlTypeForSelect = Self::SqlType;
         }
 
+        #[allow(non_camel_case_types)]
+        impl<$($arg_name),*, QS> $crate::expression::AppearsOnTable<QS> for $struct_name<$($arg_name),*> where
+            $($arg_name: $crate::expression::AppearsOnTable<QS>,)*
+            $struct_name<$($arg_name),*>: $crate::expression::Expression,
+        {
+        }
+
         #[allow(non_camel_case_types)]
         impl<$($arg_name),*> $crate::expression::NonAggregate for $struct_name<$($arg_name),*> where
             $($arg_name: $crate::expression::NonAggregate,)*
@@ -136,6 +146,9 @@ macro_rules! no_arg_sql_function_body_except_to_sql {
             type SqlTypeForSelect = $return_type;
         }
 
+        impl<QS> $crate::expression::AppearsOnTable<QS> for $type_name {
+        }
+
         impl $crate::expression::NonAggregate for $type_name {
         }
 

diff --git a/diesel/src/expression/grouped.rs b/diesel/src/expression/grouped.rs
@@ -1,5 +1,5 @@
 use backend::Backend;
-use expression::{Expression, SelectableExpression, NonAggregate};
+use expression::{Expression, NonAggregate};
 use query_builder::*;
 use result::QueryResult;
 
@@ -29,13 +29,7 @@ impl<T: QueryFragment<DB>, DB: Backend> QueryFragment<DB> for Grouped<T> {
 }
 
 impl_query_id!(Grouped<T>);
-
-impl<T, QS> SelectableExpression<QS> for Grouped<T> where
-    T: SelectableExpression<QS>,
-    Grouped<T>: Expression,
-{
-    type SqlTypeForSelect = T::SqlTypeForSelect;
-}
+impl_selectable_expression!(Grouped<T>);
 
 impl<T: NonAggregate> NonAggregate for Grouped<T> where
     Grouped<T>: Expression,

diff --git a/diesel/src/expression/mod.rs b/diesel/src/expression/mod.rs
@@ -102,6 +102,27 @@ impl<T: Expression> AsExpression<T::SqlType> for T {
     }
 }
 
+/// Indicates that all elements of an expression are valid given a from clause.
+/// This is used to ensure that `users.filter(posts::id.eq(1))` fails to
+/// compile. This constraint is only used in places where the nullability of a
+/// SQL type doesn't matter (everything except `select` and `returning`). For
+/// places where nullability is important, `SelectableExpression` is used
+/// instead.
+pub trait AppearsOnTable<QS>: Expression {
+}
+
+impl<T: ?Sized, QS> AppearsOnTable<QS> for Box<T> where
+    T: AppearsOnTable<QS>,
+    Box<T>: Expression,
+{
+}
+
+impl<'a, T: ?Sized, QS> AppearsOnTable<QS> for &'a T where
+    T: AppearsOnTable<QS>,
+    &'a T: Expression,
+{
+}
+
 /// Indicates that an expression can be selected from a source. The associated
 /// type is usually the same as `Expression::SqlType`, but is used to indicate
 /// that a column is always nullable when it appears on the right side of a left
@@ -110,20 +131,20 @@ impl<T: Expression> AsExpression<T::SqlType> for T {
 /// Columns will implement this for their table. Certain special types, like
 /// `CountStar` and `Bound` will implement this for all sources. All other
 /// expressions will inherit this from their children.
-pub trait SelectableExpression<QS>: Expression {
+pub trait SelectableExpression<QS>: AppearsOnTable<QS> {
     type SqlTypeForSelect;
 }
 
 impl<T: ?Sized, QS> SelectableExpression<QS> for Box<T> where
     T: SelectableExpression<QS>,
-    Box<T>: Expression,
+    Box<T>: AppearsOnTable<QS>,
 {
     type SqlTypeForSelect = T::SqlTypeForSelect;
 }
 
 impl<'a, T: ?Sized, QS> SelectableExpression<QS> for &'a T where
     T: SelectableExpression<QS>,
-    &'a T: Expression,
+    &'a T: AppearsOnTable<QS>,
 {
     type SqlTypeForSelect = T::SqlTypeForSelect;
 }

diff --git a/diesel/src/expression/nullable.rs b/diesel/src/expression/nullable.rs
@@ -1,5 +1,5 @@
 use backend::Backend;
-use expression::{Expression, SelectableExpression, NonAggregate};
+use expression::*;
 use query_builder::*;
 use result::QueryResult;
 use types::IntoNullable;
@@ -42,11 +42,17 @@ impl<T, DB> QueryFragment<DB> for Nullable<T> where
 /// nullable.
 impl<T, QS> SelectableExpression<QS> for Nullable<T> where
     T: SelectableExpression<QS>,
-    Nullable<T>: Expression,
+    Nullable<T>: AppearsOnTable<QS>,
 {
     type SqlTypeForSelect = Self::SqlType;
 }
 
+impl<T, QS> AppearsOnTable<QS> for Nullable<T> where
+    T: AppearsOnTable<QS>,
+    Nullable<T>: Expression,
+{
+}
+
 impl<T: QueryId> QueryId for Nullable<T> {
     type QueryId = T::QueryId;