Fix ICE with explicit late-bound lifetimes #72441
Conversation
rust-highfive
added
the
S-waiting-on-review
| /// Decorates the result of a generic argument count mismatch | ||
| /// check with whether explicit late bounds were provided. | ||
| #[derive(Clone)] | ||
| pub struct GenericArgCountResult { | ||
| pub explicit_late_bound: ExplicitLateBound, | ||
| pub correct: Result<(), GenericArgCountMismatch>, | ||
| } |
There was a problem hiding this comment.
Couldn't this information go into the GenericArgCountMismatch?
There was a problem hiding this comment.
The problem is we want to warn not error at the moment (introducing an error would be a breaking change - see #42868), so basically we end up with situations where we might not have a generic arg count mismatch but we still have explicit late bounds. That's part of what's causing the ICE. (If you #[deny(warnings)] the ICE disappears.)
There was a problem hiding this comment.
Can we consider trying to close #42868? I guess we still need to make some expressiveness improvements to do so?
There was a problem hiding this comment.
So #42868 lists two future PRs, one to make the lint deny-by-default, and the other to turn the warning into a hard error. Making the lint deny-by-default would still require this patch, but if we jumped straight to a hard error, then none of this work is necessary. It doesn't resolve the issue that this is still a breaking change though, but I guess that might not matter so much seeing as it's already broken?
|
r? @nikomatsakis (maybe we should discuss how to get rid of the early/late-bound lifetime split...) |
|
Reading, I think @nikomatsakis's suggestion here is probably close to the right answer, but we'd definitely have to wait for a new edition before it could be integrated. |
| /// Decorates the result of a generic argument count mismatch | ||
| /// check with whether explicit late bounds were provided. | ||
| #[derive(Clone)] | ||
| pub struct GenericArgCountResult { | ||
| pub explicit_late_bound: ExplicitLateBound, | ||
| pub correct: Result<(), GenericArgCountMismatch>, | ||
| } |
There was a problem hiding this comment.
Can we consider trying to close #42868? I guess we still need to make some expressiveness improvements to do so?
| S.func::<'a, U>() | ||
| //~^ WARN cannot specify lifetime arguments explicitly | ||
| //~| WARN this was previously accepted | ||
| } |
There was a problem hiding this comment.
Do we have tests for all the other random scenarios that can occur? It seems like we should try to test them all -- in particular the case of "got a lifetime but expected something else"...
There was a problem hiding this comment.
I was under the impression that
- src/test/ui/methods/method-call-lifetime-args.rs
- src/test/ui/methods/method-call-lifetime-args-fail.rs
cover pretty much everything but I could be wrong
There was a problem hiding this comment.
Those tests look pretty good. I guess they don't cover lifetimes + types intermixed. But I'm satisfied.
|
So I guess r=me with comment applied |
|
@bors r+ |
|
📌 Commit fa351ee has been approved by |
bors
added
S-waiting-on-bors
Rollup of 9 pull requests Successful merges: - rust-lang#72299 (more `LocalDefId`s) - rust-lang#72368 (Resolve overflow behavior for RangeFrom) - rust-lang#72441 (Fix ICE with explicit late-bound lifetimes) - rust-lang#72499 (Override Box::<[T]>::clone_from) - rust-lang#72521 (Properly handle InlineAsmOperand::SymFn when collecting monomorphized items) - rust-lang#72540 (mir: adjust conditional in recursion limit check) - rust-lang#72563 (multiple Return terminators are possible) - rust-lang#72585 (Only capture tokens for items with outer attributes) - rust-lang#72607 (Eagerly lower asm sub-expressions to HIR even if there is an error) Failed merges: r? @ghost
Rather than returning an explicit late-bound lifetime as a generic argument count mismatch (which is not necessarily true), this PR propagates the presence of explicit late-bound lifetimes.
This avoids an ICE that can occur due to the presence of explicit late-bound lifetimes when building generic substitutions by explicitly ignoring them.
r? @varkor
cc @davidtwco (this removes a check you introduced in #60892)
Resolves #72278