-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
resolve: prohibit anon const non-static lifetimes #76739
resolve: prohibit anon const non-static lifetimes #76739
Conversation
src/test/ui/const-generics/min_const_generics/forbid-non-static-lifetimes.rs
Show resolved
Hide resolved
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.
Does this PR break the following snippet which already compiles on stable?
This restriction should only apply to anonymous constants, associated constants should not be affected here.
edit: ignore the above questions, didn't see the check on AnonConst
, this already works correctly
#![feature(min_const_generics)]
struct Foo<'a>(&'a ());
impl<'a> Foo<'a> {
const ASSOC: usize = {
let x: &'a usize = &7;
*x
};
}
It might be easier to forbid these lifetimes during name resolution instead where we hopefully should be able to replace them with ty::ReStatic
. I would expect this be a less invasive change which does not require us to change bug
to a delay_span_bug
.
7e1b938
to
5690e39
Compare
Pushed a change that makes the test check both
As you've noticed, it doesn't break that snippet and only applies to anonymous constants.
I can give this approach a go if you'd prefer. |
5690e39
to
8de6087
Compare
8de6087
to
4f7f0b0
Compare
@lcnr I've updated the PR to implement this in name resolution (cc @petrochenkov), which did end up being much cleaner. |
4f7f0b0
to
7d3dac2
Compare
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.
I think this looks good now, but I am not too familiar with lifetime resolution, so maybe
r? @nikomatsakis for the final approval.
7d3dac2
to
129a7cb
Compare
One request: Right now, it's hard to tell from the test which parts cause an ICE and which parts compile on |
129a7cb
to
9f9e2ee
Compare
I removed the example from this comment from the test because it gives a very similar but still different ICE on CI and that causes it to fail - I wasn't able to work out why that was, even after rebasing locally.
Given the above, I've kept a single test since the ICE is only produced from the cases that emit an error under |
src/test/ui/const-generics/min_const_generics/forbid-non-static-lifetimes.full.stderr
Outdated
Show resolved
Hide resolved
src/test/ui/const-generics/min_const_generics/forbid-non-static-lifetimes.rs
Outdated
Show resolved
Hide resolved
This commit modifies name resolution to emit an error when non-static lifetimes are used in anonymous constants when the `min_const_generics` feature is enabled. Signed-off-by: David Wood <david@davidtw.co>
9f9e2ee
to
eacfb2b
Compare
Thanks! @bors r+ rollup |
📌 Commit eacfb2b has been approved by |
…as-schievink Rollup of 12 pull requests Successful merges: - rust-lang#76101 (Update RELEASES.md for 1.47.0) - rust-lang#76739 (resolve: prohibit anon const non-static lifetimes) - rust-lang#76811 (Doc alias name restriction) - rust-lang#77405 (Add tracking issue of iter_advance_by feature) - rust-lang#77409 (Add example for iter chain struct) - rust-lang#77415 (Better error message for `async` blocks in a const-context) - rust-lang#77423 (Add `-Zprecise-enum-drop-elaboration`) - rust-lang#77432 (Use posix_spawn on musl targets) - rust-lang#77441 (Fix AVR stack corruption bug) - rust-lang#77442 (Clean up on example doc fixes for ptr::copy) - rust-lang#77444 (Fix span for incorrect pattern field and add label) - rust-lang#77453 (Stop running macOS builds on Azure Pipelines) Failed merges: r? `@ghost`
Fixes #75323, fixes #74447 and fixes #73375.
This PR prohibits non-static lifetimes in anonymous constants when only the
min_const_generics
feature is enabled.To do so,to_region_vid
'sbug!
had to be changed into a delayed bug, which unfortunately required providing it aTyCtxt
.While I am happy with how the implementation of the error turned out inrustc_passes::check_const
, emitting an error wasn't sufficient to avoid hitting the ICE later. I also tried implementing the error inrustc_mir::transform::check_consts::validation
and that worked, but it didn't silence the ICE either. To silence the ICE, I changed it to a delayed bug which worked but was more invasive that I would have liked, and required I return an incorrect lifetime. It's possible that this check should be implemented earlier in the compiler to make the invasive changes unnecessary, but I wasn't sure where that would be and wanted to get some feedback first.The approach taken by this PR has been changed to implement the error in name resolution, which ended up being much simpler.
cc @rust-lang/wg-const-eval
r? @lcnr