Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracking issue for "universe transition" #56105

Open
nikomatsakis opened this issue Nov 20, 2018 · 22 comments
Open

tracking issue for "universe transition" #56105

nikomatsakis opened this issue Nov 20, 2018 · 22 comments
Labels
A-lifetimes Area: lifetime related C-future-compatibility Category: future compatibility lints C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-impl-incomplete T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 20, 2018

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.

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Nov 20, 2018
@nikomatsakis nikomatsakis changed the title universe transition compatibility issue tracking issue for "universe transition" Nov 20, 2018
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jan 2, 2019
@SimonSapin
Copy link
Contributor

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?

bors-servo pushed a commit to servo/servo that referenced this issue Jan 4, 2019
Upgrade to rustc 1.33.0-nightly (c0bbc3927 2019-01-03)

This deals with a breaking change in the language that affect stable code, in the exact opposite way from a previous breaking change from August 2017. See rust-lang/rust#56105 (comment)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22612)
<!-- Reviewable:end -->
JohnHeitmann pushed a commit to JohnHeitmann/rust that referenced this issue Jan 5, 2019
@dtolnay
Copy link
Member

dtolnay commented Jan 5, 2019

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
Copy link
Contributor Author

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
Copy link
Member

aturon commented Feb 20, 2019

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
Copy link
Contributor

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
Copy link
Member

aturon commented Feb 20, 2019

@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
Copy link
Member

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 pietroalbini added I-nominated T-release Relevant to the release subteam, which will review and decide on the PR/issue. labels Feb 20, 2019
@pietroalbini
Copy link
Member

Nominating for the release team meeting this evening.

@pietroalbini
Copy link
Member

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.

@nikomatsakis
Copy link
Contributor Author

@alexcrichton thanks for noting that. I do intend to look into what we can do to fix wasm-bindgen, yes, but I'm glad it's written out.

@jonas-schievink jonas-schievink added A-lifetimes Area: lifetime related and removed T-release Relevant to the release subteam, which will review and decide on the PR/issue. labels May 12, 2020
@nikomatsakis
Copy link
Contributor Author

Update: I've posted #72493. It affects this tracking issue in that it makes some patterns into hard-errors. However, the patterns we've seen reported thus far (which are actually the same pattern) still work with a warning. The MCP rust-lang/compiler-team#295 describes my overall plan here and my hope is to keep those patterns working long term.

Manishearth added a commit to Manishearth/rust that referenced this issue Jun 23, 2020
…thewjasper

 move leak-check to during coherence, candidate eval

Implementation of MCP rust-lang/compiler-team#295.

I'd like to do a crater run on this.

Note to @rust-lang/lang: This PR is a breaking change (bugfix). It causes tests like the following to go from a future-compatibility warning rust-lang#56105 to a hard error:

```rust
trait Trait {}
impl Trait for for<'a, 'b> fn(&'a u32, &'b u32) {}
impl Trait for for<'c> fn(&'c u32, &'c u32) {} // now rejected, used to warn
```

I am not aware of any instances of this code in the wild, but that is why we are doing a crater run. The reason for this change is that those two types are, in fact, the same type, and hence the two impls are overlapping.

There will still be impls that trigger rust-lang#56105 after this lands, however -- I hope that we will eventually just accept those impls without warning, for the most part. One example of such an impl is this pattern, which is used by wasm-bindgen and other crates as well:

```rust
trait Trait {}
impl<T> Trait for fn(&T) { }
impl<T> Trait for fn(T) { } // still accepted, but warns
```
@Tamschi Tamschi mentioned this issue Mar 7, 2021
3 tasks
@wesleywiser wesleywiser added WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 S-tracking-impl-incomplete labels Jun 24, 2022
@lcnr lcnr added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 labels Jul 15, 2022
@gyscos
Copy link

gyscos commented Oct 24, 2022

It looks like the situation is still the same as when @quark-zju posted about it - a deprecation warning, and no current workaround?

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Nov 6, 2022

trait Test {}

impl Test for fn(&'static u8) { }
impl Test for for<'a> fn(&'a u8) { }

The code above triggers the coherence_leak_check lint. Is that intentional? If I understand correctly, impl Test for for<'a> fn(&'a u8) { } would become impl Test for fn(&'empty u8) { }. 'empty is a different lifetime than 'static, so I don't think these impls overlap?

EDIT: oh, but 'static <: 'empty, and I guess you can't have a different impl for a subtype and a supertype? Is that the bug that this change is trying to fix?

@Jules-Bertholet
Copy link
Contributor

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

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

resolve the issue?

@Jules-Bertholet
Copy link
Contributor

@rustbot label C-future-compatibility

@rustbot rustbot added the C-future-compatibility Category: future compatibility lints label Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: lifetime related C-future-compatibility Category: future compatibility lints C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. S-tracking-impl-incomplete T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

No branches or pull requests