Merge pull request diesel-rs#1545 from diesel-rs/sg-fix-select-expressions

Fix `SelectStatement` being incorrectly allowed in an expression context

Author: sgrif

CHANGELOG.md

@@ -75,6 +75,11 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
 * `#[derive(Identifiable)]` now correctly associates `#[primary_key]` with the
   column name, not field name.
 
+* Select statements can no longer incorrectly appear in an expression context.
+
+* `exists` can no longer incorrectly receive values other than select
+  statements.
+
 ### Jokes
 
 * Diesel is now powered by the blockchain because it's 2018.

diesel/src/expression/array_comparison.rs

@@ -1,5 +1,6 @@
 use backend::Backend;
 use expression::*;
+use expression::subselect::Subselect;
 use query_builder::*;
 use result::QueryResult;
 use sql_types::Bool;
@@ -103,7 +104,6 @@ where
 impl_selectable_expression!(In<T, U>);
 impl_selectable_expression!(NotIn<T, U>);
 
-use std::marker::PhantomData;
 use query_builder::{BoxedSelectStatement, SelectStatement};
 
 pub trait AsInExpression<T> {
@@ -136,10 +136,7 @@ where
     type InExpression = Subselect<Self, ST>;
 
     fn as_in_expression(self) -> Self::InExpression {
-        Subselect {
-            values: self,
-            _sql_type: PhantomData,
-        }
+        Subselect::new(self)
     }
 }
 
@@ -150,10 +147,7 @@ where
     type InExpression = Subselect<Self, ST>;
 
     fn as_in_expression(self) -> Self::InExpression {
-        Subselect {
-            values: self,
-            _sql_type: PhantomData,
-        }
+        Subselect::new(self)
     }
 }
 
@@ -209,43 +203,3 @@ impl<T> QueryId for Many<T> {
 
     const HAS_STATIC_QUERY_ID: bool = false;
 }
-
-#[derive(Debug, Copy, Clone, QueryId)]
-pub struct Subselect<T, ST> {
-    values: T,
-    _sql_type: PhantomData<ST>,
-}
-
-impl<T: Expression, ST> Expression for Subselect<T, ST> {
-    type SqlType = ST;
-}
-
-impl<T, ST> MaybeEmpty for Subselect<T, ST> {
-    fn is_empty(&self) -> bool {
-        false
-    }
-}
-
-impl<T, ST, QS> SelectableExpression<QS> for Subselect<T, ST>
-where
-    Subselect<T, ST>: AppearsOnTable<QS>,
-    T: SelectableExpression<QS>,
-{
-}
-
-impl<T, ST, QS> AppearsOnTable<QS> for Subselect<T, ST>
-where
-    Subselect<T, ST>: Expression,
-    T: AppearsOnTable<QS>,
-{
-}
-
-impl<T, ST, DB> QueryFragment<DB> for Subselect<T, ST>
-where
-    DB: Backend,
-    T: QueryFragment<DB>,
-{
-    fn walk_ast(&self, pass: AstPass<DB>) -> QueryResult<()> {
-        self.values.walk_ast(pass)
-    }
-}

diesel/src/expression/exists.rs

@@ -1,14 +1,14 @@
 use backend::Backend;
-use expression::{Expression, NonAggregate};
+use expression::{AppearsOnTable, Expression, NonAggregate, SelectableExpression};
+use expression::subselect::Subselect;
 use query_builder::*;
 use result::QueryResult;
 use sql_types::Bool;
 
 /// Creates a SQL `EXISTS` expression.
 ///
-/// The argument must be a complete SQL query. The result of this could in
-/// theory be passed to `.filter`, but since the query cannot reference columns
-/// from the outer query, this is of limited usefulness.
+/// The argument must be a complete SQL query. The query may reference columns
+/// from the outer table.
 ///
 /// # Example
 ///
@@ -30,15 +30,15 @@ use sql_types::Bool;
 /// # }
 /// ```
 pub fn exists<T>(query: T) -> Exists<T> {
-    Exists(query)
+    Exists(Subselect::new(query))
 }
 
 #[derive(Debug, Clone, Copy, QueryId)]
-pub struct Exists<T>(T);
+pub struct Exists<T>(Subselect<T, ()>);
 
 impl<T> Expression for Exists<T>
 where
-    T: Expression,
+    Subselect<T, ()>: Expression,
 {
     type SqlType = Bool;
 }
@@ -58,4 +58,16 @@ where
     }
 }
 
-impl_selectable_expression!(Exists<T>);
+impl<T, QS> SelectableExpression<QS> for Exists<T>
+where
+    Self: AppearsOnTable<QS>,
+    Subselect<T, ()>: SelectableExpression<QS>,
+{
+}
+
+impl<T, QS> AppearsOnTable<QS> for Exists<T>
+where
+    Self: Expression,
+    Subselect<T, ()>: AppearsOnTable<QS>,
+{
+}

diesel/src/expression/mod.rs

@@ -43,6 +43,8 @@ pub mod nullable;
 pub mod operators;
 #[doc(hidden)]
 pub mod sql_literal;
+#[doc(hidden)]
+pub mod subselect;
 
 #[doc(hidden)]
 pub mod dsl {

diesel/src/expression/subselect.rs

@@ -0,0 +1,60 @@
+use std::marker::PhantomData;
+
+use expression::*;
+use expression::array_comparison::MaybeEmpty;
+use query_builder::*;
+use result::QueryResult;
+
+#[derive(Debug, Copy, Clone, QueryId)]
+pub struct Subselect<T, ST> {
+    values: T,
+    _sql_type: PhantomData<ST>,
+}
+
+impl<T, ST> Subselect<T, ST> {
+    pub(crate) fn new(values: T) -> Self {
+        Self {
+            values,
+            _sql_type: PhantomData,
+        }
+    }
+}
+
+impl<T: SelectQuery, ST> Expression for Subselect<T, ST> {
+    type SqlType = ST;
+}
+
+impl<T, ST> MaybeEmpty for Subselect<T, ST> {
+    fn is_empty(&self) -> bool {
+        false
+    }
+}
+
+impl<T, ST, QS> SelectableExpression<QS> for Subselect<T, ST>
+where
+    Subselect<T, ST>: AppearsOnTable<QS>,
+    T: ValidSubselect<QS>,
+{
+}
+
+impl<T, ST, QS> AppearsOnTable<QS> for Subselect<T, ST>
+where
+    Subselect<T, ST>: Expression,
+    T: ValidSubselect<QS>,
+{
+}
+
+impl<T, ST> NonAggregate for Subselect<T, ST> {}
+
+impl<T, ST, DB> QueryFragment<DB> for Subselect<T, ST>
+where
+    DB: Backend,
+    T: QueryFragment<DB>,
+{
+    fn walk_ast(&self, mut out: AstPass<DB>) -> QueryResult<()> {
+        self.values.walk_ast(out.reborrow())?;
+        Ok(())
+    }
+}
+
+pub trait ValidSubselect<QS> {}

diesel/src/pg/expression/array_comparison.rs

@@ -1,4 +1,5 @@
 use expression::{AsExpression, Expression, NonAggregate};
+use expression::subselect::Subselect;
 use pg::Pg;
 use query_builder::*;
 use result::QueryResult;
@@ -29,7 +30,7 @@ use sql_types::Array;
 /// ```
 pub fn any<ST, T>(vals: T) -> Any<T::Expression>
 where
-    T: AsExpression<Array<ST>>,
+    T: AsArrayExpression<ST>,
 {
     Any::new(vals.as_expression())
 }
@@ -57,7 +58,7 @@ where
 /// ```
 pub fn all<ST, T>(vals: T) -> All<T::Expression>
 where
-    T: AsExpression<Array<ST>>,
+    T: AsArrayExpression<ST>,
 {
     All::new(vals.as_expression())
 }
@@ -139,3 +140,43 @@ where
     Expr: NonAggregate,
 {
 }
+
+pub trait AsArrayExpression<ST> {
+    type Expression: Expression<SqlType = Array<ST>>;
+
+    fn as_expression(self) -> Self::Expression;
+}
+
+impl<ST, T> AsArrayExpression<ST> for T
+where
+    T: AsExpression<Array<ST>>,
+{
+    type Expression = <T as AsExpression<Array<ST>>>::Expression;
+
+    fn as_expression(self) -> Self::Expression {
+        AsExpression::as_expression(self)
+    }
+}
+
+impl<ST, S, F, W, O, L, Of, G, FU> AsArrayExpression<ST>
+    for SelectStatement<S, F, W, O, L, Of, G, FU>
+where
+    Self: SelectQuery<SqlType = ST>,
+{
+    type Expression = Subselect<Self, Array<ST>>;
+
+    fn as_expression(self) -> Self::Expression {
+        Subselect::new(self)
+    }
+}
+
+impl<'a, ST, QS, DB> AsArrayExpression<ST> for BoxedSelectStatement<'a, ST, QS, DB>
+where
+    Self: SelectQuery<SqlType = ST>,
+{
+    type Expression = Subselect<Self, Array<ST>>;
+
+    fn as_expression(self) -> Self::Expression {
+        Subselect::new(self)
+    }
+}

diesel/src/pg/query_builder/distinct_on.rs

@@ -1,8 +1,8 @@
 use pg::Pg;
 use query_dsl::methods::DistinctOnDsl;
-use query_builder::{AstPass, QueryFragment, SelectStatement};
+use query_builder::{AstPass, QueryFragment, SelectQuery, SelectStatement};
 use result::QueryResult;
-use expression::{Expression, SelectableExpression};
+use expression::SelectableExpression;
 
 /// Represents `DISTINCT ON (...)`
 #[derive(Debug, Clone, Copy, QueryId)]
@@ -24,8 +24,8 @@ impl<ST, F, S, D, W, O, L, Of, G, Selection> DistinctOnDsl<Selection>
     for SelectStatement<F, S, D, W, O, L, Of, G>
 where
     Selection: SelectableExpression<F>,
-    Self: Expression<SqlType = ST>,
-    SelectStatement<F, S, DistinctOnClause<Selection>, W, O, L, Of, G>: Expression<SqlType = ST>,
+    Self: SelectQuery<SqlType = ST>,
+    SelectStatement<F, S, DistinctOnClause<Selection>, W, O, L, Of, G>: SelectQuery<SqlType = ST>,
 {
     type Output = SelectStatement<F, S, DistinctOnClause<Selection>, W, O, L, Of, G>;
 

diesel/src/query_builder/mod.rs

@@ -108,6 +108,19 @@ impl<'a, T: Query> Query for &'a T {
     type SqlType = T::SqlType;
 }
 
+/// Indicates that a type is a `SELECT` statement.
+///
+/// This trait differs from `Query` in two ways:
+/// - It is implemented only for select statements, rather than all queries
+///   which return a value.
+/// - It has looser constraints. A type implementing `SelectQuery` is known to
+///   be potentially valid if used as a subselect, but it is not necessarily
+///   able to be executed.
+pub trait SelectQuery {
+    /// The SQL type of the `SELECT` clause
+    type SqlType;
+}
+
 /// An untyped fragment of SQL.
 ///
 /// This may be a complete SQL command (such as an update statement without a

diesel/src/query_builder/select_statement/boxed.rs

@@ -3,6 +3,7 @@ use std::marker::PhantomData;
 use backend::Backend;
 use dsl::AsExprOf;
 use expression::*;
+use expression::subselect::ValidSubselect;
 use insertable::Insertable;
 use query_builder::*;
 use query_builder::distinct_clause::DistinctClause;
@@ -64,14 +65,14 @@ where
     type SqlType = ST;
 }
 
-impl<'a, ST, QS, DB> Expression for BoxedSelectStatement<'a, ST, QS, DB>
+impl<'a, ST, QS, DB> SelectQuery for BoxedSelectStatement<'a, ST, QS, DB>
 where
-    Self: Query<SqlType = ST>,
+    DB: Backend,
 {
     type SqlType = ST;
 }
 
-impl<'a, ST, QS, QS2, DB> AppearsOnTable<QS2> for BoxedSelectStatement<'a, ST, QS, DB>
+impl<'a, ST, QS, QS2, DB> ValidSubselect<QS2> for BoxedSelectStatement<'a, ST, QS, DB>
 where
     Self: Query<SqlType = ST>,
 {