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 the change to LUB/GLB #45852

Closed
nikomatsakis opened this issue Nov 7, 2017 · 2 comments
Closed

Tracking issue for the change to LUB/GLB #45852

nikomatsakis opened this issue Nov 7, 2017 · 2 comments
Labels
C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 7, 2017

This issue proposes to change the LUB/GLB strategy in rustc to adopt something that is more conservative, but also more correct, and also more forwards compatible.

How this affects you

In some limited cases, you may need to alter your Rust code to "cast" the types of functions more explicitly. This can occur specifically when combining function pointers or trait objects that involve lifetimes. For example, the following function no longer compiles:

fn foo(
    x: fn(&u8, &u8),
    y: for<'a> fn(&'a u8, &'a u8),
) {       
    let z = match 22 {
        0 => x,
        _ => y,
    };
}

The fix is to "upcast" x and y into a common type. In this case, since x accepts arguments with two distinct lifetimes, but y requires a single lifetime, we would want to pick the type of y (which is more selective).

fn foo(
    x: fn(&u8, &u8),
    y: for<'a> fn(&'a u8, &'a u8),
) {       
    let z = match 22 {
        0 => x as for<'a> fn(&'a u8, &'a u8),
        //     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ explicit cast
        _ => y,
    };
}

Background: why is this change being made?

Simply put, the existing code for handling function types of this kind if known to be buggy and incomplete. Since this scenario -- combining two function pointers with distinct signatures -- occurs quite rarely (in fact, no known instances were encountered), it was decided to adopt a simpler, more conservative approach that is known to be correct.

More specifically, we adopted a rule that computing the least-upper-bound (common supertype) of two higher-ranked types (e.g., function pointers, trait objects) is done by requiring them to be equal.

What happens now?

Given that we did not find any occurrences of this code in our crater run, and that a full warning period is difficult to achieve, we opted to move straight to a hard-error with this code. If you are having problems fixing your code, or would like to object to this decision, please leave a comment (cc @nikomatsakis) or -- perhaps better yet -- reach out on IRC (nmatsakis).

@nikomatsakis nikomatsakis added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 7, 2017
bors added a commit that referenced this issue Nov 14, 2017
Simplify higher-ranked LUB/GLB

This is a better version of #44211. It still makes higher-ranked LUB/GLB into a hard equality test, however, it does try to identify that something changed and issue a notice to the user. I wroteup #45852 as a tracking issue for this change.

Currently, this moves straight to a hard-error, on the basis that the crater run in #44211 saw no impact. It might be good to retest -- or perhaps to try for a warning period. Trying to do the latter in a precise way would be somewhat painful, but an imprecise way might suffice -- that is, we could issue warning *whenever* a LUB/GLB operation succeeds that will later fail, even if it doesn't ultimately impact the type check. I could experiment with this.

~~I am *mildly* wary about landing this independently of other code that moves to a universe-based system. In particular, I was nervous that this change would make coherence accepts new pairs of impls that will later be errors. I have the code for the universe-based approach available, I hope to open an PR and run some tests on its impact very shortly.~~ @arielb1 points out that I was being silly.

r? @arielb1
bors added a commit that referenced this issue Nov 17, 2017
Simplify higher-ranked LUB/GLB

This is a better version of #44211. It still makes higher-ranked LUB/GLB into a hard equality test, however, it does try to identify that something changed and issue a notice to the user. I wroteup #45852 as a tracking issue for this change.

Currently, this moves straight to a hard-error, on the basis that the crater run in #44211 saw no impact. It might be good to retest -- or perhaps to try for a warning period. Trying to do the latter in a precise way would be somewhat painful, but an imprecise way might suffice -- that is, we could issue warning *whenever* a LUB/GLB operation succeeds that will later fail, even if it doesn't ultimately impact the type check. I could experiment with this.

~~I am *mildly* wary about landing this independently of other code that moves to a universe-based system. In particular, I was nervous that this change would make coherence accepts new pairs of impls that will later be errors. I have the code for the universe-based approach available, I hope to open an PR and run some tests on its impact very shortly.~~ @arielb1 points out that I was being silly.

r? @arielb1
bors added a commit that referenced this issue Nov 17, 2017
Simplify higher-ranked LUB/GLB

This is a better version of #44211. It still makes higher-ranked LUB/GLB into a hard equality test, however, it does try to identify that something changed and issue a notice to the user. I wroteup #45852 as a tracking issue for this change.

Currently, this moves straight to a hard-error, on the basis that the crater run in #44211 saw no impact. It might be good to retest -- or perhaps to try for a warning period. Trying to do the latter in a precise way would be somewhat painful, but an imprecise way might suffice -- that is, we could issue warning *whenever* a LUB/GLB operation succeeds that will later fail, even if it doesn't ultimately impact the type check. I could experiment with this.

~~I am *mildly* wary about landing this independently of other code that moves to a universe-based system. In particular, I was nervous that this change would make coherence accepts new pairs of impls that will later be errors. I have the code for the universe-based approach available, I hope to open an PR and run some tests on its impact very shortly.~~ @arielb1 points out that I was being silly.

r? @arielb1
@Mark-Simulacrum
Copy link
Member

@nikomatsakis Can you verify that the text in this tracking issue is up to date and remove the note about the PR not being landed yet?

@jonas-schievink jonas-schievink added the C-future-incompatibility Category: Future-incompatibility lints label Nov 26, 2019
@jonas-schievink
Copy link
Contributor

Given that the PR making this change has landed 2 years ago, there's no future-compatibility lint to be tracked here, and nobody has complained about breakage so far, I'm going to go ahead and just close this issue. Feel free to reopen if that's wrong though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants