Update E0088 to new format

[yossi-k]

Fixes #35226.
Part of #35233.

r? @jonathandturner

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jonathandturner (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.

[sophiajt]

Now that we handle multiple cases, can you add to the unit test to check for these new cases?

[sophiajt]

Thanks for the PR!

Looks like we'll need to test for some of your new cases. After that I think we're probably good to go.

[yossi-k]

@jonathandturner I'm not much of a rust expert, but does the second case make sense? I can't think of a code to trigger this, perhaps you can point me in some useful direction?

[sophiajt]

You could try an example that tests against two different functions, one with a lifetime and one without. Something like:

fn f() {}

fn g<'a>() {}

fn main() {
    f::<'static>(); //~ ERROR E0088
    //~^ unexpected lifetime parameter

    g::<'static, 'static>();
}

[yossi-k]

@jonathandturner That was also my first guess, but g doesn't expect any lifetimes at all, we need something that expects more than zero.

[sophiajt]

@yossi-k - looks like you're right. When I try it with functions, the lifetime parameters seem to be ignored. If I try it with traits, I just get a different error message:

fn f() {}

fn g<'a>(x: i32) -> &'a str {}

trait Foo<'a> { }

fn main() {
    f::<'static>(); //~ ERROR E0088
    //~^ unexpected lifetime parameter

    g::<'static, 'static>(4);

    let x: Foo<'static, 'static>;
}

It may not be possible to invoke the code path. If that's the case, it may make sense to always say "unexpected lifetime parameters" in the label

[yossi-k]

@jonathandturner So considering that it doesn't seem possible to actually provide a lifetime parameter to a function, and providing lifetimes to other things (such as traits) is covered by a whole different set of errors (e.g. E0107), is this code segment actually needed?

[sophiajt]

@yossi-k - you could try taking it out and running the unit test suite again and see what happens. If all of the errors are still caught by E0107, you might be right. And it would help us simplify things.

[yossi-k]

@jonathandturner All tests seem to pass, so I'm guessing we're okay.
Also I merged the Spans of all provided lifetime parameters.

[sophiajt]

@yossi-k - unfortunately, you can't safely merge multiple spans together, since spans can come from macros and might give you a broken span as a result.

[yossi-k]

@jonathandturner What should I do in this case? Only reference one lifetime parameter, perhaps each individually or maybe just the entire function call?

[sophiajt]

@yossi-k - I just added a way to safely merge spans to CodeMap. It's a new method called merge_spans that will merge the spans if it's safe to do so.

[sophiajt]

@yossi-k - after working some with @GuillaumeGomez, I think a better solution than merging spans would be to instead point at the argument when it's unexpected. You can see an example on E0050

[GuillaumeGomez]

@jonathandturner: It hasn't been updated yet. :p

[sophiajt]

@GuillaumeGomez - moved it from a comment to the issue text. Should be updated now

[alexcrichton]

Closing due to inactivity, but feel free to resubmit with a rebase!

[eddyb]

@jonathandturner You should've asked a rustc developer 😭. This (through #37835) introduced a regression which I only found because I was rewriting lifetime elision and I had to update this check.

Thankfully it's only propagated to beta so we can just backport a fix, but this is scary that it just happened.
Well, not that scary, what will happen is inference variables will be created for the missing lifetimes.

What's confusing here is the distinction between early-bound and late-bound fn lifetime parameters.
Early-bound lifetime parameters work like they do in type definitions, they get passed in the path and applied to the type to get the type of the path, and all uses of the resulting value get the same lifetime.
They only end up existing when the syntactic lifetime is used in a bound (e.g. 'b: 'a and T: Trait<'a>).

Late-bound lifetimes, explicitly expressed by for<'a> when nested in types, are the most common case, and they allow a signature to stay generic (and work with e.g. Fn(&T) aka for<'a> Fn(&'a T)).

For some historical reason or another, we ended up mapping lifetimes passed to a path to only the early-bound lifetimes and have no mechanism for "downgrading" late- to early-bound for paths with lifetimes.
We may want to change to something like this, I'm not sure (explicit lifetimes to value paths are rare IME).

To wrap it up, this is the test I used (errors on 1.14 stable, incorrectly passes on beta/nightly):

fn foo<'a: 'b, 'b: 'a>() {}
fn main() {
    foo::<'static>();
}

cc @rust-lang/compiler @rust-lang/lang

[arielb1]

For some historical reason or another, we ended up mapping lifetimes passed to a path to only the early-bound lifetimes and have no mechanism for "downgrading" late- to early-bound for paths with lifetimes.

That's just an implementation artifact, methinks.

[sophiajt]

@eddyb - you're commenting on a closed PR, I assume you meant the merged one?

Glad we could catch it in time. Looking at it now, definitely an oversight on my part. Clearly they changed the logic rather than just changing the message.

[sophiajt]

@eddyb - potential fix at #38859