diff --git a/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs b/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs index b06955cf638bad93689f11cba98fccb1e4f4688f..28790d750e1f899201e6419de6fa0f5902cb99b9 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.Tree.cs @@ -7,7 +7,6 @@ using Microsoft.CodeAnalysis.CSharp.Symbols; using Microsoft.CodeAnalysis.PooledObjects; using Roslyn.Utilities; -using System.Linq; namespace Microsoft.CodeAnalysis.CSharp { @@ -40,7 +39,13 @@ public sealed class Scope /// in this scope or nested scopes. "Declared" refers to the start of the variable /// lifetime (which, at this point in lowering, should be equivalent to lexical scope). /// - public ArrayBuilder DeclaredVariables { get; } = ArrayBuilder.GetInstance(); + /// + /// It's important that this is a set and that enumeration order is deterministic. We loop + /// over this list to generate proxies and if we loop out of order this will cause + /// non-deterministic compilation, and if we generate duplicate proxies we'll generate + /// wasteful code in the best case and incorrect code in the worst. + /// + public SetWithInsertionOrder DeclaredVariables { get; } = new SetWithInsertionOrder(); /// /// The bound node representing this scope. This roughly corresponds to the bound @@ -54,7 +59,7 @@ public sealed class Scope /// The closure that this scope is nested inside. Null if this scope is not nested /// inside a closure. /// - public Closure ContainingClosure { get; } + public Closure ContainingClosureOpt { get; } public Scope(Scope parent, BoundNode boundNode, Closure containingClosure) { @@ -62,7 +67,7 @@ public Scope(Scope parent, BoundNode boundNode, Closure containingClosure) Parent = parent; BoundNode = boundNode; - ContainingClosure = containingClosure; + ContainingClosureOpt = containingClosure; } public void Free() @@ -78,7 +83,6 @@ public void Free() closure.Free(); } Closures.Free(); - DeclaredVariables.Free(); } public override string ToString() => BoundNode.Syntax.GetText().ToString(); @@ -118,7 +122,7 @@ public void Free() /// Optimizes local functions such that if a local function only references other local functions /// that capture no variables, we don't need to create capture environments for any of them. /// - private void RemoveUnneededReferences() + private void RemoveUnneededReferences(ParameterSymbol thisParam) { var methodGraph = new MultiDictionary(); var capturesThis = new HashSet(); @@ -132,7 +136,7 @@ private void RemoveUnneededReferences() { methodGraph.Add(localFunc, closure.OriginalMethodSymbol); } - else if (capture == _topLevelMethod.ThisParameter) + else if (capture == thisParam) { if (capturesThis.Add(closure.OriginalMethodSymbol)) { @@ -168,7 +172,7 @@ private void RemoveUnneededReferences() } if (capturesThis.Contains(closure.OriginalMethodSymbol)) { - closure.CapturedVariables.Add(_topLevelMethod.ThisParameter); + closure.CapturedVariables.Add(thisParam); } }); } @@ -191,7 +195,7 @@ public static void VisitClosures(Scope scope, Action action) /// /// Builds a tree of nodes corresponding to a given method. - /// visits the bound tree and translates + /// visits the bound tree and translates /// information from the bound tree about variable scope, declared variables, and /// variable captures into the resulting tree. /// @@ -211,32 +215,53 @@ private class ScopeTreeBuilder : BoundTreeWalkerWithStackGuardWithoutRecursionOn /// private readonly SmallDictionary _localToScope = new SmallDictionary(); - private readonly Analysis _analysis; + private readonly MethodSymbol _topLevelMethod; - private ScopeTreeBuilder(Scope rootScope, Analysis analysis) + private bool _sawClosure = false; + + /// + /// If a local function is in the set, at some point in the code it is converted + /// to a delegate and should then not be optimized to a struct closure. + /// Also contains all lambdas (as they are converted to delegates implicitly). + /// + private readonly HashSet _methodsConvertedToDelegates; + + private ScopeTreeBuilder( + Scope rootScope, + MethodSymbol topLevelMethod, + HashSet methodsConvertedToDelegates) { Debug.Assert(rootScope != null); - Debug.Assert(analysis != null); + Debug.Assert(topLevelMethod != null); + Debug.Assert(methodsConvertedToDelegates != null); _currentScope = rootScope; - _analysis = analysis; + _topLevelMethod = topLevelMethod; + _methodsConvertedToDelegates = methodsConvertedToDelegates; } - public static Scope Build(BoundNode node, Analysis analysis) + public static Scope Build( + BoundNode node, + MethodSymbol topLevelMethod, + HashSet methodsConvertedToDelegates) { // This should be the top-level node Debug.Assert(node == FindNodeToAnalyze(node)); + Debug.Assert(topLevelMethod != null); var rootScope = new Scope(parent: null, boundNode: node, containingClosure: null); - var builder = new ScopeTreeBuilder(rootScope, analysis); + var builder = new ScopeTreeBuilder( + rootScope, + topLevelMethod, + methodsConvertedToDelegates); builder.Build(); - return rootScope; + return builder._sawClosure ? rootScope : null; } private void Build() { // Set up the current method locals - DeclareLocals(_currentScope, _analysis._topLevelMethod.Parameters); + DeclareLocals(_currentScope, _topLevelMethod.Parameters); Visit(_currentScope.BoundNode); } @@ -260,6 +285,11 @@ public override BoundNode VisitBlock(BoundBlock node) public override BoundNode VisitCatchBlock(BoundCatchBlock node) { + if (node.Locals.IsDefaultOrEmpty) + { + return base.VisitCatchBlock(node); + } + var oldScope = _currentScope; _currentScope = CreateOrReuseScope(node, node.Locals); var result = base.VisitCatchBlock(node); @@ -269,6 +299,11 @@ public override BoundNode VisitCatchBlock(BoundCatchBlock node) public override BoundNode VisitSequence(BoundSequence node) { + if (node.Locals.IsDefaultOrEmpty) + { + return base.VisitSequence(node); + } + var oldScope = _currentScope; _currentScope = CreateOrReuseScope(node, node.Locals); var result = base.VisitSequence(node); @@ -296,7 +331,7 @@ public override BoundNode VisitLambda(BoundLambda node) var oldInExpressionTree = _inExpressionTree; _inExpressionTree |= node.Type.IsExpressionTree(); - _analysis.MethodsConvertedToDelegates.Add(node.Symbol.OriginalDefinition); + _methodsConvertedToDelegates.Add(node.Symbol.OriginalDefinition); var result = VisitClosure(node.Symbol, node.Body); _inExpressionTree = oldInExpressionTree; @@ -323,7 +358,7 @@ public override BoundNode VisitDelegateCreationExpression(BoundDelegateCreationE // Use OriginalDefinition to strip generic type parameters var method = node.MethodOpt.OriginalDefinition; AddIfCaptured(method); - _analysis.MethodsConvertedToDelegates.Add(method); + _methodsConvertedToDelegates.Add(method); } return base.VisitDelegateCreationExpression(node); } @@ -342,13 +377,13 @@ public override BoundNode VisitLocal(BoundLocal node) public override BoundNode VisitBaseReference(BoundBaseReference node) { - AddIfCaptured(_analysis._topLevelMethod.ThisParameter); + AddIfCaptured(_topLevelMethod.ThisParameter); return base.VisitBaseReference(node); } public override BoundNode VisitThisReference(BoundThisReference node) { - var thisParam = _analysis._topLevelMethod.ThisParameter; + var thisParam = _topLevelMethod.ThisParameter; if (thisParam != null) { AddIfCaptured(thisParam); @@ -373,6 +408,8 @@ private BoundNode VisitClosure(MethodSymbol closureSymbol, BoundBlock body) { Debug.Assert((object)closureSymbol != null); + _sawClosure |= true; + // Closure is declared (lives) in the parent scope, but its // variables are in a nested scope var closure = new Closure(closureSymbol); @@ -426,11 +463,11 @@ private void AddIfCaptured(Symbol symbol) closure.CapturedVariables.Add(symbol); // Also mark captured in enclosing scopes - while (scope.ContainingClosure == closure) + while (scope.ContainingClosureOpt == closure) { scope = scope.Parent; } - closure = scope.ContainingClosure; + closure = scope.ContainingClosureOpt; } // Also record where the captured variable lives diff --git a/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs b/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs index c89d9dee0294009c0e8c9b1a08be83acb7898014..1d98fdb31c88562fec5134819ec72faa7f5fdad0 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.cs @@ -28,11 +28,6 @@ internal sealed partial class Analysis : BoundTreeWalkerWithStackGuardWithoutRec /// private bool _inExpressionLambda; - /// - /// Set to true of any lambda expressions were seen in the analyzed method body. - /// - public bool SeenLambda { get; private set; } - /// /// For each scope that defines variables, identifies the nearest enclosing scope that defines variables. /// @@ -116,12 +111,15 @@ public static Analysis Analyze(BoundNode node, MethodSymbol method) { var analysis = new Analysis(method); analysis.Analyze(node); - return analysis; + return analysis.ScopeTree == null ? null : analysis; } private void Analyze(BoundNode node) { - ScopeTree = ScopeTreeBuilder.Build(node, this); + ScopeTree = ScopeTreeBuilder.Build( + node, + _topLevelMethod, + MethodsConvertedToDelegates); _currentScope = FindNodeToAnalyze(node); Debug.Assert(!_inExpressionLambda); @@ -181,9 +179,9 @@ private static BoundNode FindNodeToAnalyze(BoundNode node) /// /// Create the optimized plan for the location of lambda methods and whether scopes need access to parent scopes /// - internal void ComputeLambdaScopesAndFrameCaptures() + internal void ComputeLambdaScopesAndFrameCaptures(ParameterSymbol thisParam) { - RemoveUnneededReferences(); + RemoveUnneededReferences(thisParam); LambdaScopes = new Dictionary(ReferenceEqualityComparer.Instance); NeedsParentFrame = new HashSet(); @@ -284,7 +282,76 @@ void RecordClosureScope(Scope innermost, Scope outermost, Closure closure) } } } + } + } + + /// + /// Walk up the scope tree looking for a variable declaration. + /// + public static Scope GetVariableDeclarationScope(Scope startingScope, Symbol variable) + { + if (variable is ParameterSymbol p && p.IsThis) + { + return null; + } + + var currentScope = startingScope; + while (currentScope != null) + { + if (variable.Kind == SymbolKind.Parameter || variable.Kind == SymbolKind.Local) + { + if (currentScope.DeclaredVariables.Contains(variable)) + { + return currentScope; + } + } + else + { + if (currentScope.Closures.Contains(c => variable == c.OriginalMethodSymbol)) + { + return currentScope; + } + } + currentScope = currentScope.Parent; + } + return null; + } + + /// + /// Find the parent of the corresponding to + /// the given . + /// + public static Scope GetScopeParent(Scope treeRoot, BoundNode scopeNode) + { + var correspondingScope = GetScopeWithMatchingBoundNode(treeRoot, scopeNode); + return correspondingScope.Parent; + } + + /// + /// Finds a with a matching + /// as the one given. + /// + public static Scope GetScopeWithMatchingBoundNode(Scope treeRoot, BoundNode node) + { + return Helper(treeRoot) ?? throw ExceptionUtilities.Unreachable; + + Scope Helper(Scope currentScope) + { + if (currentScope.BoundNode == node) + { + return currentScope; + } + + foreach (var nestedScope in currentScope.NestedScopes) + { + var found = Helper(nestedScope); + if (found != null) + { + return found; + } + } + return null; } } @@ -488,7 +555,6 @@ public override BoundNode VisitLocalFunctionStatement(BoundLocalFunctionStatemen private BoundNode VisitLambdaOrFunction(IBoundLambdaOrFunction node) { Debug.Assert((object)node.Symbol != null); - SeenLambda = true; var oldParent = _currentParent; var oldBlock = _currentScope; _currentParent = node.Symbol; diff --git a/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs b/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs index 6228f785c567ab49aa0817c40b2c5602ec08cafd..189196ac0394e4f3394d37e2f8fcd785b01a6e06 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs @@ -228,13 +228,8 @@ protected override bool NeedsProxy(Symbol localOrParameter) Debug.Assert(compilationState.ModuleBuilderOpt != null); var analysis = Analysis.Analyze(loweredBody, method); - if (!analysis.SeenLambda) + if (analysis == null) { - // Unreachable anonymous functions are ignored by the analyzer. - // No closures or lambda methods are generated. - // E.g. - // int y = 0; - // var b = false && (from z in new X(y) select f(z + y)) return loweredBody; } @@ -252,7 +247,7 @@ protected override bool NeedsProxy(Symbol localOrParameter) diagnostics, assignLocals); - analysis.ComputeLambdaScopesAndFrameCaptures(); + analysis.ComputeLambdaScopesAndFrameCaptures(thisParameter); rewriter.MakeFrames(closureDebugInfoBuilder); // First, lower everything but references (calls, delegate conversions) @@ -341,8 +336,14 @@ private void MakeFrames(ArrayBuilder closureDebugInfo) foreach (var captured in capturedVars) { - if (!_analysis.VariableScope.TryGetValue(captured, out BoundNode captureScope)) + var declarationScope = Analysis.GetVariableDeclarationScope(scope, captured); + if (_analysis.VariableScope.TryGetValue(captured, out BoundNode captureScope)) { + Debug.Assert(declarationScope.BoundNode == captureScope); + } + else + { + Debug.Assert(declarationScope == null); continue; } @@ -357,7 +358,7 @@ private void MakeFrames(ArrayBuilder closureDebugInfo) continue; } - LambdaFrame frame = GetFrameForScope(captureScope, closureDebugInfo); + LambdaFrame frame = GetFrameForScope(declarationScope, closureDebugInfo); if (captured.Kind != SymbolKind.Method && !proxies.ContainsKey(captured)) { @@ -450,26 +451,28 @@ private void MakeFrames(ArrayBuilder closureDebugInfo) return result; } - private LambdaFrame GetFrameForScope(BoundNode scope, ArrayBuilder closureDebugInfo) + private LambdaFrame GetFrameForScope(Analysis.Scope scope, ArrayBuilder closureDebugInfo) { + var scopeBoundNode = scope.BoundNode; LambdaFrame frame; - if (!_frames.TryGetValue(scope, out frame)) + if (!_frames.TryGetValue(scopeBoundNode, out frame)) { - var syntax = scope.Syntax; + var syntax = scopeBoundNode.Syntax; Debug.Assert(syntax != null); DebugId methodId = GetTopLevelMethodId(); DebugId closureId = GetClosureId(syntax, closureDebugInfo); - var canBeStruct = !_analysis.ScopesThatCantBeStructs.Contains(scope); + var canBeStruct = !_analysis.ScopesThatCantBeStructs.Contains(scopeBoundNode); - var containingMethod = _analysis.ScopeOwner[scope]; + var containingMethod = scope.ContainingClosureOpt?.OriginalMethodSymbol ?? _topLevelMethod; + Debug.Assert(containingMethod == _analysis.ScopeOwner[scopeBoundNode]); if (_substitutedSourceMethod != null && containingMethod == _topLevelMethod) { containingMethod = _substitutedSourceMethod; } frame = new LambdaFrame(_topLevelMethod, containingMethod, canBeStruct, syntax, methodId, closureId); - _frames.Add(scope, frame); + _frames.Add(scopeBoundNode, frame); CompilationState.ModuleBuilderOpt.AddSynthesizedDefinition(ContainingType, frame); if (frame.Constructor != null) @@ -701,14 +704,20 @@ private BoundNode IntroduceFrame(BoundNode node, LambdaFrame frame, Func.GetInstance(); foreach (var variable in _analysis.CapturedVariables.Keys) { - BoundNode varNode; - if (!_analysis.VariableScope.TryGetValue(variable, out varNode) || varNode != node) + if (variable.Kind != SymbolKind.Method && + _analysis.VariableScope.TryGetValue(variable, out var varNode) && + varNode == node) { - continue; + variablesInThisScope.Add(variable); } - + } + Debug.Assert(variablesInThisScope.SequenceEqual(scope.DeclaredVariables)); + foreach (var variable in variablesInThisScope) + { InitVariableProxy(syntax, variable, framePointer, prologue); } @@ -1360,7 +1369,8 @@ private DebugId GetLambdaId(SyntaxNode syntax, ClosureKind closureKind, int clos // strictly need `this`. if (_analysis.CanTakeRefParameters(node.Symbol)) { - lambdaScope = _analysis.ScopeParent[node.Body]; + lambdaScope = Analysis.GetScopeParent(_analysis.ScopeTree, node.Body)?.BoundNode; + Debug.Assert(lambdaScope == _analysis.ScopeParent[node.Body]); _ = _frames.TryGetValue(lambdaScope, out containerAsFrame); structClosures = GetStructClosures(containerAsFrame, lambdaScope); } @@ -1468,12 +1478,18 @@ private ImmutableArray GetStructClosures(LambdaFrame containerAsFram bool FindParentFrame(ref LambdaFrame container, ref BoundNode scope) { - while (_analysis.ScopeParent.TryGetValue(scope, out scope)) + var temp = Analysis.GetScopeParent(_analysis.ScopeTree, scope)?.BoundNode; + Debug.Assert(temp == null || _analysis.ScopeParent.TryGetValue(scope, out var tempScope) && tempScope == temp); + scope = temp; + while (scope != null) { if (_frames.TryGetValue(scope, out container)) { return true; } + temp = Analysis.GetScopeParent(_analysis.ScopeTree, scope)?.BoundNode; + Debug.Assert(temp == null || _analysis.ScopeParent.TryGetValue(scope, out tempScope) && tempScope == temp); + scope = temp; } return false; } @@ -1552,8 +1568,9 @@ private BoundNode RewriteLambdaConversion(BoundLambda node) // NOTE: We require "lambdaScope != null". // We do not want to introduce a field into an actual user's class (not a synthetic frame). + Debug.Assert(Analysis.GetScopeParent(_analysis.ScopeTree, node.Body).BoundNode == _analysis.ScopeParent[node.Body]); var shouldCacheInLoop = lambdaScope != null && - lambdaScope != _analysis.ScopeParent[node.Body] && + lambdaScope != Analysis.GetScopeParent(_analysis.ScopeTree, node.Body).BoundNode && InLoopOrLambda(node.Syntax, lambdaScope.Syntax); if (shouldCacheForStaticMethod || shouldCacheInLoop)