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

Elided lifetimes in error messages can be incorrect, as well as misleading #87763

Closed
ssbr opened this issue Aug 4, 2021 · 0 comments · Fixed by #89416
Closed

Elided lifetimes in error messages can be incorrect, as well as misleading #87763

ssbr opened this issue Aug 4, 2021 · 0 comments · Fixed by #89416
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ssbr
Copy link

ssbr commented Aug 4, 2021

Given the following code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a08f48d6e07780f4d6a881eddf3459d2

struct S;

trait MyFrom<T> {
    fn from(x: T) -> Self;
}

impl MyFrom<&S> for &S {
    fn from(x: &S) -> &S {
        x
    }
}

The current output is:

error: `impl` item signature doesn't match `trait` item signature
 --> src/lib.rs:8:5
  |
4 |     fn from(x: T) -> Self;
  |     ---------------------- expected `fn(&S) -> &S`
...
8 |     fn from(x: &S) -> &S {
  |     ^^^^^^^^^^^^^^^^^^^^ found `fn(&S) -> &S`
  |
  = note: expected `fn(&S) -> &S`
             found `fn(&S) -> &S`
help: the lifetime requirements from the `impl` do not correspond to the requirements in the `trait`
 --> src/lib.rs:4:16
  |
4 |     fn from(x: T) -> Self;
  |                ^     ^^^^ consider borrowing this type parameter in the trait
  |                |
  |                consider borrowing this type parameter in the trait

error: aborting due to previous error

Ideally the output should look like:

error: `impl` item signature doesn't match `trait` item signature
 --> src/lib.rs:8:5
  |
4 |     fn from(x: T) -> Self;
  |     ---------------------- expected `fn(&'_1 S) -> &'_2 S`
...
8 |     fn from(x: &S) -> &S {
  |     ^^^^^^^^^^^^^^^^^^^^ found `fn(&'_3 S) -> &'_3 S`
  |
  = note: expected `fn(&'_1 S) -> &'_2 S`
             found `fn(&'_3 S) -> &'_3 S`
    note: the expected lifetimes and found lifetimes are both elided, but differ.
help: the lifetime requirements from the `impl` do not correspond to the requirements in the `trait`
 --> src/lib.rs:4:16
  |
4 |     fn from(x: T) -> Self;
  |                ^     ^^^^ consider borrowing this type parameter in the trait
  |                |
  |                consider borrowing this type parameter in the trait

error: aborting due to previous error

Right now, the error message just outputs &S several times, yet there are three different lifetime variables involved here:

  1. '_1, which is the lifetime for the reference that is T
  2. '_2, which is the lifetime for the returned reference that is Self
  3. '_3 the lifetime parameter for the from function in the impl block (which I guess is ~hidden by a for<'_3>)

Because it always elides lifetimes in the error message, the error message produces a false statement of what it expected (it claims to expect fn(&S) -> &S, which is untrue), and presents textually identical "expected" and "found" types.

(Incidentally, the help section about consider borrowing is actually, perhaps accidentally, helpful -- if you move the references to the fn declaration, then they have the same lifetime elision rules as the impl, and everything lines up.)

@ssbr ssbr added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 4, 2021
ssbr added a commit to ssbr/nomicon that referenced this issue Aug 4, 2021
Currently, the lifetime elision doc only documents function definitions, but lifetime elision is also allowed in the following other locations:

* `fn` types, such as `fn(&T)`
* `Fn`/`FnMut`/`FnOnce`, such as `Fn(&T)`
* `impl` headers

To demo this up, I made some type aliases for `fn`/`Fn` which you can pass `&T` as a parameter to (to follow the lifetime rules of the surrounding context), and compared what you get with that instead of using `fn`/`Fn` directly, where lifetime elision takes on the rules from `fn`/`Fn`/etc.

I also demoed up an `impl` header that used lifetime elision twice, although the error message in that case is broken (filed rust-lang/rust#87763)

The demo was half for this change description, and half just to make sure I understand Rust -- in particular, I really had to reverse engineer it for `impl` because I wasn't sure, and it didn't seem to be documented anywhere (at least not here!)

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f82b280de4b992f225bc32121f333e96
notriddle added a commit to notriddle/rust that referenced this issue Oct 1, 2021
As you can see in src/test/ui/traits/self-without-lifetime-constraint.stderr
you can get very confusing type names if you don't have this.

Fixes rust-lang#87763
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 22, 2021
…fetimes-in-region-errors, r=jackh726

nice_region_error: Include lifetime placeholders in error output

As you can see in src/test/ui/traits/self-without-lifetime-constraint.stderr
you can get very confusing type names if you don't have this.

Fixes rust-lang#87763
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 23, 2021
…fetimes-in-region-errors, r=jackh726

nice_region_error: Include lifetime placeholders in error output

As you can see in src/test/ui/traits/self-without-lifetime-constraint.stderr
you can get very confusing type names if you don't have this.

Fixes rust-lang#87763
@bors bors closed this as completed in 98ed554 Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant