Diesel's build is failing on beta and nightly

[sgrif]

There appears to be some regression in trait resolution between beta and nightly. Our builds are failing on nightly with this error: https://travis-ci.org/diesel-rs/diesel/jobs/324077372#L1081

I haven't yet looked into what the exact cause is, but the same code is compiling successfully on stable and beta.

[sgrif]

This is now a regression from stable to beta

[shssoichiro]

I have some time now, I can go through and do a bisect to find the cause.

[sgrif]

@shssoichiro Thank you! <3 I'm sorry I haven't provided a more specific reproduction case

[shssoichiro]

It looks like the regression originated in 8c59418. My first guess at what's happening is that the blanket impl<T> SliceContains for T where T: PartialEq is causing an infinite recursion in trying to resolve diesel's Insertable trait, although I haven't created a minimized reproduction.

[nikomatsakis]

Hmm. Any idea how those two traits are connected? That seems surprising.

[nikomatsakis]

I'd like to dig a bit more into this and understand better what's going on.

[sgrif]

I'm not sure how SliceContains could affect Diesel, but I agree that it's the only thing in that commit that looks like it could possibly cause any problems here.

[sgrif]

FWIW the impls it appears to be recursing through look like this:

impl<T, Tab> Insertable<Tab> for Option<T>
where
    T: Insertable<Tab>,
    T::Values: Default,
{
    // ...
}

impl<'a, T, Tab> Insertable<Tab> for &'a Option<T>
where
    Option<&'a T>: Insertable<Tab>,
{
    // ...
}

[sgrif]

I'm no longer seeing this on nightly (our builds are still failing due to #47462). The bug was fixed somewhere in b98fd524e...6828cf901

[sgrif]

I don't see anything in that diff that looks like it could have in any way affected this.

[nikomatsakis]

@sgrif huh, creepy.

[nikomatsakis]

@sgrif but still failing on beta?

[nickbabcock]

I've noticed that I no longer see the issue on 1.24.0-beta.7 (c7037ff 2018-01-21), so it looks like a recent beta version fixed it.

[sgrif]

beta.7 is still failing on Travis for us. https://travis-ci.org/diesel-rs/diesel/jobs/331917195

[sgrif]

As is the most recent nightly https://travis-ci.org/diesel-rs/diesel/jobs/331917198

[nickbabcock]

Weird, I have both the beta.7 and nightly for diesel_cli 1.1.0 working fine. And a couple weeks ago this was broken.

[nikomatsakis]

This failure seems like it might be a plausible model:

https://play.rust-lang.org/?gist=30e5797dec3f425a6d82f1f6a12dc0d0&version=stable

[nikomatsakis]

The play snippet above fails with this:

error[E0275]: overflow evaluating the requirement `_#127t: std::marker::Sized`
  --> /home/nmatsakis/tmp/issue-47139.rs:42:12
   |
42 |     foo::< <Option<&_> as Insertable<()>>::Values >();
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: consider adding a `#![recursion_limit="128"]` attribute to your crate
   = note: required because of the requirements on the impl of `Insertable<()>` for `&'_#31r std::option::Option<_#127t>`
   = note: required because of the requirements on the impl of `Insertable<()>` for `std::option::Option<&'_#31r std::option::Option<_#127t>>`
   = note: required because of the requirements on the impl of `Insertable<()>` for `&'_#30r std::option::Option<std::option::Option<_#127t>>`
   = note: required because of the requirements on the impl of `Insertable<()>` for `std::option::Option<&'_#30r std::option::Option<std::option::Option<_#127t>>>`
   = note: required because of the requirements on the impl of `Insertable<()>` for `&'_#29r std::option::Option<std::option::Option<std::option::Option<_#127t>>>`
   = note: required because of the requirements on the impl of `Insertable<()>` for `std::option::Option<&'_#29r std::option::Option<std::option::Option<std::option::Option<_#127t>>>>`
   = note: required because of the requirements on the impl of `Insertable<()>` for `&'_#28r std::option::Option<std::option::Option<std::option::Option<std::option::Option<_#127t>>>>`

where diesel fails with:

error[E0275]: overflow evaluating the requirement `<&'_#32r _#127t as insertable::Insertable<_#130t>>::Values`
  |
  = help: consider adding a `#![recursion_limit="128"]` attribute to your crate
  = note: required because of the requirements on the impl of `insertable::Insertable<_#128t>` for `std::option::Option<&'_#32r _#127t>`
  = note: required because of the requirements on the impl of `insertable::Insertable<_#126t>` for `&'_#31r std::option::Option<_#127t>`
  = note: required because of the requirements on the impl of `insertable::Insertable<_#124t>` for `std::option::Option<&'_#31r std::option::Option<_#127t>>`
  = note: required because of the requirements on the impl of `insertable::Insertable<_#122t>` for `&'_#30r std::option::Option<std::option::Option<_#127t>>`
  = note: required because of the requirements on the impl of `insertable::Insertable<_#120t>` for `std::option::Option<&'_#30r std::option::Option<std::option::Option<_#127t>>>`
  = note: required because of the requirements on the impl of `insertable::Insertable<_#118t>` for `&'_#29r std::option::Option<std::option::Option<std::option::Option<_#127t>>>`
  = note: required because of the requirements on the impl of `insertable::Insertable<_#116t>` for `std::option::Option<&'_#29r std::option::Option<std::option::Option<std::option::Option<_#127t>>>>`
  = note: required because of the requirements on the impl of `insertable::Insertable<_#114t>` for `&'_#28r std::option::Option<std::option::Option<std::option::Option<std::option::Option<_#127t>>>>`

[nikomatsakis]

We can simplify by removing the <Tab> type parameter.

What I don't know is why this failure occurs only sometimes in Diesel (vs predictably for that snippet above). It may be that I'm reproducing a different problem.

UPDATE: Looking at the logging more, I'm pretty sure it's the same basic pattern.

[nikomatsakis]

@arielb1

try to evaluate <(&$0, &$1, &$2) as Insertable<$3>>

Interesting. I feel like I'm observing a different cycle, though one with a similar shape (I mean locally). I hadn't observed those impls yet.

[nikomatsakis]

So it seems to me that extending the "select-project-recursion" nexus is going to be somewhat intrusive. I haven't pushed too hard on it, but it doesn't sound like the kind of thing I would want to backport. I am going to investigate a bit why this error is not happening on nightly -- I find that confusing.

[sgrif]

I am going to investigate a bit why this error is not happening on nightly

This error is definitely happening on nightly. https://travis-ci.org/diesel-rs/diesel/jobs/332968985

[nikomatsakis]

Digging in some more, the overflow is arising (in Diesel) as a result of a coherence check comparing these two impls:

impl<Conn, DB, T> ExecuteDsl<Conn, DB> for T
where
    Conn: Connection<Backend = DB>,
    DB: Backend,
    T: QueryFragment<DB> + QueryId,
{
}

#[cfg(feature = "sqlite")]
impl<'a, T, U, Op> ExecuteDsl<SqliteConnection> for InsertStatement<T, &'a [U], Op>
where
    &'a U: Insertable<T>,
    InsertStatement<T, <&'a U as Insertable<T>>::Values, Op>: QueryFragment<Sqlite>,
    T: Copy,
    Op: Copy,
{
}

[nikomatsakis]

@sgrif Huh. At least in the commit I am looking at, it does not reproduce for some reason.

[sgrif]

@nikomatsakis Yeah, it went away for a bit, then some code in Diesel changed and it came back

[sgrif]

To save time, the way that the first impl applies is through:

impl<T, U, Op, Ret, DB> QueryFragment<DB> for InsertStatement<T, U, Op, Ret>
where
    DB: Backend,
    T: Table,
    T::FromClause: QueryFragment<DB>,
    U: QueryFragment<DB> + CanInsertInSingleQuery<DB>,
    Op: QueryFragment<DB>,
    Ret: QueryFragment<DB>,

The important bit is the CanInsertInSingleQuery, which is what makes it disjoint. The relevant impl that makes it disjoint is:

impl<'a, T, Tab, DB> CanInsertInSingleQuery<DB> for &'a [T]
where
    DB: Backend + SupportsDefaultKeyword,

where we know Sqlite: !SupportsDefaultKeyword. If you look at ExecuteDsl you'll notice some weird shit with the second type parameter (which is fully redundant with the first). It's basically a giant workaround to deal with the fact that associated types aren't taken into account WRT disjointness

[sgrif]

Also just to rule this out, the weird shit we do with the second type parameter to ExecuteDsl has nothing to do with it. The issue still occurs with this patch applied to Diesel master (a patch I want to one day actually be able to commit...)

diff --git a/diesel/src/query_dsl/load_dsl.rs b/diesel/src/query_dsl/load_dsl.rs
index e3a1ea8..3b502b1 100644
--- a/diesel/src/query_dsl/load_dsl.rs
+++ b/diesel/src/query_dsl/load_dsl.rs
@@ -1,4 +1,3 @@
-use backend::Backend;
 use connection::Connection;
 use deserialize::Queryable;
 use query_builder::{AsQuery, QueryFragment, QueryId};
@@ -38,17 +37,15 @@ where
 /// to call `execute` from generic code.
 ///
 /// [`RunQueryDsl`]: ../trait.RunQueryDsl.html
-pub trait ExecuteDsl<Conn: Connection<Backend = DB>, DB: Backend = <Conn as Connection>::Backend>
-    : Sized {
+pub trait ExecuteDsl<Conn>: Sized {
     /// Execute this command
     fn execute(query: Self, conn: &Conn) -> QueryResult<usize>;
 }
 
-impl<Conn, DB, T> ExecuteDsl<Conn, DB> for T
+impl<Conn, T> ExecuteDsl<Conn> for T
 where
-    Conn: Connection<Backend = DB>,
-    DB: Backend,
-    T: QueryFragment<DB> + QueryId,
+    Conn: Connection,
+    T: QueryFragment<Conn::Backend> + QueryId,
 {
     fn execute(query: Self, conn: &Conn) -> QueryResult<usize> {
         conn.execute_returning_count(&query)

[sgrif]

And the commit that cause the issue to re-appear is diesel-rs/diesel@8da028e

[nikomatsakis]

OK, I found out how to make it build again in a fairly surgical way. The problem arises in this part of the coherence check, which is really just an effort to make an improved error message:

rust/src/librustc/traits/select.rs

Lines 1097 to 1099 in ed9751a

	if !candidate_set.ambiguous && candidate_set.vec.iter().all(|c| {
	!self.evaluate_candidate(stack, &c).may_apply()
	}) {

The order of the vector returned by assemble_candidates seems to vary here, which is why the problem comes and goes. The danger is that some of the candidates in that vector cause overflows and some cause errors. Since this is a call to all(), if we happen to encounter one that returns false before the overflow, we're all set.

One simple fix that is obviously safe for beta backport is just not to do this call at all.

[sgrif]

Fixed by #47738