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

Fix symbol display for backing fields #76000

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

RikkiGibson
Copy link
Contributor

Closes #75529

@RikkiGibson RikkiGibson requested review from a team as code owners November 21, 2024 00:08
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 21, 2024
@RikkiGibson RikkiGibson marked this pull request as draft November 21, 2024 03:40
{
AddPropertyNameAndParameters(associatedProperty);
AddPunctuation(SyntaxKind.DotToken);
AddKeyword(SyntaxKind.FieldKeyword);
Copy link
Contributor

Choose a reason for hiding this comment

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

AddKeyword(SyntaxKind.FieldKeyword);

We probably don't want to do this when a metadata name is requested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The closest flag I found was SymbolDisplayCompilerInternalOptions.UseMetadataMethodNames, was this the one you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes something like this. I guess, we might want to make it more general, or add a similar flag for field names and specify it for existing formats that are using UseMetadataMethodNames today.

Copy link
Contributor Author

@RikkiGibson RikkiGibson Nov 22, 2024

Choose a reason for hiding this comment

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

For testing purposes I am going to try checking and using this flag. I expect this will fix a large number of failing tests, which had started using the new symbol display for backing field after the 1st iteration of this PR.

After I see results of that, I will try to figure out a more permanent solution, perhaps by renaming the flag to UseMetadataMemberNames.

I also noticed that this flag is not respected when displaying a property accessor (e.g. Type.Prop.get is still used when the flag is set). I added a test to show that. I'm not sure whether we want to change the behavior for that case.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

@@ -59,7 +59,13 @@ public override void VisitField(IFieldSymbol symbol)
AddPunctuation(SyntaxKind.DotToken);
}

if (symbol.ContainingType.TypeKind == TypeKind.Enum)
if (symbol.AssociatedSymbol is IPropertySymbol associatedProperty)
Copy link
Contributor

Choose a reason for hiding this comment

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

if (symbol.AssociatedSymbol is IPropertySymbol associatedProperty)

Is this going to affect how VB symbols are displayed? We probably don't want that because backing fields in VB have speak-able, user-referenceable, names. We might want to do this substitution only for C# symbols.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am expecting this change to only affect C# symbols due to being in C# layer. I will look for, or add, a VB test to demonstrate that an appropriate display of the backing field is used there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am expecting this change to only affect C# symbols due to being in C# layer.

I do not think this is the case. This layer can be invoked on VB symbols. It operates on common interfaces. I am sure we have a bunch of tests checking C# display for VB symbols and checking VB display for C# symbols.

@@ -59,7 +59,13 @@ public override void VisitField(IFieldSymbol symbol)
AddPunctuation(SyntaxKind.DotToken);
}

if (symbol.ContainingType.TypeKind == TypeKind.Enum)
if (symbol.AssociatedSymbol is IPropertySymbol associatedProperty)
Copy link
Contributor

Choose a reason for hiding this comment

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

if (symbol.AssociatedSymbol is IPropertySymbol associatedProperty)

Is this check going to work for symbols imported from metadata? Please add a test.

@RikkiGibson RikkiGibson marked this pull request as ready for review November 22, 2024 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quick info on field exposes generated member name, unlike on accessors
2 participants