Correctly enforce nullability with nested joins

When multiple joins are nested together, the resulting type is
essentially a binary tree of each individal join. What this means is
that Diesel only has to consider one join at a time in trait
implementations.

Prior to this commit, when considering nullability, we mistakenly only
considered the outermost join. "Considering nullability" in this case
means `impl SelectableExpression<TheJoin> for column`. If that trait is
implemented, we allow selecting the column directly. If it appears on
the right side of a left join, we do not want that trait to be
implemented, but we do want it implemented for `Nullable<column>`.

The problem code lived in two generated impls in the `table!` macro. For
each individual column, we generate two relevant `SelectableExpression`
impls: one for inner joins, and one for left joins. Both of these impls
were wrong.

The impl for left joins was written as:

    Left: AppearsInFromClause<$table, Count=Once>,
    Right: AppearsInFromClause<$table, Count=Never>,

which in English means "where my table appears once in the left side of
the join, and doesn't appear in the right side of the join". This
incorrectly assumes that `$table` is the left-most table, or all joins
contained in `Left` are inner joins. If `Left == Join<other_table,
$table, LeftOuter>`, this constraint incorrectly holds. This first case
was much easier to solve. The first line is changed to:

    Self: SelectableExpression<Left>

The inner join case was much harder to solve. The constraint was written
as:

    Join<Left, Right, Inner>: AppearsInFromClause<$table, Count=Once>,

In English, this means "my table appears in this join clause exactly
once". This fails to consider that the position the table appears in may
be the right side of a left join. What we really want to write here is
something like this:

    Self: SelectableExpression<Left> | SelectableExpression<Right>,

Where the pipe means "either of these constraints are true". Of course
no such impl exists. If we write two separate impls, they are
overlapping (and overlapping marker traits are not stable). So we need
something different.

Since the problem case applies only to columns, we know that the only
reason that `SelectableExpression<Left>` and
`SelectableExpression<Right>` both hold is if the table appears more
than once in the from clause, which we already reject. Because of this,
we know that when we want this impl to apply, `(<Left as
AppearsInFromClause<$table>>::Count, <Right as
AppearsInFromClause<$table>>::Count>)` will either be `(Once, Never)` or
`(Never, Once)`.

Now that we know we have two disjoint types, we can use that to
implement a new trait, which gives us either `Left` or `Right` depending
on which of the two counts was `Once`, and use that as the parameter for
`SelectableExpression`. In English, the new bounds mean "I am selectable
from whichever of these two type parameters contains my table"

Since this new trait is only meant as a hack for this single impl, and
is not public API, I wanted to avoid having it appear in compiler
errors. For this reason, I've added additional impls for all the invalid
count combinations. This will lead to an error such as:

> table3::id: SelectableExpression<this_table_does_not_appear_in_the_from_clause>

instead of

> (Never, Never): Select<table1, table2>

I did not add explicit compile tests for these errors, as I want to
allow for the deletion of those impls if they become a problem in the
future.

This lead to the final challenge which is that our `impl
SelectableExpression<Join<...>> for Nullable<T>` actually relied on this
bug. We considered `Nullable<T>: SelectableExpression<AnyJoin>` if `T:
SelectableExpression<InnerJoin>`. This of course would break if the
actual table is on the right side of a nested left join. To work around
this, we've introduced another new trait which recursively converts all
joins in a tree to inner joins, which we then use for our check. This
also simplified the various impls for `Nullable<T>`, and should
correctly ensure that `Nullable<T>` is always usable.

Fixes #2001.

CHANGELOG.md

@@ -18,15 +18,11 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
 * All expression methods can now be called on expressions of nullable types.
 
 * Added `BoxedSqlQuery`. This allows users to do a variable amount of `.sql` or
-
   `.bind` calls without changing the underlying type.
 
 * Added `.sql` to `SqlQuery` and `UncheckedBind` to allow appending SQL code to
-
   an existing query.
 
-
-
 ### Removed
 
 * All previously deprecated items have been removed.
@@ -61,6 +57,12 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
   have been. All types in Diesel are now correctly only considered
   non-aggregate if their parts are.
 
+* Nullability requirements are now properly enforced for nested joins.
+  Previously, only the rules for the outer-most join were considered. For
+  example, `users.left_join(posts).left_join(comments)` would allow selecting
+  any columns from `posts`. That will now fail to compile, and any selections
+  from `posts` will need to be made explicitly nullable.
+
 ### Deprecated
 
 * `diesel_(prefix|postfix|infix)_operator!` have been deprecated. These macros

diesel/src/expression/nullable.rs

@@ -1,7 +1,7 @@
 use backend::Backend;
 use expression::*;
 use query_builder::*;
-use query_source::Table;
+use query_source::joins::ToInnerJoin;
 use result::QueryResult;
 use sql_types::IntoNullable;
 
@@ -50,7 +50,7 @@ impl<T: QueryId> QueryId for Nullable<T> {
 impl<T, QS> SelectableExpression<QS> for Nullable<T>
 where
     Self: AppearsOnTable<QS>,
-    T: SelectableExpression<QS>,
-    QS: Table,
+    QS: ToInnerJoin,
+    T: SelectableExpression<QS::InnerJoin>,
 {
 }

diesel/src/lib.rs

@@ -95,8 +95,8 @@
 
 #![cfg_attr(feature = "unstable", feature(specialization))]
 // Built-in Lints
-#![deny(
-    warnings,
+#![deny(warnings)]
+#![warn(
     missing_debug_implementations,
     missing_copy_implementations,
     missing_docs

diesel/src/macros/mod.rs

@@ -43,7 +43,9 @@ macro_rules! __diesel_column {
             Join<Left, Right, LeftOuter>,
         > for $column_name where
             $column_name: AppearsOnTable<Join<Left, Right, LeftOuter>>,
-            Left: AppearsInFromClause<$table, Count=Once>,
+            Self: SelectableExpression<Left>,
+            // If our table is on the right side of this join, only
+            // `Nullable<Self>` can be selected
             Right: AppearsInFromClause<$table, Count=Never>,
         {
         }
@@ -52,7 +54,12 @@ macro_rules! __diesel_column {
             Join<Left, Right, Inner>,
         > for $column_name where
             $column_name: AppearsOnTable<Join<Left, Right, Inner>>,
-            Join<Left, Right, Inner>: AppearsInFromClause<$table, Count=Once>,
+            Left: AppearsInFromClause<$table>,
+            Right: AppearsInFromClause<$table>,
+            (Left::Count, Right::Count): Select<Left, Right>,
+            Self: SelectableExpression<<
+                <(Left::Count, Right::Count) as Select<Left, Right>>::Selection,
+            >,
         {
         }
 
@@ -790,7 +797,7 @@ macro_rules! __diesel_table_impl {
                 use $crate::backend::Backend;
                 use $crate::query_builder::{QueryFragment, AstPass, SelectStatement};
                 use $crate::query_source::joins::{Join, JoinOn, Inner, LeftOuter};
-                use $crate::query_source::{AppearsInFromClause, Once, Never};
+                use $crate::query_source::{AppearsInFromClause, Once, Never, Select};
                 use $crate::result::QueryResult;
                 $($imports)*
 

diesel/src/query_source/joins.rs

@@ -121,35 +121,6 @@ where
     }
 }
 
-impl<Left, Right, Kind, T> SelectableExpression<Join<Left, Right, Kind>> for Nullable<T>
-where
-    T: SelectableExpression<Join<Left, Right, Inner>>,
-    Nullable<T>: AppearsOnTable<Join<Left, Right, Kind>>,
-{
-}
-
-// FIXME: Remove this when overlapping marker traits are stable
-impl<Join, On, T> SelectableExpression<JoinOn<Join, On>> for Nullable<T>
-where
-    Nullable<T>: SelectableExpression<Join>,
-    Nullable<T>: AppearsOnTable<JoinOn<Join, On>>,
-{
-}
-
-// FIXME: Remove this when overlapping marker traits are stable
-impl<From, T> SelectableExpression<SelectStatement<From>> for Nullable<T>
-where
-    Nullable<T>: SelectableExpression<From>,
-    Nullable<T>: AppearsOnTable<SelectStatement<From>>,
-{
-}
-
-// FIXME: We want these blanket impls when overlapping marker traits are stable
-// impl<T, Join, On> SelectableExpression<JoinOn<Join, On>> for T where
-//     T: SelectableExpression<Join> + AppearsOnTable<JoinOn<Join, On>>,
-// {
-// }
-
 /// Indicates that two tables can be joined without an explicit `ON` clause.
 ///
 /// Implementations of this trait are generated by invoking [`joinable!`].
@@ -300,3 +271,41 @@ where
         (rhs.source, rhs.on)
     }
 }
+
+#[doc(hidden)]
+/// Convert any joins in a `FROM` clause into an inner join.
+///
+/// This trait is used to determine whether
+/// `Nullable<T>: SelectableExpression<SomeJoin>`. We consider it to be
+/// selectable if `T: SelectableExpression<InnerJoin>`. Since `SomeJoin`
+/// may be deeply nested, we need to recursively change any appearances of
+/// `LeftOuter` to `Inner` in order to perform this check.
+pub trait ToInnerJoin {
+    type InnerJoin;
+}
+
+impl<Left, Right, Kind> ToInnerJoin for Join<Left, Right, Kind>
+where
+    Left: ToInnerJoin,
+    Right: ToInnerJoin,
+{
+    type InnerJoin = Join<Left::InnerJoin, Right::InnerJoin, Inner>;
+}
+
+impl<Join, On> ToInnerJoin for JoinOn<Join, On>
+where
+    Join: ToInnerJoin,
+{
+    type InnerJoin = JoinOn<Join::InnerJoin, On>;
+}
+
+impl<From> ToInnerJoin for SelectStatement<From>
+where
+    From: ToInnerJoin,
+{
+    type InnerJoin = SelectStatement<From::InnerJoin>;
+}
+
+impl<T: Table> ToInnerJoin for T {
+    type InnerJoin = T;
+}

diesel/src/query_source/mod.rs

@@ -76,3 +76,83 @@ pub trait AppearsInFromClause<QS> {
     /// How many times does `Self` appear in `QS`?
     type Count;
 }
+
+#[doc(hidden)]
+/// Used to determine which of two from clauses contains a given table.
+///
+/// This trait can be used to emulate "or" conditions in where clauses when
+/// we want a trait to be implemented with one of two type parameters.
+///
+/// For example, if we wanted to write:
+///
+/// ```rust,ignore
+/// where
+///     T: SelectableExpression<Left> | SelectableExpression<Right>,
+/// ```
+///
+/// we can emulate this by writing:
+///
+/// ```rust,ignore
+/// where
+///     Left: AppearsInFromClause<T::Table>,
+///     Right: AppearsInFromClause<T::Table>,
+///     (Left::Count, Right::Count): Select<Left, Right>,
+///     T: SelectableExpression<
+///         <(Left::Count, Right::Count) as Select<Left, Right>>::Selection,
+///     >,
+/// ```
+///
+/// In order to aquire the counts in the first place, we must already know
+/// the table we're searching for.
+pub trait Select<Left, Right> {
+    /// The selected type.
+    ///
+    /// For `(Once, Never)` this type will be `Left`. For `(Never, Once)`, this type will be
+    /// `Right`
+    type Selection;
+}
+
+impl<Left, Right> Select<Left, Right> for (Once, Never) {
+    type Selection = Left;
+}
+
+impl<Left, Right> Select<Left, Right> for (Never, Once) {
+    type Selection = Right;
+}
+
+#[doc(hidden)]
+#[allow(
+    non_camel_case_types,
+    missing_debug_implementations,
+    missing_copy_implementations
+)]
+/// Everything in this module is here to give something more helpful than:
+///
+/// > (Never, Never): Select<table1, table2> is not satisifed
+///
+/// Any of these impls can be deleted if they are getting in the way of
+/// other functionality. Any code which is using these impls is already
+/// failing to compile.
+mod impls_which_are_only_here_to_improve_error_messages {
+    use super::*;
+
+    pub struct this_table_doesnt_appear_in_the_from_clause_of_your_query;
+
+    impl<Left, Right> Select<Left, Right> for (Never, Never) {
+        type Selection = this_table_doesnt_appear_in_the_from_clause_of_your_query;
+    }
+
+    pub struct this_table_appears_in_your_query_more_than_once_and_must_be_aliased;
+
+    impl<Left, Right, OtherCount> Select<Left, Right> for (MoreThanOnce, OtherCount) {
+        type Selection = this_table_appears_in_your_query_more_than_once_and_must_be_aliased;
+    }
+
+    impl<Left, Right> Select<Left, Right> for (Never, MoreThanOnce) {
+        type Selection = this_table_appears_in_your_query_more_than_once_and_must_be_aliased;
+    }
+
+    impl<Left, Right> Select<Left, Right> for (Once, MoreThanOnce) {
+        type Selection = this_table_appears_in_your_query_more_than_once_and_must_be_aliased;
+    }
+}

diesel_compile_tests/tests/compile-fail/right_side_of_left_join_requires_nullable.rs

@@ -18,26 +18,122 @@ table! {
     }
 }
 
+table! {
+    pets {
+        id -> Integer,
+        user_id -> Integer,
+        name -> Text,
+    }
+}
+
 joinable!(posts -> users (user_id));
-allow_tables_to_appear_in_same_query!(posts, users);
+joinable!(pets -> users (user_id));
+allow_tables_to_appear_in_same_query!(posts, users, pets);
 sql_function!(fn lower(x: Text) -> Text);
 
 fn main() {
-    let conn = PgConnection::establish("some url").unwrap();
+}
+
+fn direct_joins() {
     let join = users::table.left_outer_join(posts::table);
 
     // Invalid, only Nullable<title> is selectable
     let _ = join.select(posts::title);
     //~^ ERROR E0271
+    //~| ERROR E0277
+    // Valid
+    let _ = join.select(posts::title.nullable());
+    // Valid -- NULL to a function will return null
+    let _ = join.select(lower(posts::title).nullable());
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(lower(posts::title));
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Invalid, Nullable<title> is selectable, but lower expects not-null
+    let _ = join.select(lower(posts::title.nullable()));
+    //~^ ERROR E0271
     //~| ERROR E0271
+}
+
+fn nested_outer_joins_left_associative() {
+
+    let join = users::table.left_outer_join(posts::table).left_outer_join(pets::table);
+
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(posts::title);
+    //~^ ERROR E0271
+    //~| ERROR E0277
     // Valid
     let _ = join.select(posts::title.nullable());
     // Valid -- NULL to a function will return null
     let _ = join.select(lower(posts::title).nullable());
     // Invalid, only Nullable<title> is selectable
     let _ = join.select(lower(posts::title));
     //~^ ERROR E0271
+    //~| ERROR E0277
+    // Invalid, Nullable<title> is selectable, but lower expects not-null
+    let _ = join.select(lower(posts::title.nullable()));
+    //~^ ERROR E0271
     //~| ERROR E0271
+}
+
+fn nested_mixed_joins_left_associative() {
+    let join = users::table.left_outer_join(posts::table).inner_join(pets::table);
+
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(posts::title);
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Valid
+    let _ = join.select(posts::title.nullable());
+    // Valid -- NULL to a function will return null
+    let _ = join.select(lower(posts::title).nullable());
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(lower(posts::title));
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Invalid, Nullable<title> is selectable, but lower expects not-null
+    let _ = join.select(lower(posts::title.nullable()));
+    //~^ ERROR E0271
+    //~| ERROR E0271
+}
+
+fn nested_outer_joins_right_associative() {
+    let join = pets::table.left_outer_join(users::table.left_outer_join(posts::table));
+
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(posts::title);
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Valid
+    let _ = join.select(posts::title.nullable());
+    // Valid -- NULL to a function will return null
+    let _ = join.select(lower(posts::title).nullable());
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(lower(posts::title));
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Invalid, Nullable<title> is selectable, but lower expects not-null
+    let _ = join.select(lower(posts::title.nullable()));
+    //~^ ERROR E0271
+    //~| ERROR E0271
+}
+
+fn nested_mixed_joins_right_associative() {
+    let join = pets::table.inner_join(users::table.left_outer_join(posts::table));
+
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(posts::title);
+    //~^ ERROR E0271
+    //~| ERROR E0277
+    // Valid
+    let _ = join.select(posts::title.nullable());
+    // Valid -- NULL to a function will return null
+    let _ = join.select(lower(posts::title).nullable());
+    // Invalid, only Nullable<title> is selectable
+    let _ = join.select(lower(posts::title));
+    //~^ ERROR E0271
+    //~| ERROR E0277
     // Invalid, Nullable<title> is selectable, but lower expects not-null
     let _ = join.select(lower(posts::title.nullable()));
     //~^ ERROR E0271