Skip to content

SymbolDisplay: Show underlying non-default type for enums#52603

Merged
CyrusNajmabadi merged 10 commits intodotnet:mainfrom
MaStr11:SymbolDisplayShowNonDefaultUnderylingEnumType
Apr 14, 2021
Merged

SymbolDisplay: Show underlying non-default type for enums#52603
CyrusNajmabadi merged 10 commits intodotnet:mainfrom
MaStr11:SymbolDisplayShowNonDefaultUnderylingEnumType

Conversation

@MaStr11
Copy link
Copy Markdown
Contributor

@MaStr11 MaStr11 commented Apr 13, 2021

Fixes #52490

GmzWNndhpn

@MaStr11 MaStr11 requested a review from a team as a code owner April 13, 2021 15:03
@ghost ghost added the Area-Compilers label Apr 13, 2021
Comment thread src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs Outdated
Comment thread src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs Outdated
Comment thread src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs Outdated
SymbolDisplayPartKind.Keyword,
SymbolDisplayPartKind.Space,
SymbolDisplayPartKind.EnumName);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so this should only be shown in the case where we're checking on the top level type. if the enum is referenced anywhere (like in a method signature) it woudl not be shown there. Can you write tests showing that that's the case.

Also, we'd likely need this for VB.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so this should only be shown in the case where we're checking on the top level type

Yes. Adding the underlying type unconditionaly breaks all kinds of features (CI build failures confirm this). I haven't found an obvious way to prevent this. I think we need a new formatOption. But which one and where exactly should it go?

If such an option is present I could turn it of before any VisitNamedType calls e.g. if the SymbolDisplay is requested for an EnumMember.

Can you please give me some hints about what exactly you would expect here to solve this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes. Adding the underlying type unconditionaly breaks all kinds of features

I think we should only doing this in the codepth that is also adding enum. So we only would show: enum E : byte not E : byte just because E was used in a reference location. The SymbolDisplay code should already be partitioned to support this concept afaict.

@jcouv jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 13, 2021
@MaStr11 MaStr11 requested a review from a team as a code owner April 14, 2021 11:24
@MaStr11
Copy link
Copy Markdown
Contributor Author

MaStr11 commented Apr 14, 2021

@CyrusNajmabadi Ready for review. I started from scratch and moved away from SymbolDisplayVisitor. I added the logic to AbstractSymbolDisplayService.AbstractSymbolDescriptionBuilder instead, which I felt is the correct place.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

That works for me.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

LGTM. Thanks!

@CyrusNajmabadi CyrusNajmabadi merged commit e185b26 into dotnet:main Apr 14, 2021
@ghost ghost added this to the Next milestone Apr 14, 2021
@dibarbet dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display enum underlying type on mouse hover (intellisense)

5 participants