Propagate DynamicallyAccessedMembers annotations to auto-properties only (#119329) (#119407)

In the trimmer and ILC, we searched for backing fields with heuristics that validated a backing field had a `CompilerGeneratedAttribute`. In C# 14, the `field` keyword / semi-auto property feature means that some properties will have a compiler-generated field, but a user-written accessor. This can cause holes and unexpected behavior with the current heuristics. The semi-auto property accessors do not have `CompilerGeneratedAttribute`, so we can tighten the heuristic to only find backing fields when all accessors _and_ the found backing field have `CompilerGeneratedAttribute`. This should apply backing field propagation heuristics to properties with at least one auto accessor (e.g. `int Prop { get => field; set; }`) only (and also warns when it is unable to find the backing field for these). For all other properties, it is possible to annotate the backing field and accessors to achieve the same behavior.

Additional context: #117072 (comment) and #117072 (comment)

Author: jtschuster

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Diagnostics;
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.Reflection.Metadata;
@@ -523,7 +524,7 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key)
                             // Look for the compiler generated backing field. If it doesn't work out simply move on. In such case we would still
                             // propagate the annotation to the setter/getter and later on when analyzing the setter/getter we will warn
                             // that the field (which ever it is) must be annotated as well.
-                            ScanMethodBodyForFieldAccess(methodBody, write: true, out backingFieldFromSetter);
+                            backingFieldFromSetter = GetAutoPropertyCompilerGeneratedField(methodBody, isWriteAccessor: true);
                         }
 
                         MethodAnnotations? setterAnnotation = null;
@@ -560,16 +561,14 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key)
                     MethodDesc getMethod = property.GetMethod;
                     if (getMethod != null)
                     {
-
-                        // Abstract property backing field propagation doesn't make sense, and any derived property will be validated
-                        // to have the exact same annotations on getter/setter, and thus if it has a detectable backing field that will be validated as well.
+                        // Look only at compiler-generated accessors
                         MethodIL methodBody = _ilProvider.GetMethodIL(getMethod);
                         if (methodBody != null)
                         {
                             // Look for the compiler generated backing field. If it doesn't work out simply move on. In such case we would still
                             // propagate the annotation to the setter/getter and later on when analyzing the setter/getter we will warn
                             // that the field (which ever it is) must be annotated as well.
-                            ScanMethodBodyForFieldAccess(methodBody, write: false, out backingFieldFromGetter);
+                            backingFieldFromGetter = GetAutoPropertyCompilerGeneratedField(methodBody, isWriteAccessor: false);
                         }
 
                         MethodAnnotations? getterAnnotation = null;
@@ -593,27 +592,41 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key)
                         }
                     }
 
-                    FieldDesc? backingField;
-                    if (backingFieldFromGetter != null && backingFieldFromSetter != null &&
-                        backingFieldFromGetter != backingFieldFromSetter)
-                    {
-                        _logger.LogWarning(property, DiagnosticId.DynamicallyAccessedMembersCouldNotFindBackingField, property.GetDisplayName());
-                        backingField = null;
-                    }
-                    else
-                    {
-                        backingField = backingFieldFromGetter ?? backingFieldFromSetter;
-                    }
-
-                    if (backingField != null)
+                    if (IsAutoProperty(property))
                     {
-                        if (annotatedFields.Any(a => a.Field == backingField))
+                        FieldDesc? backingField = null;
+                        if ((property.SetMethod is not null
+                                && property.SetMethod.HasCustomAttribute("System.Runtime.CompilerServices", "CompilerGeneratedAttribute")
+                                && backingFieldFromSetter is null)
+                            || (property.GetMethod is not null
+                                && property.GetMethod.HasCustomAttribute("System.Runtime.CompilerServices", "CompilerGeneratedAttribute")
+                                && backingFieldFromGetter is null))
                         {
-                            _logger.LogWarning(backingField, DiagnosticId.DynamicallyAccessedMembersOnPropertyConflictsWithBackingField, property.GetDisplayName(), backingField.GetDisplayName());
+                            // We failed to find the backing field of an auto-property accessor
+                            _logger.LogWarning(property, DiagnosticId.DynamicallyAccessedMembersCouldNotFindBackingField, property.GetDisplayName());
+                        }
+                        else if (backingFieldFromGetter is not null && backingFieldFromSetter is not null
+                            && backingFieldFromSetter != backingFieldFromGetter)
+                        {
+                            // We found two different backing fields for the getter and the setter
+                            _logger.LogWarning(property, DiagnosticId.DynamicallyAccessedMembersCouldNotFindBackingField, property.GetDisplayName());
                         }
                         else
                         {
-                            annotatedFields.Add(new FieldAnnotation(backingField, annotation));
+                            // We either have a single auto-property accessor or both accessors point to the same backing field
+                            backingField = backingFieldFromSetter ?? backingFieldFromGetter;
+                        }
+
+                        if (backingField != null)
+                        {
+                            if (annotatedFields.Any(a => a.Field == backingField))
+                            {
+                                _logger.LogWarning(backingField, DiagnosticId.DynamicallyAccessedMembersOnPropertyConflictsWithBackingField, property.GetDisplayName(), backingField.GetDisplayName());
+                            }
+                            else
+                            {
+                                annotatedFields.Add(new FieldAnnotation(backingField, annotation));
+                            }
                         }
                     }
                 }
@@ -651,59 +664,69 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key)
                 return attrs;
             }
 
-            private static bool ScanMethodBodyForFieldAccess(MethodIL body, bool write, out FieldDesc? found)
+            /// <summary>
+            /// Returns true if the property has a single accessor which is compiler generated,
+            /// indicating that it is an auto-property.
+            /// </summary>
+            /// <remarks>
+            /// Ideally this would be tightened to only return true if both accessors are auto-property accessors,
+            /// but it allows for either for back compatibility with existing behavior.
+            /// </remarks>
+            private static bool IsAutoProperty(PropertyPseudoDesc property)
+            {
+                return property.SetMethod?.HasCustomAttribute("System.Runtime.CompilerServices", "CompilerGeneratedAttribute") == true
+                    || property.GetMethod?.HasCustomAttribute("System.Runtime.CompilerServices", "CompilerGeneratedAttribute") == true;
+            }
+
+            private static FieldDesc? GetAutoPropertyCompilerGeneratedField(MethodIL body, bool isWriteAccessor)
             {
                 // Tries to find the backing field for a property getter/setter.
                 // Returns true if this is a method body that we can unambiguously analyze.
                 // The found field could still be null if there's no backing store.
-                found = null;
 
+                // Only analyze compiler-generated accessors
+                if (!body.OwningMethod.HasCustomAttribute("System.Runtime.CompilerServices", "CompilerGeneratedAttribute"))
+                {
+                    return null;
+                }
+
+                FieldDesc? found = null;
                 ILReader ilReader = new ILReader(body.GetILBytes());
 
                 while (ilReader.HasNext)
                 {
                     ILOpcode opcode = ilReader.ReadILOpcode();
                     switch (opcode)
                     {
-                        case ILOpcode.ldsfld when !write:
-                        case ILOpcode.ldfld when !write:
-                        case ILOpcode.stsfld when write:
-                        case ILOpcode.stfld when write:
+                        case ILOpcode.ldsfld when !isWriteAccessor:
+                        case ILOpcode.ldfld when !isWriteAccessor:
+                        case ILOpcode.stsfld when isWriteAccessor:
+                        case ILOpcode.stfld when isWriteAccessor:
+                        {
+                            // Multiple field accesses - ambiguous, fail
+                            if (found != null)
                             {
-                                // This writes/reads multiple fields - can't guess which one is the backing store.
-                                // Return failure.
-                                if (found != null)
-                                {
-                                    found = null;
-                                    return false;
-                                }
-                                found = (FieldDesc)body.GetObject(ilReader.ReadILToken());
+                                return null;
                             }
+                            found = (FieldDesc)body.GetObject(ilReader.ReadILToken());
                             break;
+                        }
                         default:
                             ilReader.Skip(opcode);
                             break;
                     }
                 }
 
-                if (found == null)
-                {
-                    // Doesn't access any fields. Could be e.g. "Type Foo => typeof(Bar);"
-                    // Return success.
-                    return true;
-                }
-
-                if (found.OwningType != body.OwningMethod.OwningType ||
+                if (found is null ||
+                    found.OwningType != body.OwningMethod.OwningType ||
                     found.IsStatic != body.OwningMethod.Signature.IsStatic ||
                     !found.HasCustomAttribute("System.Runtime.CompilerServices", "CompilerGeneratedAttribute"))
                 {
-                    // A couple heuristics to make sure we got the right field.
-                    // Return failure.
-                    found = null;
-                    return false;
+                    // Heuristics failed - not a backing field
+                    return null;
                 }
 
-                return true;
+                return found;
             }
         }
 

src/tools/illink/src/ILLink.RoslynAnalyzer/IPropertySymbolExtensions.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
 
 namespace ILLink.RoslynAnalyzer
 {
@@ -30,5 +31,37 @@ public static class IPropertySymbolExtensions
             }
             return setMethod;
         }
+
+        public static bool IsAutoProperty(this IPropertySymbol property)
+        {
+            if (property.IsAbstract)
+                return false;
+
+            return (property.GetMethod?.IsAutoAccessor() ?? false) || (property.SetMethod?.IsAutoAccessor() ?? false);
+        }
+
+        private static bool IsAutoAccessor(this IMethodSymbol method)
+        {
+            if (method == null || method.IsAbstract)
+                return false;
+
+            foreach (var decl in method.DeclaringSyntaxReferences)
+            {
+                var syntax = decl.GetSyntax();
+                // Auto property accessors have no body in their syntax
+                switch (syntax)
+                {
+                    case AccessorDeclarationSyntax a:
+                        if (a.Body is not null)
+                            return false;
+                        if (a.ExpressionBody is not null)
+                            return false;
+                        return true;
+                    default:
+                        break;
+                }
+            }
+            return false;
+        }
     }
 }

src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs

@@ -117,6 +117,13 @@ internal static DynamicallyAccessedMemberTypes GetFieldAnnotation(IFieldSymbol f
             if (!field.OriginalDefinition.Type.IsTypeInterestingForDataflow(isByRef: field.RefKind is not RefKind.None))
                 return DynamicallyAccessedMemberTypes.None;
 
+            if (field.AssociatedSymbol is IPropertySymbol property)
+            {
+                // If this is an auto property, we get the property annotation
+                if (property.IsAutoProperty())
+                    return property.GetDynamicallyAccessedMemberTypes();
+            }
+
             return field.GetDynamicallyAccessedMemberTypes();
         }