Cleanup; more tests

dotnet · Sep 18, 2024 · f036e7c · f036e7c
1 parent eb352eb
commit f036e7c
Show file tree

Hide file tree

Showing 5 changed files with 614 additions and 69 deletions.
diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
@@ -1749,12 +1749,11 @@ private DiagnosticInfo GetBadEventUsageDiagnosticInfo(EventSymbol eventSymbol)
                 new CSDiagnosticInfo(ErrorCode.ERR_BadEventUsageNoField, leastOverridden);
         }
 
-        internal static bool AccessingAutoPropertyFromConstructor(BoundPropertyAccess propertyAccess, Symbol fromMember, bool useAsLvalue = false)
+        internal static bool AccessingAutoPropertyFromConstructor(BoundPropertyAccess propertyAccess, Symbol fromMember, bool useAsLvalue)
         {
             return AccessingAutoPropertyFromConstructor(propertyAccess.ReceiverOpt, propertyAccess.PropertySymbol, fromMember, useAsLvalue);
         }
 
-        // PROTOTYPE: Is useAsLvalue set correctly in all callers?
         private static bool AccessingAutoPropertyFromConstructor(BoundExpression receiver, PropertySymbol propertySymbol, Symbol fromMember, bool useAsLvalue)
         {
             if (!propertySymbol.IsDefinition && propertySymbol.ContainingType.Equals(propertySymbol.ContainingType.OriginalDefinition, TypeCompareKind.IgnoreNullableModifiersForReferenceTypes))

diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
@@ -2026,7 +2026,6 @@ protected virtual void PropertySetter(BoundExpression node, BoundExpression rece
             VisitReceiverAfterCall(receiver, setter);
         }
 
-        // PROTOTYPE: Test all uses of this method.
         // returns false if expression is not a property access
         // or if the property has a backing field
         // and accessed in a corresponding constructor
@@ -2173,7 +2172,8 @@ public override BoundNode VisitPropertyAccess(BoundPropertyAccess node)
         {
             var property = node.PropertySymbol;
 
-            if (Binder.AccessingAutoPropertyFromConstructor(node, _symbol))
+            // PROTOTYPE: Is VisitPropertyAccess only called when the property access is an r-value?
+            if (Binder.AccessingAutoPropertyFromConstructor(node, _symbol, useAsLvalue: false))
             {
                 var backingField = (property as SourcePropertySymbolBase)?.BackingField;
                 if (backingField != null)

diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs
@@ -1425,7 +1425,7 @@ private bool IsAssigned(BoundExpression node, out int unassignedSlot)
                 case BoundKind.PropertyAccess:
                     {
                         var propertyAccess = (BoundPropertyAccess)node;
-                        if (Binder.AccessingAutoPropertyFromConstructor(propertyAccess, this.CurrentSymbol))
+                        if (Binder.AccessingAutoPropertyFromConstructor(propertyAccess, this.CurrentSymbol, useAsLvalue: false)) // PROTOTYPE: Is useAsLvalue: false correct?
                         {
                             var property = propertyAccess.PropertySymbol;
                             var backingField = (property as SourcePropertySymbolBase)?.BackingField;
@@ -2676,7 +2676,8 @@ public override BoundNode VisitFieldAccess(BoundFieldAccess node)
         public override BoundNode VisitPropertyAccess(BoundPropertyAccess node)
         {
             var result = base.VisitPropertyAccess(node);
-            if (Binder.AccessingAutoPropertyFromConstructor(node, this.CurrentSymbol))
+            // PROTOTYPE: Is VisitPropertyAccess only called when the property access is an r-value?
+            if (Binder.AccessingAutoPropertyFromConstructor(node, this.CurrentSymbol, useAsLvalue: false))
             {
                 var property = node.PropertySymbol;
                 var backingField = (property as SourcePropertySymbolBase)?.BackingField;

diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
@@ -661,7 +661,7 @@ void enforceMemberNotNull(SyntaxNode? syntaxOpt, LocalState state)
                             // If this constructor has `SetsRequiredMembers`, then we need to check the state of _all_ required properties, regardless of whether they are auto-properties or not.
                             // For auto-properties, `GetMembersUnordered()` will return the backing field, and `checkStateOnConstructorExit` will follow that to the property itself, so we only need
                             // to force property analysis if the member is required and _does not_ have a backing field.
-                            // PROTOTYPE: This check for BackingField seems too generous. Shouldn't we also check the appropriate accessors are missing or auto-implemented?
+                            // PROTOTYPE: Is checking BackingField sufficient or should we check the appropriate accessors are missing or auto-implemented?
                             var shouldForcePropertyAnalysis = !constructorEnforcesRequiredMembers && member is not SourcePropertySymbolBase { BackingField: not null } && member.IsRequired();
                             checkMemberStateOnConstructorExit(method, member, state, thisSlot, exitLocation, membersWithStateEnforcedByRequiredMembers, forcePropertyAnalysis: shouldForcePropertyAnalysis);
                         }
@@ -1074,11 +1074,9 @@ static IEnumerable<Symbol> getAllMembersToBeDefaulted(Symbol requiredMember)
                             }
                         }
 
-                        // PROTOTYPE: Why are we returning the auto-property backing field rather than using the property?
-                        // VisitMemberAccess does the opposite. What are the implications of the inconsistency?
-                        // PROTOTYPE: This check for BackingField seems too generous. Shouldn't we also check the appropriate accessors are missing or auto-implemented?
+                        // PROTOTYPE: Is checking IsAutoProperty sufficient or should we check the appropriate accessors are missing or auto-implemented?
                         static Symbol getFieldSymbolToBeInitialized(Symbol requiredMember)
-                            => requiredMember is SourcePropertySymbol { IsAutoProperty: true } prop ? prop.BackingField : requiredMember; // PROTOTYPE: This is the only use of IsAutoProperty.
+                            => requiredMember is SourcePropertySymbol { IsAutoProperty: true } prop ? prop.BackingField : requiredMember;
                     }
                 }
             }