Tracking issue for future-incompatibility lint coherence_leak_check

[nikomatsakis]

This is the tracking issue for the "universe transition". The goal of this page is describe why this change was made and how you can fix code that is affected by it. It also provides a place to ask questions or register a complaint if you feel the change should not be made. For more information on the policy around bug fixes that affect existing code, see our breaking change policy guidelines.

What was the change and what code is affected?

This change (introduced in #55517) fixed a number of bugs (e.g., #32330) in how the Rust compiler handled higher-ranked types (e.g., fn(&u8) or -- more explicitly -- for<'a> fn(&'a u8)) and trait bounds (e.g., for<'a> T: Trait<'a>). One of these changes could however affect existing code. In particular, the compiler in the past would accept trait implementations for identical functions that differed only in where the lifetime binder appeared:

trait SomeTrait { }
impl SomeTrait for for<'a> fn(&'a u8) { }
impl<'a> SomeTrait for fn(&'a u8) { }

We no longer accept both of these impls. Code relying on this pattern would now have to introduce "newtypes", like struct Foo(for<'a> fn(&'a u8)).

History of this change

This change was first made in #55517 without a "warning period". This was done because (a) it would be challenging to issue precise warnings for affected code and (b) a crater run found zero regressions.

However, the change was subsequently backed out in #58592, owing to concerns about unintended side-effects.

The change was re-added in #65232, but this time with a warning period. We are currently completing the implementation and trying to figure out how to draw the line in terms of what should become a hard error.

Affected crates and patterns

This section is for collecting patterns and examples to look into or other related issues.

• wasm-bindgen was reported to get warnings in replace the leak check with universes, take 2 #65232
• @quark-zju commented that their crate was affected here
• coercion becomes order dependent for higher-ranked functions #73154 -- order dependent coercion

[SimonSapin]

Oh hey, this one again.

This breaking change is the exact opposite of another breaking change from August 2017 made in #43880 and discussed FCP’ed in #44224.

Like before, Servo is affected. The fix is to remove the generic trait impl for fn(&A) -> B (keeping the one for fn(A) -> B) that we added in servo/servo#18327 to fix the other breaking change. I don’t know how much code outside of Servo is affected.

In the abstract, the behavior now in Nightly makes more sense to be. fn(&A) -> B is a special case of fn(AA) -> B (with AA = &A), so it should be covered by a trait impl for the latter without a separate impl being needed.

However, it doesn’t feel good that we pushed the ecosystem through a breakage (even if it might only affect a small fraction of the code out there) with lengthy technical arguments (that I don’t entirely understand) in #44224 to justify it, only to change our mind a couple years later and cause breakage again. Do those arguments no longer apply? Did something change, or were they not good arguments in the first place? What is the confidence that this time, reverting is the right decision?

The goal of this page is describe why this change was made and how you can fix code that is affected by it.

As far as I can find, the issue description says what the change is but not why it was made.

a crater run found zero regressions

I’m worried that we rely on Crater so much for this kind of decision. It can only test a sample of all the Rust code that exists in the world, and that sample likely has a heavy selection bias toward (relatively) small libraries rather than large applications.

How much of https://www.rust-lang.org/production/users is tested by crater?

[dtolnay]

I filed #57362 to follow up on a confusing sort of error message introduced in #55517.

error[E0308]: mismatched types
  --> src/main.rs:13:7
   |
13 |     a.f();
   |       ^ one type is more general than the other
   |
   = note: expected type `Trait`
              found type `Trait`

[nikomatsakis]

OK, so, due to the All Hands etc, this issue wound up somewhat neglected for too long. But @aturon and I have been digging into it lately and I wanted to summarize current thinking

Perhaps the TL;DR is that I am considering trying to land #58592, which kind of "reimplements" the old check we used to do, but using the infrastructure of the newer system. The reason for this is not because the old behavior was more correct, but to give us time to explore the new behavior in more depth, as there are concerns that it may interact in unsound ways with other parts of the system which would have to be corrected.

However, I am not sure if this is a good idea. The new behavior has not yet hit stable, but it's been on nightly for some time. Crater runs did not show major disruption (though there have been a few bugs reported since). Nightly apps, like servo, have adapted (more on that in a bit). Moreover, as far as we know, the new higher-ranked code is basically correct -- we are mostly concerns that there may be interactions with other parts of the system that will require further adjustments, and hence there is reason to want to tread cautiously here (e.g., maybe we would want to update those other parts of the system before adopting the new universe system).

Let me review briefly what was affected.

Subtyping, type equality, and 'empty. The newer system permits some forms of type equality that were a bit surprising to us, but which make sense when you follow through the implications. An example is that fn(&'empty u8) and for<'a> fn(&'a u8) are considered the same type. If you have never heard of 'empty, don't worry -- it's an internal compiler region that represents a reference that is never valid. It's not something that was meant to be "exposed" to end-users in any particular way, it's more of an intermediate inference state. Intuitively, what this is saying is that "a function which takes a reference that it never uses (fn(&'empty u8)) can safely be considered as a function that takes a reference with any lifetime (fn(&u8))" (and vice versa). The vice versa part is a touch sketchy -- for it to be correct, it relies on other rules which say that the function could never be called, because when a call occurs, we require the argument types to be in-scope. This is making @aturon and I a bit nervous, although we've not yet pinpointed a problem.

A side-effect of this change is #46989: an impl<A> Foo for fn(A) can be applied to fn(&u8), by inferring A to be &'empty u8.** Another side-effect is that the older coherence rules changed** -- they permitted both the impl<A> Foo for fn(A) and impl Foo for fn(&u8), but both of those implies apply equally well to fn(&'empty u8). (This was expected, but it affects servo, as I'll discuss later on.)

This change to subtyping and equality seems "correct" on its own. However, there are two concerns:

• Until now, 'empty wasn't really "exposed" in this way -- it was basically a special region used during region inference, but it didn't really show up in the "final types" that we inferred at any point. Or at least I don't think it did.
• Allowing 'empty to be inferred in more places may interact in surprising (or even unsound) ways with other parts of Rust's overall setup. For example, the RFC 1214 rules assume that an associated type project like <T as Trait>::Output outlives 'a if all of its "input types" outlive 'a -- so e.g. if T: 'a. But you can have a setup like this:

impl<A> Foo for fn(A) {
   type Output = A;
}

and now if you have <fn(&u8) as Foo>::Output, the resulting type is &'empty u8. Clearly &'empty u8: 'static does not hold, even though fn(&u8): 'static does hold. As it happens, this particular part of RFC 1214 is something that I've been wanting to revisit, and it may be that we have to because of interactions like this. That said, @aturon and I haven't yet found any clear problems (I'll be posting a video soon where we discuss this and other things for about an hour). Other parts of the system that are worth diving into around this area are the implicit bounds and perhaps the dropck setup.

Meanwhile, we have #57639 -- this regression is different. It is a pre-existing bug in the trait solver (#21974), but one that is now affecting more code. We think we have plans to solve this too, but some experimentation is warranted, particularly when it comes to the older trait solve (the chalk solver would require a different approach).

Finally, Servo has been affected here by two independent changes that interact. First, we modified method resolution to use the trait checking code rather than "rolling its own" checks. The trait solver code however was too conservative, and thus wound up assuming that an impl for fn(A) for all A did not apply to fn(&u32). As I've just noted above, this is actually -- arguably -- wrong, though it depends on 'empty for that to be true. The workaround that @SimonSapin added was to add a separate impl, but that is not needed with this new change (and is in fact illegal).

[aturon]

Thanks @nikomatsakis for the writeup and the PR.

FWIW, it seems wisest to me to go ahead and land the leak check just to keep our options maximally open, and to explore these issues without time pressure.

[SimonSapin]

Servo can adapt one way or another, and we already have to deal with worse breakage on Nightly. Don’t worry about us. But Servo might be the canary in the coal mine.

What I’m worried about is projects who might be using similar code on Stable:

impl<A, B> MyTrait for fn(A) -> B {}
impl<'a, A, B> MyTrait fn(&'a A) -> B {}

Some time in 2017, this project updated their compiler and had to add the second impl: #44224

I think intuitively that this second impl shouldn’t be necessary (all types that it covers should already be covered by the first one through simple substitution). But that’s perhaps less important than the fact that any breakage on Stable weakens or “stability without stagnation” promise. But maybe fixing something was worth it in this case, we (the Rust project) do occasionally make minor breaking changes when they are justified.

#55517 landed in the 1.33 cycle, which reaches the Stable channel in 9 days. As it is, this hypothetical project will experience another breaking change when they upgrade to 1.33. Maybe that’s not what’s happening, but the perception is that we were wrong to make the change in 2017 and the whole thing could have been avoided. To someone who hasn’t been following multiple long in-depth discussions, it might even look frivolous or careless.

Now we are talking about temporarily reverting the reversal. If this lands in 1.34 or later, affected project who thought they could rely on stability might have to change their code up to four times, worsening this perception even further.

So please strongly consider backporting to 1.33 beta this [temporary reversal of the reversal] so that #55517 does not reach the Stable channel, or not doing it at all. But time is getting very short.

@rust-lang/release given that builds are made a few days in advance, how much time before the planned release date should a backport land in the beta branch in order to make it in a given cycle?

[aturon]

@SimonSapin

So please strongly consider backporting to 1.33 beta this [temporary reversal of the reversal] so that #55517 does not reach the Stable channel, or not doing it at all. But time is getting very short.

To clarify, that is the ultimate intent of the PR, and the reason we are scrambling to get it together.

[pietroalbini]

rust-lang/release given that builds are made a few days in advance, how much time before the planned release date should a backport land in the beta branch in order to make it in a given cycle?

The promotion from beta to stable happens on Monday, so by Sunday it should be at least beta-approved and merged on master (we can include backports directly in the PR doing the promotion, but it has to be on master first).

[pietroalbini]

Nominating for the release team meeting this evening.

[pietroalbini]

Discussed this a bit in the release meeting, we're willing to delay beta -> stable promotion a bit if this doesn't make it in.

[Jules-Bertholet]

For wasm-bindgen: with negative impls integrated into coherence, would

impl<'a, T: ?Sized> !FromWasmAbi for &'a T {}

resolve the issue?

[Jules-Bertholet]

@rustbot label C-future-compatibility