Remove RemoveUnneededReferences from LamdaRewriter (#21367)

Currently, the lambda rewriter has an early optimization pass in
analysis that tries to find all local functions that only capture 'this'
and remove references to local functions that do the same. There are two
problems with this approach:

    1) Generally, removing information from the tree is a bad idea
    because it hurts further analysis passes that may have needed that
    information.

    2) The optimization strategy itself is very tricky and has a number
    of complex corner cases. This has lead to bugs, for example #19033.

This PR deletes the current method and adds a new optimization routine
at the end of the analysis, operating on assigned scopes and
environments rather than removing captured variable analysis. The new
optimization is as follows: if we end up with an environment containing
only 'this', the environment can be removed, all containing methods can
be moved to the top-level type, and all environments which capture the
'this' environment can instead directly capture the 'this' parameter.
This produces almost the same results as the previous optimization, but
is easier to validate as an algebraic equivalence.

The baseline changes come from the new optimization being less aggressive 
about moving functions which only capture 'this' to the top level. This
appears to be a wash -- some codegen gets slightly better, some gets
slightly worse.

Fixes #19033
Fixes #20577

src/Compilers/CSharp/Portable/Compiler/TypeCompilationState.cs

@@ -66,7 +66,7 @@ internal MethodWithBody(MethodSymbol method, BoundStatement body, ImportChain im
 
         public readonly CSharpCompilation Compilation;
 
-        public ClosureEnvironment StaticLambdaFrame;
+        public SynthesizedClosureEnvironment StaticLambdaFrame;
 
         /// <summary>
         /// A graph of method->method references for this(...) constructor initializers.

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

@@ -31,7 +31,7 @@ private LambdaCapturedVariable(SynthesizedContainer frame, TypeSymbol type, stri
             _isThis = isThisParameter;
         }
 
-        public static LambdaCapturedVariable Create(ClosureEnvironment frame, Symbol captured, ref int uniqueId)
+        public static LambdaCapturedVariable Create(SynthesizedClosureEnvironment frame, Symbol captured, ref int uniqueId)
         {
             Debug.Assert(captured is LocalSymbol || captured is ParameterSymbol);
 
@@ -87,7 +87,7 @@ private static TypeSymbol GetCapturedVariableFieldType(SynthesizedContainer fram
             if ((object)local != null)
             {
                 // if we're capturing a generic frame pointer, construct it with the new frame's type parameters
-                var lambdaFrame = local.Type.OriginalDefinition as ClosureEnvironment;
+                var lambdaFrame = local.Type.OriginalDefinition as SynthesizedClosureEnvironment;
                 if ((object)lambdaFrame != null)
                 {
                     // lambdaFrame may have less generic type parameters than frame, so trim them down (the first N will always match)

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

@@ -119,6 +119,23 @@ public sealed class Closure
 
                 public ClosureEnvironment ContainingEnvironmentOpt;
 
+                private bool _capturesThis;
+
+                /// <summary>
+                /// True if this closure directly or transitively captures 'this' (captures
+                /// a local function which directly or indirectly captures 'this').
+                /// Calculated in <see cref="MakeAndAssignEnvironments"/>.
+                /// </summary>
+                public bool CapturesThis
+                {
+                    get => _capturesThis;
+                    set
+                    {
+                        Debug.Assert(value);
+                        _capturesThis = value;
+                    }
+                }
+
                 public Closure(MethodSymbol symbol)
                 {
                     Debug.Assert(symbol != null);
@@ -132,100 +149,73 @@ public void Free()
                 }
             }
 
+            public sealed class ClosureEnvironment
+            {
+                public readonly SetWithInsertionOrder<Symbol> CapturedVariables;
+                
                 /// <summary>
-            /// 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.
+                /// Represents a <see cref="SynthesizedEnvironment"/> that had its environment
+                /// pointer (a local pointing to the environment) captured like a captured
+                /// variable. Assigned in
+                /// <see cref="ComputeLambdaScopesAndFrameCaptures(ParameterSymbol)"/>
                 /// </summary>
-            private void RemoveUnneededReferences(ParameterSymbol thisParam)
-            {
-                var methodGraph = new MultiDictionary<MethodSymbol, MethodSymbol>();
-                var capturesThis = new HashSet<MethodSymbol>();
-                var capturesVariable = new HashSet<MethodSymbol>();
-                var visitStack = new Stack<MethodSymbol>();
-                VisitClosures(ScopeTree, (scope, closure) =>
-                {
-                    foreach (var capture in closure.CapturedVariables)
-                    {
-                        if (capture is MethodSymbol localFunc)
-                        {
-                            methodGraph.Add(localFunc, closure.OriginalMethodSymbol);
-                        }
-                        else if (capture == thisParam)
-                        {
-                            if (capturesThis.Add(closure.OriginalMethodSymbol))
-                            {
-                                visitStack.Push(closure.OriginalMethodSymbol);
-                            }
-                        }
-                        else if (capturesVariable.Add(closure.OriginalMethodSymbol) &&
-                                 !capturesThis.Contains(closure.OriginalMethodSymbol))
-                        {
-                            visitStack.Push(closure.OriginalMethodSymbol);
-                        }
-                    }
-                });
+                public bool CapturesParent;
 
-                while (visitStack.Count > 0)
-                {
-                    var current = visitStack.Pop();
-                    var setToAddTo = capturesVariable.Contains(current) ? capturesVariable : capturesThis;
-                    foreach (var capturesCurrent in methodGraph[current])
+                public readonly bool IsStruct;
+                internal SynthesizedClosureEnvironment SynthesizedEnvironment;
+
+                public ClosureEnvironment(IEnumerable<Symbol> capturedVariables, bool isStruct)
                 {
-                        if (setToAddTo.Add(capturesCurrent))
+                    CapturedVariables = new SetWithInsertionOrder<Symbol>();
+                    foreach (var item in capturedVariables)
                     {
-                            visitStack.Push(capturesCurrent);
+                        CapturedVariables.Add(item);
                     }
+                    IsStruct = isStruct;
                 }
             }
 
-                // True if there are any closures in the tree which
-                // capture 'this' and another variable
-                bool captureMoreThanThis = false;
-
-                VisitClosures(ScopeTree, (scope, closure) =>
-                {
-                    if (!capturesVariable.Contains(closure.OriginalMethodSymbol))
-                    {
-                        closure.CapturedVariables.Clear();
-                    }
-
-                    if (capturesThis.Contains(closure.OriginalMethodSymbol))
+            /// <summary>
+            /// Visit all closures in all nested scopes and run the <paramref name="action"/>.
+            /// </summary>
+            public static void VisitClosures(Scope scope, Action<Scope, Closure> action)
             {
-                        closure.CapturedVariables.Add(thisParam);
-                        if (closure.CapturedVariables.Count > 1)
+                foreach (var closure in scope.Closures)
                 {
-                            captureMoreThanThis |= true;
-                        }
+                    action(scope, closure);
                 }
-                });
 
-                if (!captureMoreThanThis && capturesThis.Count > 0)
+                foreach (var nested in scope.NestedScopes)
                 {
-                    // If we have closures which capture 'this', and nothing else, we can
-                    // remove 'this' from the declared variables list, since we don't need
-                    // to create an environment to hold 'this' (since we can emit the
-                    // lowered methods directly onto the containing class)
-                    bool removed = ScopeTree.DeclaredVariables.Remove(thisParam);
-                    Debug.Assert(removed);
+                    VisitClosures(nested, action);
                 }
             }
 
             /// <summary>
-            /// Visit all closures in all nested scopes and run the <paramref name="action"/>.
+            /// Visit all the closures and return true when the <paramref name="func"/> returns
+            /// true. Otherwise, returns false.
             /// </summary>
-            public static void VisitClosures(Scope scope, Action<Scope, Closure> action)
+            public static bool CheckClosures(Scope scope, Func<Scope, Closure, bool> func)
             {
                 foreach (var closure in scope.Closures)
                 {
-                    action(scope, closure);
+                    if (func(scope, closure))
+                    {
+                        return true;
+                    }
                 }
 
                 foreach (var nested in scope.NestedScopes)
                 {
-                    VisitClosures(nested, action);
+                    if (CheckClosures(nested, func))
+                    {
+                        return true;
                     }
                 }
 
+                return false;
+            }
+
             /// <summary>
             /// Visit the tree with the given root and run the <paramref name="action"/>
             /// </summary>
@@ -491,6 +481,13 @@ private void AddIfCaptured(Symbol symbol, SyntaxNode syntax)
                         return;
                     }
 
+                    if (symbol is MethodSymbol method &&
+                        _currentClosure.OriginalMethodSymbol == method)
+                    {
+                        // Is this recursion? If so there's no capturing
+                        return;
+                    }
+
                     if (symbol.ContainingSymbol != _currentClosure.OriginalMethodSymbol)
                     {
                         // Restricted types can't be hoisted, so they are not permitted to be captured

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

 // 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.Collections.Immutable;
 using System.Diagnostics;
 using System.Linq;
 using Microsoft.CodeAnalysis.CodeGen;
@@ -32,15 +33,6 @@ internal sealed partial class Analysis
                                                                         // We can't rewrite delegate signatures
                                                                         || MethodsConvertedToDelegates.Contains(closure));
 
-            /// <summary>
-            /// Blocks that are positioned between a block declaring some lifted variables
-            /// and a block that contains the lambda that lifts said variables.
-            /// If such block itself requires a closure, then it must lift parent frame pointer into the closure
-            /// in addition to whatever else needs to be lifted.
-            /// <see cref="ComputeLambdaScopesAndFrameCaptures"/> needs to be called to compute this.
-            /// </summary>
-            public readonly PooledHashSet<BoundNode> NeedsParentFrame = PooledHashSet<BoundNode>.GetInstance();
-
             /// <summary>
             /// The root of the scope tree for this method.
             /// </summary>
@@ -97,9 +89,9 @@ internal sealed partial class Analysis
                     slotAllocatorOpt,
                     compilationState);
 
-                analysis.RemoveUnneededReferences(method.ThisParameter);
-                analysis.MakeAndAssignEnvironments(closureDebugInfo);
+                analysis.MakeAndAssignEnvironments();
                 analysis.ComputeLambdaScopesAndFrameCaptures(method.ThisParameter);
+                analysis.InlineThisOnlyEnvironments();
                 return analysis;
             }
 
@@ -146,104 +138,184 @@ private void ComputeLambdaScopesAndFrameCaptures(ParameterSymbol thisParam)
             {
                 VisitClosures(ScopeTree, (scope, closure) =>
                 {
-                    if (closure.CapturedVariables.Count > 0)
+                    if (closure.CapturedEnvironments.Count > 0)
                     {
-                        (Scope innermost, Scope outermost) = FindLambdaScopeRange(closure, scope);
-                        RecordClosureScope(innermost, outermost, closure);
-                    }
-                });
+                        var capturedEnvs = PooledHashSet<ClosureEnvironment>.GetInstance();
+                        capturedEnvs.AddAll(closure.CapturedEnvironments);
 
-                (Scope innermost, Scope outermost) FindLambdaScopeRange(Closure closure, Scope closureScope)
+                        // Find the nearest captured class environment, if one exists
+                        var curScope = scope;
+                        while (curScope != null)
                         {
-                    // If the closure only captures this, put the method directly in the
-                    // top-level method's containing type
-                    if (closure.CapturedVariables.Count == 1 &&
-                        closure.CapturedVariables.Single() is ParameterSymbol param &&
-                        param.IsThis)
+                            if (capturedEnvs.RemoveAll(curScope.DeclaredEnvironments))
                             {
-                        return (null, null);
+                                // Right now we only create one environment per scope
+                                Debug.Assert(curScope.DeclaredEnvironments.Count == 1);
+                                var env = curScope.DeclaredEnvironments[0];
+                                if (!env.IsStruct)
+                                {
+                                    closure.ContainingEnvironmentOpt = env;
+                                    break;
+                                }
+                            }
+                            curScope = curScope.Parent;
                         }
 
-                    Scope innermost = null;
-                    Scope outermost = null;
-
-                    var capturedVars = PooledHashSet<Symbol>.GetInstance();
-                    capturedVars.AddAll(closure.CapturedVariables);
+                        // Now we need to walk up the scopes to find environment captures
+                        var oldEnv = curScope?.DeclaredEnvironments[0];
+                        while (curScope != null)
+                        {
+                            if (capturedEnvs.Count == 0)
+                            {
+                                break;
+                            }
 
-                    // If any of the captured variables are local functions we'll need
-                    // to add the captured variables of that local function to the current
-                    // set. This has the effect of ensuring that if the local function
-                    // captures anything "above" the current scope then parent frame
-                    // is itself captured (so that the current lambda can call that
-                    // local function).
-                    foreach (var captured in closure.CapturedVariables)
+                            var envs = curScope.DeclaredEnvironments.Where(e => !e.IsStruct);
+                            if (!envs.IsEmpty())
                             {
-                        if (captured is LocalFunctionSymbol localFunc)
+                                // Right now we only create one environment per scope
+                                Debug.Assert(envs.IsSingle());
+                                var env = envs.First();
+                                Debug.Assert(!oldEnv.IsStruct);
+                                oldEnv.CapturesParent = true;
+                                oldEnv = env;
+                            }
+                            capturedEnvs.RemoveAll(curScope.DeclaredEnvironments);
+                            curScope = curScope.Parent;
+                        }
+
+                        if (capturedEnvs.Count > 0)
                         {
-                            var (found, _) = GetVisibleClosure(closureScope, localFunc);
-                            capturedVars.AddAll(found.CapturedVariables);
+                            throw ExceptionUtilities.Unreachable;
                         }
+
+                        capturedEnvs.Free();
+
+                    }
+                });
             }
 
-                    for (var curScope = closureScope;
-                         curScope != null && capturedVars.Count > 0;
-                         curScope = curScope.Parent)
+            /// <summary>
+            /// We may have ended up with a closure environment containing only
+            /// 'this'. This is basically equivalent to the containing type itself,
+            /// so we can inline the 'this' parameter into environments that
+            /// reference this one or lower closures directly onto the containing
+            /// type.
+            /// </summary>
+            private void InlineThisOnlyEnvironments()
             {
-                        if (!(capturedVars.RemoveAll(curScope.DeclaredVariables) ||
-                              capturedVars.RemoveAll(curScope.Closures.Select(c => c.OriginalMethodSymbol))))
+                // First make sure 'this' even exists
+                if (!_topLevelMethod.TryGetThisParameter(out var thisParam) ||
+                    thisParam == null)
                 {
-                            continue;
+                    return;
                 }
 
-                        outermost = curScope;
-                        if (innermost == null)
+                var topLevelEnvs = ScopeTree.DeclaredEnvironments;
+
+                // If it does exist, 'this' is always in the top-level environment
+                if (topLevelEnvs.Count == 0)
                 {
-                            innermost = curScope;
-                        }
+                    return;
                 }
 
-                    // If any captured variables are left, they're captured above method scope
-                    if (capturedVars.Count > 0)
+                Debug.Assert(topLevelEnvs.Count == 1);
+                var env = topLevelEnvs[0];
+
+                // The environment must contain only 'this' to be inlined
+                if (env.CapturedVariables.Count > 1 || 
+                    !env.CapturedVariables.Contains(thisParam))
                 {
-                        outermost = null;
+                    return;
                 }
 
-                    capturedVars.Free();
+                if (env.IsStruct)
+                {
+                    // If everything that captures the 'this' environment
+                    // lives in the containing type, we can remove the env
+                    bool cantRemove = CheckClosures(ScopeTree, (scope, closure) =>
+                    {
+                        return closure.CapturedEnvironments.Contains(env) &&
+                            closure.ContainingEnvironmentOpt != null;
+                    });
                     
-                    return (innermost, outermost);
+                    if (!cantRemove)
+                    {
+                        RemoveEnv();
                     }
-
-                void RecordClosureScope(Scope innermost, Scope outermost, Closure closure)
+                }
+                else
                 {
-                    // 1) if there is innermost scope, lambda goes there as we cannot go any higher.
-                    // 2) scopes in [innermostScope, outermostScope) chain need to have access to the parent scope.
-                    //
-                    // Example: 
-                    //   if a lambda captures a method's parameter and `this`, 
-                    //   its innermost scope is the root Scope (method locals and parameters) 
-                    //   and outermost Scope is null
-                    //   Such lambda will be placed in a closure frame that corresponds to the method's outer block
-                    //   and this frame will also lift original `this` as a field when created by its parent.
-                    //   Note that it is completely irrelevant how deeply the lexical scope of the lambda was originally nested.
-                    if (innermost != null)
+                    // Class-based 'this' closures can move member functions
+                    // to the top-level type and environments which capture
+                    // the 'this' environment can capture 'this' directly
+                    RemoveEnv();
+                    VisitClosures(ScopeTree, (scope, closure) =>
+                    {
+                        if (closure.ContainingEnvironmentOpt == env)
                         {
-                        closure.ContainingEnvironmentOpt = innermost.DeclaredEnvironments[0];
+                            closure.ContainingEnvironmentOpt = null;
+                        }
+                    });
 
-                        while (innermost != outermost)
+                    // Find all environments in the scope below that could
+                    // capture the parent. If there are any, add 'this' to
+                    // the list of captured variables and remove the parent
+                    // link
+                    VisitFirstLevelScopes(ScopeTree);
+                    void VisitFirstLevelScopes(Scope scope)
                     {
-                            NeedsParentFrame.Add(innermost.BoundNode);
-                            innermost = innermost.Parent;
+                        var classEnvs = scope.DeclaredEnvironments.Where(e => !e.IsStruct);
+                        if (classEnvs.IsEmpty())
+                        {
+                            // Keep looking for nested environments
+                            foreach (var nested in scope.NestedScopes)
+                            {
+                                VisitFirstLevelScopes(nested);
+                            }
+                        }
+                        else
+                        {
+                            foreach (var declEnv in classEnvs)
+                            {
+                                if (declEnv.CapturesParent)
+                                {
+                                    declEnv.CapturedVariables.Insert(0, thisParam);
+                                    declEnv.CapturesParent = false;
+                                }
                             }
                         }
                     }
                 }
 
-            private void MakeAndAssignEnvironments(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
+                void RemoveEnv()
+                {
+                    topLevelEnvs.RemoveAt(topLevelEnvs.IndexOf(env));
+                    VisitClosures(ScopeTree, (scope, closure) =>
+                    {
+                        var index = closure.CapturedEnvironments.IndexOf(env);
+                        if (index >= 0)
+                        {
+                            closure.CapturedEnvironments.RemoveAt(index);
+                        }
+                    });
+                }
+            }
+
+            private void MakeAndAssignEnvironments()
             {
                 VisitScopeTree(ScopeTree, scope =>
                 {
-                    if (scope.DeclaredVariables.Count > 0)
+                    // Currently all variables declared in the same scope are added
+                    // to the same closure environment
+                    var variablesInEnvironment = scope.DeclaredVariables;
+
+                    // Don't create empty environments
+                    if (variablesInEnvironment.Count == 0)
                     {
+                        return;
+                    }
+
                     // First walk the nested scopes to find all closures which
                     // capture variables from this scope. They all need to capture
                     // this environment. This includes closures which captured local
@@ -253,6 +325,16 @@ private void MakeAndAssignEnvironments(ArrayBuilder<ClosureDebugInfo> closureDeb
                     bool isStruct = true;
                     var closures = new SetWithInsertionOrder<Closure>();
                     bool addedItem;
+
+                    // This loop is O(n), where n is the length of the chain
+                    //   L_1 <- L_2 <- L_3 ...
+                    // where L_1 represents a local function that directly captures the current
+                    // environment, L_2 represents a local function that directly captures L_1,
+                    // L_3 represents a local function that captures L_2, and so on.
+                    //
+                    // Each iteration of the loop runs a visitor that is proportional to the
+                    // number of closures in nested scopes, so we hope that the total number
+                    // of nested functions and function chains is small in any real-world code.
                     do
                     {
                         addedItem = false;
@@ -270,43 +352,19 @@ private void MakeAndAssignEnvironments(ArrayBuilder<ClosureDebugInfo> closureDeb
                     } while (addedItem == true);
 
                     // Next create the environment and add it to the declaration scope
-                        // Currently all variables declared in the same scope are added
-                        // to the same closure environment
-                        var env = MakeEnvironment(scope, scope.DeclaredVariables, isStruct);
+                    var env = new ClosureEnvironment(variablesInEnvironment, isStruct);
                     scope.DeclaredEnvironments.Add(env);
 
+                    _topLevelMethod.TryGetThisParameter(out var thisParam);
                     foreach (var closure in closures)
                     {
                         closure.CapturedEnvironments.Add(env);
-                        }
-                    }
-                });
-
-                ClosureEnvironment MakeEnvironment(Scope scope, IEnumerable<Symbol> capturedVariables, bool isStruct)
+                        if (thisParam != null && env.CapturedVariables.Contains(thisParam))
                         {
-                    var scopeBoundNode = scope.BoundNode;
-
-                    var syntax = scopeBoundNode.Syntax;
-                    Debug.Assert(syntax != null);
-
-                    DebugId methodId = GetTopLevelMethodId();
-                    DebugId closureId = GetClosureId(syntax, closureDebugInfo);
-
-                    var containingMethod = scope.ContainingClosureOpt?.OriginalMethodSymbol ?? _topLevelMethod;
-                    if ((object)_substitutedSourceMethod != null && containingMethod == _topLevelMethod)
-                    {
-                        containingMethod = _substitutedSourceMethod;
+                            closure.CapturesThis = true;
                         }
-
-                    return new ClosureEnvironment(
-                        capturedVariables,
-                        _topLevelMethod,
-                        containingMethod,
-                        isStruct,
-                        syntax,
-                        methodId,
-                        closureId);
                     }
+                });
             }
 
             internal DebugId GetTopLevelMethodId()
@@ -314,7 +372,7 @@ internal DebugId GetTopLevelMethodId()
                 return _slotAllocatorOpt?.MethodId ?? new DebugId(_topLevelMethodOrdinal, _compilationState.ModuleBuilderOpt.CurrentGenerationOrdinal);
             }
 
-            private DebugId GetClosureId(SyntaxNode syntax, ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
+            internal DebugId GetClosureId(SyntaxNode syntax, ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
             {
                 Debug.Assert(syntax != null);
 
@@ -470,7 +528,6 @@ Closure Helper(Scope scope)
             public void Free()
             {
                 MethodsConvertedToDelegates.Free();
-                NeedsParentFrame.Free();
                 ScopeTree.Free();
             }
         }

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

@@ -140,7 +140,7 @@ public BoundStatement RewriteLocalFunctionReferences(BoundStatement loweredBody)
                         _framePointers.TryGetValue(synthesizedLambda.ContainingType, out _innermostFramePointer);
                     }
 
-                    var containerAsFrame = synthesizedLambda.ContainingType as ClosureEnvironment;
+                    var containerAsFrame = synthesizedLambda.ContainingType as SynthesizedClosureEnvironment;
 
                     // Includes type parameters from the containing type iff
                     // the containing type is a frame. If it is a frame then
@@ -201,11 +201,11 @@ public BoundStatement RewriteLocalFunctionReferences(BoundStatement loweredBody)
                     // will always be a LambdaFrame, it's always a capture frame
                     var frameType = (NamedTypeSymbol)loweredSymbol.Parameters[i].Type.OriginalDefinition;
 
-                    Debug.Assert(frameType is ClosureEnvironment);
+                    Debug.Assert(frameType is SynthesizedClosureEnvironment);
 
                     if (frameType.Arity > 0)
                     {
-                        var typeParameters = ((ClosureEnvironment)frameType).ConstructedFromTypeParameters;
+                        var typeParameters = ((SynthesizedClosureEnvironment)frameType).ConstructedFromTypeParameters;
                         Debug.Assert(typeParameters.Length == frameType.Arity);
                         var subst = this.TypeMap.SubstituteTypeParameters(typeParameters);
                         frameType = frameType.Construct(subst);

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

@@ -30,7 +30,7 @@ namespace Microsoft.CodeAnalysis.CSharp
     /// have captured variables.  The result of this analysis is left in <see cref="_analysis"/>.
     /// 
     /// Then we make a frame, or compiler-generated class, represented by an instance of
-    /// <see cref="ClosureEnvironment"/> for each scope with captured variables.  The generated frames are kept
+    /// <see cref="SynthesizedClosureEnvironment"/> for each scope with captured variables.  The generated frames are kept
     /// in <see cref="_frames"/>.  Each frame is given a single field for each captured
     /// variable in the corresponding scope.  These are maintained in <see cref="MethodToClassRewriter.proxies"/>.
     /// 
@@ -73,7 +73,7 @@ internal sealed partial class LambdaRewriter : MethodToClassRewriter
 
         // lambda frame for static lambdas. 
         // initialized lazily and could be null if there are no static lambdas
-        private ClosureEnvironment _lazyStaticLambdaFrame;
+        private SynthesizedClosureEnvironment _lazyStaticLambdaFrame;
 
         // A mapping from every lambda parameter to its corresponding method's parameter.
         private readonly Dictionary<ParameterSymbol, ParameterSymbol> _parameterMap = new Dictionary<ParameterSymbol, ParameterSymbol>();
@@ -93,7 +93,7 @@ public MappedLocalFunction(SynthesizedLambdaMethod symbol, ClosureKind closureKi
         private readonly Dictionary<LocalFunctionSymbol, MappedLocalFunction> _localFunctionMap = new Dictionary<LocalFunctionSymbol, MappedLocalFunction>();
 
         // for each block with lifted (captured) variables, the corresponding frame type
-        private readonly Dictionary<BoundNode, ClosureEnvironment> _frames = new Dictionary<BoundNode, ClosureEnvironment>();
+        private readonly Dictionary<BoundNode, Analysis.ClosureEnvironment> _frames = new Dictionary<BoundNode, Analysis.ClosureEnvironment>();
 
         // the current set of frame pointers in scope.  Each is either a local variable (where introduced),
         // or the "this" parameter when at the top level.  Keys in this map are never constructed types.
@@ -268,7 +268,7 @@ protected override bool NeedsProxy(Symbol localOrParameter)
                 diagnostics,
                 assignLocals);
 
-            rewriter.SynthesizeClosureEnvironments();
+            rewriter.SynthesizeClosureEnvironments(closureDebugInfoBuilder);
 
             // First, lower everything but references (calls, delegate conversions)
             // to local functions
@@ -341,10 +341,10 @@ protected override NamedTypeSymbol ContainingType
         static partial void CheckLocalsDefined(BoundNode node);
 
         /// <summary>
-        /// Adds <see cref="ClosureEnvironment"/> synthesized types to the compilation state
+        /// Adds <see cref="SynthesizedClosureEnvironment"/> synthesized types to the compilation state
         /// and creates hoisted fields for all locals captured by the environments.
         /// </summary>
-        private void SynthesizeClosureEnvironments()
+        private void SynthesizeClosureEnvironments(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
         {
             Analysis.VisitScopeTree(_analysis.ScopeTree, scope =>
             {
@@ -356,32 +356,59 @@ private void SynthesizeClosureEnvironments()
                     Debug.Assert(scope.DeclaredEnvironments.Count == 1);
 
                     var env = scope.DeclaredEnvironments[0];
+                    var frame = MakeFrame(scope, env.IsStruct);
+                    env.SynthesizedEnvironment = frame;
 
-                    CompilationState.ModuleBuilderOpt.AddSynthesizedDefinition(ContainingType, env);
-                    if (env.Constructor != null)
+                    CompilationState.ModuleBuilderOpt.AddSynthesizedDefinition(ContainingType, frame);
+                    if (frame.Constructor != null)
                     {
                         AddSynthesizedMethod(
-                            env.Constructor,
+                            frame.Constructor,
                             FlowAnalysisPass.AppendImplicitReturn(
-                                MethodCompiler.BindMethodBody(env.Constructor, CompilationState, null),
-                                env.Constructor));
+                                MethodCompiler.BindMethodBody(frame.Constructor, CompilationState, null),
+                                frame.Constructor));
                     }
 
                     foreach (var captured in env.CapturedVariables)
                     {
                         Debug.Assert(!proxies.ContainsKey(captured));
 
-                        var hoistedField = LambdaCapturedVariable.Create(env, captured, ref _synthesizedFieldNameIdDispenser);
+                        var hoistedField = LambdaCapturedVariable.Create(frame, captured, ref _synthesizedFieldNameIdDispenser);
                         proxies.Add(captured, new CapturedToFrameSymbolReplacement(hoistedField, isReusable: false));
-                        CompilationState.ModuleBuilderOpt.AddSynthesizedDefinition(env, hoistedField);
+                        CompilationState.ModuleBuilderOpt.AddSynthesizedDefinition(frame, hoistedField);
                     }
 
                     _frames.Add(scope.BoundNode, env);
                 }
             });
+
+            SynthesizedClosureEnvironment MakeFrame(Analysis.Scope scope, bool isStruct)
+            {
+                var scopeBoundNode = scope.BoundNode;
+
+                var syntax = scopeBoundNode.Syntax;
+                Debug.Assert(syntax != null);
+
+                DebugId methodId = _analysis.GetTopLevelMethodId();
+                DebugId closureId = _analysis.GetClosureId(syntax, closureDebugInfo);
+
+                var containingMethod = scope.ContainingClosureOpt?.OriginalMethodSymbol ?? _topLevelMethod;
+                if ((object)_substitutedSourceMethod != null && containingMethod == _topLevelMethod)
+                {
+                    containingMethod = _substitutedSourceMethod;
                 }
 
-        private ClosureEnvironment GetStaticFrame(DiagnosticBag diagnostics, IBoundLambdaOrFunction lambda)
+                return new SynthesizedClosureEnvironment(
+                    _topLevelMethod,
+                    containingMethod,
+                    isStruct,
+                    syntax,
+                    methodId,
+                    closureId);
+        }
+    }
+
+        private SynthesizedClosureEnvironment GetStaticFrame(DiagnosticBag diagnostics, IBoundLambdaOrFunction lambda)
         {
             if (_lazyStaticLambdaFrame == null)
             {
@@ -406,8 +433,7 @@ private ClosureEnvironment GetStaticFrame(DiagnosticBag diagnostics, IBoundLambd
                     DebugId closureId = default(DebugId);
                     // using _topLevelMethod as containing member because the static frame does not have generic parameters, except for the top level method's
                     var containingMethod = isNonGeneric ? null : (_substitutedSourceMethod ?? _topLevelMethod);
-                    _lazyStaticLambdaFrame = new ClosureEnvironment(
-                        SpecializedCollections.EmptyEnumerable<Symbol>(),
+                    _lazyStaticLambdaFrame = new SynthesizedClosureEnvironment(
                         _topLevelMethod,
                         containingMethod,
                         isStruct: false,
@@ -530,11 +556,12 @@ private static void InsertAndFreePrologue(ArrayBuilder<BoundStatement> result, A
         /// Introduce a frame around the translation of the given node.
         /// </summary>
         /// <param name="node">The node whose translation should be translated to contain a frame</param>
-        /// <param name="frame">The frame for the translated node</param>
+        /// <param name="env">The environment for the translated node</param>
         /// <param name="F">A function that computes the translation of the node.  It receives lists of added statements and added symbols</param>
         /// <returns>The translated statement, as returned from F</returns>
-        private BoundNode IntroduceFrame(BoundNode node, ClosureEnvironment frame, Func<ArrayBuilder<BoundExpression>, ArrayBuilder<LocalSymbol>, BoundNode> F)
+        private BoundNode IntroduceFrame(BoundNode node, Analysis.ClosureEnvironment env, Func<ArrayBuilder<BoundExpression>, ArrayBuilder<LocalSymbol>, BoundNode> F)
         {
+            var frame = env.SynthesizedEnvironment;
             var frameTypeParameters = ImmutableArray.Create(StaticCast<TypeSymbol>.From(_currentTypeParameters).SelectAsArray(TypeMap.TypeSymbolAsTypeWithModifiers), 0, frame.Arity);
             NamedTypeSymbol frameType = frame.ConstructIfGeneric(frameTypeParameters);
 
@@ -562,14 +589,7 @@ private BoundNode IntroduceFrame(BoundNode node, ClosureEnvironment frame, Func<
             if ((object)_innermostFramePointer != null)
             {
                 proxies.TryGetValue(_innermostFramePointer, out oldInnermostFrameProxy);
-                if (_analysis.NeedsParentFrame.Contains(node) &&
-                    // If the frame pointer is a struct type that means the frame is a struct
-                    // passed by-ref to a local function. Capturing parent frames for local
-                    // functions is performed in RemapLambdaOrLocalFunction, rather than here
-                    // (since struct frame pointers should never be captured, but instead be
-                    // passed in a list to the needed local functions).
-                    !(_innermostFramePointer.Kind == SymbolKind.Local &&
-                      ((LocalSymbol)_innermostFramePointer).Type.IsValueType))
+                if (env.CapturesParent)
                 {
                     var capturedFrame = LambdaCapturedVariable.Create(frame, _innermostFramePointer, ref _synthesizedFieldNameIdDispenser);
                     FieldSymbol frameParent = capturedFrame.AsMember(frameType);
@@ -590,7 +610,7 @@ private BoundNode IntroduceFrame(BoundNode node, ClosureEnvironment frame, Func<
 
             // Capture any parameters of this block.  This would typically occur
             // at the top level of a method or lambda with captured parameters.
-            foreach (var variable in frame.CapturedVariables)
+            foreach (var variable in env.CapturedVariables)
             {
                 InitVariableProxy(syntax, variable, framePointer, prologue);
             }
@@ -728,7 +748,7 @@ public override BoundNode VisitBaseReference(BoundBaseReference node)
             out NamedTypeSymbol constructedFrame)
         {
             var translatedLambdaContainer = synthesizedMethod.ContainingType;
-            var containerAsFrame = translatedLambdaContainer as ClosureEnvironment;
+            var containerAsFrame = translatedLambdaContainer as SynthesizedClosureEnvironment;
 
             // All of _currentTypeParameters might not be preserved here due to recursively calling upwards in the chain of local functions/lambdas
             Debug.Assert((typeArgumentsOpt.IsDefault && !originalMethod.IsGenericMethod) || (typeArgumentsOpt.Length == originalMethod.Arity));
@@ -866,9 +886,8 @@ private BoundSequence RewriteSequence(BoundSequence node, ArrayBuilder<BoundExpr
 
         public override BoundNode VisitBlock(BoundBlock node)
         {
-            ClosureEnvironment frame;
             // Test if this frame has captured variables and requires the introduction of a closure class.
-            if (_frames.TryGetValue(node, out frame))
+            if (_frames.TryGetValue(node, out var frame))
             {
                 return IntroduceFrame(node, frame, (ArrayBuilder<BoundExpression> prologue, ArrayBuilder<LocalSymbol> newLocals) =>
                     RewriteBlock(node, prologue, newLocals));
@@ -924,8 +943,7 @@ public override BoundNode VisitScope(BoundScope node)
         public override BoundNode VisitCatchBlock(BoundCatchBlock node)
         {
             // Test if this frame has captured variables and requires the introduction of a closure class.
-            ClosureEnvironment frame;
-            if (_frames.TryGetValue(node, out frame))
+            if (_frames.TryGetValue(node, out var frame))
             {
                 return IntroduceFrame(node, frame, (ArrayBuilder<BoundExpression> prologue, ArrayBuilder<LocalSymbol> newLocals) =>
                 {
@@ -991,9 +1009,8 @@ private BoundNode RewriteCatch(BoundCatchBlock node, ArrayBuilder<BoundExpressio
 
         public override BoundNode VisitSequence(BoundSequence node)
         {
-            ClosureEnvironment frame;
             // Test if this frame has captured variables and requires the introduction of a closure class.
-            if (_frames.TryGetValue(node, out frame))
+            if (_frames.TryGetValue(node, out var frame))
             {
                 return IntroduceFrame(node, frame, (ArrayBuilder<BoundExpression> prologue, ArrayBuilder<LocalSymbol> newLocals) =>
                 {
@@ -1008,10 +1025,9 @@ public override BoundNode VisitSequence(BoundSequence node)
 
         public override BoundNode VisitStatementList(BoundStatementList node)
         {
-            ClosureEnvironment frame;
             // Test if this frame has captured variables and requires the introduction of a closure class.
             // That can occur for a BoundStatementList if it is the body of a method with captured parameters.
-            if (_frames.TryGetValue(node, out frame))
+            if (_frames.TryGetValue(node, out var frame))
             {
                 return IntroduceFrame(node, frame, (ArrayBuilder<BoundExpression> prologue, ArrayBuilder<LocalSymbol> newLocals) =>
                 {
@@ -1034,9 +1050,8 @@ public override BoundNode VisitStatementList(BoundStatementList node)
 
         public override BoundNode VisitSwitchStatement(BoundSwitchStatement node)
         {
-            ClosureEnvironment frame;
             // Test if this frame has captured variables and requires the introduction of a closure class.
-            if (_frames.TryGetValue(node, out frame))
+            if (_frames.TryGetValue(node, out var frame))
             {
                 return IntroduceFrame(node, frame, (ArrayBuilder<BoundExpression> prologue, ArrayBuilder<LocalSymbol> newLocals) =>
                 {
@@ -1105,7 +1120,7 @@ public override BoundNode VisitLocalFunctionStatement(BoundLocalFunctionStatemen
         {
             ClosureKind closureKind;
             NamedTypeSymbol translatedLambdaContainer;
-            ClosureEnvironment containerAsFrame;
+            SynthesizedClosureEnvironment containerAsFrame;
             BoundNode lambdaScope;
             DebugId topLevelMethodId;
             DebugId lambdaId;
@@ -1178,7 +1193,7 @@ private DebugId GetLambdaId(SyntaxNode syntax, ClosureKind closureKind, int clos
             IBoundLambdaOrFunction node,
             out ClosureKind closureKind,
             out NamedTypeSymbol translatedLambdaContainer,
-            out ClosureEnvironment containerAsFrame,
+            out SynthesizedClosureEnvironment containerAsFrame,
             out BoundNode lambdaScope,
             out DebugId topLevelMethodId,
             out DebugId lambdaId)
@@ -1186,23 +1201,13 @@ private DebugId GetLambdaId(SyntaxNode syntax, ClosureKind closureKind, int clos
             Analysis.Closure closure = Analysis.GetClosureInTree(_analysis.ScopeTree, node.Symbol);
 
             var structClosures = closure.CapturedEnvironments
-                .Where(env => env.IsStructType()).AsImmutable();
+                .Where(env => env.IsStruct).Select(env => env.SynthesizedEnvironment).AsImmutable();
+
             int closureOrdinal;
             if (closure.ContainingEnvironmentOpt != null)
             {
-                containerAsFrame = closure.ContainingEnvironmentOpt;
+                containerAsFrame = closure.ContainingEnvironmentOpt?.SynthesizedEnvironment;
 
-                if (containerAsFrame?.IsValueType == true)
-                {
-                    // Lower directly onto the containing type
-                    containerAsFrame = null;
-                    lambdaScope = null;
-                    closureKind = ClosureKind.Static; // not exactly... but we've rewritten the receiver to be a by-ref parameter
-                    translatedLambdaContainer = _topLevelMethod.ContainingType;
-                    closureOrdinal = LambdaDebugInfo.StaticClosureOrdinal;
-                }
-                else
-                {
                 closureKind = ClosureKind.General;
                 translatedLambdaContainer = containerAsFrame;
                 closureOrdinal = containerAsFrame.ClosureOrdinal;
@@ -1218,8 +1223,15 @@ private DebugId GetLambdaId(SyntaxNode syntax, ClosureKind closureKind, int clos
                 Debug.Assert(tmpScope != null);
                 lambdaScope = tmpScope;
             }
+            else if (closure.CapturesThis)
+            {
+                lambdaScope = null;
+                containerAsFrame = null;
+                translatedLambdaContainer = _topLevelMethod.ContainingType;
+                closureKind = ClosureKind.ThisOnly;
+                closureOrdinal = LambdaDebugInfo.ThisOnlyClosureOrdinal;
             }
-            else if (closure.CapturedVariables.Count == 0)
+            else if (closure.CapturedEnvironments.Count == 0)
             {
                 if (_analysis.MethodsConvertedToDelegates.Contains(node.Symbol))
                 {
@@ -1238,11 +1250,12 @@ private DebugId GetLambdaId(SyntaxNode syntax, ClosureKind closureKind, int clos
             }
             else
             {
-                lambdaScope = null;
+                // Lower directly onto the containing type
                 containerAsFrame = null;
+                lambdaScope = null;
+                closureKind = ClosureKind.Static; // not exactly... but we've rewritten the receiver to be a by-ref parameter
                 translatedLambdaContainer = _topLevelMethod.ContainingType;
-                closureKind = ClosureKind.ThisOnly;
-                closureOrdinal = LambdaDebugInfo.ThisOnlyClosureOrdinal;
+                closureOrdinal = LambdaDebugInfo.StaticClosureOrdinal;
             }
 
             // Move the body of the lambda to a freshly generated synthetic method on its frame.
@@ -1338,7 +1351,7 @@ private BoundNode RewriteLambdaConversion(BoundLambda node)
 
             ClosureKind closureKind;
             NamedTypeSymbol translatedLambdaContainer;
-            ClosureEnvironment containerAsFrame;
+            SynthesizedClosureEnvironment containerAsFrame;
             BoundNode lambdaScope;
             DebugId topLevelMethodId;
             DebugId lambdaId;

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaFrame.cs → src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/SynthesizedClosureEnvironment.cs

@@ -10,10 +10,9 @@
 namespace Microsoft.CodeAnalysis.CSharp
 {
     /// <summary>
-    /// A class that represents the set of variables in a scope that have been
-    /// captured by nested functions within that scope.
+    /// The synthesized type added to a compilation to hold captured variables for closures.
     /// </summary>
-    internal sealed class ClosureEnvironment : SynthesizedContainer, ISynthesizedMethodBodyImplementationSymbol
+    internal sealed class SynthesizedClosureEnvironment : SynthesizedContainer, ISynthesizedMethodBodyImplementationSymbol
     {
         private readonly MethodSymbol _topLevelMethod;
         internal readonly SyntaxNode ScopeSyntaxOpt;
@@ -25,13 +24,11 @@ internal sealed class ClosureEnvironment : SynthesizedContainer, ISynthesizedMet
         internal readonly MethodSymbol OriginalContainingMethodOpt;
         internal readonly FieldSymbol SingletonCache;
         internal readonly MethodSymbol StaticConstructor;
-        public readonly IEnumerable<Symbol> CapturedVariables;
 
         public override TypeKind TypeKind { get; }
         internal override MethodSymbol Constructor { get; }
 
-        internal ClosureEnvironment(
-            IEnumerable<Symbol> capturedVariables,
+        internal SynthesizedClosureEnvironment(
             MethodSymbol topLevelMethod,
             MethodSymbol containingMethod,
             bool isStruct,
@@ -40,11 +37,10 @@ internal sealed class ClosureEnvironment : SynthesizedContainer, ISynthesizedMet
             DebugId closureId)
             : base(MakeName(scopeSyntaxOpt, methodId, closureId), containingMethod)
         {
-            CapturedVariables = capturedVariables;
             TypeKind = isStruct ? TypeKind.Struct : TypeKind.Class;
             _topLevelMethod = topLevelMethod;
             OriginalContainingMethodOpt = containingMethod;
-            Constructor = isStruct ? null : new LambdaFrameConstructor(this);
+            Constructor = isStruct ? null : new SynthesizedClosureEnvironmentConstructor(this);
             this.ClosureOrdinal = closureId.Ordinal;
 
             // static lambdas technically have the class scope so the scope syntax is null 

src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaFrameConstructor.cs → src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/SynthesizedClosureEnvironmentConstructor.cs

@@ -4,9 +4,9 @@
 
 namespace Microsoft.CodeAnalysis.CSharp
 {
-    internal sealed class LambdaFrameConstructor : SynthesizedInstanceConstructor, ISynthesizedMethodBodyImplementationSymbol
+    internal sealed class SynthesizedClosureEnvironmentConstructor : SynthesizedInstanceConstructor, ISynthesizedMethodBodyImplementationSymbol
     {
-        internal LambdaFrameConstructor(ClosureEnvironment frame)
+        internal SynthesizedClosureEnvironmentConstructor(SynthesizedClosureEnvironment frame)
             : base(frame)
         {
         }

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

@@ -19,7 +19,7 @@ internal sealed class SynthesizedLambdaMethod : SynthesizedMethodBaseSymbol, ISy
 
         internal SynthesizedLambdaMethod(
             NamedTypeSymbol containingType,
-            ImmutableArray<ClosureEnvironment> structEnvironments,
+            ImmutableArray<SynthesizedClosureEnvironment> structEnvironments,
             ClosureKind closureKind,
             MethodSymbol topLevelMethod,
             DebugId topLevelMethodId,
@@ -43,9 +43,9 @@ internal sealed class SynthesizedLambdaMethod : SynthesizedMethodBaseSymbol, ISy
             TypeMap typeMap;
             ImmutableArray<TypeParameterSymbol> typeParameters;
             ImmutableArray<TypeParameterSymbol> constructedFromTypeParameters;
-            ClosureEnvironment lambdaFrame;
+            SynthesizedClosureEnvironment lambdaFrame;
 
-            lambdaFrame = this.ContainingType as ClosureEnvironment;
+            lambdaFrame = this.ContainingType as SynthesizedClosureEnvironment;
             switch (closureKind)
             {
                 case ClosureKind.Singleton: // all type parameters on method (except the top level method's)

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenClosureLambdaTests.cs

@@ -11,6 +11,53 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen
 {
     public class CodeGenClosureLambdaTests : CSharpTestBase
     {
+        [Fact]
+        public void EnvironmentChainContainsUnusedEnvironment()
+        {
+            CompileAndVerify(@"
+using System;
+class C
+{
+    void M(int x)
+    {
+        {
+            int y = 10;
+            Action f1 = () => Console.WriteLine(y);
+
+            {
+                int z = 5;
+                Action f2 = () => Console.WriteLine(z + x);
+                f2();
+            }
+            f1();
+        }
+    }
+    public static void Main() => new C().M(3);
+}", expectedOutput: @"8
+10");
+        }
+
+        [Fact]
+        public void CaptureThisAsFramePointer()
+        {
+            var comp = @"
+using System;
+using System.Collections.Generic;
+
+class C
+{
+    int _z = 0;
+    void M(IEnumerable<int> xs)
+    {
+        foreach (var x in xs)
+        {
+            Func<int, int> captureFunc = k => x + _z;
+        }
+    }
+}";
+            CompileAndVerify(comp);
+        }
+
         [Fact]
         public void StaticClosure01()
         {
@@ -3655,8 +3702,8 @@ .maxstack  2
   IL_000d:  ldarg.0
   IL_000e:  call       ""bool Program.c1.T()""
   IL_0013:  brfalse.s  IL_002e
-  IL_0015:  ldarg.0
-  IL_0016:  ldftn      ""bool Program.c1.<Test>b__1_0(int)""
+  IL_0015:  ldloc.0
+  IL_0016:  ldftn      ""bool Program.c1.<>c__DisplayClass1_0.<Test>b__0(int)""
   IL_001c:  newobj     ""System.Func<int, bool>..ctor(object, System.IntPtr)""
   IL_0021:  ldc.i4.s   42
   IL_0023:  callvirt   ""bool System.Func<int, bool>.Invoke(int)""

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs

@@ -30,6 +30,98 @@ public static IMethodSymbol FindLocalFunction(this CompilationVerifier verifier,
     [CompilerTrait(CompilerFeature.LocalFunctions)]
     public class CodeGenLocalFunctionTests : CSharpTestBase
     {
+        [Fact]
+        public void EnvironmentChainContainsStructEnvironment()
+        {
+            CompileAndVerify(@"
+using System;
+class C
+{
+    void M(int x)
+    {
+        {
+            int y = 10;
+            void L() => Console.WriteLine(y);
+
+            {
+                int z = 5;
+                Action f2 = () => Console.WriteLine(z + x);
+                f2();
+            }
+            L();
+        }
+    }
+    public static void Main() => new C().M(3);
+}", expectedOutput: @"8
+10");
+        }
+
+        [Fact]
+        public void Repro20577()
+        {
+            var comp = CreateStandardCompilation(@"
+using System.Linq;
+
+public class Program {
+    public static void Main(string[] args) {
+        object v;
+
+        void AAA() {
+            object BBB(object v2) {
+                var a = v;
+                ((object[])v2).Select(i => BBB(i));
+                return null;
+            }
+        }
+    }
+}", references: new[] { LinqAssemblyRef });
+            CompileAndVerify(comp);
+        }
+
+        [Fact]
+        public void Repro19033()
+        {
+            CompileAndVerify(@"
+using System;
+
+class Program
+{
+    void Q(int n = 0)
+    {
+        {
+            object mc;
+
+            string B(object map)
+            {
+                Action<int> a = _ => B(new object());
+                return n.ToString();
+            }
+        }
+    }
+}");
+        }
+
+        [Fact]
+        public void Repro19033_2()
+        {
+            CompileAndVerify(@"
+using System;
+class C
+{
+    static void F(Action a)
+    {
+        object x = null;
+        {
+            object y = null;
+            void G(object z)
+            {
+                F(() => G(x));
+            }
+        }
+    }
+}");
+        }
+
         [Fact]
         [WorkItem(18814, "https://github.com/dotnet/roslyn/issues/18814")]
         [WorkItem(18918, "https://github.com/dotnet/roslyn/issues/18918")]
@@ -78,7 +170,7 @@ void L5()
 1");
             verifier.VerifyIL("C.M()", @"
 {
-  // Code size       46 (0x2e)
+  // Code size       47 (0x2f)
   .maxstack  2
   .locals init (C.<>c__DisplayClass2_0 V_0) //CS$<>8__locals0
   IL_0000:  ldloca.s   V_0
@@ -90,24 +182,24 @@ .maxstack  2
   IL_0010:  ldarg.0
   IL_0011:  ldfld      ""int C._x""
   IL_0016:  call       ""void System.Console.WriteLine(int)""
-  IL_001b:  ldloca.s   V_0
-  IL_001d:  call       ""void C.<M>g__L12_0(ref C.<>c__DisplayClass2_0)""
-  IL_0022:  ldarg.0
-  IL_0023:  ldfld      ""int C._x""
-  IL_0028:  call       ""void System.Console.WriteLine(int)""
-  IL_002d:  ret
+  IL_001b:  ldarg.0
+  IL_001c:  ldloca.s   V_0
+  IL_001e:  call       ""void C.<M>g__L12_0(ref C.<>c__DisplayClass2_0)""
+  IL_0023:  ldarg.0
+  IL_0024:  ldfld      ""int C._x""
+  IL_0029:  call       ""void System.Console.WriteLine(int)""
+  IL_002e:  ret
 }");
 
             // L1
             verifier.VerifyIL("C.<M>g__L12_0(ref C.<>c__DisplayClass2_0)", @"
 {
-  // Code size       13 (0xd)
+  // Code size        8 (0x8)
   .maxstack  2
   IL_0000:  ldarg.0
-  IL_0001:  ldfld      ""C C.<>c__DisplayClass2_0.<>4__this""
-  IL_0006:  ldarg.0
-  IL_0007:  call       ""void C.<M>g__L22_1(ref C.<>c__DisplayClass2_0)""
-  IL_000c:  ret
+  IL_0001:  ldarg.1
+  IL_0002:  call       ""void C.<M>g__L22_1(ref C.<>c__DisplayClass2_0)""
+  IL_0007:  ret
 }");
             // L2
             verifier.VerifyIL("C.<M>g__L22_1(ref C.<>c__DisplayClass2_0)", @"
@@ -122,35 +214,34 @@ .maxstack  2
             // Skip some... L5
             verifier.VerifyIL("C.<M>g__L52_4(ref C.<>c__DisplayClass2_0, ref C.<>c__DisplayClass2_1)", @"
 {
-  // Code size        9 (0x9)
-  .maxstack  2
+  // Code size       10 (0xa)
+  .maxstack  3
   IL_0000:  ldarg.0
   IL_0001:  ldarg.1
-  IL_0002:  call       ""int C.<M>g__L62_5(ref C.<>c__DisplayClass2_0, ref C.<>c__DisplayClass2_1)""
-  IL_0007:  pop
-  IL_0008:  ret
+  IL_0002:  ldarg.2
+  IL_0003:  call       ""int C.<M>g__L62_5(ref C.<>c__DisplayClass2_0, ref C.<>c__DisplayClass2_1)""
+  IL_0008:  pop
+  IL_0009:  ret
 }");
             // L6
             verifier.VerifyIL("C.<M>g__L62_5(ref C.<>c__DisplayClass2_0, ref C.<>c__DisplayClass2_1)", @"
 {
-  // Code size       35 (0x23)
+  // Code size       25 (0x19)
   .maxstack  4
   .locals init (int V_0)
-  IL_0000:  ldarg.1
+  IL_0000:  ldarg.2
   IL_0001:  ldfld      ""int C.<>c__DisplayClass2_1.var2""
-  IL_0006:  ldarg.1
-  IL_0007:  ldfld      ""C C.<>c__DisplayClass2_1.<>4__this""
-  IL_000c:  ldarg.1
-  IL_000d:  ldfld      ""C C.<>c__DisplayClass2_1.<>4__this""
-  IL_0012:  ldfld      ""int C._x""
-  IL_0017:  stloc.0
-  IL_0018:  ldloc.0
-  IL_0019:  ldc.i4.1
-  IL_001a:  add
-  IL_001b:  stfld      ""int C._x""
-  IL_0020:  ldloc.0
-  IL_0021:  add
-  IL_0022:  ret
+  IL_0006:  ldarg.0
+  IL_0007:  ldarg.0
+  IL_0008:  ldfld      ""int C._x""
+  IL_000d:  stloc.0
+  IL_000e:  ldloc.0
+  IL_000f:  ldc.i4.1
+  IL_0010:  add
+  IL_0011:  stfld      ""int C._x""
+  IL_0016:  ldloc.0
+  IL_0017:  add
+  IL_0018:  ret
 }");
         }
 

src/Compilers/CSharp/Test/Emit/Emit/EditAndContinue/EditAndContinueClosureTests.cs

@@ -1408,8 +1408,8 @@ public int F()
 
             var reader0 = md0.MetadataReader;
             CheckNames(reader0, reader0.GetTypeDefNames(), "<Module>", "C", "<>c__DisplayClass0_0", "<>c");
-            CheckNames(reader0, reader0.GetMethodDefNames(), "F", ".ctor", "<F>b__0_1", ".ctor", "<F>b__2", ".cctor", ".ctor", "<F>b__0_0");
-            CheckNames(reader0, reader0.GetFieldDefNames(), "a", "<>9", "<>9__0_0");
+            CheckNames(reader0, reader0.GetMethodDefNames(), "F", ".ctor", ".ctor", "<F>b__1", "<F>b__2", ".cctor", ".ctor", "<F>b__0_0");
+            CheckNames(reader0, reader0.GetFieldDefNames(), "<>4__this", "a", "<>9", "<>9__0_0");
 
             var generation0 = EmitBaseline.CreateInitialBaseline(md0, v0.CreateSymReader().GetEncMethodDebugInfo);
             var diff1 = compilation1.EmitDifference(
@@ -1420,13 +1420,13 @@ public int F()
             var reader1 = diff1.GetMetadata().Reader;
 
             CheckNames(new[] { reader0, reader1 }, reader1.GetTypeDefNames(), "<>c__DisplayClass0#1_0#1");
-            CheckNames(new[] { reader0, reader1 }, reader1.GetMethodDefNames(), ".ctor", "F", "<F>b__0#1_1#1", ".ctor", "<F>b__2#1", "<F>b__0#1_0#1");
-            CheckNames(new[] { reader0, reader1 }, reader1.GetFieldDefNames(), "a", "<>9__0#1_0#1");
+            CheckNames(new[] { reader0, reader1 }, reader1.GetMethodDefNames(), ".ctor", "F", ".ctor", "<F>b__1#1", "<F>b__2#1", "<F>b__0#1_0#1");
+            CheckNames(new[] { reader0, reader1 }, reader1.GetFieldDefNames(), "<>4__this", "a", "<>9__0#1_0#1");
 
             diff1.VerifySynthesizedMembers(
-                "C: {<F>b__0#1_1#1, <>c__DisplayClass0#1_0#1, <>c}",
-                "C.<>c: {<>9__0#1_0#1, <F>b__0#1_0#1}",
-                "C.<>c__DisplayClass0#1_0#1: {a, <F>b__2#1}");
+                "C: {<>c__DisplayClass0#1_0#1, <>c}",
+                "C.<>c__DisplayClass0#1_0#1: {<>4__this, a, <F>b__1#1, <F>b__2#1}",
+                "C.<>c: {<>9__0#1_0#1, <F>b__0#1_0#1}");
 
             var diff2 = compilation2.EmitDifference(
                 diff1.NextGeneration,
@@ -1436,8 +1436,8 @@ public int F()
             var reader2 = diff2.GetMetadata().Reader;
 
             CheckNames(new[] { reader0, reader1, reader2 }, reader2.GetTypeDefNames(), "<>c__DisplayClass1#2_0#2");
-            CheckNames(new[] { reader0, reader1, reader2 }, reader2.GetMethodDefNames(), ".ctor", "F", "<F>b__1#2_1#2", ".ctor", "<F>b__2#2", "<F>b__1#2_0#2");
-            CheckNames(new[] { reader0, reader1, reader2 }, reader2.GetFieldDefNames(), "a", "<>9__1#2_0#2");
+            CheckNames(new[] { reader0, reader1, reader2 }, reader2.GetMethodDefNames(), ".ctor", "F", ".ctor", "<F>b__1#2", "<F>b__2#2", "<F>b__1#2_0#2");
+            CheckNames(new[] { reader0, reader1, reader2 }, reader2.GetFieldDefNames(), "<>4__this", "a", "<>9__1#2_0#2");
         }
 
         [Fact]
@@ -1525,8 +1525,8 @@ public int F<T>()
 
             var reader0 = md0.MetadataReader;
             CheckNames(reader0, reader0.GetTypeDefNames(), "<Module>", "C", "<>c__DisplayClass0_0`1", "<>c__0`1");
-            CheckNames(reader0, reader0.GetMethodDefNames(), "F", ".ctor", "<F>b__0_1", ".ctor", "<F>b__2", ".cctor", ".ctor", "<F>b__0_0");
-            CheckNames(reader0, reader0.GetFieldDefNames(), "a", "<>9", "<>9__0_0");
+            CheckNames(reader0, reader0.GetMethodDefNames(), "F", ".ctor", ".ctor", "<F>b__1", "<F>b__2", ".cctor", ".ctor", "<F>b__0_0");
+            CheckNames(reader0, reader0.GetFieldDefNames(), "<>4__this", "a", "<>9", "<>9__0_0");
 
             var generation0 = EmitBaseline.CreateInitialBaseline(md0, v0.CreateSymReader().GetEncMethodDebugInfo);
             var diff1 = compilation1.EmitDifference(
@@ -1537,13 +1537,13 @@ public int F<T>()
             var reader1 = diff1.GetMetadata().Reader;
 
             CheckNames(new[] { reader0, reader1 }, reader1.GetTypeDefNames(), "<>c__DisplayClass0#1_0#1`1", "<>c__0#1`1");
-            CheckNames(new[] { reader0, reader1 }, reader1.GetMethodDefNames(), "F", "<F>b__0#1_1#1", ".ctor", "<F>b__2#1", ".cctor", ".ctor", "<F>b__0#1_0#1");
-            CheckNames(new[] { reader0, reader1 }, reader1.GetFieldDefNames(), "a", "<>9", "<>9__0#1_0#1");
+            CheckNames(new[] { reader0, reader1 }, reader1.GetMethodDefNames(), "F", ".ctor", "<F>b__1#1", "<F>b__2#1", ".cctor", ".ctor", "<F>b__0#1_0#1");
+            CheckNames(new[] { reader0, reader1 }, reader1.GetFieldDefNames(), "<>4__this", "a", "<>9", "<>9__0#1_0#1");
 
             diff1.VerifySynthesizedMembers(
-                "C: {<F>b__0#1_1#1, <>c__DisplayClass0#1_0#1, <>c__0#1}",
                 "C.<>c__0#1<T>: {<>9__0#1_0#1, <F>b__0#1_0#1}",
-                "C.<>c__DisplayClass0#1_0#1<T>: {a, <F>b__2#1}");
+                "C: {<>c__DisplayClass0#1_0#1, <>c__0#1}",
+                "C.<>c__DisplayClass0#1_0#1<T>: {<>4__this, a, <F>b__1#1, <F>b__2#1}");
 
             var diff2 = compilation2.EmitDifference(
                 diff1.NextGeneration,
@@ -1553,8 +1553,8 @@ public int F<T>()
             var reader2 = diff2.GetMetadata().Reader;
 
             CheckNames(new[] { reader0, reader1, reader2 }, reader2.GetTypeDefNames(), "<>c__DisplayClass1#2_0#2`1", "<>c__1#2`1");
-            CheckNames(new[] { reader0, reader1, reader2 }, reader2.GetMethodDefNames(), "F", "<F>b__1#2_1#2", ".ctor", "<F>b__2#2", ".cctor", ".ctor", "<F>b__1#2_0#2");
-            CheckNames(new[] { reader0, reader1, reader2 }, reader2.GetFieldDefNames(), "a", "<>9", "<>9__1#2_0#2");
+            CheckNames(new[] { reader0, reader1, reader2 }, reader2.GetMethodDefNames(), "F", ".ctor", "<F>b__1#2", "<F>b__2#2", ".cctor", ".ctor", "<F>b__1#2_0#2");
+            CheckNames(new[] { reader0, reader1, reader2 }, reader2.GetFieldDefNames(), "<>4__this", "a", "<>9", "<>9__1#2_0#2");
         }
 
         [Fact]
@@ -1669,9 +1669,9 @@ public int F()
                     new SemanticEdit(SemanticEditKind.Update, main0, main1, preserveLocalVariables: true)));
 
             diff1.VerifySynthesizedMembers(
-                "C: {<F>b__1#1_1#1, <>c__DisplayClass1#1_0#1, <>c}",
                 "C.<>c: {<>9__1#1_0#1, <F>b__1#1_0#1}",
-                "C.<>c__DisplayClass1#1_0#1: {a, <F>b__2#1}");
+                "C.<>c__DisplayClass1#1_0#1: {<>4__this, a, <F>b__1#1, <F>b__2#1}",
+                "C: {<>c__DisplayClass1#1_0#1, <>c}");
 
             var diff2 = compilation2.EmitDifference(
                 diff1.NextGeneration,
@@ -1680,10 +1680,10 @@ public int F()
                     new SemanticEdit(SemanticEditKind.Update, main1, main2, preserveLocalVariables: true)));
 
             diff2.VerifySynthesizedMembers(
-                "C: {<F>b__1#2_1#2, <>c__DisplayClass1#2_0#2, <>c, <F>b__1#1_1#1, <>c__DisplayClass1#1_0#1}",
+                "C.<>c__DisplayClass1#2_0#2: {<>4__this, a, <F>b__1#2, <F>b__2#2}",
+                "C: {<>c__DisplayClass1#2_0#2, <>c, <>c__DisplayClass1#1_0#1}",
                 "C.<>c: {<>9__1#2_0#2, <F>b__1#2_0#2, <>9__1#1_0#1, <F>b__1#1_0#1}",
-                "C.<>c__DisplayClass1#1_0#1: {a, <F>b__2#1}",
-                "C.<>c__DisplayClass1#2_0#2: {a, <F>b__2#2}");
+                "C.<>c__DisplayClass1#1_0#1: {<>4__this, a, <F>b__1#1, <F>b__2#1}");
 
             var diff3 = compilation3.EmitDifference(
                 diff2.NextGeneration,
@@ -1691,10 +1691,10 @@ public int F()
                     new SemanticEdit(SemanticEditKind.Update, main2, main3, preserveLocalVariables: true)));
 
             diff3.VerifySynthesizedMembers(
-                "C: {<F>b__1#2_1#2, <>c__DisplayClass1#2_0#2, <>c, <F>b__1#1_1#1, <>c__DisplayClass1#1_0#1}",
+                "C.<>c__DisplayClass1#1_0#1: {<>4__this, a, <F>b__1#1, <F>b__2#1}",
                 "C.<>c: {<>9__1#2_0#2, <F>b__1#2_0#2, <>9__1#1_0#1, <F>b__1#1_0#1}",
-                "C.<>c__DisplayClass1#1_0#1: {a, <F>b__2#1}",
-                "C.<>c__DisplayClass1#2_0#2: {a, <F>b__2#2}");
+                "C.<>c__DisplayClass1#2_0#2: {<>4__this, a, <F>b__1#2, <F>b__2#2}",
+                "C: {<>c__DisplayClass1#2_0#2, <>c, <>c__DisplayClass1#1_0#1}");
         }
 
         [Fact]

src/Compilers/CSharp/Test/Emit/PDB/PDBLambdaTests.cs

@@ -537,50 +537,50 @@ public C(int a, int b) : base(() => a)
           <closure offset=""0"" />
           <lambda offset=""-2"" closure=""0"" />
           <lambda offset=""41"" closure=""0"" />
-          <lambda offset=""63"" closure=""this"" />
+          <lambda offset=""63"" closure=""0"" />
           <lambda offset=""87"" closure=""1"" />
         </encLambdaMap>
       </customDebugInfo>
       <sequencePoints>
         <entry offset=""0x0"" hidden=""true"" />
         <entry offset=""0x14"" startLine=""13"" startColumn=""30"" endLine=""13"" endColumn=""43"" />
-        <entry offset=""0x27"" hidden=""true"" />
-        <entry offset=""0x2d"" startLine=""14"" startColumn=""5"" endLine=""14"" endColumn=""6"" />
-        <entry offset=""0x2e"" startLine=""15"" startColumn=""9"" endLine=""15"" endColumn=""19"" />
-        <entry offset=""0x35"" startLine=""16"" startColumn=""9"" endLine=""16"" endColumn=""21"" />
-        <entry offset=""0x47"" startLine=""17"" startColumn=""9"" endLine=""17"" endColumn=""23"" />
-        <entry offset=""0x59"" startLine=""18"" startColumn=""9"" endLine=""18"" endColumn=""21"" />
-        <entry offset=""0x6b"" startLine=""19"" startColumn=""5"" endLine=""19"" endColumn=""6"" />
+        <entry offset=""0x2e"" hidden=""true"" />
+        <entry offset=""0x34"" startLine=""14"" startColumn=""5"" endLine=""14"" endColumn=""6"" />
+        <entry offset=""0x35"" startLine=""15"" startColumn=""9"" endLine=""15"" endColumn=""19"" />
+        <entry offset=""0x3c"" startLine=""16"" startColumn=""9"" endLine=""16"" endColumn=""21"" />
+        <entry offset=""0x4e"" startLine=""17"" startColumn=""9"" endLine=""17"" endColumn=""23"" />
+        <entry offset=""0x60"" startLine=""18"" startColumn=""9"" endLine=""18"" endColumn=""21"" />
+        <entry offset=""0x72"" startLine=""19"" startColumn=""5"" endLine=""19"" endColumn=""6"" />
       </sequencePoints>
-      <scope startOffset=""0x0"" endOffset=""0x6c"">
-        <local name=""CS$&lt;&gt;8__locals0"" il_index=""0"" il_start=""0x0"" il_end=""0x6c"" attributes=""0"" />
-        <scope startOffset=""0x27"" endOffset=""0x6c"">
-          <local name=""CS$&lt;&gt;8__locals1"" il_index=""1"" il_start=""0x27"" il_end=""0x6c"" attributes=""0"" />
+      <scope startOffset=""0x0"" endOffset=""0x73"">
+        <local name=""CS$&lt;&gt;8__locals0"" il_index=""0"" il_start=""0x0"" il_end=""0x73"" attributes=""0"" />
+        <scope startOffset=""0x2e"" endOffset=""0x73"">
+          <local name=""CS$&lt;&gt;8__locals1"" il_index=""1"" il_start=""0x2e"" il_end=""0x73"" attributes=""0"" />
         </scope>
       </scope>
     </method>
-    <method containingType=""C"" name=""&lt;.ctor&gt;b__3_2"">
+    <method containingType=""C+&lt;&gt;c__DisplayClass3_0"" name=""&lt;.ctor&gt;b__0"">
       <customDebugInfo>
         <forward declaringType=""B"" methodName="".ctor"" parameterNames=""f"" />
       </customDebugInfo>
       <sequencePoints>
-        <entry offset=""0x0"" startLine=""17"" startColumn=""19"" endLine=""17"" endColumn=""22"" />
+        <entry offset=""0x0"" startLine=""13"" startColumn=""41"" endLine=""13"" endColumn=""42"" />
       </sequencePoints>
     </method>
-    <method containingType=""C+&lt;&gt;c__DisplayClass3_0"" name=""&lt;.ctor&gt;b__0"">
+    <method containingType=""C+&lt;&gt;c__DisplayClass3_0"" name=""&lt;.ctor&gt;b__1"">
       <customDebugInfo>
         <forward declaringType=""B"" methodName="".ctor"" parameterNames=""f"" />
       </customDebugInfo>
       <sequencePoints>
-        <entry offset=""0x0"" startLine=""13"" startColumn=""41"" endLine=""13"" endColumn=""42"" />
+        <entry offset=""0x0"" startLine=""16"" startColumn=""19"" endLine=""16"" endColumn=""20"" />
       </sequencePoints>
     </method>
-    <method containingType=""C+&lt;&gt;c__DisplayClass3_0"" name=""&lt;.ctor&gt;b__1"">
+    <method containingType=""C+&lt;&gt;c__DisplayClass3_0"" name=""&lt;.ctor&gt;b__2"">
       <customDebugInfo>
         <forward declaringType=""B"" methodName="".ctor"" parameterNames=""f"" />
       </customDebugInfo>
       <sequencePoints>
-        <entry offset=""0x0"" startLine=""16"" startColumn=""19"" endLine=""16"" endColumn=""20"" />
+        <entry offset=""0x0"" startLine=""17"" startColumn=""19"" endLine=""17"" endColumn=""22"" />
       </sequencePoints>
     </method>
     <method containingType=""C+&lt;&gt;c__DisplayClass3_1"" name=""&lt;.ctor&gt;b__3"">

src/Compilers/Core/Portable/InternalUtilities/SetWithInsertionOrder.cs

@@ -37,6 +37,36 @@ public bool Add(T value)
             return true;
         }
 
+        public bool Insert(int index, T value)
+        {
+            if (_set == null)
+            {
+                if (index > 0)
+                {
+                    throw new IndexOutOfRangeException();
+                }
+                Add(value);
+            }
+            else
+            {
+                if (!_set.Add(value))
+                {
+                    return false;
+                }
+
+                try
+                {
+                    _elements.Insert(index, value);
+                }
+                catch
+                {
+                    _set.Remove(value);
+                    throw;
+                }
+            }
+            return true;
+        }
+
         public bool Remove(T value)
         {
             if (!_set.Remove(value))
@@ -57,5 +87,7 @@ public IEnumerator<T> GetEnumerator()
         IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
 
         public ImmutableArray<T> AsImmutable() => _elements.ToImmutableArrayOrEmpty();
+
+        public T this[int i] => _elements[i];
     }
 }

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/LocalFunctionTests.cs

@@ -123,7 +123,7 @@ int G()
   // Code size        7 (0x7)
   .maxstack  1
   .locals init (int V_0)
-  IL_0000:  ldarg.0
+  IL_0000:  ldarg.1
   IL_0001:  ldfld      ""C C.<>c__DisplayClass1_0.<>4__this""
   IL_0006:  ret
 }");
@@ -132,7 +132,7 @@ .locals init (int V_0)
   // Code size        7 (0x7)
   .maxstack  1
   .locals init (int V_0)
-  IL_0000:  ldarg.0
+  IL_0000:  ldarg.1
   IL_0001:  ldfld      ""int C.<>c__DisplayClass1_0.y""
   IL_0006:  ret
 }");
@@ -146,7 +146,7 @@ .locals init (int V_0)
   // Code size       13 (0xd)
   .maxstack  2
   .locals init (int V_0)
-  IL_0000:  ldarg.0
+  IL_0000:  ldarg.1
   IL_0001:  ldfld      ""C C.<>c__DisplayClass1_0.<>4__this""
   IL_0006:  ldc.i4.1
   IL_0007:  callvirt   ""void C.F(int)""