-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Expose IPropertySymbol.BackingField and IEventSymbol.BackingField #49659
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,14 @@ ITypeSymbol IEventSymbol.Type | |
} | ||
} | ||
|
||
IFieldSymbol? IEventSymbol.BackingField | ||
{ | ||
get | ||
{ | ||
return _underlying.AssociatedField.GetPublicSymbol(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this symbol returned by GetMembers API? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. Event backing fields are not, starting in 2011 judging by the comments that were left that hinted at removing property backing fields as well which has never happened. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think I am comfortable with this inconsistency In reply to: 532762816 [](ancestors = 532762816) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What do you suggest? This new API is motivated largely as a way to ignore the underlying inconsistency from the perspective of analyzer writers. |
||
} | ||
} | ||
|
||
IEventSymbol IEventSymbol.OriginalDefinition | ||
{ | ||
get | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,11 @@ IMethodSymbol IPropertySymbol.SetMethod | |
get { return _underlying.SetMethod.GetPublicSymbol(); } | ||
} | ||
|
||
IFieldSymbol IPropertySymbol.BackingField | ||
{ | ||
get { return (_underlying as SourcePropertySymbolBase)?.BackingField.GetPublicSymbol(); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be desirable to pull There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It doesn't look like this implementation is going to work as advertised for properties imported from metadata, retargeting and substituted properties. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #49659 (comment). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this symbol returned by GetMembers API? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. (See #49659 (comment).) |
||
} | ||
|
||
IPropertySymbol IPropertySymbol.OriginalDefinition | ||
{ | ||
get | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -360,6 +360,8 @@ public void TestGetDeclaredSymbolFromIndexer() | |
Assert.Equal(MethodKind.PropertySet, setterSymbol.MethodKind); | ||
Assert.Equal(propertySymbol.SetMethod, setterSymbol); | ||
Assert.Equal("void C.this[System.Int32 x, System.Int32 y].set", setterSymbol.ToTestDisplayString()); | ||
|
||
Assert.Null(propertySymbol.BackingField); | ||
} | ||
|
||
[Fact] | ||
|
@@ -389,6 +391,7 @@ event System.Action E | |
Assert.Equal(2, accessorList.Count); | ||
Assert.Same(eventSymbol.AddMethod, model.GetDeclaredSymbol(accessorList[0])); | ||
Assert.Same(eventSymbol.RemoveMethod, model.GetDeclaredSymbol(accessorList[1])); | ||
Assert.Null(eventSymbol.BackingField); | ||
} | ||
|
||
[WorkItem(543494, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/543494")] | ||
|
@@ -406,11 +409,13 @@ public void TestGetDeclaredSymbolFromFieldLikeEvent() | |
var typeDecl = (TypeDeclarationSyntax)root.Members[0]; | ||
var eventDecl = (EventFieldDeclarationSyntax)typeDecl.Members[0]; | ||
var model = compilation.GetSemanticModel(tree); | ||
var eventSymbol = model.GetDeclaredSymbol(eventDecl.Declaration.Variables[0]); | ||
var eventSymbol = (IEventSymbol)model.GetDeclaredSymbol(eventDecl.Declaration.Variables[0]); | ||
Assert.NotNull(eventSymbol); | ||
Assert.Null(model.GetDeclaredSymbol(eventDecl)); // the event decl may have multiple variable declarators, the result for GetDeclaredSymbol will always be null | ||
Assert.Equal("E", eventSymbol.Name); | ||
Assert.IsType<SourceFieldLikeEventSymbol>(eventSymbol.GetSymbol()); | ||
Assert.NotNull(eventSymbol.BackingField); | ||
Assert.Equal(eventSymbol, eventSymbol.BackingField.AssociatedSymbol); | ||
} | ||
|
||
[WorkItem(543574, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/543574")] | ||
|
@@ -2883,6 +2888,8 @@ void I.M() {} | |
Assert.Equal("I.set_P", explicitPropertySetterSymbol.Name); | ||
Assert.Equal(1, explicitPropertySetterSymbol.ExplicitInterfaceImplementations.Length); | ||
Assert.Same(explicitPropertySymbol.SetMethod, explicitPropertySetterSymbol); | ||
|
||
Assert.NotNull(explicitPropertySymbol.BackingField); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think we need a set of pure symbol tests (not involving SemanticModel at all) for the new APIs, which should cover the breadth of scenarios: declared in source, declared in metadata, substituted, retargeting, etc. |
||
} | ||
|
||
[WorkItem(527284, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/527284")] | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -76,6 +76,11 @@ public interface IPropertySymbol : ISymbol | |||||
/// </summary> | ||||||
IMethodSymbol? SetMethod { get; } | ||||||
|
||||||
/// <summary> | ||||||
/// The backing field of the property, or null if the property does not use a backing field. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think the doc comment could be a bit more descriptive here. As written I think it could be interpreted as both user and compiler defined backing fields while this should be only supporting the former. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this sound?
Suggested change
|
||||||
/// </summary> | ||||||
IFieldSymbol? BackingField { get; } | ||||||
|
||||||
/// <summary> | ||||||
/// The original definition of the property. If the property is constructed from another | ||||||
/// symbol by type substitution, OriginalDefinition gets the original symbol, as it was | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -302,6 +302,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols | |
End Get | ||
End Property | ||
|
||
Private ReadOnly Property IEventSymbol_BackingField As IFieldSymbol Implements IEventSymbol.BackingField | ||
Get | ||
Return Me.AssociatedField | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this symbol returned by GetMembers API? |
||
End Get | ||
End Property | ||
|
||
Private ReadOnly Property IEventSymbol_OriginalDefinition As IEventSymbol Implements IEventSymbol.OriginalDefinition | ||
Get | ||
Return Me.OriginalDefinition | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -503,6 +503,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols | |
End Get | ||
End Property | ||
|
||
Private ReadOnly Property IPropertySymbol_BackingField As IFieldSymbol Implements IPropertySymbol.BackingField | ||
Get | ||
Return Me.AssociatedField | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this symbol returned by GetMembers API? |
||
End Get | ||
End Property | ||
|
||
Private ReadOnly Property IPropertySymbol_ReturnsByRef As Boolean Implements IPropertySymbol.ReturnsByRef | ||
Get | ||
Return Me.ReturnsByRef | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -62,5 +62,7 @@ public override void Accept(SymbolVisitor visitor) | |||||
public IEventSymbol? OverriddenEvent => null; | ||||||
|
||||||
public static ImmutableArray<CustomModifier> TypeCustomModifiers => ImmutableArray.Create<CustomModifier>(); | ||||||
|
||||||
public IFieldSymbol BackingField => null; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like this implementation is going to work as advertised for events imported from metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to making AssociatedField behave more uniformly between source/metadata/other properties and events. On the other hand I think this is fine and we could change the advertising. There are two goals:
I think a subgoal here is for analyzers to be able to point to the source declaration that caused a field to exist e.g. for providing diagnostics or naming members in error messages. This goal is not defined except for source properties and events. Once things go to metadata, the analyzer will be talking about whatever it sees in GetMembers without assuming it came from C# source.
I think both goals are met with the current implementation when considering both source properties and metadata properties. We'd have to fix the advertising which is in conflict as you point out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide clear specification for the new APIs and a set of targeted API tests that verify that they behave as specified.
In reply to: 532768881 [](ancestors = 532768881)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it could be less surprising and a matter of convenience for analyzer authors if Roslyn heuristically paired backing fields in metadata back up if they fit past emit patterns. I'd be willing to put effort into this. Would that be a contribution you'd be willing to accept?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, one cannot rely on presence of fields "to reason about object instance state" because, for example, private fields are not imported from metadata by default.
In reply to: 532774220 [](ancestors = 532774220,532768881)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's a great point. That refocuses the whole first goal to be specifically about source properties.
I will go ahead with this. What form should the specification be in, e.g. edited into the PR body at the top?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me. In particular, while I understand the reasoning behind adding these APIs, I'm somewhat concerned about the metadata vs source inconsistencies. I think we need a very clear understanding of what these APIs return, when they work, and when they do not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what exact conclusion you are making here. If one cannot get fields for imported symbols, why is it so important for source? Analyzers and other API consumers often deal with instances of types declared in metadata, therefore, we prefer to not expose APIs that do not provide consistent experience, especially if they are advertised as a way to detect something "very important". For example, we have IPropertySymbol.IsWithEvents property. It works consistently across source and metadata even though in order to calculate it for metadata we need to look at a private field, which is often not imported.
As the doc comment on the API(s) would be fine. Consumers of the API should be able to understand what to expect and should be able to relatively easily predict behavior of the API in different scenarios.
In reply to: 532787537 [](ancestors = 532787537)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's important for source in a way that it's not important for metadata because I tried to write an analyzer to enforce that a class or struct declaration uses
.With
methods for each piece of state it contains.Good to know. I'd be interested in implementing a similar thing for IPropertySymbol.BackingField, even though it would behave like IsWithEvents in the presence of unimported metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at this point it would be good to have complete proposal for the new APIs in the form of specification. Then we can evaluate it and make the decision.
In reply to: 532774783 [](ancestors = 532774783)