Make event backing fields accessible to third-party analyzers #40572

jnm2 · 2019-12-24T02:39:00Z

Fixes #36259. Other than where I commented, all changes are meant to restore identical outcomes now that the event backing field comes along with the other fields for free. I seemed to remove a lot of workarounds.

…perty backing fields

…h the same name as the event of interest

…d prior to being added to the type members collection

… a ref to the event, so taking a ref/pointer to the whole struct should apply to field-like events as well as fields.

jnm2 · 2019-12-24T02:48:03Z

src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs

+                if ((member as FieldSymbol)?.AssociatedSymbol is EventSymbol)
+                {
+                    continue;
+                }


Do you have suggestions for alternatives? This is added to avoid the assertion on line 72:

roslyn/src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs

Line 72 in 374d128

Debug.Assert(result.IsMultiViable || result.IsClear || result.Error != null);

jnm2 · 2019-12-24T02:49:05Z

src/Compilers/CSharp/Portable/FlowAnalysis/UnassignedFieldsWalker.cs

-                var symbol = member switch
-                {
-                    FieldSymbol { AssociatedSymbol: PropertySymbol p } => p,
-                    _ => member
-                };
+
+                var symbol = field.AssociatedSymbol is PropertySymbol || field.AssociatedSymbol is EventSymbol ? field.AssociatedSymbol : field;


What do you think of making this field.AssociatedSymbol ?? field?

jnm2 · 2019-12-24T02:51:03Z

src/Compilers/CSharp/Portable/FlowAnalysis/UnassignedFieldsWalker.cs

                {
                    continue;
                }
+
+                var fieldType = field.TypeWithAnnotations;


To be perfectly equivalent to the original code, this would be:

var fieldType = (field.AssociatedField as EventSymbol)?.TypeWithAnnotations ?? field.TypeWithAnnotations;

But it seems unlikely that they would differ.

jnm2 · 2019-12-24T02:53:02Z

src/Compilers/CSharp/Portable/Symbols/Symbol.cs

                    case SymbolKind.Field:
+                        if (((FieldSymbol)this).AssociatedSymbol is EventSymbol)
+                            return false;
+                        break;


Is this correct?

jnm2 · 2019-12-24T03:01:11Z

src/Compilers/CSharp/Test/Symbol/Symbols/Metadata/PE/LoadingEvents.cs

+        private static MetadataReference CompileToPEReference(string source)
+        {
+            using var peStream = new MemoryStream();
+
+            Assert.True(CreateCompilation(source).Emit(peStream).Success);
+
+            peStream.Position = 0;
+
+            return MetadataReference.CreateFromStream(peStream);
+        }
+
+        private static IAssemblySymbol GetAssemblySymbolForReference(MetadataReference reference)
+        {
+            var compilation = (Compilation)CSharpCompilation.Create(
+                assemblyName: null,
+                references: new[] { reference },
+                options: new CSharpCompilationOptions(
+                    OutputKind.DynamicallyLinkedLibrary,
+                    metadataImportOptions: MetadataImportOptions.All));
+
+            return (IAssemblySymbol)compilation.GetAssemblyOrModuleSymbol(reference);
+        }


Are there existing helpers I should use/places to add tests if I want to specifically test end to end, source to PublicModel.ISymbol?

If not, where should this code go?

AlekseyTs · 2019-12-26T18:18:35Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

-                        }
-                    }
-
+                    members.AddRange(fieldMembers);


members.AddRange(fieldMembers); [](start = 19, length = 32)

Does this also affects fields that back auto-properties?

According to the original Debug.Assert, field.AssociatedSymbol is either null or its Kind is SymbolKind.Event. That means that property backing fields are never in fieldMembers at this point. I'm not sure why.

AlekseyTs

I don't think we would want to change the set of members returned for a NamedTypeSymbol. The model is meant to reflect the language and, form the C# language point of view, there is no backing field member. As you can see we went through a big trouble to hide fields like that and that was intentional. If analyzers or other tools need to get to the backing fields, it would be interesting to understand the real world scenarios. And if we will find them compelling, then perhaps we can add AssociatedSymbol API (or AssociatedField API) to IEventSymbol interface.

jnm2 · 2019-12-26T18:58:25Z

@AlekseyTs

I don't think we would want to change the set of members returned for a NamedTypeSymbol. The model is meant to reflect the language and, form the C# language point of view, there is no backing field member. As you can see we went through a big trouble to hide fields like that and that was intentional.

Why is it that property backing fields are returned in GetMembers? One real-world scenario cares about backing fields but doesn't care about the difference between properties and events.

💡 Is there any way you'd consider removing property backing fields from GetMembers? That would solve an unrelated problem that is complicating a C# feature.

333fred · 2019-12-26T18:59:37Z

I don't think we would want to change the set of members returned for a NamedTypeSymbol. The model is meant to reflect the language and, form the C# language point of view, there is no backing field member. As you can see we went through a big trouble to hide fields like that and that was intentional. If analyzers or other tools need to get to the backing fields, it would be interesting to understand the real world scenarios. And if we will find them compelling, then perhaps we can add AssociatedSymbol API (or AssociatedField API) to IEventSymbol interface.

@AlekseyTs clearly there is a community ask to expose these members. Even our documentation for IFieldSymbol.AssociatedSymbol suggests that we should be exposing the backing field: If this field serves as a backing variable for an automatically generated property or a field-like event, returns that property/event. Given that, I think it makes sense to make a change here, and do it in the same way that we expose the backing fields for autoproperties.

AlekseyTs · 2019-12-26T19:31:23Z

@333fred

clearly there is a community ask to expose these members

Then it should be easy to present real world compelling scenarios.

Given that, I think it makes sense to make a change here, and do it in the same way that we expose the backing fields for autoproperties.

This is a significant design change not merely a bug fix. Why don't you bring this to public API council?

jnm2 · 2019-12-26T19:46:21Z

Here are the two real-world use cases I know of: #36259 (comment)

AlekseyTs · 2019-12-26T19:53:21Z

Here are the two real-world use cases I know of: #36259 (comment)

Someone will have distill these to clear summary about the use cases, so that multiple people wouldn't have to sift trough discussion threads, and/or try to infer something from code snippets, etc

AlekseyTs · 2019-12-26T20:02:47Z

Is there any way you'd consider removing property backing fields from GetMembers? That would solve an unrelated problem that is complicating a C# feature.

Yes, we could consider doing this. Again, having a compelling scenario can help us make a decision. In fact, there is a "TODO" comment in code indicating that we probably had intention to do so, and it is not clear whether we explicitly decided against that, or simply didn't get to implementing the change (https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs#L3098):

                            FieldSymbol backingField = property.BackingField;

                            // TODO: can we leave this out of the member list?
                            // From the 10/12/11 design notes:
                            //   In addition, we will change autoproperties to behavior in 
                            //   a similar manner and make the autoproperty fields private.

333fred · 2020-02-28T00:36:16Z

@jnm2 can we please get a clear summary of the use case here? A motivating example will help us understand if this is the correct way to solve the issue, or if a separate API is needed. Thanks.

jnm2 · 2020-03-12T14:56:48Z

I believe a separate API is needed, so I'll close this.

Further, it would unblock design options for dotnet/csharplang#140 if property backing fields were also deliberately excluded from the ITypeSymbol.GetMembers() collection the way event backing fields are. That would necessitate an API to retrieve backing fields for property symbols as well as event symbols for the sake of third-party analyzers.

jnm2 added 9 commits December 21, 2019 16:04

Add failing test that event backing fields are provided just like pro…

9015586

…perty backing fields

Include event backing fields in type members

ab2b3b4

Fix tests unintentionally asserting that there is no other member wit…

7f8aa13

…h the same name as the event of interest

Update tests

0d16a5f

Allow event backing field to have declaring syntax reference as it di…

d156114

…d prior to being added to the type members collection

This is more consistent. 'Event is never used' disappears if you take…

0ea8109

… a ref to the event, so taking a ref/pointer to the whole struct should apply to field-like events as well as fields.

Make PE-backed symbols behave the same as source symbols

a76964a

More GetEvent

a50e2c7

Update tests

374d128

jnm2 requested review from a team as code owners December 24, 2019 02:39

jnm2 changed the title ~~Event field symbol~~ Make event backing fields accessible to third-party analyzers Dec 24, 2019

jnm2 commented Dec 24, 2019

View reviewed changes

jnm2 mentioned this pull request Dec 24, 2019

No associated IFieldSymbol is provided for a field-like event #36259

Closed

jnm2 commented Dec 24, 2019

View reviewed changes

jcouv added the Area-Compilers label Dec 24, 2019

AlekseyTs reviewed Dec 26, 2019

View reviewed changes

AlekseyTs requested changes Dec 26, 2019

View reviewed changes

333fred added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 26, 2019

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 7, 2020

jnm2 closed this Mar 12, 2020

jnm2 mentioned this pull request Jun 7, 2020

Finish removing backing fields from GetMembers #44932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make event backing fields accessible to third-party analyzers #40572

Make event backing fields accessible to third-party analyzers #40572

jnm2 commented Dec 24, 2019 •

edited

Loading

jnm2 Dec 24, 2019

jnm2 Dec 24, 2019

jnm2 Dec 24, 2019

jnm2 Dec 24, 2019

jnm2 Dec 24, 2019 •

edited

Loading

AlekseyTs Dec 26, 2019

jnm2 Dec 26, 2019 •

edited

Loading

AlekseyTs left a comment

jnm2 commented Dec 26, 2019

333fred commented Dec 26, 2019

AlekseyTs commented Dec 26, 2019

jnm2 commented Dec 26, 2019

AlekseyTs commented Dec 26, 2019

AlekseyTs commented Dec 26, 2019

333fred commented Feb 28, 2020

jnm2 commented Mar 12, 2020

Make event backing fields accessible to third-party analyzers #40572

Make event backing fields accessible to third-party analyzers #40572

Conversation

jnm2 commented Dec 24, 2019 • edited Loading

jnm2 Dec 24, 2019

Choose a reason for hiding this comment

jnm2 Dec 24, 2019

Choose a reason for hiding this comment

jnm2 Dec 24, 2019

Choose a reason for hiding this comment

jnm2 Dec 24, 2019

Choose a reason for hiding this comment

jnm2 Dec 24, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Dec 26, 2019

Choose a reason for hiding this comment

jnm2 Dec 26, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

jnm2 commented Dec 26, 2019

333fred commented Dec 26, 2019

AlekseyTs commented Dec 26, 2019

jnm2 commented Dec 26, 2019

AlekseyTs commented Dec 26, 2019

AlekseyTs commented Dec 26, 2019

333fred commented Feb 28, 2020

jnm2 commented Mar 12, 2020

jnm2 commented Dec 24, 2019 •

edited

Loading

jnm2 Dec 24, 2019 •

edited

Loading

jnm2 Dec 26, 2019 •

edited

Loading