fix and test #43475

[Twey]

Fix #43475

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[eddyb]

r? @nikomatsakis

[estebank]

This PR introduces a failure to run-pass/specialization/assoc-ty-graph-cycle.rs.

[arielb1]

@nikomatsakis is on vacation until Aug 15 and I'll like him to have a look on this PR, but try to fix your test failure beforehand.

[arielb1]

[00:45:23] ---- [run-pass] run-pass/specialization/assoc-ty-graph-cycle.rs stdout ----
[00:45:23] 	
[00:45:23] error: compilation failed
[00:45:23] status: exit code: 101
[00:45:23] command: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc /checkout/src/test/run-pass/specialization/assoc-ty-graph-cycle.rs -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass --target=x86_64-unknown-linux-gnu -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/specialization/assoc-ty-graph-cycle.stage2-x86_64-unknown-linux-gnu.run-pass.libaux -C prefer-dynamic -o /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/specialization/assoc-ty-graph-cycle.stage2-x86_64-unknown-linux-gnu -Crpath -O -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers
[00:45:23] stdout:
[00:45:23] ------------------------------------------
[00:45:23] 
[00:45:23] ------------------------------------------
[00:45:23] stderr:
[00:45:23] ------------------------------------------
[00:45:23] error: internal compiler error: /checkout/src/librustc/traits/specialize/mod.rs:98: When translating substitutions for specialization, the expected specialization failed to hold
[00:45:23]
[00:45:23] note: the compiler unexpectedly panicked. this is a bug.
[00:45:23] 
[00:45:23] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:45:23] 
[00:45:23] note: rustc 1.21.0-dev (ae337e946 2017-08-04) running on x86_64-unknown-linux-gnu
[00:45:23] 
[00:45:23] thread 'rustc' panicked at 'Box<Any>', /checkout/src/librustc_errors/lib.rs:486:8
[00:45:23] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:45:23] 
[00:45:23] 
[00:45:23] ------------------------------------------
[00:45:23] 
[00:45:23] thread '[run-pass] run-pass/specialization/assoc-ty-graph-cycle.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2499:8
[00:45:23] note: Run with `RUST_BACKTRACE=1` for a backtrace.

[00:45:23] 

[00:45:23] 

[00:45:23] failures:

[00:45:23]     [run-pass] run-pass/specialization/assoc-ty-graph-cycle.rs
[00:45:23] 

[carols10cents]

Hi @Twey! Do you need any help with the test failure?

[Twey]

Sorry guys, I was also out sick and then on holiday the past week. Am back now and working on this. If anybody can immediately see what's causing this I'd appreciate the hint; currently adding a ton of debug information to track down where the behaviour on this test diverges.

[nikomatsakis]

Back now, will try to get to this ASAP!

[Twey]

Think I isolated the issue; made that test pass, but I have to run, so pushing now. Hopefully works.

[arielb1]

I think you want to use normalize_with_depth to avoid cycles

[Twey]

I'm suspicious of line 1445. Since we have an instance of the trait, surely any ‘obligations’ incurred by normalizing the trait bound are actually hypotheses. But I don't know where I could put these.

[Twey]

I've removed that line since it was getting in the way of something I wanted to do, but presumably I should be doing something with those obligations. Or is it always true that it's safe to ignore them, since we have a trait bound there anyway?

[carols10cents]

ping @nikomatsakis! pinging you on IRC too!

[nikomatsakis]

Sorry for being slow.

[nikomatsakis]

@Twey

I've removed that line since it was getting in the way of something I wanted to do, but presumably I should be doing something with those obligations

Can you see a bit more about how it was getting in your way?

I agree we should be doing something with those obligations. I am a bit reluctant to land this PR since I am sort in the midst of trying to overhaul how we handle normalization and I worry that this will affect compilation times and so forth, but it seems like the right general idea. I think in general that we ought to be combining the obligations (rather than ignoring them...) and propagating them back out: certainly dropping them on the floor is no good. For example, the inference value may have some variables in it which are ultimately constrained by those obligations.

[Twey]

AFAIUI, skol_trait_ref refers to the obligation to be fulfilled, whereas trait_bound refers to the bound that is in scope and against which we hope to match skol_trait_ref. That is, trait_bound is already fulfilled, somewhere, and therefore (I believe) any obligations that may arise from normalizing it must also be fulfilled, perhaps by other trait bounds that are in scope — they're not obligations at all, but elaborations of the call bounds. So adding them to inferred_obligations is never the right thing to do, and can cause an unnecessary overflow on hypothetically-correct code as we re-introduce already-fulfilled obligations, which happened to me :). I previously believed that the right thing to do was to add them to a list of bounds that are in scope, but it's not clear to me where that would be, and I now believe that would be redundant anyway.

Anecdotally, this change doesn't seem to drastically affect compile times where it isn't required — on my machines rustc (+ stdlib) build time seems to be ~50 minutes with or without the change, though I suspect Travis could give more information to someone better able to interpret its output.

[nikomatsakis]

@Twey

That is, trait_bound is already fulfilled, somewhere, and therefore (I believe) any obligations that may arise from normalizing it must also be fulfilled, perhaps by other trait bounds that are in scope — they're not obligations at all, but elaborations of the call bounds.

Right, so that sorta' makes sense, but there are cases where indeed we do have to fulfill, in order to unify various variables. (That is, the result of normalization can include fresh type variables which are unconstrained except in those returned predicates.) So, as an extreme example, we might have something like:

impl<T, U> Iterator for MyFoo<T>
    where T: Iterator<Item = U>
{
    type Item = U;
    ...
}

Now if we have an impl that references <MyFoo<X> as Iterator>::Item, this will get normalized to a fresh inference variable (let's call it ?Y) and we will return a constraint like X: Iterator<Item = ?Y>. Processing that constraint is what gives us the value of ?Y. If we just drop the constraint on the floor, that might never get processed at all, and ?Y would remain unresolved.

So adding them to inferred_obligations is never the right thing to do, and can cause an unnecessary overflow on hypothetically-correct code as we re-introduce already-fulfilled obligations, which happened to me :).

Do you have an example? Or did this happen when bootstrapping or something like that?

[Twey]

Alas, since it didn't compile, I think I scrapped it. I'll try to build rustc with the additional obligations and do a bit of a git dive in the next few days in case I've relied on it somewhere that I can genericize into a MWE, but I can't make any promises. It was quite a finicky one involving recursively-bounded associated types.

[Twey]

Managed to dig it up!

#![feature(never_type, specialization)]

pub struct Foo<P>(!, P);
pub trait Bar { type Arthur; }
pub trait Baz<P> { type Arthur: Baz<<P as Bar>::Arthur>; }
impl<P> Bar for P { default type Arthur = !; }
impl<P: Bar> Bar for Foo<P> { type Arthur = P; }
pub trait BazFoo<P>: Baz<Foo<P>> { }

This builds with eda00c0 but overflows with a44cc9a, because the constraint Arthur: Sized is treated as an obligation rather than a hypothesis. But in fact we know that if we have an impl T: Foo<P> then P: Sized, so I believe that it is correct for this code to compile.

[carols10cents]

@nikomatsakis @Twey what's the status of this PR? I'm having a hard time telling-- I started breaking out in hives when I saw skol ;)

[Twey]

@carols10cents I'm waiting for @nikomatsakis to respond with a better suggestion as to what to do with the incurred obligations that doesn't break correct code or a rationale as to why my counterexample actually shouldn't compile.

Edit: if I understand correctly, the difference between our cases is that I want to drop bounds that are specified on the trait, but you're saying that the important bounds we want to keep arise from the impl, right? So perhaps the correct behaviour is for normalize_with_depth to not return bounds on traits as obligations in the first place? Then we could always safely add the returned obligations to the obligation stack.

[nikomatsakis]

Sorry, I've been slow to respond. Partly this is because I think that the need for this PR will be obviated by some of the changes we've been planning for trait matching and selection -- in particular by the ongoing work on lazy normalization. But that branch is not yet to fruition and will probably be some time coming, so I should invest some time in thinking through this PR to see if we can take this approach in the meantime I suppose.

[carols10cents]

ping @nikomatsakis, how's the thinking going? :)

[Twey]

MWE with some extra constraints but without any feature flags, in case that makes it easier to think about:

pub struct Foo<P>(P);
pub trait Bar { type Arthur: Bar; }
pub trait Baz<P: Bar> { type Arthur: Baz<P::Arthur>; }
impl<P: Bar> Bar for Foo<P> { type Arthur = P; }
pub trait BazFoo<P>: Baz<Foo<P>> where P: Bar { }

[shepmaster]

Ping @rust-lang/compiler. It's now been 15 days since we last heard from @nikomatsakis. Anyone care to help think things through?

[shepmaster]

Thank you so much for your hard work, @Twey!

Unfortunately, I checked with some members of the compiler team and they believe that this PR happens to come at a bad time. It turns out that trait selection is under redesign, and it's believed that the issue at hand should be fixed by lazy normalization.

Because of that, we are going to go ahead and close this. I've made a note on the original issue to make sure the test case makes its way into that eventual work.

Please don't let this discourage you from making further contributions!

[aidanhs]

I have an interest in this issue as well - is there anywhere we can follow lazy normalization work?

[shepmaster]

@aidanhs I don't know fully, but I hope that @arielb1 could point you in the right direction.