Better exclude local functions from region analysis (#20004)

By design, when data flow analyzes local functions it does so devoid of
surrounding context and records possibly unassigned variables rather
than reporting a diagnostic. Thus, all captured variables are expected
to be marked unassigned during data flow analysis without reporting a
diagnostic.

However, reporting a diagnostic is not the only side effect of running
data flow analysis. By calling virtual methods data flow analysis
informs derived types of unassigned variables. While the diagnostic is
suppressed and recorded, currently the virtual method is still called.
This has negative consequences for region analysis, which considers
these calls as indications of variables flowing out of the given
region, which may not be the case for local functions.

This PR changes data flow analysis to only call the ReportUnassigned
virtual method if not inside a local function, excluding local functions
from all unassignment reporting.

Fixes #17165, #18347

src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.LocalFunctions.cs

@@ -94,7 +94,7 @@ private void CheckIfAssignedDuringLocalFunctionReplay(Symbol symbol, SyntaxNode
                         // Local functions can "call forward" to after a variable has
                         // been declared but before it has been assigned, so we can never
                         // consider the declaration location when reporting errors.
-                        ReportUnassigned(symbol, node, slot, skipIfUseBeforeDeclaration: false);
+                        ReportUnassignedIfNotCapturedInLocalFunction(symbol, node, slot, skipIfUseBeforeDeclaration: false);
                     }
                 }
             }

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

@@ -903,43 +903,57 @@ protected void CheckAssigned(Symbol symbol, SyntaxNode node)
                     if (slot >= this.State.Assigned.Capacity) Normalize(ref this.State);
                     if (slot > 0 && !this.State.IsAssigned(slot))
                     {
-                        ReportUnassigned(symbol, node);
+                        ReportUnassignedIfNotCapturedInLocalFunction(symbol, node, slot);
                     }
                 }
             }
         }
 
-        /// <param name="symbol">Symbol to variable that is unassigned.</param>
-        /// <param name="node">Syntax where read occurs.</param>
-        /// <param name="slotOpt">Optional slot where variable is located.</param>
-        /// <param name="skipIfUseBeforeDeclaration">
-        /// True if error reporting should consider the location where the
-        /// variable is declared (for instance, eliding errors about reading
-        /// variables that have not yet been declared).
-        /// </param>
-        private void ReportUnassigned(Symbol symbol, SyntaxNode node, int? slotOpt, bool skipIfUseBeforeDeclaration = true)
+        private void ReportUnassignedIfNotCapturedInLocalFunction(Symbol symbol, SyntaxNode node, int slot, bool skipIfUseBeforeDeclaration = true)
         {
-            int slot = slotOpt ?? VariableSlot(symbol);
-            if (slot <= 0) return;
+            // If the symbol is captured by the nearest
+            // local function, record the read and skip the diagnostic
+            if (IsCapturedInLocalFunction(slot))
+            {
+                RecordReadInLocalFunction(slot);
+                return;
+            }
+
+            ReportUnassigned(symbol, node, slot, skipIfUseBeforeDeclaration);
+        }
+
+        /// <summary>
+        /// Report a given variable as not definitely assigned.  Once a variable has been so
+        /// reported, we suppress further reports of that variable.
+        /// </summary>
+        protected virtual void ReportUnassigned(Symbol symbol, SyntaxNode node, int slot, bool skipIfUseBeforeDeclaration)
+        {
+            if (slot <= 0)
+            {
+                return;
+            }
 
             // If this is a constant, constants are always definitely assigned
             // so we should skip reporting. This can happen in a local function
             // where we use a constant before we actually visit its definition
-            // (since local function declarations are visited before other statements).
-            if (symbol.Kind == SymbolKind.Local && ((LocalSymbol)symbol).IsConst)
+            // (since local function declarations are visited before other statements)
+            // e.g.
+            // void M()
+            // {
+            //   L();
+            //   const int x = 0;
+            //   int L() => x;
+            // }
+            if (symbol is LocalSymbol local && local.IsConst)
             {
                 return;
             }
 
-            // If the symbol is captured by the nearest
-            // local function, record the read and skip the diagnostic
-            if (IsCapturedInLocalFunction(slot))
+            if (slot >= _alreadyReported.Capacity)
             {
-                RecordReadInLocalFunction(slot);
-                return;
+                _alreadyReported.EnsureCapacity(nextVariableSlot);
             }
 
-            if (slot >= _alreadyReported.Capacity) _alreadyReported.EnsureCapacity(nextVariableSlot);
             if (skipIfUseBeforeDeclaration &&
                 symbol.Kind == SymbolKind.Local &&
                 (symbol.Locations.Length == 0 || node.Span.End < symbol.Locations[0].SourceSpan.Start))
@@ -992,20 +1006,11 @@ private void ReportUnassigned(Symbol symbol, SyntaxNode node, int? slotOpt, bool
             _alreadyReported[slot] = true;
         }
 
-        /// <summary>
-        /// Report a given variable as not definitely assigned.  Once a variable has been so
-        /// reported, we suppress further reports of that variable.
-        /// </summary>
-        protected virtual void ReportUnassigned(Symbol symbol,
-                                                SyntaxNode node) =>
-            ReportUnassigned(symbol, node, slotOpt: null);
-
         protected virtual void CheckAssigned(BoundExpression expr, FieldSymbol fieldSymbol, SyntaxNode node)
         {
-            int unassignedSlot;
-            if (this.State.Reachable && !IsAssigned(expr, out unassignedSlot))
+            if (this.State.Reachable && !IsAssigned(expr, out int unassignedSlot))
             {
-                ReportUnassigned(fieldSymbol, unassignedSlot, node);
+                ReportUnassignedIfNotCapturedInLocalFunction(fieldSymbol, node, unassignedSlot);
             }
 
             NoteRead(expr);
@@ -1103,13 +1108,6 @@ private bool IsAssigned(BoundExpression node, out int unassignedSlot)
             return this.State.IsAssigned(unassignedSlot);
         }
 
-        /// <summary>
-        /// Report a given variable as not definitely assigned.  Once a variable has been so
-        /// reported, we suppress further reports of that variable.
-        /// </summary>
-        protected virtual void ReportUnassigned(FieldSymbol fieldSymbol, int unassignedSlot, SyntaxNode node) =>
-            ReportUnassigned(fieldSymbol, node, slotOpt: unassignedSlot);
-
         protected Symbol GetNonFieldSymbol(int slot)
         {
             VariableIdentifier variableId = variableBySlot[slot];
@@ -2168,7 +2166,7 @@ public override BoundNode VisitPropertyAccess(BoundPropertyAccess node)
                         int unassignedSlot;
                         if (this.State.Reachable && !IsAssigned(node, out unassignedSlot))
                         {
-                            ReportUnassigned(backingField, unassignedSlot, node.Syntax);
+                            ReportUnassignedIfNotCapturedInLocalFunction(backingField, node.Syntax, unassignedSlot);
                         }
                     }
                 }

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

@@ -91,15 +91,17 @@ public override BoundNode VisitRangeVariable(BoundRangeVariable node)
             return null;
         }
 
-        protected override void ReportUnassigned(Symbol symbol, SyntaxNode node)
+        protected override void ReportUnassigned(Symbol symbol, SyntaxNode node, int slot, bool skipIfUseBeforeDeclaration)
         {
             // TODO: how to handle fields of structs?
-            if (RegionContains(node.Span) && !(symbol is FieldSymbol))
+            if (RegionContains(node.Span))
             {
-                _dataFlowsIn.Add(symbol);
+                // if the field access is reported as unassigned it should mean the original local
+                // or parameter flows in, so we should get the symbol associated with the expression
+                _dataFlowsIn.Add(symbol.Kind == SymbolKind.Field ? GetNonFieldSymbol(slot) : symbol);
             }
 
-            base.ReportUnassigned(symbol, node);
+            base.ReportUnassigned(symbol, node, slot, skipIfUseBeforeDeclaration);
         }
 
         protected override void ReportUnassignedOutParameter(
@@ -114,17 +116,5 @@ protected override void ReportUnassigned(Symbol symbol, SyntaxNode node)
 
             base.ReportUnassignedOutParameter(parameter, node, location);
         }
-
-        protected override void ReportUnassigned(FieldSymbol fieldSymbol, int unassignedSlot, SyntaxNode node)
-        {
-            if (RegionContains(node.Span))
-            {
-                //  if the field access is reported as unassigned it should mean the original local 
-                //  or parameter flows in, so we should get the symbol associated with the expression
-                _dataFlowsIn.Add(GetNonFieldSymbol(unassignedSlot));
-            }
-
-            base.ReportUnassigned(fieldSymbol, unassignedSlot, node);
-        }
     }
 }

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

@@ -232,13 +232,16 @@ public override BoundNode VisitQueryClause(BoundQueryClause node)
             return base.VisitQueryClause(node);
         }
 
-        protected override void ReportUnassigned(Symbol symbol, SyntaxNode node)
+        protected override void ReportUnassigned(Symbol symbol, SyntaxNode node, int slot, bool skipIfUseBeforeDeclaration)
         {
-            if (!_dataFlowsOut.Contains(symbol) && !(symbol is FieldSymbol) && !IsInside)
+            if (!IsInside)
             {
-                _dataFlowsOut.Add(symbol);
+                // If the field access is reported as unassigned it should mean the original local
+                // or parameter flows out, so we should get the symbol associated with the expression
+                _dataFlowsOut.Add(symbol.Kind == SymbolKind.Field ? GetNonFieldSymbol(slot) : symbol);
             }
-            base.ReportUnassigned(symbol, node);
+
+            base.ReportUnassigned(symbol, node, slot, skipIfUseBeforeDeclaration);
         }
 
         protected override void ReportUnassignedOutParameter(ParameterSymbol parameter, SyntaxNode node, Location location)
@@ -249,20 +252,5 @@ protected override void ReportUnassignedOutParameter(ParameterSymbol parameter,
             }
             base.ReportUnassignedOutParameter(parameter, node, location);
         }
-
-        protected override void ReportUnassigned(FieldSymbol fieldSymbol, int unassignedSlot, SyntaxNode node)
-        {
-            if (!IsInside)
-            {
-                //  if the field access is reported as unassigned it should mean the original local 
-                //  or parameter flows out, so we should get the symbol associated with the expression
-                var symbol = GetNonFieldSymbol(unassignedSlot);
-                if (!_dataFlowsOut.Contains(symbol))
-                {
-                    _dataFlowsOut.Add(symbol);
-                }
-            }
-            base.ReportUnassigned(fieldSymbol, unassignedSlot, node);
-        }
     }
 }

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

@@ -48,15 +48,7 @@ private new HashSet<PrefixUnaryExpressionSyntax> Analyze(ref bool badRegion)
             return _result;
         }
 
-        protected override void ReportUnassigned(Symbol symbol, SyntaxNode node)
-        {
-            if (node.Parent.Kind() == SyntaxKind.AddressOfExpression)
-            {
-                _result.Add((PrefixUnaryExpressionSyntax)node.Parent);
-            }
-        }
-
-        protected override void ReportUnassigned(FieldSymbol fieldSymbol, int unassignedSlot, SyntaxNode node)
+        protected override void ReportUnassigned(Symbol symbol, SyntaxNode node, int slot, bool skipIfUseBeforeDeclaration)
         {
             if (node.Parent.Kind() == SyntaxKind.AddressOfExpression)
             {

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

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
 using System.Collections.Generic;
+using System.Diagnostics;
 using Microsoft.CodeAnalysis.CSharp.Symbols;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Text;
@@ -48,26 +49,24 @@ private new HashSet<Symbol> Analyze(ref bool badRegion)
             return _result;
         }
 
-        protected override void ReportUnassigned(Symbol symbol, SyntaxNode node)
+        protected override void ReportUnassigned(Symbol symbol, SyntaxNode node, int slot, bool skipIfUseBeforeDeclaration)
         {
             // TODO: how to handle fields of structs?
             if (symbol.Kind != SymbolKind.Field)
             {
                 _result.Add(symbol);
             }
+            else
+            {
+                _result.Add(GetNonFieldSymbol(slot));
+                base.ReportUnassigned(symbol, node, slot, skipIfUseBeforeDeclaration);
+            }
         }
 
         protected override void ReportUnassignedOutParameter(ParameterSymbol parameter, SyntaxNode node, Location location)
         {
             _result.Add(parameter);
             base.ReportUnassignedOutParameter(parameter, node, location);
         }
-
-        protected override void ReportUnassigned(FieldSymbol fieldSymbol, int unassignedSlot, SyntaxNode node)
-        {
-            Symbol variable = GetNonFieldSymbol(unassignedSlot);
-            if ((object)variable != null) _result.Add(variable);
-            base.ReportUnassigned(fieldSymbol, unassignedSlot, node);
-        }
     }
 }

src/Compilers/CSharp/Portable/Lowering/StateMachineRewriter/IteratorAndAsyncCaptureWalker.cs

@@ -224,11 +224,18 @@ protected override void EnterParameter(ParameterSymbol parameter)
             GetOrCreateSlot(parameter);
         }
 
-        protected override void ReportUnassigned(Symbol symbol, SyntaxNode node)
+        protected override void ReportUnassigned(Symbol symbol, SyntaxNode node, int slot, bool skipIfUseBeforeDeclaration)
         {
-            if (symbol is LocalSymbol || symbol is ParameterSymbol)
+            switch (symbol.Kind)
             {
-                CaptureVariable(symbol, node);
+                case SymbolKind.Field:
+                    symbol = GetNonFieldSymbol(slot);
+                    goto case SymbolKind.Local;
+
+                case SymbolKind.Local:
+                case SymbolKind.Parameter:
+                    CaptureVariable(symbol, node);
+                    break;
             }
         }
 
@@ -239,11 +246,6 @@ protected override LocalState UnreachableState()
             return this.State;
         }
 
-        protected override void ReportUnassigned(FieldSymbol fieldSymbol, int unassignedSlot, SyntaxNode node)
-        {
-            CaptureVariable(GetNonFieldSymbol(unassignedSlot), node);
-        }
-
         protected override void VisitLvalueParameter(BoundParameter node)
         {
             TryHoistTopLevelParameter(node);