From e7f0317e6a38a9fdcbc3f89105e474e1741ed196 Mon Sep 17 00:00:00 2001 From: Neal Gafter Date: Fri, 26 Jul 2019 16:15:18 -0400 Subject: [PATCH] Fix region tracking for regions with local functions (#37450) * Fix region tracking for regions with local functions Fixes #37427 * Add to test pre review comment. --- .../Portable/FlowAnalysis/AbstractFlowPass.cs | 2 + .../AbstractRegionDataFlowPass.cs | 2 +- .../FlowAnalysis/DataFlowsOutWalker.cs | 6 ++ .../FlowAnalysis/DefiniteAssignment.cs | 49 ++++++++------ .../FlowAnalysis/RegionAnalysisTests.cs | 65 ++++++++++++++++++- 5 files changed, 100 insertions(+), 24 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index c1206ca1db6..1ea109f35f6 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -202,6 +202,8 @@ protected void Unsplit() _nonMonotonicTransfer = nonMonotonicTransferFunction; } + protected bool TrackingRegions => _trackRegions; + protected abstract string Dump(TLocalState state); protected string Dump() diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractRegionDataFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractRegionDataFlowPass.cs index 046c581317e..757dd790735 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractRegionDataFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractRegionDataFlowPass.cs @@ -7,7 +7,7 @@ namespace Microsoft.CodeAnalysis.CSharp { - // Note: this code has a copy-and-paste sibling in AbstractRegionDataFlowPass. + // Note: this code has a copy-and-paste sibling in AbstractRegionControlFlowPass. // Any fix to one should be applied to the other. internal abstract class AbstractRegionDataFlowPass : DefiniteAssignmentPass { diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowsOutWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowsOutWalker.cs index c9a07fcd6c5..8d2d476f700 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowsOutWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowsOutWalker.cs @@ -58,6 +58,12 @@ private HashSet Analyze(ref bool badRegion) return _dataFlowsOut; } + protected override ImmutableArray Scan(ref bool badRegion) + { + _dataFlowsOut.Clear(); + return base.Scan(ref badRegion); + } + protected override void EnterRegion() { // to handle loops properly, we must assume that every variable that flows in is diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index 1a91292b0f0..7e8082a80ea 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -1477,23 +1477,23 @@ public override BoundNode VisitBlock(BoundBlock node) private void VisitStatementsWithLocalFunctions(BoundBlock block) { - // Visit the statements in two phases: - // 1. Local function declarations - // 2. Everything else - // - // The idea behind visiting local functions first is - // that we may be able to gather the captured variables - // they read and write ahead of time in a single pass, so - // when they are used by other statements in the block we - // won't have to recompute the set by doing multiple passes. - // - // If the local functions contain forward calls to other local - // functions then we may have to do another pass regardless, - // but hopefully that will be an uncommon case in real-world code. - - // First phase - if (!block.LocalFunctions.IsDefaultOrEmpty) - { + if (!TrackingRegions && !block.LocalFunctions.IsDefaultOrEmpty) + { + // Visit the statements in two phases: + // 1. Local function declarations + // 2. Everything else + // + // The idea behind visiting local functions first is + // that we may be able to gather the captured variables + // they read and write ahead of time in a single pass, so + // when they are used by other statements in the block we + // won't have to recompute the set by doing multiple passes. + // + // If the local functions contain forward calls to other local + // functions then we may have to do another pass regardless, + // but hopefully that will be an uncommon case in real-world code. + + // First phase foreach (var stmt in block.Statements) { if (stmt.Kind == BoundKind.LocalFunctionStatement) @@ -1501,12 +1501,19 @@ private void VisitStatementsWithLocalFunctions(BoundBlock block) VisitAlways(stmt); } } - } - // Second phase - foreach (var stmt in block.Statements) + // Second phase + foreach (var stmt in block.Statements) + { + if (stmt.Kind != BoundKind.LocalFunctionStatement) + { + VisitStatement(stmt); + } + } + } + else { - if (stmt.Kind != BoundKind.LocalFunctionStatement) + foreach (var stmt in block.Statements) { VisitStatement(stmt); } diff --git a/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs b/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs index 625c530bf15..bca4e6f65bc 100644 --- a/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs @@ -10,13 +10,16 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests { /// + /// Tests for the region analysis APIs. + /// + /// /// Please add your tests to other files if possible: /// * FlowDiagnosticTests.cs - all tests on Diagnostics /// * IterationJumpYieldStatementTests.cs - while, do, for, foreach, break, continue, goto, iterator (yield break, yield return) /// * TryLockUsingStatementTests.cs - try-catch-finally, lock, & using statement /// * PatternsVsRegions.cs - region analysis tests for pattern matching - /// - public partial class FlowAnalysisTests : FlowTestBase + /// + public partial class RegionAnalysisTests : FlowTestBase { #region "Expressions" @@ -6360,6 +6363,64 @@ void Method(out object test) Assert.Equal("this, test, a", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenOutside)); } + [Fact, WorkItem(37427, "https://github.com/dotnet/roslyn/issues/37427")] + public void RegionWithLocalFunctions() + { + // local functions inside the region + var s1 = @" +class A +{ + static void M(int p) + { + int i, j; + i = 1; + /**/ + int L1() => 1; + int k; + j = i; + int L2() => 2; + /**/ + k = j; + } +} +"; + // local functions outside the region + var s2 = @" +class A +{ + static void M(int p) + { + int i, j; + i = 1; + int L1() => 1; + /**/ + int k; + j = i; + /**/ + int L2() => 2; + k = j; + } +} +"; + foreach (var s in new[] { s1, s2 }) + { + var analysisResults = CompileAndAnalyzeDataFlowStatements(s); + var dataFlowAnalysisResults = analysisResults; + Assert.True(dataFlowAnalysisResults.Succeeded); + Assert.Equal("k", GetSymbolNamesJoined(dataFlowAnalysisResults.VariablesDeclared)); + Assert.Equal("j", GetSymbolNamesJoined(dataFlowAnalysisResults.AlwaysAssigned)); + Assert.Equal(null, GetSymbolNamesJoined(dataFlowAnalysisResults.Captured)); + Assert.Equal(null, GetSymbolNamesJoined(dataFlowAnalysisResults.CapturedInside)); + Assert.Equal(null, GetSymbolNamesJoined(dataFlowAnalysisResults.CapturedOutside)); + Assert.Equal("i", GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsIn)); + Assert.Equal("j", GetSymbolNamesJoined(dataFlowAnalysisResults.DataFlowsOut)); + Assert.Equal("i", GetSymbolNamesJoined(dataFlowAnalysisResults.ReadInside)); + Assert.Equal("j", GetSymbolNamesJoined(dataFlowAnalysisResults.ReadOutside)); + Assert.Equal("j", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenInside)); + Assert.Equal("p, i, k", GetSymbolNamesJoined(dataFlowAnalysisResults.WrittenOutside)); + } + } + #endregion } } -- GitLab