Fix bug in 'this' optimization for classes (#21510)

In the introduction of the new 'this' optimization routine,
one of the things which was changed was to treat 'this' more
like a formal parameter of the method, as opposed to a variable
living in an implicit, higher scope. This has some advantages
in simplicity for analysis, but created a problem when it came
to proxies. The current analysis builds the proxy list by walking
the tree and finding all captured variables and adding them
to the proxy dictionary keyed by the original variable symbol.
For instance, if a local variable is captured to a field, during
rewriting it will be added to the proxy list as (original symbol,
hoisted field). Since most symbols are only ever captured to a
single replacement field, this usually works fine -- all proxies
can exist side-by-side in the proxy list since there is no intersection.

However, this is not true for captured environment pointers. When
a new environment is introduced, a local will be created to point
to that environment. That local may itself be captured by nested
variables, creating a linked list from nested scopes to parent scope.
Most notably, *multiple* nested environments may capture the *same*
environment pointer in *different* hoisted fields. This means that
the proxies dictionary cannot hold all mappings at once, since the
mapping for a given captured environment pointer will depend on the
current scope.

The current code actually accounts for this already by adding a
captured environment pointer to the proxy list on a nested scope's
introduction, and removing it upon leaving that scope.

By changing 'this' to be treated like a formal parameter, I
circumvented this logic, introducing a bug. When two scopes tried
to capture the 'this' pointer, the compiler crashed due to trying
to add two mappings to the same key.

This change fixes this problem by treating the 'this' parameter
like an environment pointer for the purposes of capturing and
hoisting. It's possible that we want to treat it like a formal
parameter, but if so it's probably better to treat all captured
environment pointers the same way and introduce a scope-aware
notion of proxies, rather than having a global dictionary.

Fixes #21506

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

@@ -246,9 +246,13 @@ private void InlineThisOnlyEnvironments()
                 }
                 else
                 {
-                    // Class-based 'this' closures can move member functions
-                    // to the top-level type and environments which capture
-                    // the 'this' environment can capture 'this' directly
+                    // Class-based 'this' closures can move member functions to
+                    // the top-level type and environments which capture the 'this'
+                    // environment can capture 'this' directly.
+                    // Note: the top-level type is treated as the initial containing
+                    // environment, so by removing the 'this' environment, all
+                    // nested environments which captured a pointer to the 'this'
+                    // environment will now capture 'this'
                     RemoveEnv();
                     VisitClosures(ScopeTree, (scope, closure) =>
                     {
@@ -257,35 +261,6 @@ private void InlineThisOnlyEnvironments()
                             closure.ContainingEnvironmentOpt = null;
                         }
                     });
-
-                    // 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)
-                    {
-                        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;
-                                }
-                            }
-                        }
-                    }
                 }
 
                 void RemoveEnv()

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

@@ -187,8 +187,7 @@ public MappedLocalFunction(SynthesizedLambdaMethod symbol, ClosureKind closureKi
             _assignLocals = assignLocals;
             _currentTypeParameters = method.TypeParameters;
             _currentLambdaBodyTypeMap = TypeMap.Empty;
-            _innermostFramePointer = null;
-            _currentFrameThis = thisParameterOpt;
+            _innermostFramePointer = _currentFrameThis = thisParameterOpt;
             _framePointers[thisType] = thisParameterOpt;
             _seenBaseCall = method.MethodKind != MethodKind.Constructor; // only used for ctors
             _synthesizedFieldNameIdDispenser = 1;

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

@@ -30,6 +30,72 @@ public static IMethodSymbol FindLocalFunction(this CompilationVerifier verifier,
     [CompilerTrait(CompilerFeature.LocalFunctions)]
     public class CodeGenLocalFunctionTests : CSharpTestBase
     {
+        [Fact]
+        public void CaptureThisInDifferentScopes()
+        {
+            CompileAndVerify(@"
+using System;
+class C
+{
+    int _x;
+    void M()
+    {
+        {
+            int y = 0;
+            Func<int> f1 = () => _x + y;
+        }
+        {
+            int y = 0;
+            Func<int> f2 = () => _x + y;
+        }
+    }
+}");
+        }
+
+        [Fact]
+        public void CaptureThisInDifferentScopes2()
+        {
+            CompileAndVerify(@"
+using System;
+class C
+{
+    int _x;
+    void M()
+    {
+        {
+            int y = 0;
+            int L1() => _x + y;
+        }
+        {
+            int y = 0;
+            int L2() => _x + y;
+        }
+    }
+}");
+        }
+
+        [Fact]
+        public void CaptureFramePointerInDifferentScopes()
+        {
+            CompileAndVerify(@"
+using System;
+class C
+{
+    void M(int x)
+    {
+        Func<int> f1 = () => x;
+        {
+            int z = 0;
+            Func<int> f2 = () => x + z;
+        }
+        {
+            int z = 0;
+            Func<int> f3 = () => x + z;
+        }
+    }
+}");
+        }
+
         [Fact]
         public void EnvironmentChainContainsStructEnvironment()
         {