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

disallow non-static lifetimes in const generics #74051

Merged
merged 2 commits into from
Jul 21, 2020

Conversation

yodaldevoid
Copy link
Contributor

Disallow non-static lifetimes in const generics in order to to patch over an ICE caused when we encounter a non-static lifetime in a const generic during borrow checking. This restriction may be relaxed in the future, but we need more discussion before then, and in the meantime we should still deal with this ICE.

Fixes issue #60814

@rust-highfive
Copy link
Collaborator

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2020
@yodaldevoid yodaldevoid force-pushed the issue_60814 branch 2 times, most recently from 9947e53 to 1c33920 Compare July 5, 2020 05:05
@yodaldevoid yodaldevoid marked this pull request as ready for review July 5, 2020 05:07
@yodaldevoid yodaldevoid force-pushed the issue_60814 branch 2 times, most recently from 94b76ba to 04593af Compare July 5, 2020 16:20
@lcnr lcnr added A-const-generics Area: const generics (parameters and arguments) F-const_generics `#![feature(const_generics)]` labels Jul 8, 2020
@bors
Copy link
Contributor

bors commented Jul 14, 2020

☔ The latest upstream changes (presumably #74330) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb
Copy link
Member

eddyb commented Jul 15, 2020

r? @matthewjasper cc @nikomatsakis

@rust-highfive rust-highfive assigned matthewjasper and unassigned eddyb Jul 15, 2020
@nikomatsakis
Copy link
Contributor

I'd cc @varkor and perhaps @lcnr, who seem to be spearheading const generics at this moment. I'm not sure what I think of this PR -- const generics is flagged as unstable, and it seems like papering over ICEs isn't something we necessarily have to do, as opposed to taking the time to fix them the right way. This change adds a new error code and modifies name resolution, which seems a bit "non-local" to me -- i.e., it's affecting code that really shouldn't have much to do with const generics at all. On the other hand, it's not a lot of code, and wouldn't be hard to remove per se. But I worry it might confuse people -- at minimum I'd want some FIXME comments.

@lcnr
Copy link
Contributor

lcnr commented Jul 17, 2020

I fairly strongly believe this PR is useful.

Slightly elaborating on the sentiment in #74052 (comment), I think allowing lifetimes in the type of const parameters has (nearly) the same difficulty as allowing types (which we forbid in #74159). We have to think about how type inference should work without causing query cycles and what exactly non static lifetimes mean in a const expression.

@yodaldevoid can you add this as a test here (I would expect it to compile)

#![feature(const_generics)]

fn test<const N: usize>() {}

fn wow<'a>() -> &'a () {
    test::<{
        let _: &'a ();
        3
    }>();
    &()
}

I can't really judge the implementation itself here as I am not familiar with this code.

@nikomatsakis
Copy link
Contributor

Ok, I'd be ok with it if the code is tagged with a FIXME comment that points to some representation issue saying "we should revisit this question at some point"

@yodaldevoid
Copy link
Contributor Author

I'll update this in the next couple days to add the suggested test and some FIXME statements.

@lcnr
Copy link
Contributor

lcnr commented Jul 18, 2020

Somewhat related, we also can't really handle lifetimes in anon consts inside of type defaults rn, so we should forbid something like

struct Foo<'a, T = [u8; {
    let _: &'a ();
    7
}]>(&'a (), T);

This currently ICEs on stable, I am adding the same restriction for other generic params in #74487

This has been put in place to patch over an ICE caused when we encounter
a non-static lifetime in a const generic during borrow checking. This
restriction may be relaxed in the future, but we need more discussion
before then, and in the meantime we should still deal with this ICE.

Fixes issue rust-lang#60814
@yodaldevoid
Copy link
Contributor Author

I've added a FIXME comment on the error-emitting function pointing to #74052 and the test asked for by @lcnr.

@@ -828,6 +831,10 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> {
self.insert_lifetime(lifetime_ref, Region::Static);
return;
}
if self.is_in_const_generic && lifetime_ref.name != LifetimeName::Error {
self.emit_non_static_lt_in_const_generic_error(lifetime_ref);
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess we don't have a RegionKind::Error to use here... unfortunate. I don't really remember what happens is we fail to store some kind of resolution for a lifetime in type check -- are we relying on not running type check here? Ah, ok, we insert 'static or an inference variable:

None => {
self.re_infer(def, lifetime.span).unwrap_or_else(|| {
// This indicates an illegal lifetime
// elision. `resolve_lifetime` should have
// reported an error in this case -- but if
// not, let's error out.
tcx.sess.delay_span_bug(lifetime.span, "unelided lifetime in signature");
// Supply some dummy value. We don't have an
// `re_error`, annoyingly, so use `'static`.
tcx.lifetimes.re_static
})

I guess that's ok.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 20, 2020

📌 Commit c60a035 has been approved by nikomatsakis

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 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 Jul 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 20, 2020
…arth

Rollup of 13 pull requests

Successful merges:

 - rust-lang#72714 (Fix debug assertion in typeck)
 - rust-lang#73197 (Impl Default for ranges)
 - rust-lang#73323 (wf: check foreign fn decls for well-formedness)
 - rust-lang#74051 (disallow non-static lifetimes in const generics)
 - rust-lang#74376 (test caching opt_const_param_of on disc)
 - rust-lang#74501 (Ayu theme: Use different background color for Run button)
 - rust-lang#74505 (Fix search input focus in ayu theme)
 - rust-lang#74522 (Update sanitizer docs)
 - rust-lang#74546 (Fix duplicate maybe_uninit_extra attribute)
 - rust-lang#74552 (Stabilize TAU constant.)
 - rust-lang#74555 (Improve "important traits" popup display on mobile)
 - rust-lang#74557 (Fix an ICE on an invalid `binding @ ...` in a tuple struct pattern)
 - rust-lang#74561 (update backtrace-rs)

Failed merges:

r? @ghost
@bors bors merged commit 991da05 into rust-lang:master Jul 21, 2020
@yodaldevoid yodaldevoid deleted the issue_60814 branch July 21, 2020 01:30
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) 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
Development

Successfully merging this pull request may close these issues.

9 participants