diff --git a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DynamicAnalysisInjector.cs b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DynamicAnalysisInjector.cs index 65589a913ad28dcfd1c52e1f49082c9457c57ae2..42037df50727cfa6d37b55615751f2c7e2cfaa82 100644 --- a/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DynamicAnalysisInjector.cs +++ b/src/Compilers/CSharp/Portable/Lowering/Instrumentation/DynamicAnalysisInjector.cs @@ -105,17 +105,24 @@ internal sealed class DynamicAnalysisInjector : CompoundInstrumenter _spansBuilder = ArrayBuilder.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) diff --git a/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs b/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs index 83ecc40f2de5b003a4a5e5400f05ece354a53b15..ed199db700be6d9bb493fc427261e28ac070adce 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs @@ -74,7 +74,7 @@ internal sealed class Analysis : BoundTreeWalkerWithStackGuardWithoutRecursionOn public readonly HashSet MethodsConvertedToDelegates = new HashSet(); /// - /// 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. /// 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; diff --git a/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs b/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs index 240104ba8a610db24856667e7b3a030a8b43e3ac..24b1cbb764ab0c488fa38f95da5acfeb83013ec7 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs @@ -333,9 +333,9 @@ private void MakeFrames(ArrayBuilder 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) 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); + 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); } // Move the body of the lambda to a freshly generated synthetic method on its frame. diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs index bbc02ec1f888caa376a56134d374411873806695..aa493b3cb1d6195159417bd2d3d5cc51f79d7785 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs +++ b/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.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.g__L22_1(ref C.<>c__DisplayClass2_1)"" + IL_000c: ret +}"); + // L2 + verifier.VerifyIL("C.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.g__L32_2(ref C.<>c__DisplayClass2_1)"" + IL_0007: ret +}"); + // Skip some... L5 + verifier.VerifyIL("C.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.g__L62_5(ref C.<>c__DisplayClass2_1, ref C.<>c__DisplayClass2_0)"" + IL_0007: pop + IL_0008: ret +}"); + // L6 + verifier.VerifyIL("C.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()