Fix pretty printing of enums without variants#94093

tmiasko · 2022-02-17T17:55:29Z

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

oli-obk · 2022-02-17T20:08:41Z

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

tmiasko · 2022-02-18T00:01:40Z

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.

compiler/rustc_const_eval/src/const_eval/mod.rs

RalfJung · 2022-02-18T01:03:32Z

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 · 2022-02-18T17:54:19Z

(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 · 2022-02-18T17:56:46Z

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 · 2022-02-19T13:49:10Z

@bors r+ rollup

bors · 2022-02-19T13:49:12Z

📌 Commit 863c8229aa1be2c4fe5f91139a25419d1f1a72b5 has been approved by oli-obk

bors · 2022-02-19T15:37:48Z

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

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 · 2022-02-19T16:24:05Z

@bors r+

bors · 2022-02-19T16:24:06Z

📌 Commit c2da477 has been approved by oli-obk

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`

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``

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```

…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

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 assigned oli-obk Feb 17, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2022

RalfJung reviewed Feb 18, 2022

View reviewed changes

compiler/rustc_const_eval/src/const_eval/mod.rs Outdated Show resolved Hide resolved

tmiasko force-pushed the pp-no-variants branch from 66c08bd to 863c822 Compare February 18, 2022 17:20

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 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 force-pushed the pp-no-variants branch from 863c822 to c2da477 Compare February 19, 2022 16:11

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 mentioned this pull request Feb 19, 2022

Rollup of 12 pull requests #94162

Closed

matthiaskrgr mentioned this pull request Feb 19, 2022

Rollup of 11 pull requests #94164

Closed

matthiaskrgr mentioned this pull request Feb 19, 2022

Rollup of 10 pull requests #94167

Closed

matthiaskrgr mentioned this pull request Feb 19, 2022

Rollup of 14 pull requests #94174

Merged

bors merged commit f2d4ffe into rust-lang:master Feb 20, 2022

rustbot added this to the 1.61.0 milestone Feb 20, 2022

tmiasko deleted the pp-no-variants branch February 20, 2022 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix pretty printing of enums without variants#94093