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

Expose IPropertySymbol.BackingField and IEventSymbol.BackingField #49659

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,14 @@ ITypeSymbol IEventSymbol.Type
}
}

IFieldSymbol? IEventSymbol.BackingField
{
get
{
return _underlying.AssociatedField.GetPublicSymbol();
Copy link
Contributor

Choose a reason for hiding this comment

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

return _underlying.AssociatedField.GetPublicSymbol(); [](start = 16, length = 53)

It doesn't look like this implementation is going to work as advertised for events imported from metadata.

Copy link
Contributor Author

@jnm2 jnm2 Nov 30, 2020

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:

  • Make it possible for analyzers to reason about object instance state, regardless of whether backing fields are included in GetMembers. Today, since event backing fields are not included in GetMembers, analyzers can't tell if an object holds state if it declares only events.
    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.
  • Make it more convenient to go from a property to the matching backing field in GetMembers, if there is a backing field in GetMembers for the property.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand I think this is fine and we could change the advertising.

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)

Copy link
Contributor Author

@jnm2 jnm2 Nov 30, 2020

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Make it possible for analyzers to reason about object instance state, regardless of whether backing fields are included in GetMembers.

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)

Copy link
Contributor Author

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.

Thanks, that's a great point. That refocuses the whole first goal to be specifically about source properties.

Please provide clear specification for the new APIs and a set of targeted API tests that verify that they behave as specified.

I will go ahead with this. What form should the specification be in, e.g. edited into the PR body at the top?

Copy link
Member

Choose a reason for hiding this comment

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

What form should the specification be in, e.g. edited into the PR body at the top?

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

That refocuses the whole first goal to be specifically about source properties.

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.

What form should the specification be in, e.g. edited into the PR body at the top?

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)

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

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.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be willing to put effort into this. Would that be a contribution you'd be willing to accept?

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

_underlying.AssociatedField.GetPublicSymbol(); [](start = 23, length = 46)

Is this symbol returned by GetMembers API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this symbol returned by GetMembers API?

No. Event backing fields are not

I don't think I am comfortable with this inconsistency


In reply to: 532762816 [](ancestors = 532762816)

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 don't think I am comfortable with this inconsistency

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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ IMethodSymbol IPropertySymbol.SetMethod
get { return _underlying.SetMethod.GetPublicSymbol(); }
}

IFieldSymbol IPropertySymbol.BackingField
{
get { return (_underlying as SourcePropertySymbolBase)?.BackingField.GetPublicSymbol(); }
Copy link
Contributor Author

@jnm2 jnm2 Nov 29, 2020

Choose a reason for hiding this comment

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

I thought the name BackingField was more instantly recognizable than AssociatedField, so I went with it as soon as I saw this precedent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be desirable to pull SourcePropertySymbolBase.BackingField up to PropertySymbol as either abstract or virtual returning null?

Copy link
Contributor

Choose a reason for hiding this comment

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

get { return (_underlying as SourcePropertySymbolBase)?.BackingField.GetPublicSymbol(); } [](start = 12, length = 89)

It doesn't look like this implementation is going to work as advertised for properties imported from metadata, retargeting and substituted properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

get { return (_underlying as SourcePropertySymbolBase)?.BackingField.GetPublicSymbol(); } [](start = 12, length = 89)

Is this symbol returned by GetMembers API?

Copy link
Contributor Author

@jnm2 jnm2 Nov 30, 2020

Choose a reason for hiding this comment

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

Yes. (See #49659 (comment).)

}

IPropertySymbol IPropertySymbol.OriginalDefinition
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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")]
Expand All @@ -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")]
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Assert.NotNull(explicitPropertySymbol.BackingField); [](start = 12, length = 52)

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")]
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/Core/Portable/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Microsoft.CodeAnalysis.GeneratorAttribute.GeneratorAttribute(string firstLanguage, params string[] additionalLanguages) -> void
Microsoft.CodeAnalysis.GeneratorAttribute.Languages.get -> string[]
Microsoft.CodeAnalysis.IEventSymbol.BackingField.get -> Microsoft.CodeAnalysis.IFieldSymbol
Microsoft.CodeAnalysis.IPropertySymbol.BackingField.get -> Microsoft.CodeAnalysis.IFieldSymbol
Microsoft.CodeAnalysis.ITypeSymbol.IsRecord.get -> bool
Microsoft.CodeAnalysis.SymbolDisplayPartKind.RecordName = 31 -> Microsoft.CodeAnalysis.SymbolDisplayPartKind
const Microsoft.CodeAnalysis.WellKnownDiagnosticTags.CompilationEnd = "CompilationEnd" -> string
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/Core/Portable/Symbols/IEventSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ public interface IEventSymbol : ISymbol
/// </summary>
IMethodSymbol? RaiseMethod { get; }

/// <summary>
/// The backing field of the event, or null if the event does not use a backing field.
/// </summary>
IFieldSymbol? BackingField { get; }

/// <summary>
/// The original definition of the event. If the event is constructed from another
/// symbol by type substitution, OriginalDefinition gets the original symbol, as it was
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/Core/Portable/Symbols/IPropertySymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does this sound?

Suggested change
/// The backing field of the property, or null if the property does not use a backing field.
/// The compiler-generated backing field of the property, or null if one is not generated.

/// </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
Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/VisualBasic/Portable/Symbols/EventSymbol.vb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Me.AssociatedField [](start = 23, length = 18)

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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Me.AssociatedField [](start = 23, length = 18)

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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1098,9 +1098,12 @@ End Class

propertySymbol = GetPropertySymbol(compilation, bindings, "c.vb", "Property P As Integer", propertySyntax)
Assert.NotNull(propertySymbol)
Assert.Null(propertySymbol.BackingField)

propertySymbol = GetPropertySymbol(compilation, bindings, "c.vb", "Property Q As Object", propertySyntax)
Assert.NotNull(propertySymbol)
Assert.NotNull(propertySymbol.BackingField)
Assert.Equal(propertySymbol, propertySymbol.BackingField.AssociatedSymbol)

propertySymbol = GetPropertySymbol(compilation, bindings, "c.vb", "Property R(index As Integer)", propertySyntax)
Assert.NotNull(propertySymbol)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public ImmutableArray<IEventSymbol> ExplicitInterfaceImplementations
public IEventSymbol? OverriddenEvent => _symbol.OverriddenEvent;
public IMethodSymbol? RaiseMethod => _symbol.RaiseMethod;
public IMethodSymbol? RemoveMethod => _symbol.RemoveMethod;
public IFieldSymbol? BackingField => _symbol.BackingField;
public ITypeSymbol Type => _symbol.Type;
public NullableAnnotation NullableAnnotation => _symbol.NullableAnnotation;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ public ImmutableArray<IPropertySymbol> ExplicitInterfaceImplementations

public IMethodSymbol SetMethod => _symbol.SetMethod;

public IFieldSymbol BackingField => _symbol.BackingField;

public ITypeSymbol Type => _symbol.Type;

public NullableAnnotation NullableAnnotation => _symbol.NullableAnnotation;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public IFieldSymbol BackingField => null;
public IFieldSymbol? BackingField => null;

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,5 +88,7 @@ public override TResult Accept<TResult>(SymbolVisitor<TResult> visitor)
public ImmutableArray<CustomModifier> RefCustomModifiers => ImmutableArray<CustomModifier>.Empty;

public ImmutableArray<CustomModifier> TypeCustomModifiers => ImmutableArray<CustomModifier>.Empty;

public IFieldSymbol BackingField => null;
}
}