Merge pull request #20563 from dotnet/merges/dev15.3.x-to-master-20170630-070010

Merge dev15.3.x to master

src/Compilers/CSharp/Portable/Lowering/Instrumentation/DynamicAnalysisInjector.cs

@@ -105,17 +105,24 @@ internal sealed class DynamicAnalysisInjector : CompoundInstrumenter
             _spansBuilder = ArrayBuilder<SourceSpan>.GetInstance();
             TypeSymbol payloadElementType = methodBodyFactory.SpecialType(SpecialType.System_Boolean);
             _payloadType = ArrayTypeSymbol.CreateCSharpArray(methodBodyFactory.Compilation.Assembly, payloadElementType);
-            _methodPayload = methodBodyFactory.SynthesizedLocal(_payloadType, kind: SynthesizedLocalKind.InstrumentationPayload, syntax: methodBody.Syntax);
             _diagnostics = diagnostics;
             _debugDocumentProvider = debugDocumentProvider;
             _methodBodyFactory = methodBodyFactory;
 
+            // Set the factory context to generate nodes for the current method
+            var oldMethod = methodBodyFactory.CurrentMethod;
+            methodBodyFactory.CurrentMethod = method;
+
+            _methodPayload = methodBodyFactory.SynthesizedLocal(_payloadType, kind: SynthesizedLocalKind.InstrumentationPayload, syntax: methodBody.Syntax);
             // The first point indicates entry into the method and has the span of the method definition.
             SyntaxNode syntax = MethodDeclarationIfAvailable(methodBody.Syntax);
             if (!method.IsImplicitlyDeclared)
             {
                 _methodEntryInstrumentation = AddAnalysisPoint(syntax, SkipAttributes(syntax), methodBodyFactory);
             }
+
+            // Restore context
+            methodBodyFactory.CurrentMethod = oldMethod;
         }
 
         private static bool IsExcludedFromCodeCoverage(MethodSymbol method)

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

@@ -74,7 +74,7 @@ internal sealed class Analysis : BoundTreeWalkerWithStackGuardWithoutRecursionOn
             public readonly HashSet<MethodSymbol> MethodsConvertedToDelegates = new HashSet<MethodSymbol>();
 
             /// <summary>
-            /// True if the method signature can't be rewritten to contain ref/out parameters.
+            /// True if the method signature can be rewritten to contain ref/out parameters.
             /// </summary>
             public bool CanTakeRefParameters(MethodSymbol closure) => !(closure.IsAsync
                                                                         || closure.IsIterator
@@ -534,28 +534,43 @@ private BoundNode VisitLambdaOrFunction(IBoundLambdaOrFunction node)
 
             private void ReferenceVariable(SyntaxNode syntax, Symbol symbol)
             {
-                var localSymbol = symbol as LocalSymbol;
-                if ((object)localSymbol != null && localSymbol.IsConst)
+                if (symbol is LocalSymbol localSymbol && localSymbol.IsConst)
                 {
                     // "constant variables" need not be captured
                     return;
                 }
 
-                // using generic MethodSymbol here and not LambdaSymbol because of local functions
-                MethodSymbol lambda = _currentParent as MethodSymbol;
                 // "symbol == lambda" could happen if we're recursive
-                if ((object)lambda != null && symbol != lambda && symbol.ContainingSymbol != lambda)
+                if (_currentParent is MethodSymbol lambda && symbol != lambda && symbol.ContainingSymbol != lambda)
                 {
                     CapturedVariables.Add(symbol, syntax);
 
                     // mark the variable as captured in each enclosing lambda up to the variable's point of declaration.
-                    for (; (object)lambda != null && symbol != lambda && symbol.ContainingSymbol != lambda; lambda = lambda.ContainingSymbol as MethodSymbol)
+                    while ((object)lambda != null &&
+                           symbol != lambda &&
+                           symbol.ContainingSymbol != lambda &&
+                           // Necessary because the EE can insert non-closure synthesized method symbols
+                           IsClosure(lambda))
                     {
                         CapturedVariablesByLambda.Add(lambda, symbol);
+                        lambda = lambda.ContainingSymbol as MethodSymbol;
                     }
                 }
             }
 
+            private static bool IsClosure(MethodSymbol symbol)
+            {
+                switch (symbol.MethodKind)
+                {
+                    case MethodKind.LambdaMethod:
+                    case MethodKind.LocalFunction:
+                        return true;
+
+                    default:
+                        return false;
+                }
+            }
+
             private BoundNode VisitSyntaxWithReceiver(SyntaxNode syntax, BoundNode receiver)
             {
                 var previousSyntax = _syntaxWithReceiver;

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

@@ -333,9 +333,9 @@ private void MakeFrames(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
             foreach (var closure in closures)
             {
                 var capturedVars = _analysis.CapturedVariablesByLambda[closure];
+                bool canTakeRefParams = _analysis.CanTakeRefParameters(closure);
 
-                if (closure.MethodKind == MethodKind.LocalFunction &&
-                    OnlyCapturesThis((LocalFunctionSymbol)closure, capturedVars))
+                if (canTakeRefParams && OnlyCapturesThis((LocalFunctionSymbol)closure, capturedVars))
                 {
                     continue;
                 }
@@ -348,6 +348,17 @@ private void MakeFrames(ArrayBuilder<ClosureDebugInfo> closureDebugInfo)
                         continue;
                     }
 
+                    // If this is a local function that can take ref params, skip
+                    // frame creation for local function calls. This is semantically
+                    // important because otherwise we may create a struct frame which
+                    // is empty, which crashes in emit.
+                    // This is not valid for lambdas or local functions which can't take
+                    // take ref params since they will be lowered into their own frames.
+                    if (canTakeRefParams && captured.Kind == SymbolKind.Method)
+                    {
+                        continue;
+                    }
+
                     LambdaFrame frame = GetFrameForScope(scope, closureDebugInfo);
 
                     if (captured.Kind != SymbolKind.Method && !proxies.ContainsKey(captured))
@@ -1339,15 +1350,32 @@ private DebugId GetLambdaId(SyntaxNode syntax, ClosureKind closureKind, int clos
                     closureKind = ClosureKind.Static;
                     closureOrdinal = LambdaDebugInfo.StaticClosureOrdinal;
                 }
-                structClosures = default(ImmutableArray<TypeSymbol>);
+                structClosures = default;
             }
             else
             {
+                // GetStructClosures is currently overly aggressive in gathering
+                // closures since the only information it has at this point is
+                // NeedsParentFrame, which doesn't say what exactly is needed from
+                // the parent frame. If `this` is captured, that's enough to mark
+                // NeedsParentFrame for the current closure, so we need to gather
+                // struct closures for all intermediate frames, even if they only
+                // strictly need `this`.
+                if (_analysis.CanTakeRefParameters(node.Symbol))
+                {
+                    lambdaScope = _analysis.ScopeParent[node.Body];
+                    _ = _frames.TryGetValue(lambdaScope, out containerAsFrame);
+                    structClosures = GetStructClosures(containerAsFrame, lambdaScope);
+                }
+                else
+                {
+                    structClosures = default;
+                }
+
                 containerAsFrame = null;
                 translatedLambdaContainer = _topLevelMethod.ContainingType;
                 closureKind = ClosureKind.ThisOnly;
                 closureOrdinal = LambdaDebugInfo.ThisOnlyClosureOrdinal;
-                structClosures = default(ImmutableArray<TypeSymbol>);
             }
 
             // Move the body of the lambda to a freshly generated synthetic method on its frame.

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

@@ -30,6 +30,189 @@ public static IMethodSymbol FindLocalFunction(this CompilationVerifier verifier,
     [CompilerTrait(CompilerFeature.LocalFunctions)]
     public class CodeGenLocalFunctionTests : CSharpTestBase
     {
+        [Fact]
+        [WorkItem(18814, "https://github.com/dotnet/roslyn/issues/18814")]
+        [WorkItem(18918, "https://github.com/dotnet/roslyn/issues/18918")]
+        public void IntermediateStructClosures1()
+        {
+            var verifier = CompileAndVerify(@"
+using System;
+class C
+{
+    int _x = 0;
+
+    public static void Main() => new C().M();
+
+    public void M()
+    {
+        int var1 = 0;
+        void L1()
+        {
+            void L2()
+            {
+                void L3()
+                {   
+                    void L4()
+                    {
+                        int var2 = 0;
+                        void L5()
+                        {
+                            int L6() => var2 + _x++;
+                            L6();
+                        }
+                        L5();
+                    }
+                    L4();
+                }
+                L3();
+            }
+            L2();
+            int L8() => var1;
+        }
+        Console.WriteLine(_x);
+        L1();
+        Console.WriteLine(_x);
+    }
+}", expectedOutput: 
+@"0
+1");
+            // L1
+            verifier.VerifyIL("C.<M>g__L12_0(ref C.<>c__DisplayClass2_1)", @"
+{
+  // Code size       13 (0xd)
+  .maxstack  2
+  IL_0000:  ldarg.0
+  IL_0001:  ldfld      ""C C.<>c__DisplayClass2_1.<>4__this""
+  IL_0006:  ldarg.0
+  IL_0007:  call       ""void C.<M>g__L22_1(ref C.<>c__DisplayClass2_1)""
+  IL_000c:  ret
+}");
+            // L2
+            verifier.VerifyIL("C.<M>g__L22_1(ref C.<>c__DisplayClass2_1)", @"
+{
+  // Code size        8 (0x8)
+  .maxstack  2
+  IL_0000:  ldarg.0
+  IL_0001:  ldarg.1
+  IL_0002:  call       ""void C.<M>g__L32_2(ref C.<>c__DisplayClass2_1)""
+  IL_0007:  ret
+}");
+            // Skip some... L5
+            verifier.VerifyIL("C.<M>g__L52_4(ref C.<>c__DisplayClass2_1, ref C.<>c__DisplayClass2_0)", @"
+{
+  // Code size        9 (0x9)
+  .maxstack  2
+  IL_0000:  ldarg.0
+  IL_0001:  ldarg.1
+  IL_0002:  call       ""int C.<M>g__L62_5(ref C.<>c__DisplayClass2_1, ref C.<>c__DisplayClass2_0)""
+  IL_0007:  pop
+  IL_0008:  ret
+}");
+            // L6
+            verifier.VerifyIL("C.<M>g__L62_5(ref C.<>c__DisplayClass2_1, ref C.<>c__DisplayClass2_0)", @"
+{
+  // Code size       35 (0x23)
+  .maxstack  4
+  .locals init (int V_0)
+  IL_0000:  ldarg.1
+  IL_0001:  ldfld      ""int C.<>c__DisplayClass2_0.var2""
+  IL_0006:  ldarg.1
+  IL_0007:  ldfld      ""C C.<>c__DisplayClass2_0.<>4__this""
+  IL_000c:  ldarg.1
+  IL_000d:  ldfld      ""C C.<>c__DisplayClass2_0.<>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
+}");
+        }
+
+        [Fact]
+        [WorkItem(18814, "https://github.com/dotnet/roslyn/issues/18814")]
+        [WorkItem(18918, "https://github.com/dotnet/roslyn/issues/18918")]
+        public void IntermediateStructClosures2()
+        {
+            CompileAndVerify(@"
+class C
+{
+    int _x;
+    void M()
+    {
+        int y = 0;
+        void L1()
+        {
+            void L2()
+            {
+                int z = 0;
+                int L3() => z + _x;
+            }
+            y++;
+        }
+    }
+}");
+        }
+
+        [Fact]
+        [WorkItem(18814, "https://github.com/dotnet/roslyn/issues/18814")]
+        public void Repro18814()
+        {
+            CompileAndVerify(@"
+class Program
+{
+    private void ResolvingPackages()
+    {
+        string outerScope(int a) => """";
+
+        void C1(int cabinetIdx)
+        {
+            void modifyState()
+            {
+                var no = outerScope(cabinetIdx);
+            }
+
+            modifyState();
+        }
+    }
+}");
+        }
+
+        [Fact]
+        [WorkItem(18918, "https://github.com/dotnet/roslyn/issues/18918")]
+        public void Repro18918()
+        {
+            CompileAndVerify(@"
+public class Test
+{
+    private int _field;
+
+    public void OuterMethod(int outerParam)
+    {
+        void InnerMethod1()
+        {
+            void InnerInnerMethod(int innerInnerParam)
+            {
+                InnerInnerInnerMethod();
+                
+                bool InnerInnerInnerMethod()
+                {
+                    return innerInnerParam != _field;
+                }
+            }
+
+            void InnerMethod2()
+            {
+                var temp = outerParam;
+            }  
+        }
+    }
+}");
+        }
+
         [Fact]
         [WorkItem(17719, "https://github.com/dotnet/roslyn/issues/17719")]
         public void Repro17719()