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.

Sign up for GitHub

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

Jump to bottom

Fix pretty printing of enums without variants #94093

Merged
merged 1 commit into from
Feb 20, 2022
Merged

Fix pretty printing of enums without variants #94093

merged 1 commit into from
Feb 20, 2022

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Feb 17, 2022

92d20c4 removed no-variants special case from try_destructure_const with expectation that this case would be handled gracefully when read_discriminant returns an error.

Alas in that case read_discriminant succeeds while returning a non-existing variant, so the special case is still necessary.

Fixes #94073.

r? @oli-obk

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 17, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Feb 17, 2022

Huh... why does read_discriminant succeed when there are no variants, that seems like a bug?

@tmiasko
Copy link
Contributor Author

tmiasko commented Feb 18, 2022

For types without discriminants, which includes zero variant enums, read_discriminant returns zero. For zero variant enums specifically this was allowed back in #90910 (the change was from an ICE to a zero variant index)

cc @RalfJung.

@RalfJung
Copy link
Member

Huh... why does read_discriminant succeed when there are no variants, that seems like a bug?

It has to succeed on types without discriminants. This is exposed to safe code via https://doc.rust-lang.org/nightly/std/mem/fn.discriminant.html for any type T.

Requiring full validity didn't work, due to #91029.

@RalfJung
Copy link
Member

(For checking what changed in a PR, it'd be easier if you don't rebase onto newer master -- right now the force-push-diff that GH generates is useless if I want to see what you changed in the latest version of this PR, compared to a previous one. During review, I think it is best if force-pushes are avoided entirely, since GH is not great at keeping track of multiple revisions of a PR, and can lose diffs if there are multiple consecutive force-pushes. Our highfive bot also at least used to recommend that contributors do not force-push until review is done; not sure if it still does that.)

@RalfJung
Copy link
Member

Huh... why does read_discriminant succeed when there are no variants, that seems like a bug?

So basically we have two invariants that we would like to maintain but cannot both maintain:

  • read_discriminant always returns some discriminant, even for types that don't have any
  • read_discriminant on an enum always returns a valid discriminant index

I am not sure which invariant is less painful to break. This PR is necessary since we recently switched from breaking the former to breaking the latter.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 19, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 19, 2022

📌 Commit 863c8229aa1be2c4fe5f91139a25419d1f1a72b5 has been approved by oli-obk

@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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2022
@bors
Copy link
Contributor

bors commented Feb 19, 2022

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 19, 2022
@tmiasko
Fix pretty printing of enums without variants
c2da477 
92d20c4 removed no-variants special
case from try_destructure_const with expectation that this case would be
handled gracefully when read_discriminant returns an error.

Alas in that case read_discriminant succeeds while returning a
non-existing variant, so the special case is still necessary.
@oli-obk
Copy link
Contributor

oli-obk commented Feb 19, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 19, 2022

📌 Commit c2da477 has been approved by oli-obk

@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 Feb 19, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
@matthiaskrgr
Rollup merge of rust-lang#94093 - tmiasko:pp-no-variants, r=oli-obk
0f2be0d 
Fix pretty printing of enums without variants

92d20c4 removed no-variants special case from `try_destructure_const` with expectation that this case would be handled gracefully when `read_discriminant` returns an error.

Alas in that case `read_discriminant` succeeds while returning a non-existing variant, so the special case is still necessary.

Fixes rust-lang#94073.

r? `@oli-obk`
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 19, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
@matthiaskrgr
Rollup merge of rust-lang#94093 - tmiasko:pp-no-variants, r=oli-obk
8517581 
Fix pretty printing of enums without variants

92d20c4 removed no-variants special case from `try_destructure_const` with expectation that this case would be handled gracefully when `read_discriminant` returns an error.

Alas in that case `read_discriminant` succeeds while returning a non-existing variant, so the special case is still necessary.

Fixes rust-lang#94073.

r? ``@oli-obk``
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 19, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 19, 2022
@matthiaskrgr
Rollup merge of rust-lang#94093 - tmiasko:pp-no-variants, r=oli-obk
7d5a3a3 
Fix pretty printing of enums without variants

92d20c4 removed no-variants special case from `try_destructure_const` with expectation that this case would be handled gracefully when `read_discriminant` returns an error.

Alas in that case `read_discriminant` succeeds while returning a non-existing variant, so the special case is still necessary.

Fixes rust-lang#94073.

r? ```@oli-obk```
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 19, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 19, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2022
@bors
Auto merge of rust-lang#94174 - matthiaskrgr:rollup-snyrlhy, r=matthi…
25ad89e 
…askrgr

Rollup of 14 pull requests

Successful merges:

 - rust-lang#93580 (Stabilize pin_static_ref.)
 - rust-lang#93639 (Release notes for 1.59)
 - rust-lang#93686 (core: Implement ASCII trim functions on byte slices)
 - rust-lang#94002 (rustdoc: Avoid duplicating macros in sidebar)
 - rust-lang#94019 (removing architecture requirements for RustyHermit)
 - rust-lang#94023 (adapt static-nobundle test to use llvm-nm)
 - rust-lang#94091 (Fix rustdoc const computed value)
 - rust-lang#94093 (Fix pretty printing of enums without variants)
 - rust-lang#94097 (Add module-level docs for `rustc_middle::query`)
 - rust-lang#94112 (Optimize char_try_from_u32)
 - rust-lang#94113 (document rustc_middle::mir::Field)
 - rust-lang#94122 (Fix miniz_oxide types showing up in std docs)
 - rust-lang#94142 (rustc_typeck: adopt let else in more places)
 - rust-lang#94146 (Adopt let else in more places)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f2d4ffe into rust-lang:master Feb 20, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 20, 2022
@tmiasko tmiasko deleted the pp-no-variants branch February 20, 2022 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.

ICE: for_variant called on zero-variant enum: rustc_middle/src/ty/layout.rs:2254:25
6 participants
@tmiasko @oli-obk @RalfJung @bors @rust-highfive @rustbot