Various changes to name resolution of anon consts #111215

BoxyUwU · 2023-05-04T19:18:36Z

Sorry this PR is kind of all over the place ^^'

Rewrites anon const nameres to all go through fn resolve_anon_const explicitly instead of visit_anon_const to ensure that we do not accidentally resolve anon consts as if they are allowed to use generics when they aren't. Also means that we dont have bits of code for resolving anon consts that will get out of sync (i.e. legacy const generics and resolving path consts that were parsed as type arguments)
Renames two of the LifetimeRibKind, AnonConst -> ConcreteAnonConst and ConstGeneric -> ConstParamTy
Noticed while doing this that under generic_const_exprs all lifetimes currently get resolved to errors without any error being emitted which was causing a bunch of tests to pass without their bugs having been fixed, incidentally fixed that in this PR and marked those tests as // known-bug:. I'm fine to break those since generic_const_exprs is a very unstable incomplete feature and this PR does make generic_const_exprs "less broken" as a whole, also I can't be assed to figure out what the underlying causes of all of them are. This PR reopens ICE: cannot relate bound region: ReLateBound #77357 ICE: index out of bounds: the len is 0 but the index is 18446744073709551615 #83993
Changed generics_of to stop providing generics and predicates to enum variant discriminant anon consts since those are not allowed to use generic parameters
Updated the error for non 'static lifetime in const arguments and the error for non 'static lifetime in const param tys to use derive(Diagnostic)

I have a vague idea why const-arg-in-const-arg.rs, in-closure.rs and simple.rs have started failing which is unfortunate since these were deliberately made to work, I think lifetime resolution being broken just means this regressed at some point and nobody noticed because the tests were not testing anything :( I'm fine breaking these too for the same reason as the tests for #77357 #83993. I couldn't get // known-bug to work for these ICEs and just kept getting different stderr between CI and local --bless so I just removed them and will create an issue to track re-adding (and fixing) the bugs if this PR lands.

r? @cjgillot cc @compiler-errors

tests/ui/const-generics/const-arg-in-const-arg.rs

cjgillot

Would it be possible to separate the changes to error types in their own commit, so they don't pollute semantic changes?

cjgillot · 2023-05-05T17:05:14Z

compiler/rustc_resolve/src/errors.rs

@@ -326,6 +326,18 @@ pub(crate) struct ParamInTyOfConstParam {
    #[label]
    pub(crate) span: Span,
    pub(crate) name: Symbol,
+    #[subdiagnostic]
+    pub(crate) sub_is_type: Option<ParamInTyOfConstParamIsType>,


I don't understand this terminology. Is this the kind of the param that appears in type of const param?

I'm not completely sure what the naming scheme is tbh I just copied it. I think the sub is short for "subdiagnostic", the is_type is to determine what kind of generic parameter was found. I've renamed all this to be more clear since I found it rather confusing too and so did others I spoke to.

It's now pub(crate) param_kind: Option<ParamKindInTyOfConstParam>.

As far as I know none of these ParamKindIn<blank> can be merged into a single type as they all have different diagnostic messages with their #[note]/#[help]. It's definitely unfortunate because there's a lot of them 😅

compiler/rustc_resolve/src/errors.rs

compiler/rustc_resolve/src/late.rs

tests/ui/const-generics/late-bound-vars/in_closure.rs

tests/ui/const-generics/late-bound-vars/in_closure.stderr

tests/ui/const-generics/late-bound-vars/simple.rs

tests/ui/const-generics/const-arg-in-const-arg.rs

cjgillot · 2023-05-05T17:29:22Z

tests/ui/const-generics/const-param-type-depends-on-const-param.full.stderr

@@ -3,12 +3,16 @@ error[E0770]: the type of const parameters must not depend on other generic para
   |
 LL | pub struct Dependent<const N: usize, const X: [u8; N]>([(); N]);
   |                                                    ^ the type must not depend on the parameter `N`
+   |
+   = note: const parameters may not be used in the type of const parameters


Suggested change

= note: const parameters may not be used in the type of const parameters

= note: const parameters may not be used in the type of other const parameters

?

I think technically no because const parameters cannot be used in the type of themselves either- const N: [(); N] would error. That would be a pretty nonsensical type though, its not really possible to construct a valid one even if we lifted the restriction that const parameter type's cant be generic lol

BoxyUwU · 2023-05-05T20:02:46Z

I think I've split the diagnostic changes out from the other changes, sorry I should have done that in the first place. I had thought it was going to be a lot harder to do retroactively, its not a perfect split but it should hopefully be much better now.

BoxyUwU · 2023-05-05T20:25:08Z

I tried to explain a bit in the PR description what is going on with the test/revision removals and things turning into ICEs but I think I didn't give enough information.

So for context (as i discovered while making this PR) right now on nightly lifetime resolution for anon consts under feature(generic_const_exprs) is really broken. Every lifetime you write that isnt '_ or 'static gets resolved to an error without an error actually having been emitted, this goes for when you do actually have a lifetime in scope (i.e. fn foo<'a>() -> [(); { let _: &'a (); 10 }] {}) or when you are writing lifetimes that haven't been declared (i.e. fn foo() -> [(); { let _: &'a (); 10 }] {}). I have added tests/ui/const-generics/generic_const_exprs/unresolved_lifetimes_error.rs to test that we emit errors when lifetimes are unresolved inside const arguments when generic_const_exprs is enabled (if you try that code on playground on nightly you'll see it doesnt error)

It hasn't always been like this and at some point it broke and started acting this way, this PR incidentally fixes this bug by not having the LifetimeRibKind::ConcreteAnonConst around anon consts. What was happening previously was ConcreteAnonConst was around every anon const, and when resolving a lifetime if we hit that rib kind we emit an error and store that the lifetime resolved to an error. With feature(generic_const_exprs) enabled we would silence the error caused by ConcreteAnonConst but still resolve the lifetime to an error.

As far as I can tell this is why 5 of the tests have started ICEing instead of passing, but this PR hasn't actually introduced any broken code. Generally all 5 of the tests started ICEing because the compiler cannot handle lifetimes in const args for one reason or another but it was being masked due to them always being resolved to errors.

const-arg-in-const-arg.rs's full revision, late-bound-vars/simple.rs and late-bound-vars/in-closure.rs are all ICEing because the logic to convert all regions to variables for borrowck doesn't work correctly for anon consts under generic_const_exprs. This definitely used to function correctly at one point and the tests were deliberately added because bugs were fixed. Then when lifetime resolution broke for generic_const_exprs these tests stopped actually testing anything because none of the lifetimes were resolved, and some PR (not entirely sure which) regressed these tests but it wasnt caught (until this PR made them start testing things again).
- I removed const-arg-in-const-arg.rs's full revision because I couldn't get stderr normalization to work correctly and give consistent results between CI and local for the ICE message 😅
issue-77357.rs and issue-83993.rs I do not believe have ever functioned correctly, the tests were added because glacier stuff started passing. This was solely because lifetime resolution broke and masked the ICEs.
- I removed these tests because I couldn't get stderr normalization to work correctly and give consistent results between CI and local for the ICE message 😅

cjgillot · 2023-05-06T07:34:15Z

compiler/rustc_resolve/src/late.rs

                    self.record_lifetime_res(
                        lifetime.id,
                        LifetimeRes::Error,
                        LifetimeElisionCandidate::Ignore,
                    );
                    return;
                }
-                LifetimeRibKind::AnonConst => {
-                    self.maybe_emit_forbidden_non_static_lifetime_error(lifetime);
+                LifetimeRibKind::ConcreteAnonConst(cause) => {


Every lifetime you write that isnt '_ or 'static gets resolved to an error without an error actually having been emitted, this goes for when you do actually have a lifetime in scope (i.e. fn foo<'a>() -> [(); { let _: &'a (); 10 }] {}) or when you are writing lifetimes that haven't been declared (i.e. fn foo() -> [(); { let _: &'a (); 10 }] {}).

The bug is here. We need the feature check for generic_const_exprs be done on this whole block, instead of only doing it in emit_forbidden_non_static_lifetime_error. This way, lifetime resolution will continue walking ribs, instead of returning a LifetimeRes::Error straight.

yeah, this PR accomplishes that by just not adding the ConcreteAnonConst rib if generic_const_exprs is enabled, so the only time we encounter LifetimeRibKind::ConcreteAnonConst should be when we need to error. Super subtle 😅

cjgillot · 2023-05-07T13:35:47Z

Thanks for the explanations.
@bors r+

bors · 2023-05-07T13:35:49Z

📌 Commit 73b3ce2 has been approved by cjgillot

It is now in the queue for this repository.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#97320 (Stabilize const_ptr_read) - rust-lang#110770 (Limit lifetime of format_args!() with inlined args.) - rust-lang#111021 (Move some tests) - rust-lang#111215 (Various changes to name resolution of anon consts) - rust-lang#111242 (support set `rpath` option for each target independently) - rust-lang#111282 (Remove some `assume`s from slice iterators that don't do anything) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned cjgillot May 4, 2023

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 May 4, 2023

BoxyUwU changed the title ~~im so sorry?~~ Various changes to name resolution of anon consts May 4, 2023

BoxyUwU force-pushed the resolve_anon_consts_differently branch from d66e493 to 316a493 Compare May 4, 2023 19:21