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

Correctly deny late-bound lifetimes from parent in anon consts and TAITs #115486

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Sep 2, 2023

Reuse the AnonConstBoundary scope (introduced in #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):

if !parent_id.is_owner() {
struct_span_err!(
self.tcx.sess,
lifetime.ident.span,
E0657,
"`impl Trait` can only capture lifetimes bound at the fn or impl level"
)
.emit();
self.uninsert_lifetime_on_error(lifetime, def.unwrap());
}
if let hir::Node::Item(hir::Item {
kind: hir::ItemKind::OpaqueTy { .. }, ..
}) = self.tcx.hir().get(parent_id)
{
let mut err = self.tcx.sess.struct_span_err(
lifetime.ident.span,
"higher kinded lifetime bounds on nested opaque types are not supported yet",
);
err.span_note(self.tcx.def_span(def_id), "lifetime declared here");
err.emit();
self.uninsert_lifetime_on_error(lifetime, def.unwrap());
}
but the semantics about what we're allowed to capture there are slightly different, so I'm leaving that untouched.

Fixes #115474

@rustbot
Copy link
Collaborator

rustbot commented Sep 2, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 2, 2023
@rust-log-analyzer

This comment has been minimized.

Comment on lines 3 to 10
// 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
Copy link
Member Author

@compiler-errors compiler-errors Sep 2, 2023

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
Copy link
Contributor

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?

Copy link
Member Author

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

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 11, 2023

📌 Commit f479a7a has been approved by cjgillot

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2023
@bors
Copy link
Contributor

bors commented Sep 11, 2023

⌛ Testing commit f479a7a with merge 0c9529b...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2023
…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
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Sep 11, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 11, 2023
@compiler-errors
Copy link
Member Author

@bors retry no idea why it failed lol

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2023
@bors
Copy link
Contributor

bors commented Sep 19, 2023

⌛ Testing commit f479a7a with merge 2d56d38...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2023
…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
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Sep 19, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 19, 2023
@compiler-errors
Copy link
Member Author

Another spurious failure? lmao

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2023
@bors
Copy link
Contributor

bors commented Sep 20, 2023

⌛ Testing commit f479a7a with merge 4b91288...

@bors
Copy link
Contributor

bors commented Sep 20, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 4b91288 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 20, 2023
@bors bors merged commit 4b91288 into rust-lang:master Sep 20, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 20, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4b91288): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.6% [-0.6%, -0.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-0.6%, -0.6%] 1

Max RSS (memory usage)

Results

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

mean range count
Regressions ❌
(primary)
2.5% [2.5%, 2.5%] 1
Regressions ❌
(secondary)
2.3% [0.4%, 3.3%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.5% [2.5%, 2.5%] 1

Cycles

Results

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

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.7% [-5.7%, -2.2%] 5
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 630.757s -> 631.686s (0.15%)
Artifact size: 317.76 MiB -> 317.84 MiB (0.03%)

@matthiaskrgr
Copy link
Member

Neat, this might have fixed a ton of ices :D
ices

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ice Not enough bound vars: BoundRegion
9 participants