-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Correctly deny late-bound lifetimes from parent in anon consts and TAITs #115486
Conversation
r? @wesleywiser (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
6ad745c
to
54d3a81
Compare
// If we want this to compile, then we'd need to do something like RPITs do, | ||
// where nested associated constants have early-bound versions of their captured | ||
// late-bound vars inserted into their generics. This gives us substitutable | ||
// lifetimes to actually use when borrow-checking the associated const, which is | ||
// lowered as a totally separate body from its parent. Since this doesn't exist, | ||
// so we should just error rather than resolving this late-bound var with no | ||
// binder to actually attach it to, or worse, as a free region that can't even be | ||
// substituted correctly, and ICEing. - @compiler-errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this comment is to ramble a bit about a more general solution than what would be needed to fix just this test and its related test (in_closure.rs
and simple.rs
) -- the simplest fix for just those tests would be to just go restore behavior of resolving late-bound region captured by anon consts as free regions in the parent function's body, and fix NLL to do the free region -> vid mapping correctly again (#111215 discusses this regression in the way that NLL maps free regions, cc @BoxyUwU).
But, for the record, #79298 never really correctly supported referencing late-bound regions in anon consts that show up in general positions (i.e. other positions where the const wasn't just referencing a free region in the scope of a body), e.g.:
// ICEs even on 1.51 after #79298
#![feature(const_generics)] // `generic_const_exprs` after it was renamed.
#![allow(incomplete_features)]
const fn inner<'a: 'a>() -> usize {
3
}
fn test<'a>() -> [u8; { inner::<'a>() }] {
[1, 2, 3]
}
fn main() {
test();
}
or in other binders, like:
trait Trait {
type Assoc;
}
fn test() {
let x: dyn for<'a> Trait<Assoc = [u8; { inner::<'a>() }]>;
}
This isn't necessarily the only solution to fix this -- I guess we could (ab)use free regions to correctly borrowck consts that show up in the binders above, but it seems pretty obvious to me that the clearest and easiest way to do this is to just represent late-bound vars captured by nested consts as early-bound regions owned by those consts.
@@ -1259,6 +1270,22 @@ impl<'a, 'tcx> BoundVarContext<'a, 'tcx> { | |||
if let Some(mut def) = result { | |||
if let ResolvedArg::EarlyBound(..) = def { | |||
// Do not free early-bound regions, only late-bound ones. | |||
} else if let ResolvedArg::LateBound(_, _, param_def_id) = def |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both this branch and the next (if let Some(body_id) = outermost_body
) should only be executed for late-bound variables. Should they both be in an if let ResolvedArg::LateBound(_, _, param_def_id) = def
to make it clearer?
For my curiosity, what would happen if we made lifetimes in anon-consts ResolvedArg::Free
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my curiosity [...]
See my comment here #115486 (comment), I kinda went into it there -- this idea would, in fact, fix at least tests like const-generics/late-bound-vars/in_closure.rs
and const-generics/late-bound-vars/simple.rs
, and was how #79298 worked, afaict.
It doesn't seem like a general solution, though, since when it comes to, e.g., a binder on a where clause like where for<'a> [(); { /* const that uses 'a */ }]: ...
, it doesn't really make sense to use ReFree
for that usage of 'a
, since the late-bound region does not originate from the function's generics (and therefore the fn_sig
's binder).
This also doesn't lead us towards a correct solution for handling things like for<const C: usize> [(); C]: ...
, since there's no equivalent notion of a "free" type or const var (nor does it make sense, since they must be substitutable).
65ea743
to
f479a7a
Compare
@bors r+ |
…s, r=cjgillot Correctly deny late-bound lifetimes from parent in anon consts and TAITs Reuse the `AnonConstBoundary` scope (introduced in rust-lang#108553, renamed in this PR to `LateBoundary`) to deny late-bound vars of *all* kinds (ty/const/lifetime) in anon consts and TAITs. Side-note, but I would like to consolidate this with the error reporting for RPITs (E0657): https://github.com/rust-lang/rust/blob/c4f25777a08cd64b710e8a9a6159e67cbb35e6f5/compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs#L733-L754 but the semantics about what we're allowed to capture there are slightly different, so I'm leaving that untouched. Fixes rust-lang#115474
💔 Test failed - checks-actions |
@bors retry no idea why it failed lol |
…s, r=cjgillot Correctly deny late-bound lifetimes from parent in anon consts and TAITs Reuse the `AnonConstBoundary` scope (introduced in rust-lang#108553, renamed in this PR to `LateBoundary`) to deny late-bound vars of *all* kinds (ty/const/lifetime) in anon consts and TAITs. Side-note, but I would like to consolidate this with the error reporting for RPITs (E0657): https://github.com/rust-lang/rust/blob/c4f25777a08cd64b710e8a9a6159e67cbb35e6f5/compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs#L733-L754 but the semantics about what we're allowed to capture there are slightly different, so I'm leaving that untouched. Fixes rust-lang#115474
💔 Test failed - checks-actions |
Another spurious failure? lmao @bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (4b91288): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 630.757s -> 631.686s (0.15%) |
Reuse the
AnonConstBoundary
scope (introduced in #108553, renamed in this PR toLateBoundary
) to deny late-bound vars of all kinds (ty/const/lifetime) in anon consts and TAITs.Side-note, but I would like to consolidate this with the error reporting for RPITs (E0657):
rust/compiler/rustc_hir_analysis/src/collect/resolve_bound_vars.rs
Lines 733 to 754 in c4f2577
Fixes #115474