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

resolve: prohibit anon const non-static lifetimes #76739

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Sep 15, 2020

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's bug! had to be changed into a delayed bug, which unfortunately required providing it a TyCtxt.


While I am happy with how the implementation of the error turned out in rustc_passes::check_const, emitting an error wasn't sufficient to avoid hitting the ICE later. I also tried implementing the error in rustc_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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2020
Copy link
Contributor

@lcnr lcnr left a 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.

@davidtwco davidtwco force-pushed the issue-75323-non-static-lifetime-in-anonconst branch from 7e1b938 to 5690e39 Compare September 15, 2020 12:46
@davidtwco
Copy link
Member Author

Pushed a change that makes the test check both min_const_generics and const_generics.

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.

As you've noticed, it doesn't break that snippet and only applies to anonymous constants.

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.

I can give this approach a go if you'd prefer.

@davidtwco davidtwco force-pushed the issue-75323-non-static-lifetime-in-anonconst branch from 5690e39 to 8de6087 Compare September 15, 2020 13:46
@davidtwco davidtwco changed the title passes: prohibit non-static lifetime in anon const resolve: prohibit anon const non-static lifetimes Sep 15, 2020
@davidtwco davidtwco force-pushed the issue-75323-non-static-lifetime-in-anonconst branch from 8de6087 to 4f7f0b0 Compare September 15, 2020 13:48
@davidtwco
Copy link
Member Author

@lcnr I've updated the PR to implement this in name resolution (cc @petrochenkov), which did end up being much cleaner.

@davidtwco davidtwco force-pushed the issue-75323-non-static-lifetime-in-anonconst branch from 4f7f0b0 to 7d3dac2 Compare September 15, 2020 14:22
Copy link
Contributor

@lcnr lcnr left a 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.

@lcnr lcnr assigned nikomatsakis and petrochenkov and unassigned lcnr and petrochenkov Sep 16, 2020
@davidtwco davidtwco force-pushed the issue-75323-non-static-lifetime-in-anonconst branch from 7d3dac2 to 129a7cb Compare September 16, 2020 09:15
@hameerabbasi
Copy link
Contributor

One request: Right now, it's hard to tell from the test which parts cause an ICE and which parts compile on full. It might be good to split the test up into multiple tests into src/test/ui/const-generics/issues, but this is only a suggestion.

@davidtwco davidtwco force-pushed the issue-75323-non-static-lifetime-in-anonconst branch from 129a7cb to 9f9e2ee Compare September 16, 2020 10:18
@davidtwco
Copy link
Member Author

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.

One request: Right now, it's hard to tell from the test which parts cause an ICE and which parts compile on full. It might be good to split the test up into multiple tests into src/test/ui/const-generics/issues, but this is only a suggestion.

Given the above, I've kept a single test since the ICE is only produced from the cases that emit an error under min_const_generics and each ICE is the same.

@varkor varkor added the F-const_generics `#![feature(const_generics)]` label Oct 2, 2020
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 <[email protected]>
@davidtwco davidtwco force-pushed the issue-75323-non-static-lifetime-in-anonconst branch from 9f9e2ee to eacfb2b Compare October 2, 2020 13:30
@varkor
Copy link
Member

varkor commented Oct 2, 2020

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 2, 2020

📌 Commit eacfb2b has been approved by varkor

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2020
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 2, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2020
…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`
@bors bors merged commit c8eb205 into rust-lang:master Oct 2, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 2, 2020
@davidtwco davidtwco deleted the issue-75323-non-static-lifetime-in-anonconst branch October 3, 2020 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_generics `#![feature(const_generics)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
9 participants