Expose IPropertySymbol.BackingField and IEventSymbol.BackingField #49659

jnm2 · 2020-11-29T20:42:21Z

Closes #40103 and closes #42355. Covers #46682 as well.

I considered adding commits demonstrating how useful this is to the IDE layer, particularly in places where features just assume there are no event backing fields to worry about. (Unlike property backing fields, event backing fields are no longer included in ISymbol.GetMembers since they were removed in 2011.) In case that could detract from the simplicity of the PR, I'd like to follow up with some IDE fixes after this PR merges.

jnm2 · 2020-11-29T20:44:59Z

src/Compilers/CSharp/Portable/Symbols/PublicModel/PropertySymbol.cs

@@ -58,6 +58,11 @@ IMethodSymbol IPropertySymbol.SetMethod
            get { return _underlying.SetMethod.GetPublicSymbol(); }
        }

+        IFieldSymbol IPropertySymbol.BackingField
+        {
+            get { return (_underlying as SourcePropertySymbolBase)?.BackingField.GetPublicSymbol(); }


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

jnm2 · 2020-11-29T20:53:30Z

src/Compilers/CSharp/Portable/Symbols/PublicModel/PropertySymbol.cs

@@ -58,6 +58,11 @@ IMethodSymbol IPropertySymbol.SetMethod
            get { return _underlying.SetMethod.GetPublicSymbol(); }
        }

+        IFieldSymbol IPropertySymbol.BackingField
+        {
+            get { return (_underlying as SourcePropertySymbolBase)?.BackingField.GetPublicSymbol(); }


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

jaredpar · 2020-11-30T14:23:04Z

@333fred as this is an API related change.

jaredpar · 2020-11-30T14:24:56Z

src/Compilers/Core/Portable/Symbols/IPropertySymbol.cs

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


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.

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.

jaredpar · 2020-11-30T14:25:13Z

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationEventSymbol.cs

@@ -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;


Suggested change

public IFieldSymbol BackingField => null;

public IFieldSymbol? BackingField => null;

AlekseyTs · 2020-11-30T16:45:03Z

src/Compilers/CSharp/Portable/Symbols/PublicModel/EventSymbol.cs

+        {
+            get
+            {
+                return _underlying.AssociatedField.GetPublicSymbol();


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.

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.

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)

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?

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)

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?

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.

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)

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.

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)

AlekseyTs · 2020-11-30T16:49:11Z

src/Compilers/CSharp/Portable/Symbols/PublicModel/EventSymbol.cs

+        {
+            get
+            {
+                return _underlying.AssociatedField.GetPublicSymbol();


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

Is this symbol returned by GetMembers API?

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.

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)

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.

AlekseyTs · 2020-11-30T16:54:38Z

src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs

@@ -2883,6 +2888,8 @@ class C : I
            Assert.Equal("I.set_P", explicitPropertySetterSymbol.Name);
            Assert.Equal(1, explicitPropertySetterSymbol.ExplicitInterfaceImplementations.Length);
            Assert.Same(explicitPropertySymbol.SetMethod, explicitPropertySetterSymbol);
+
+            Assert.NotNull(explicitPropertySymbol.BackingField);


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.

AlekseyTs · 2020-11-30T16:56:19Z

src/Compilers/CSharp/Portable/Symbols/PublicModel/PropertySymbol.cs

@@ -58,6 +58,11 @@ IMethodSymbol IPropertySymbol.SetMethod
            get { return _underlying.SetMethod.GetPublicSymbol(); }
        }

+        IFieldSymbol IPropertySymbol.BackingField
+        {
+            get { return (_underlying as SourcePropertySymbolBase)?.BackingField.GetPublicSymbol(); }


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.

See #49659 (comment).

AlekseyTs · 2020-11-30T16:56:49Z

src/Compilers/CSharp/Portable/Symbols/PublicModel/PropertySymbol.cs

@@ -58,6 +58,11 @@ IMethodSymbol IPropertySymbol.SetMethod
            get { return _underlying.SetMethod.GetPublicSymbol(); }
        }

+        IFieldSymbol IPropertySymbol.BackingField
+        {
+            get { return (_underlying as SourcePropertySymbolBase)?.BackingField.GetPublicSymbol(); }


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

Is this symbol returned by GetMembers API?

Yes. (See #49659 (comment).)

AlekseyTs · 2020-11-30T17:00:25Z

For symbols that were not exposed before (through GetMembers API, etc.) we should make sure that all IFieldSymbol APIs work as expected for them.

AlekseyTs · 2020-11-30T17:03:21Z

src/Compilers/VisualBasic/Portable/Symbols/EventSymbol.vb

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


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

Is this symbol returned by GetMembers API?

AlekseyTs · 2020-11-30T17:03:31Z

src/Compilers/VisualBasic/Portable/Symbols/PropertySymbol.vb

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


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

Is this symbol returned by GetMembers API?

AlekseyTs · 2020-11-30T17:05:17Z

Done with review pass (iteration 3)

333fred · 2020-12-01T19:19:26Z

@jnm2 I'm going to convert this to a draft. When you have a proposal for the API behavior please ping and feel free to convert back at that point.

jnm2 added 3 commits November 29, 2020 15:10

Expose IPropertySymbol.BackingField

e7953ee

Expose IEventSymbol.BackingField

844c087

Extrapolate tests from usages of other members

cfdfb5b

jnm2 requested review from a team as code owners November 29, 2020 20:42

Dotnet-GitSync-Bot added the Area-Compilers label Nov 29, 2020

jnm2 mentioned this pull request Nov 29, 2020

Expose IPropertySymbol.IsAutoProperty as a public API #46682

Open

jnm2 commented Nov 29, 2020

View reviewed changes

jaredpar reviewed Nov 30, 2020

View reviewed changes

AlekseyTs reviewed Nov 30, 2020

View reviewed changes

333fred marked this pull request as draft December 1, 2020 19:19

Base automatically changed from master to main March 3, 2021 23:53

jnm2 closed this Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose IPropertySymbol.BackingField and IEventSymbol.BackingField #49659

Expose IPropertySymbol.BackingField and IEventSymbol.BackingField #49659

jnm2 commented Nov 29, 2020 •

edited

Loading

jnm2 Nov 29, 2020 •

edited

Loading

jnm2 Nov 29, 2020

jaredpar commented Nov 30, 2020

jaredpar Nov 30, 2020

jnm2 Nov 30, 2020

jaredpar Nov 30, 2020

AlekseyTs Nov 30, 2020

jnm2 Nov 30, 2020 •

edited

Loading

AlekseyTs Nov 30, 2020

jnm2 Nov 30, 2020 •

edited

Loading

AlekseyTs Nov 30, 2020

jnm2 Nov 30, 2020

333fred Nov 30, 2020

AlekseyTs Nov 30, 2020

jnm2 Nov 30, 2020

AlekseyTs Dec 1, 2020

AlekseyTs Nov 30, 2020

jnm2 Nov 30, 2020

AlekseyTs Nov 30, 2020

jnm2 Nov 30, 2020

AlekseyTs Nov 30, 2020

AlekseyTs Nov 30, 2020

jnm2 Nov 30, 2020

AlekseyTs Nov 30, 2020

jnm2 Nov 30, 2020 •

edited

Loading

AlekseyTs commented Nov 30, 2020

AlekseyTs Nov 30, 2020

AlekseyTs Nov 30, 2020

AlekseyTs commented Nov 30, 2020

333fred commented Dec 1, 2020

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

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

Expose IPropertySymbol.BackingField and IEventSymbol.BackingField #49659

Expose IPropertySymbol.BackingField and IEventSymbol.BackingField #49659

Conversation

jnm2 commented Nov 29, 2020 • edited Loading

jnm2 Nov 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm2 Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm2 Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm2 Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Nov 30, 2020

333fred commented Dec 1, 2020

jnm2 commented Nov 29, 2020 •

edited

Loading

jnm2 Nov 29, 2020 •

edited

Loading

jnm2 Nov 30, 2020 •

edited

Loading

jnm2 Nov 30, 2020 •

edited

Loading

jnm2 Nov 30, 2020 •

edited

Loading