[msvc] Consistently show active variant and fix visualization for single variant enums #86636
Conversation
wesleywiser
added
A-debuginfo
rust-highfive
added
the
S-waiting-on-review
| @@ -282,6 +282,17 @@ pub fn push_debuginfo_type_name<'tcx>( | |||
| output.push_str("enum$<"); | |||
| push_item_name(tcx, def.did, true, output); | |||
| push_type_params(tcx, substs, output, visited); | |||
|
|
|||
| if let Variants::Single { index: variant_idx } = &layout.variants { | |||
| // Uninhabited enums can't be constructed and should never need to be visualized so | |||
There was a problem hiding this comment.
What happens if these end up being behind a pointer? As in: &() as as *const _ as *const Void.
There was a problem hiding this comment.
Good question! In that case, we just don't have any information to display.
There was a problem hiding this comment.
This looks great 😃 I love how the code actually becomes simpler. Thanks, @wesleywiser!
r=me with the nits addressed.
| // cdb-check: [+0x000] variant0 [Type: enum$<core::option::Option<u32>>::None] | ||
| // cdb-check: [+0x000] variant1 [Type: enum$<core::option::Option<u32>>::Some] | ||
| // cdb-check: [+0x004] __0 : 0xc [Type: unsigned int] | ||
| // cdb-check: [+0x000] discriminant : Some (0x1) [Type: core::option::Option] |
There was a problem hiding this comment.
Does the -r2 flag disable natvis? It would be great to give a minimal description of how the dx command is used here at the top of the file.
There was a problem hiding this comment.
No, the -r2 flag means "expand recursively 2 levels". The ! format specifier is what disables the natvis for this expression.
|
☔ The latest upstream changes (presumably #85269) made this pull request unmergeable. Please resolve the merge conflicts. |
Previously, directly tagged enums had a `variant$` field which would show the name of the active variant. We now show the variant using a `[variant]` synthetic item just like we do for niche-layout enums.
Prior to this, we only showed the `[variant]` synthetic property when the dataful variant is active. With this change, we now always show it so the behavior is consistent.
Previously, only the fields would be displayed with no indication of the variant name. If you already knew the enum was univariant, this was ok but if the enum was univariant because of layout, for example, a `Result<T, !>` then it could be very confusing which variant was the active one.
This isn't used anymore after rust-lang#85292
wesleywiser
force-pushed
the
misc_enum_improvements
branch
from
324cd24
to
3db6328
Compare
This comment has been minimized.
This comment has been minimized.
wesleywiser
force-pushed
the
misc_enum_improvements
branch
from
3db6328
to
9c31482
Compare
|
@bors r=michaelwoerister |
|
📌 Commit 9c31482 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 9c31482 with merge 02f9811f757f35bee5720f23e9fef27ad7437711... |
| let discr_ty = enum_layout.field(cx, tag_field).ty; | ||
| let (size, align) = cx.size_and_align_of(discr_ty); | ||
| Some(MemberDescription { | ||
| name: "discriminant".into(), |
There was a problem hiding this comment.
Maybe name this discriminant$? enum Foo { discriminant { bar: u8 } } is valid and would otherwise conflict with this I think.
There was a problem hiding this comment.
Foo in your example is Variants::Single so it doesn't fall into this case. enum Foo2 { discriminant { bar: u8 }, baz { bat: u8 } } does fall into this case but is represented as
enum Foo2 {
discriminant = 0,
baz = 1,
}
union enum$<Foo2> {
Foo2 discriminant;
struct discriminant {
byte bar;
} variant0;
struct baz {
byte bat;
} variant1;
} So the discriminant name used here can't conflict with an identifier used in Rust source.
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors r=michaelwoerister |
|
📌 Commit 38b9061 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 38b9061 with merge 7e92fb437a199962555fb4d120014a25ca804600... |
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors r=michaelwoerister |
|
📌 Commit 457165e has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Prior to this change, there were a few cases where inspecting an enum in either WinDbg or Visual Studio would not show the active variant name. After these changes, we now consistently show the active variant name as
[variant]in the debugger.We also didn't handle single variant enums very well. That is now also resolved.
Before:
After:
r? @michaelwoerister