Rust: Diesel's build is failing on beta and nightly

Created on 2 Jan 2018 · 52Comments · Source: rust-lang/rust

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.

P-high T-compiler T-libs regression-from-stable-to-beta

Source

sgrif

Most helpful comment

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:

https://github.com/rust-lang/rust/blob/ed9751a903b483769d3d418818b331c867f0417f/src/librustc/traits/select.rs#L1097-L1099

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.

nikomatsakis on 25 Jan 2018

🎉4 ❤3

All 52 comments

This is now a regression from stable to beta

sgrif on 3 Jan 2018

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

shssoichiro on 4 Jan 2018

❤1

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

sgrif on 4 Jan 2018

It looks like the regression originated in 8c5941896. 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.

shssoichiro on 4 Jan 2018

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

nikomatsakis on 4 Jan 2018

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

nikomatsakis on 4 Jan 2018

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 on 5 Jan 2018

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 on 5 Jan 2018

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 on 15 Jan 2018

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

sgrif on 15 Jan 2018

@sgrif huh, creepy.

nikomatsakis on 16 Jan 2018

@sgrif but still failing on beta?

nikomatsakis on 16 Jan 2018

Still failing on beta. I'm going to fix our usage of file! today to confirm we're actually green on nightly.

sgrif on 16 Jan 2018

Or do you mean there was a new beta release this morning that you want me to check? I haven't checked since last night

sgrif on 16 Jan 2018

@sgrif I am trying a cargo build of diesel right now with beta, actually, though if you want to check that'd be good too

nikomatsakis on 16 Jan 2018

cc me

arielb1 on 16 Jan 2018

@sgrif any tips on what I should be building? I was getting errors about missing mysql + postgres drivers, which I guess I would have to install, but maybe I'm building the wrong thing.

nikomatsakis on 16 Jan 2018

I am able to build the diesel crate on beta though (e.g., cd diesel; cargo build)

nikomatsakis on 16 Jan 2018

You should run the test suite for diesel_cli. cd diesel_cli; cargo +beta test --no-default-features --features sqlite

sgrif on 16 Jan 2018

(Yes that requires SQLite installed on the system. sudo apt-get install libsqlite3-dev on Ubuntu)

sgrif on 16 Jan 2018

It may fail before it even tries to link it though

sgrif on 16 Jan 2018

Just ran rustup update beta and can confirm it's still failing. Interestingly only the sqlite feature fails to compile. postgres and mysql compile but fail because of the cargo change. I'm pretty sure all three backends used to fail.

sgrif on 16 Jan 2018

@sgrif ok, great, I can reproduce now... thanks!

nikomatsakis on 16 Jan 2018

Thanks for working on this! :heart:

sgrif on 16 Jan 2018

Can confirm that it works on nightly, oddly enough.

nikomatsakis on 16 Jan 2018

Yeah, I tested nightlies to get that commit range I posted above (commits taken from rustc --version on the last failing nightly and the first passing nightly).

Also slightly more narrow repro is just cd diesel; cargo +beta build --features sqlite

sgrif on 16 Jan 2018

So, something between nightly 2018-01-05 and 2018-01-06 changed the behavior (as @sgrif already reported, just confirming). For the record, I am looking at diesel commit ddc376bc4e1db974d5eadd2e35f8962edda0555b

nikomatsakis on 16 Jan 2018

Some code around these traits changed in Diesel master, and I'm now seeing failures on nightly as well as beta again. It also is happening regardless of backend, instead of just when SQLite is enabled. Seems to support that the problem is a deeper issue where random code is somehow changing unrelated trait resolution, rather than something that changed in the specific commit where it started or the commit where we had stopped seeing it.

sgrif on 18 Jan 2018

Sorry I didn't get a chance to investigate today, but I plan to do more investigation tomorrow.

nikomatsakis on 18 Jan 2018

❤1

So I spent some time bisecting. It appears that the commit which "makes this fail" is 3f29e2b495a6b875af835f1fff64256e2d58d2b6:

commit 3f29e2b495a6b875af835f1fff64256e2d58d2b6
Author: Sebastian Dröge <[email protected]>
Date:   Wed Jan 3 12:26:55 2018 +0200

    Fix compilation of TrustedRandomAccess impl for slice::Chunks

    https://github.com/rust-lang/rust/pull/47113 renamed the private size
    field to chunk_size for consistency.

I don't really think this commit is at fault, mind you. =)

This previous merge commit passes (b107f720e5422bff4fa0671e54ff5458f682f603). (The intermediate commits don't build.)

nikomatsakis on 20 Jan 2018

Hmm. I don't seem to be able to so readily reproduce anymore.

nikomatsakis on 22 Jan 2018

It appears that the problem in selection is that selection's anti-recursion stack is not maintained through projection.

Log snippet for my reference:

DEBUG:rustc::traits::select: evaluate_predicate_recursively(Obligation(predicate=Binder(ProjectionPredicate(ProjectionTy { substs: Slice([&_, _]), item_def_id: DefId(0/0:686 ~ diesel[32cc]::insertable[0]::Insertable[0]::Values[0]) }, query_builder::insert_statement::ValuesClause<_, _>)),depth=60))
DEBUG:rustc::traits::select: evaluate_predicate_recursively(Obligation(predicate=Binder(ProjectionPredicate(ProjectionTy { substs: Slice([&_, _]), item_def_id: DefId(0/0:686 ~ diesel[32cc]::insertable[0]::Insertable[0]::Values[0]) }, query_builder::insert_statement::ValuesClause<_, _>)),depth=62))

DEBUG:rustc::traits::select: evaluate_candidate: depth=50 candidate=ImplCandidate(DefId(0/0:4910 ~ diesel[32cc]::type_impls[0]::tuples[0]::{{impl}}[55]))
DEBUG:rustc::traits::select: vtable_impl: impl_def_id=DefId(0/0:4910 ~ diesel[32cc]::type_impls[0]::tuples[0]::{{impl}}[55]) impl_obligations=[Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=51), Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=51), Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=51), Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=51), Obligation(predicate=Binder(TraitPredicate(<(&_, &_, &_) as insertable::Insertable<_>>)),depth=51)]
DEBUG:rustc::traits::select: candidate_from_obligation(cache_fresh_trait_pred=Binder(TraitPredicate(<(&FreshTy(0), &FreshTy(1), &FreshTy(2)) as insertable::Insertable<FreshTy(3)>>)), obligation=TraitObligationStack(Obligation(predicate=Binder(TraitPredicate(<(&_, &_, &_) as insertable::Insertable<_>>)),depth=51)))

DEBUG:rustc::traits::select: vtable_impl: impl_def_id=DefId(0/0:4907 ~ diesel[32cc]::type_impls[0]::tuples[0]::{{impl}}[54]) impl_obligations=[Obligation(predicate=Binder(ProjectionPredicate(ProjectionTy { substs: Slice([&_, _]), item_def_id: DefId(0/0:686 ~ diesel[32cc]::insertable[0]::Insertable[0]::Values[0]) }, query_builder::insert_statement::ValuesClause<_, _>)),depth=52), Obligation(predicate=Binder(ProjectionPredicate(ProjectionTy { substs: Slice([&_, _]), item_def_id: DefId(0/0:686 ~ diesel[32cc]::insertable[0]::Insertable[0]::Values[0]) }, query_builder::insert_statement::ValuesClause<_, _>)),depth=52), Obligation(predicate=Binder(ProjectionPredicate(ProjectionTy { substs: Slice([&_, _]), item_def_id: DefId(0/0:686 ~ diesel[32cc]::insertable[0]::Insertable[0]::Values[0]) }, query_builder::insert_statement::ValuesClause<_, _>)),depth=52), Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=52), Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=52), Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=52), Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=52), Obligation(predicate=Binder(TraitPredicate(<&_ as insertable::Insertable<_>>)),depth=52), Obligation(predicate=Binder(TraitPredicate(<&_ as std::marker::Sized>)),depth=52), Obligation(predicate=Binder(TraitPredicate(<&_ as insertable::Insertable<_>>)),depth=52), Obligation(predicate=Binder(TraitPredicate(<&_ as std::marker::Sized>)),depth=52), Obligation(predicate=Binder(TraitPredicate(<&_ as insertable::Insertable<_>>)),depth=52), Obligation(predicate=Binder(TraitPredicate(<&_ as std::marker::Sized>)),depth=52)]

DEBUG:rustc::traits::select: evaluate_predicate_recursively(Obligation(predicate=Binder(ProjectionPredicate(ProjectionTy { substs: Slice([&_, _]), item_def_id: DefId(0/0:686 ~ diesel[32cc]::insert
able[0]::Insertable[0]::Values[0]) }, query_builder::insert_statement::ValuesClause<_, _>)),depth=52))
COMMENT MINE: <&_ as Insertable<_>>::Values
DEBUG:rustc::traits::select: select(Obligation(predicate=Binder(TraitPredicate(<&_ as insertable::Insertable<_>>)),depth=52))

DEBUG:rustc::traits::select: vtable_impl: impl_def_id=DefId(0/0:4910 ~ diesel[32cc]::type_impls[0]::tuples[0]::{{impl}}[55]) impl_obligations=[Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=53), Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=53), Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=53), Obligation(predicate=Binder(TraitPredicate(<_ as std::marker::Sized>)),depth=53), Obligation(predicate=Binder(TraitPredicate(<(&_, &_, &_) as insertable::Insertable<_>>)),depth=53)]

arielb1 on 22 Jan 2018

So:

try to evaluate <(&$0, &$1, &$2) as Insertable<$3>>
for that, we need <&$0 as Insertable<$3>>::Values = ValueClause<_, _>, so project <&$0 as Insertable<$3>>::Values
try to evaluate <&$0 as Insertable<$3>>::Values
...that can hold via the bridge impl, if $0 = ($4, $5, $6) and &(&$4, &$5, &$6): Insertable<$3> holds.

arielb1 on 22 Jan 2018

If this was a selection-only loop, we have logic that returns an ambiguous result. However, we don't do that if there are projections within the loop.

Now we also need to create a minified version of this.

arielb1 on 22 Jan 2018

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

nickbabcock on 22 Jan 2018

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

sgrif on 23 Jan 2018

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

sgrif on 23 Jan 2018

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.

nickbabcock on 23 Jan 2018

This failure seems like it might be a plausible model:

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

nikomatsakis on 23 Jan 2018

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 on 23 Jan 2018

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 on 23 Jan 2018

@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 on 23 Jan 2018

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.

nikomatsakis on 24 Jan 2018

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

sgrif on 24 Jan 2018

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 on 24 Jan 2018

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

nikomatsakis on 24 Jan 2018

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

sgrif on 24 Jan 2018

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 on 24 Jan 2018

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 on 24 Jan 2018

And the commit that cause the issue to re-appear is https://github.com/diesel-rs/diesel/commit/8da028e12ab4374a487d69ec06ecb6ef9b12dc02

sgrif on 24 Jan 2018

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:

https://github.com/rust-lang/rust/blob/ed9751a903b483769d3d418818b331c867f0417f/src/librustc/traits/select.rs#L1097-L1099

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

nikomatsakis on 25 Jan 2018

🎉4 ❤3

Fixed by #47738

sgrif on 31 Jan 2018

🎉2

Was this page helpful?

0 / 5 - 0 ratings

Related issues

Documentation: Unclear difference between "string slices" and "string literals" for newcomers

defuz · 3Comments

ThreadRng performance bug

SharplEr · 3Comments

Internal compiler error: cannot relate bound region (likely caused by `conservative_impl_trait`)

jmegaffin · 3Comments

Integer division/remainder rounding behavior should be documented

wthrowe · 3Comments

detect when "unconstrained type parameters" could be provided explicitly to a fn call

nikomatsakis · 3Comments