Refactor out LambdaRewriter.Analysis visitors

Duplicate functionality of existing LambdaRewriter.Analysis visitors
with the new Scope tree visitors and assert that the same results are
produced.

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs

@@ -7,7 +7,6 @@
 using Microsoft.CodeAnalysis.CSharp.Symbols;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Roslyn.Utilities;
-using System.Linq;
 
 namespace Microsoft.CodeAnalysis.CSharp
 {
@@ -40,7 +39,13 @@ public sealed class Scope
                 /// in this scope or nested scopes. "Declared" refers to the start of the variable
                 /// lifetime (which, at this point in lowering, should be equivalent to lexical scope).
                 /// </summary>
-                public ArrayBuilder<Symbol> DeclaredVariables { get; } = ArrayBuilder<Symbol>.GetInstance();
+                /// <remarks>
+                /// It's important that this is a set and that enumeration order is deterministic. We loop
+                /// over this list to generate proxies and if we loop out of order this will cause
+                /// non-deterministic compilation, and if we generate duplicate proxies we'll generate
+                /// wasteful code in the best case and incorrect code in the worst.
+                /// </remarks>
+                public SetWithInsertionOrder<Symbol> DeclaredVariables { get; } = new SetWithInsertionOrder<Symbol>();
 
                 /// <summary>
                 /// The bound node representing this scope. This roughly corresponds to the bound
@@ -54,7 +59,7 @@ public sealed class Scope
                 /// The closure that this scope is nested inside. Null if this scope is not nested
                 /// inside a closure.
                 /// </summary>
-                public Closure ContainingClosure { get; }
+                public Closure ContainingClosureOpt { get; }
 
                 public Scope(Scope parent, BoundNode boundNode, Closure containingClosure)
                 {
@@ -62,7 +67,7 @@ public Scope(Scope parent, BoundNode boundNode, Closure containingClosure)
 
                     Parent = parent;
                     BoundNode = boundNode;
-                    ContainingClosure = containingClosure;
+                    ContainingClosureOpt = containingClosure;
                 }
 
                 public void Free()
@@ -78,7 +83,6 @@ public void Free()
                         closure.Free();
                     }
                     Closures.Free();
-                    DeclaredVariables.Free();
                 }
 
                 public override string ToString() => BoundNode.Syntax.GetText().ToString();
@@ -118,7 +122,7 @@ public void Free()
             /// Optimizes local functions such that if a local function only references other local functions
             /// that capture no variables, we don't need to create capture environments for any of them.
             /// </summary>
-            private void RemoveUnneededReferences()
+            private void RemoveUnneededReferences(ParameterSymbol thisParam)
             {
                 var methodGraph = new MultiDictionary<MethodSymbol, MethodSymbol>();
                 var capturesThis = new HashSet<MethodSymbol>();
@@ -132,7 +136,7 @@ private void RemoveUnneededReferences()
                         {
                             methodGraph.Add(localFunc, closure.OriginalMethodSymbol);
                         }
-                        else if (capture == _topLevelMethod.ThisParameter)
+                        else if (capture == thisParam)
                         {
                             if (capturesThis.Add(closure.OriginalMethodSymbol))
                             {
@@ -168,7 +172,7 @@ private void RemoveUnneededReferences()
                     }
                     if (capturesThis.Contains(closure.OriginalMethodSymbol))
                     {
-                        closure.CapturedVariables.Add(_topLevelMethod.ThisParameter);
+                        closure.CapturedVariables.Add(thisParam);
                     }
                 });
             }
@@ -191,7 +195,7 @@ public static void VisitClosures(Scope scope, Action<Scope, Closure> action)
 
             /// <summary>
             /// Builds a tree of <see cref="Scope"/> nodes corresponding to a given method.
-            /// <see cref="Build(BoundNode, Analysis)"/> visits the bound tree and translates
+            /// <see cref="Build(BoundNode, MethodSymbol, HashSet{MethodSymbol})"/> visits the bound tree and translates
             /// information from the bound tree about variable scope, declared variables, and
             /// variable captures into the resulting <see cref="Scope"/> tree.
             /// </summary>
@@ -211,32 +215,53 @@ private class ScopeTreeBuilder : BoundTreeWalkerWithStackGuardWithoutRecursionOn
                 /// </summary>
                 private readonly SmallDictionary<Symbol, Scope> _localToScope = new SmallDictionary<Symbol, Scope>();
 
-                private readonly Analysis _analysis;
+                private readonly MethodSymbol _topLevelMethod;
 
-                private ScopeTreeBuilder(Scope rootScope, Analysis analysis)
+                private bool _sawClosure = false;
+
+                /// <summary>
+                /// If a local function is in the set, at some point in the code it is converted
+                /// to a delegate and should then not be optimized to a struct closure.
+                /// Also contains all lambdas (as they are converted to delegates implicitly).
+                /// </summary>
+                private readonly HashSet<MethodSymbol> _methodsConvertedToDelegates;
+
+                private ScopeTreeBuilder(
+                    Scope rootScope,
+                    MethodSymbol topLevelMethod,
+                    HashSet<MethodSymbol> methodsConvertedToDelegates)
                 {
                     Debug.Assert(rootScope != null);
-                    Debug.Assert(analysis != null);
+                    Debug.Assert(topLevelMethod != null);
+                    Debug.Assert(methodsConvertedToDelegates != null);
 
                     _currentScope = rootScope;
-                    _analysis = analysis;
+                    _topLevelMethod = topLevelMethod;
+                    _methodsConvertedToDelegates = methodsConvertedToDelegates;
                 }
 
-                public static Scope Build(BoundNode node, Analysis analysis)
+                public static Scope Build(
+                    BoundNode node,
+                    MethodSymbol topLevelMethod,
+                    HashSet<MethodSymbol> methodsConvertedToDelegates)
                 {
                     // This should be the top-level node
                     Debug.Assert(node == FindNodeToAnalyze(node));
+                    Debug.Assert(topLevelMethod != null);
 
                     var rootScope = new Scope(parent: null, boundNode: node, containingClosure: null);
-                    var builder = new ScopeTreeBuilder(rootScope, analysis);
+                    var builder = new ScopeTreeBuilder(
+                        rootScope,
+                        topLevelMethod,
+                        methodsConvertedToDelegates);
                     builder.Build();
-                    return rootScope;
+                    return builder._sawClosure ? rootScope : null;
                 }
 
                 private void Build()
                 {
                     // Set up the current method locals
-                    DeclareLocals(_currentScope, _analysis._topLevelMethod.Parameters);
+                    DeclareLocals(_currentScope, _topLevelMethod.Parameters);
                     Visit(_currentScope.BoundNode);
                 }
 
@@ -260,6 +285,11 @@ public override BoundNode VisitBlock(BoundBlock node)
 
                 public override BoundNode VisitCatchBlock(BoundCatchBlock node)
                 {
+                    if (node.Locals.IsDefaultOrEmpty)
+                    {
+                        return base.VisitCatchBlock(node);
+                    }
+
                     var oldScope = _currentScope;
                     _currentScope = CreateOrReuseScope(node, node.Locals);
                     var result = base.VisitCatchBlock(node);
@@ -269,6 +299,11 @@ public override BoundNode VisitCatchBlock(BoundCatchBlock node)
 
                 public override BoundNode VisitSequence(BoundSequence node)
                 {
+                    if (node.Locals.IsDefaultOrEmpty)
+                    {
+                        return base.VisitSequence(node);
+                    }
+
                     var oldScope = _currentScope;
                     _currentScope = CreateOrReuseScope(node, node.Locals);
                     var result = base.VisitSequence(node);
@@ -296,7 +331,7 @@ public override BoundNode VisitLambda(BoundLambda node)
                     var oldInExpressionTree = _inExpressionTree;
                     _inExpressionTree |= node.Type.IsExpressionTree();
 
-                    _analysis.MethodsConvertedToDelegates.Add(node.Symbol.OriginalDefinition);
+                    _methodsConvertedToDelegates.Add(node.Symbol.OriginalDefinition);
                     var result = VisitClosure(node.Symbol, node.Body);
 
                     _inExpressionTree = oldInExpressionTree;
@@ -323,7 +358,7 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE
                         // Use OriginalDefinition to strip generic type parameters
                         var method = node.MethodOpt.OriginalDefinition;
                         AddIfCaptured(method);
-                        _analysis.MethodsConvertedToDelegates.Add(method);
+                        _methodsConvertedToDelegates.Add(method);
                     }
                     return base.VisitDelegateCreationExpression(node);
                 }
@@ -342,13 +377,13 @@ public override BoundNode VisitLocal(BoundLocal node)
 
                 public override BoundNode VisitBaseReference(BoundBaseReference node)
                 {
-                    AddIfCaptured(_analysis._topLevelMethod.ThisParameter);
+                    AddIfCaptured(_topLevelMethod.ThisParameter);
                     return base.VisitBaseReference(node);
                 }
 
                 public override BoundNode VisitThisReference(BoundThisReference node)
                 {
-                    var thisParam = _analysis._topLevelMethod.ThisParameter;
+                    var thisParam = _topLevelMethod.ThisParameter;
                     if (thisParam != null)
                     {
                         AddIfCaptured(thisParam);
@@ -373,6 +408,8 @@ private BoundNode VisitClosure(MethodSymbol closureSymbol, BoundBlock body)
                 {
                     Debug.Assert((object)closureSymbol != null);
 
+                    _sawClosure |= true;
+
                     // Closure is declared (lives) in the parent scope, but its
                     // variables are in a nested scope
                     var closure = new Closure(closureSymbol);
@@ -426,11 +463,11 @@ private void AddIfCaptured(Symbol symbol)
                             closure.CapturedVariables.Add(symbol);
 
                             // Also mark captured in enclosing scopes
-                            while (scope.ContainingClosure == closure)
+                            while (scope.ContainingClosureOpt == closure)
                             {
                                 scope = scope.Parent;
                             }
-                            closure = scope.ContainingClosure;
+                            closure = scope.ContainingClosureOpt;
                         }
 
                         // Also record where the captured variable lives

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs

@@ -28,11 +28,6 @@ internal sealed partial class Analysis : BoundTreeWalkerWithStackGuardWithoutRec
             /// </summary>
             private bool _inExpressionLambda;
 
-            /// <summary>
-            /// Set to true of any lambda expressions were seen in the analyzed method body.
-            /// </summary>
-            public bool SeenLambda { get; private set; }
-
             /// <summary>
             /// For each scope that defines variables, identifies the nearest enclosing scope that defines variables.
             /// </summary>
@@ -116,12 +111,15 @@ public static Analysis Analyze(BoundNode node, MethodSymbol method)
             {
                 var analysis = new Analysis(method);
                 analysis.Analyze(node);
-                return analysis;
+                return analysis.ScopeTree == null ? null : analysis;
             }
 
             private void Analyze(BoundNode node)
             {
-                ScopeTree = ScopeTreeBuilder.Build(node, this);
+                ScopeTree = ScopeTreeBuilder.Build(
+                    node,
+                    _topLevelMethod,
+                    MethodsConvertedToDelegates);
                 _currentScope = FindNodeToAnalyze(node);
 
                 Debug.Assert(!_inExpressionLambda);
@@ -181,9 +179,9 @@ private static BoundNode FindNodeToAnalyze(BoundNode node)
             /// <summary>
             /// Create the optimized plan for the location of lambda methods and whether scopes need access to parent scopes
             ///  </summary>
-            internal void ComputeLambdaScopesAndFrameCaptures()
+            internal void ComputeLambdaScopesAndFrameCaptures(ParameterSymbol thisParam)
             {
-                RemoveUnneededReferences();
+                RemoveUnneededReferences(thisParam);
 
                 LambdaScopes = new Dictionary<MethodSymbol, BoundNode>(ReferenceEqualityComparer.Instance);
                 NeedsParentFrame = new HashSet<BoundNode>();
@@ -284,7 +282,76 @@ void RecordClosureScope(Scope innermost, Scope outermost, Closure closure)
                             }
                         }
                     }
+                }
+            }
+
+            /// <summary>
+            /// Walk up the scope tree looking for a variable declaration.
+            /// </summary>
+            public static Scope GetVariableDeclarationScope(Scope startingScope, Symbol variable)
+            {
+                if (variable is ParameterSymbol p && p.IsThis)
+                {
+                    return null;
+                }
+
+                var currentScope = startingScope;
+                while (currentScope != null)
+                {
+                    if (variable.Kind == SymbolKind.Parameter || variable.Kind == SymbolKind.Local)
+                    {
+                        if (currentScope.DeclaredVariables.Contains(variable))
+                        {
+                            return currentScope;
+                        }
+                    }
+                    else
+                    {
+                        if (currentScope.Closures.Contains(c => variable == c.OriginalMethodSymbol))
+                        {
+                            return currentScope;
+                        }
+                    }
+                    currentScope = currentScope.Parent;
+                }
+                return null;
+            }
+
+            /// <summary>
+            /// Find the parent <see cref="Scope"/> of the <see cref="Scope"/> corresponding to
+            /// the given <see cref="BoundNode"/>.
+            /// </summary>
+            public static Scope GetScopeParent(Scope treeRoot, BoundNode scopeNode)
+            {
+                var correspondingScope = GetScopeWithMatchingBoundNode(treeRoot, scopeNode);
+                return correspondingScope.Parent;
+            }
+
+            /// <summary>
+            /// Finds a <see cref="Scope" /> with a matching <see cref="BoundNode"/>
+            /// as the one given.
+            /// </summary>
+            public static Scope GetScopeWithMatchingBoundNode(Scope treeRoot, BoundNode node)
+            {
+                return Helper(treeRoot) ?? throw ExceptionUtilities.Unreachable;
+
+                Scope Helper(Scope currentScope)
+                {
+                    if (currentScope.BoundNode == node)
+                    {
+                        return currentScope;
+                    }
 
+                    foreach (var nestedScope in currentScope.NestedScopes)
+                    {
+                        var found = Helper(nestedScope);
+                        if (found != null)
+                        {
+                            return found;
+                        }
+                    }
+
+                    return null;
                 }
             }
 
@@ -488,7 +555,6 @@ public override BoundNode VisitLocalFunctionStatement(BoundLocalFunctionStatemen
             private BoundNode VisitLambdaOrFunction(IBoundLambdaOrFunction node)
             {
                 Debug.Assert((object)node.Symbol != null);
-                SeenLambda = true;
                 var oldParent = _currentParent;
                 var oldBlock = _currentScope;
                 _currentParent = node.Symbol;

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs

@@ -228,13 +228,8 @@ protected override bool NeedsProxy(Symbol localOrParameter)
             Debug.Assert(compilationState.ModuleBuilderOpt != null);
 
             var analysis = Analysis.Analyze(loweredBody, method);
-            if (!analysis.SeenLambda)
+            if (analysis == null)
             {
-                // Unreachable anonymous functions are ignored by the analyzer.
-                // No closures or lambda methods are generated.
-                // E.g. 
-                //   int y = 0;
-                //   var b = false && (from z in new X(y) select f(z + y))
                 return loweredBody;
             }
 
@@ -252,7 +247,7 @@ protected override bool NeedsProxy(Symbol localOrParameter)
                 diagnostics,
                 assignLocals);
 
-            analysis.ComputeLambdaScopesAndFrameCaptures();
+            analysis.ComputeLambdaScopesAndFrameCaptures(thisParameter);
             rewriter.MakeFrames(closureDebugInfoBuilder);
 
             // First, lower everything but references (calls, delegate conversions)
@@ -341,8 +336,14 @@ private void MakeFrames(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
 
                 foreach (var captured in capturedVars)
                 {
-                    if (!_analysis.VariableScope.TryGetValue(captured, out BoundNode captureScope))
+                    var declarationScope = Analysis.GetVariableDeclarationScope(scope, captured);
+                    if (_analysis.VariableScope.TryGetValue(captured, out BoundNode captureScope))
                     {
+                        Debug.Assert(declarationScope.BoundNode == captureScope);
+                    }
+                    else
+                    {
+                        Debug.Assert(declarationScope == null);
                         continue;
                     }
 
@@ -357,7 +358,7 @@ private void MakeFrames(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
                         continue;
                     }
 
-                    LambdaFrame frame = GetFrameForScope(captureScope, closureDebugInfo);
+                    LambdaFrame frame = GetFrameForScope(declarationScope, closureDebugInfo);
 
                     if (captured.Kind != SymbolKind.Method && !proxies.ContainsKey(captured))
                     {
@@ -450,26 +451,28 @@ private void MakeFrames(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
             return result;
         }
 
-        private LambdaFrame GetFrameForScope(BoundNode scope, ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
+        private LambdaFrame GetFrameForScope(Analysis.Scope scope, ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
         {
+            var scopeBoundNode = scope.BoundNode;
             LambdaFrame frame;
-            if (!_frames.TryGetValue(scope, out frame))
+            if (!_frames.TryGetValue(scopeBoundNode, out frame))
             {
-                var syntax = scope.Syntax;
+                var syntax = scopeBoundNode.Syntax;
                 Debug.Assert(syntax != null);
 
                 DebugId methodId = GetTopLevelMethodId();
                 DebugId closureId = GetClosureId(syntax, closureDebugInfo);
 
-                var canBeStruct = !_analysis.ScopesThatCantBeStructs.Contains(scope);
+                var canBeStruct = !_analysis.ScopesThatCantBeStructs.Contains(scopeBoundNode);
 
-                var containingMethod = _analysis.ScopeOwner[scope];
+                var containingMethod = scope.ContainingClosureOpt?.OriginalMethodSymbol ?? _topLevelMethod;
+                Debug.Assert(containingMethod == _analysis.ScopeOwner[scopeBoundNode]);
                 if (_substitutedSourceMethod != null && containingMethod == _topLevelMethod)
                 {
                     containingMethod = _substitutedSourceMethod;
                 }
                 frame = new LambdaFrame(_topLevelMethod, containingMethod, canBeStruct, syntax, methodId, closureId);
-                _frames.Add(scope, frame);
+                _frames.Add(scopeBoundNode, frame);
 
                 CompilationState.ModuleBuilderOpt.AddSynthesizedDefinition(ContainingType, frame);
                 if (frame.Constructor != null)
@@ -701,14 +704,20 @@ private BoundNode IntroduceFrame(BoundNode node, LambdaFrame frame, Func<ArrayBu
             // Capture any parameters of this block.  This would typically occur
             // at the top level of a method or lambda with captured parameters.
             // TODO: speed up the following by computing it in analysis.
+            var scope = Analysis.GetScopeWithMatchingBoundNode(_analysis.ScopeTree, node);
+            var variablesInThisScope = ArrayBuilder<Symbol>.GetInstance();
             foreach (var variable in _analysis.CapturedVariables.Keys)
             {
-                BoundNode varNode;
-                if (!_analysis.VariableScope.TryGetValue(variable, out varNode) || varNode != node)
+                if (variable.Kind != SymbolKind.Method &&
+                    _analysis.VariableScope.TryGetValue(variable, out var varNode) &&
+                    varNode == node)
                 {
-                    continue;
+                    variablesInThisScope.Add(variable);
                 }
-
+            }
+            Debug.Assert(variablesInThisScope.SequenceEqual(scope.DeclaredVariables));
+            foreach (var variable in variablesInThisScope)
+            {
                 InitVariableProxy(syntax, variable, framePointer, prologue);
             }
 
@@ -1360,7 +1369,8 @@ private DebugId GetLambdaId(SyntaxNode syntax, ClosureKind closureKind, int clos
                 // strictly need `this`.
                 if (_analysis.CanTakeRefParameters(node.Symbol))
                 {
-                    lambdaScope = _analysis.ScopeParent[node.Body];
+                    lambdaScope = Analysis.GetScopeParent(_analysis.ScopeTree, node.Body)?.BoundNode;
+                    Debug.Assert(lambdaScope == _analysis.ScopeParent[node.Body]);
                     _ = _frames.TryGetValue(lambdaScope, out containerAsFrame);
                     structClosures = GetStructClosures(containerAsFrame, lambdaScope);
                 }
@@ -1468,12 +1478,18 @@ private ImmutableArray<TypeSymbol> GetStructClosures(LambdaFrame containerAsFram
 
             bool FindParentFrame(ref LambdaFrame container, ref BoundNode scope)
             {
-                while (_analysis.ScopeParent.TryGetValue(scope, out scope))
+                var temp = Analysis.GetScopeParent(_analysis.ScopeTree, scope)?.BoundNode;
+                Debug.Assert(temp == null || _analysis.ScopeParent.TryGetValue(scope, out var tempScope) && tempScope == temp);
+                scope = temp;
+                while (scope != null)
                 {
                     if (_frames.TryGetValue(scope, out container))
                     {
                         return true;
                     }
+                    temp = Analysis.GetScopeParent(_analysis.ScopeTree, scope)?.BoundNode;
+                    Debug.Assert(temp == null || _analysis.ScopeParent.TryGetValue(scope, out tempScope) && tempScope == temp);
+                    scope = temp;
                 }
                 return false;
             }
@@ -1552,8 +1568,9 @@ private BoundNode RewriteLambdaConversion(BoundLambda node)
 
             // NOTE: We require "lambdaScope != null". 
             //       We do not want to introduce a field into an actual user's class (not a synthetic frame).
+            Debug.Assert(Analysis.GetScopeParent(_analysis.ScopeTree, node.Body).BoundNode == _analysis.ScopeParent[node.Body]);
             var shouldCacheInLoop = lambdaScope != null &&
-                lambdaScope != _analysis.ScopeParent[node.Body] &&
+                lambdaScope != Analysis.GetScopeParent(_analysis.ScopeTree, node.Body).BoundNode &&
                 InLoopOrLambda(node.Syntax, lambdaScope.Syntax);
 
             if (shouldCacheForStaticMethod || shouldCacheInLoop)