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

typeck: always expose explicit enum discriminant AnonConsts' parent in generics_of. #70825

Merged
merged 3 commits into from
May 3, 2020

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Apr 5, 2020

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:

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 5, 2020
@@ -411,6 +411,22 @@ fn check_type_defn<'tcx, F>(
ObligationCauseCode::MiscObligation,
)
}

// Explicit `enum` discriminant values must const-evaluate successfully.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@@ -1182,14 +1182,28 @@ fn generics_of(tcx: TyCtxt<'_>, def_id: DefId) -> &ty::Generics {
}
// FIXME(#43408) enable this always when we get lazy normalization.
Node::AnonConst(_) => {
let parent_id = tcx.hir().get_parent_item(hir_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@nikomatsakis
Copy link
Contributor

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

@joelpalmer 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
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me, left a few nits

src/librustc_typeck/check/wfcheck.rs Outdated Show resolved Hide resolved
@@ -411,6 +411,22 @@ fn check_type_defn<'tcx, F>(
ObligationCauseCode::MiscObligation,
)
}

// Explicit `enum` discriminant values must const-evaluate successfully.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2020
…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
@bors

This comment has been minimized.

@nikomatsakis 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
@joelpalmer 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 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
Copy link
Member Author

eddyb commented May 2, 2020

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

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

eddyb commented May 2, 2020

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented May 2, 2020

📌 Commit 926c7a2 has been approved by nikomatsakis

@bors 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
Copy link
Contributor

bors commented May 3, 2020

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

@bors bors mentioned this pull request May 3, 2020
@Dylan-DPC-zz
Copy link

@bors retry yield

@eddyb
Copy link
Member Author

eddyb commented May 3, 2020

@bors rollup=never p=1

@bors
Copy link
Contributor

bors commented May 3, 2020

⌛ Testing commit 926c7a2 with merge e5f35df...

@bors
Copy link
Contributor

bors commented May 3, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 3, 2020
@bors bors merged commit e5f35df into rust-lang:master May 3, 2020
@eddyb eddyb deleted the enum-discr-correct-generics-parent branch June 25, 2020 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

Should enum discriminants have generics in scope?
8 participants