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

@nikomatsakis

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)):

bisected to Remove migrate borrowck mode #95565
what I'm calling the "good" error replaced the "bad" one back in 1.52.0

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)

@compiler-errors

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>)) {}
  |                                         ^^^^^^^^^^^^^^^^^^^^^^^^^

@jackh726

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

Remove migrate borrowck mode #95565

(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.

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.

@matthewjasper

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

Placeholder lifetime error cleanup #81972

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:

[NLL] Bad higher ranked subtype error #57374

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?

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

estebank commented on Aug 19, 2022

eddyb commented on Aug 19, 2022

estebank commented on Aug 19, 2022

eddyb commented on Aug 19, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions