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

Redundant error for "implementation ... not general enough" in "NLL" mode / since 1.63 (when it was made permanent). #100742

Open
eddyb opened this issue Aug 19, 2022 · 4 comments
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Aug 19, 2022

This is a classic demonstration of the limitations of ("early-bound") generics wrt HRTB:

fn type_generic<T>(_: T) {}

struct LtGen<'a>(&'a ());
fn takes_ltgen_forall_lifetimes(_: impl for<'x> FnOnce(LtGen<'x>)) {}

fn main() {
    takes_ltgen_forall_lifetimes(type_generic);
}

(I don't know what issue we have open about it - #30904 is too specific, there must be one about generic functions - and also if we ever fix this it will likely require for<'x> typeof(type_generic::<LtGen<'x>>) function types, which I've brought up to @nikomatsakis before but I haven't seen any other discussion of)

Either way, for now, the above should error (if you're curious, the simplest fix is to use a closure, that itself can be for<'x>, and which calls the function, like |x| type_generic(x)).

And for a long time (since MIR borrowck?) we've had this error:

error: implementation of `FnOnce` is not general enough
 --> <source>:7:5
  |
7 |     takes_ltgen_forall_lifetimes(type_generic);
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ implementation of `FnOnce` is not general enough
  |
  = note: `fn(LtGen<'2>) {type_generic::<LtGen<'2>>}` must implement `FnOnce<(LtGen<'1>,)>`, for any lifetime `'1`...
  = note: ...but it actually implements `FnOnce<(LtGen<'2>,)>`, for some specific lifetime `'2`

It's not perfect, but it shows the difference between some inferred lifetime ('2) and the HRTB ('1).


However, since 1.63.0 (see comparison on godbolt: https://godbolt.org/z/EzjPzE7xs), we also get:

error[E0308]: mismatched types
 --> <source>:7:5
  |
7 |     takes_ltgen_forall_lifetimes(type_generic);
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ one type is more general than the other
  |
  = note: expected trait `for<'x> FnOnce<(LtGen<'x>,)>`
             found trait `FnOnce<(LtGen<'_>,)>`
note: the lifetime requirement is introduced here
 --> <source>:4:41
  |
4 | fn takes_ltgen_forall_lifetimes(_: impl for<'x> FnOnce(LtGen<'x>)) {}
  |                                         ^^^^^^^^^^^^^^^^^^^^^^^^^

This is a weirder error, that I naively assumed would be the old regionck (but that got removed, right? so it can't be that), though I suppose it could be some generic obligation solving from inside MIR borrowck.

  • it's presented as a type mismatch when it's an unsatisfied bound specifically because of HRTB
  • '_ is used for lifetime inference variables
  • can use the ugly for<'r> auto-naming in the printout (not here because I wrote for<'x>)
  • it's not necessary because the better error is still being emitted
    • (though I would still make this one delay_span_bug if they're not emitted from exactly the same place)

EDIT: more information found since opening the issue (in #100742 (comment)):

EDIT2: better timeline in #100742 (comment) - the error was improved back in #81972 but the "bonus error" comes from "NLL mode" - e.g. #57374 (comment).


cc @estebank @rust-lang/wg-nll @lcnr (this may be subtle in nature, hopefully someone knows what happened)

cc @Manishearth (who has a similar error though unlikely related to this 1.62.0->1.63.0 change, since it's on 1.61.0 instead, and does not even have the good error showing up at all - that one still needs to be reduced)

@eddyb eddyb added A-diagnostics Area: Messages for errors, warnings, and lints A-borrow-checker Area: The borrow checker T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 19, 2022
@estebank
Copy link
Contributor

estebank commented Aug 19, 2022

cc @compiler-errors as you were just looking at something similar, @rust-lang/wg-diagnostics

We should also special case the situation of hrlts for single element closures, to suggest |x| type_generic(x) to unblock the user when possible, as mentioned in the report.

I don't agree that the E308 error is necessarily worse, some experienced devs can recognize what's going on thanks to the expected/found output, but we should likely be emitting a new more targeted error for these, with a more verbose explanation about hrlts.

Maybe something along the lines of

error[E0XXX]: expected lifetime generic closure, found specific lifetime closure
 --> <source>:7:5
  |
7 |     x(y);
  |       ^ expected a lifetime generic closure, but `y` is bound to a specific `'_` lifetime
  |
  = note: expected trait `for<'x> FnOnce<(LtGen<'x>,)>`
             found trait `FnOnce<(LtGen<'_>,)>`
  = help: `for<'x>` means that `FnOnce<(LtGen<'x>,)>` can be called for any given lifetime, but `y` can only be called for a specific bound lifetime `'_` (FIXME)
note: the lifetime requirement is introduced here
 --> <source>:4:41
  |
4 | fn takes_ltgen_forall_lifetimes(_: impl for<'x> FnOnce(LtGen<'x>)) {}
  |                                         ^^^^^^^^^^^^^^^^^^^^^^^^^

@estebank estebank added A-lifetimes Area: Lifetimes / regions D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. labels Aug 19, 2022
@eddyb
Copy link
Member Author

eddyb commented Aug 19, 2022

Unsurprisingly, cargo-bisect-rustc points to this (cc @jackh726):

(though what is surprising is that I'm on the 2021 edition, should I have used #![feature(nll)] for testing?)


I don't agree that the E308 error is necessarily worse, some experienced devs can recognize what's going on thanks to the expected/found output, but we should likely be emitting a new more targeted error for these, with a more verbose explanation about hrlts.

Sorry, that sounds like I didn't make my point clearly. The problem is that only borrowck can tell you about lifetimes, in a useful way. The '1 and '2 are the modern rendering of lifetimes, that (MIR) borrowck generates in a custom way, and it's what's being front-loaded to users as the lifetime diagnostics.

I can totally read it the "mismatched types", but it's the old error and I just did some digging and the improvement appears to date back to 1.52.0: https://godbolt.org/z/z5qeecKb1.

What you're suggesting sounds very close to the (good IMO) "error: implementation of FnOnce is not general enough" error that we already have (since 1.52.0).

AFAICT the "mismatched types" error is an anomaly that we should figure out how to get rid of, and if there any improvements to be made they should be made to the modern "not general enough" error.

@estebank
Copy link
Contributor

Fair, makes sense to do that. We also need to check for all the places where E0308 is currently being emitted as the only output for these (I believe that anecdotally there are a few). We should also give the "not general enough" error not only a wording makeover, but also its very own Error Code where we can talk about hrlts, that fun topic that keeps on giving.

@eddyb
Copy link
Member Author

eddyb commented Aug 19, 2022

Did a 1.51.0 to 1.52.0 bisection and found a likely candidate in 152f660...0148b97:

So I believe that @matthewjasper intentionally improved the error here to not look like the extra error we're getting again in 1.63.0, but to what's been consistent between 1.52.0 and 1.62.0 (inclusive).

That PR mentions this issue:

And we can see in e.g. #57374 (comment) that forcing NLL caused this issue even before 1.63.0 - but I don't really understand how this interacts with editions: did only #![feature(nll)] enable the full set of changes?

@eddyb eddyb changed the title "implementation of ... is not general enough" started getting a redundant diagnostic in 1.63. Redundant error for "implementation ... not general enough" in "NLL" mode / since 1.63 (when it was made permanent). Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. 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

2 participants