typeck: always expose explicit enum discriminant `AnonConst`s' parent in `generics_of`.#70825

eddyb · 2020-04-05T21:28:29Z

This is similar to #70452 but for explicit enum discriminant constant expressions.
However, unlike #70452, this PR should have no effect on stable code, as while it alleviates #43408 errors, there is no way to actually compile an enum with generic parameters and explicit discriminants, without #![feature(arbitrary_enum_discriminant)], as explicit discriminant expression don't count as uses of parameters (if they did, they would count as invariant uses).

There's also 2 other commits here, both related to #70453:

"ty: use delay_span_bug in ty::AdtDef::eval_explicit_discr." - hides the ICEs demonstrated on Should enum discriminants have generics in scope? #70453, when there are other errors (which the next commit adds)
"typeck/wfcheck: require that explicit enum discriminants const-evaluate succesfully." - closes Should enum discriminants have generics in scope? #70453 by picking alternative "2", i.e. erroring when a discriminant doesn't fully const-evaluate from the perspective of the enum definition

In the future, it might be possible to allow enum discriminants to actually depend on parameters, but that will likely require #68436 + some way to restrict the values so no two variants can end up with overlapping discriminants.

As this PR would close #70453, it shouldn't be merged until a decision is reached there.

r? @nikomatsakis

Centril · 2020-04-05T21:35:52Z

src/librustc_typeck/check/wfcheck.rs

Comment could use some elaboration regarding the "why" as well as a link towards #70453.

Just to make sure I'm following along, this concept of "must be evaluatable" is going to ultimately be true for any const expression, right? I think this is the basic WF criteria we are working towards?

Yes, at the minimum any ty::Const used in the typesystem (array lengths and args to const params). While enum discrminants aren't that, they do require strict checks (no overlap), for which successful evaluatability is a bare minimum.

Centril · 2020-04-05T21:48:42Z

src/librustc_typeck/collect.rs

Nit: bind let hir = tcx.hir(); earlier in the function to reduce repetition.

What's funny is that I copy-pasted these two lines from #70452 - but also I'm not fond of binding tcx.hir() due to the ideal future in which we request the tcx.hir(...) for a given LocalDefId and then use that, binding it wouldn't help with that AFAICT.

Well it's just a nit, but are there plans for that future though?

nikomatsakis · 2020-04-06T13:27:02Z

FYI: I'm planning to review this this week.

nikomatsakis

r=me, left a few nits

src/librustc_typeck/check/wfcheck.rs

nikomatsakis · 2020-04-14T14:52:23Z

src/librustc_typeck/check/wfcheck.rs

Just to make sure I'm following along, this concept of "must be evaluatable" is going to ultimately be true for any const expression, right? I think this is the basic WF criteria we are working towards?

@Skinny121

…rent, r=nikomatsakis typeck: always expose repeat count `AnonConst`s' parent in `generics_of`. This should reduce some of the confusion around rust-lang#43408, although, if you look at the changed test outputs (for the last commit), they all hit rust-lang#68436, so nothing new will start compiling. We can let counts of "repeat expressions" (`N` in `[x; N]`) always have the correct generics parenting, because they're always in a body, so nothing in the `where` clauses or `impl` trait/type of the parent can use it, and therefore no query cycles can occur. <hr/> Other potential candidates we might want to apply the same approach to, are: * ~~(easy) `enum` discriminants (see also rust-lang#70453)~~ opened rust-lang#70825 * (trickier) array *type* (not *expression*) lengths nested in: * bodies * types of (associated or not) `const`/`static` * RHS of `type` aliases and associated `type`s * `fn` signatures We should've done so from the start, the only reason we haven't is because I was squeamish about blacklisting some of the cases, but if we whitelist instead we should be fine. Also, lazy normalization is taking forever 😞. <hr/> There's also 5 other commits here: * "typeck: track any errors injected during writeback and taint tables appropriately." - fixes rust-lang#66706, as the next commit would otherwise trigger an ICE again * "typeck: workaround WF hole in `to_const`." - its purpose is to emulate most of rust-lang#70107's direct effect, at least in the case of repeat expressions, where the count always goes through `to_const` * this is the reason no new code can really compile, as the WF checks require rust-lang#68436 to bypass * however, this has more test changes than I hoped, so it should be reviewed separately, and maybe even landed separately (as rust-lang#70107 might take a while, as it's blocked on a few of my PRs) * "ty: erase lifetimes early in `ty::Const::eval`." - first attempt at fixing rust-lang#70773 * still useful, I believe the new approach is less likely to cause issues long-term * I could take this out or move it into another PR if desired or someone else could take over (cc @Skinny121) * "traits/query/normalize: add some `debug!` logging for the result." - debugging aid for rust-lang#70773 * "borrow_check/type_check: normalize `Aggregate` and `Call` operands." - actually fixes rust-lang#70773 r? @nikomatsakis cc @pnkfelix @varkor @yodaldevoid @oli-obk @estebank

eddyb · 2020-05-02T14:39:36Z

Oops, I missed that FCP completed (#70453 (comment)).

…te succesfully.

… in `generics_of`.

eddyb · 2020-05-02T18:30:08Z

@bors r=nikomatsakis

bors · 2020-05-02T18:30:10Z

📌 Commit 926c7a2 has been approved by nikomatsakis

bors · 2020-05-03T06:24:18Z

⌛ Testing commit 926c7a2 with merge 97fb24cfbf1ff1cb855868b9209675f7c5031abd...

Dylan-DPC-zz · 2020-05-03T08:52:57Z

@bors retry yield

eddyb · 2020-05-03T11:49:54Z

@bors rollup=never p=1

bors · 2020-05-03T12:05:52Z

⌛ Testing commit 926c7a2 with merge e5f35df...

bors · 2020-05-03T15:11:37Z

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing e5f35df to master...

rust-highfive assigned nikomatsakis Apr 5, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2020

This was referenced Apr 5, 2020

Should enum discriminants have generics in scope? #70453

Closed

typeck: always expose repeat count AnonConsts' parent in generics_of. #70452

Merged

Centril suggested changes Apr 5, 2020

View reviewed changes

joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 14, 2020

nikomatsakis approved these changes Apr 14, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2020

eddyb mentioned this pull request Apr 16, 2020

Do not ICE in the face of invalid enum discriminant #69903

Merged

joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2020

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 2, 2020

eddyb added 3 commits May 2, 2020 18:58

ty: use delay_span_bug in ty::AdtDef::eval_explicit_discr.

50c63e4

typeck/wfcheck: require that explicit enum discriminants const-evalua…

2c29f0c

…te succesfully.

typeck: always expose explicit enum discriminant AnonConsts' parent…

926c7a2

… in `generics_of`.

eddyb force-pushed the enum-discr-correct-generics-parent branch from 1dcd516 to 926c7a2 Compare May 2, 2020 17:44

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 2, 2020

bors mentioned this pull request May 3, 2020

Rollup of 5 pull requests #71828

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label May 3, 2020

bors merged commit e5f35df into rust-lang:master May 3, 2020

JohnTitor mentioned this pull request May 6, 2020

ICE: index out of bounds: the len is 0 but the index is 0 #67945

Closed

eddyb deleted the enum-discr-correct-generics-parent branch June 25, 2020 18:15

Uh oh!

Conversation

eddyb commented Apr 5, 2020

Uh oh!

Centril Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

eddyb Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

Centril Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

eddyb Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

Centril Apr 5, 2020

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Apr 6, 2020

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikomatsakis Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

eddyb commented May 2, 2020

Uh oh!

eddyb commented May 2, 2020

Uh oh!

bors commented May 2, 2020

Uh oh!

bors commented May 3, 2020

Uh oh!

Dylan-DPC-zz commented May 3, 2020

Uh oh!

eddyb commented May 3, 2020

Uh oh!

bors commented May 3, 2020

Uh oh!

bors commented May 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants