From c031a00f3871a6606e79a45ed209d8174c39bde9 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Thu, 12 Dec 2024 17:03:40 -0800 Subject: [PATCH 1/7] Call Enter/LeaveRegion APIs for binary expressions --- .../Portable/FlowAnalysis/AbstractFlowPass.cs | 4 +++ .../Emit3/FlowAnalysis/RegionAnalysisTests.cs | 31 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index fb8ad0ddae840..1c1410b380860 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -2553,6 +2553,7 @@ private void VisitBinaryOperatorChildren(BoundBinaryOperator node) protected virtual void VisitBinaryOperatorChildren(ArrayBuilder stack) { var binary = stack.Pop(); + EnterRegionIfNeeded(binary); // Only the leftmost operator of a left-associative binary operator chain can learn from a conditional access on the left // For simplicity, we just special case it here. @@ -2592,6 +2593,7 @@ protected virtual void VisitBinaryOperatorChildren(ArrayBuilder().ToArray()[1]; + Assert.Equal("int k = i + j + 1;", intK.ToString()); + var flowAnalysis1 = model.AnalyzeDataFlow(intK); + Assert.Equal("i, j", GetSymbolNamesJoined(flowAnalysis1.ReadInside)); + + var add = tree.GetRoot().DescendantNodes().OfType().ToArray()[1]; + Assert.Equal("i + j", add.ToString()); + var flowAnalysis = model.AnalyzeDataFlow(add); + Assert.Equal("i, j", GetSymbolNamesJoined(flowAnalysis.ReadInside)); + } + + // TODO2: testing } } From 65a5db34cc1de7cea1f0126feecda758a70ae0c4 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 13 Dec 2024 12:42:45 -0800 Subject: [PATCH 2/7] call Enter/Leave in proper traversal order --- .../Portable/FlowAnalysis/AbstractFlowPass.cs | 4 +- .../Emit3/FlowAnalysis/RegionAnalysisTests.cs | 64 ++++++++++++++++--- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index 1c1410b380860..9331dab2b5c2e 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -2541,6 +2541,7 @@ private void VisitBinaryOperatorChildren(BoundBinaryOperator node) do { stack.Push(binary); + EnterRegionIfNeeded(binary); binary = binary.Left as BoundBinaryOperator; } while (binary != null && !binary.OperatorKind.IsLogical() && binary.InterpolatedStringHandlerData is null); @@ -2550,10 +2551,10 @@ private void VisitBinaryOperatorChildren(BoundBinaryOperator node) } #nullable enable + /// Nested left-associative binary operators, pushed on from outermost to innermost. protected virtual void VisitBinaryOperatorChildren(ArrayBuilder stack) { var binary = stack.Pop(); - EnterRegionIfNeeded(binary); // Only the leftmost operator of a left-associative binary operator chain can learn from a conditional access on the left // For simplicity, we just special case it here. @@ -2619,7 +2620,6 @@ protected virtual void VisitBinaryOperatorChildren(ArrayBuilder().ToArray()[1]; - Assert.Equal("int k = i + j + 1;", intK.ToString()); - var flowAnalysis1 = model.AnalyzeDataFlow(intK); - Assert.Equal("i, j", GetSymbolNamesJoined(flowAnalysis1.ReadInside)); + var decls = tree.GetRoot().DescendantNodes().OfType().ToArray(); + Assert.Equal(2, decls.Length); + var decl = decls[1]; + Assert.Equal("int k = i + j + 1;", decl.ToString()); + var flowAnalysis = model.AnalyzeDataFlow(decl); + Assert.Equal("i, j", GetSymbolNamesJoined(flowAnalysis.ReadInside)); + + var binOps = tree.GetRoot().DescendantNodes().OfType().ToArray(); + Assert.Equal(2, binOps.Length); + var add = binOps[0]; + Assert.Equal("i + j + 1", add.ToString()); + flowAnalysis = model.AnalyzeDataFlow(add); + Assert.Equal("i, j", GetSymbolNamesJoined(flowAnalysis.ReadInside)); - var add = tree.GetRoot().DescendantNodes().OfType().ToArray()[1]; + add = binOps[1]; Assert.Equal("i + j", add.ToString()); - var flowAnalysis = model.AnalyzeDataFlow(add); + flowAnalysis = model.AnalyzeDataFlow(add); Assert.Equal("i, j", GetSymbolNamesJoined(flowAnalysis.ReadInside)); } - // TODO2: testing + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/38087")] + public void FourBinaryOperands() + { + var comp = CreateCompilation(""" + class Program + { + private static void Repro() + { + int i = 1, j = 2, k = 3, l = 4; + _ = i + j + k + l; + } + } + """); + comp.VerifyEmitDiagnostics(); + + var tree = comp.CommonSyntaxTrees[0]; + var model = comp.GetSemanticModel(tree); + + var decl = tree.GetRoot().DescendantNodes().OfType().Single(); + Assert.Equal("_ = i + j + k + l;", decl.ToString()); + var flowAnalysis = model.AnalyzeDataFlow(decl); + Assert.Equal("i, j, k, l", GetSymbolNamesJoined(flowAnalysis.ReadInside)); + + var binOps = tree.GetRoot().DescendantNodes().OfType().ToArray(); + Assert.Equal(3, binOps.Length); + var add = binOps[0]; + Assert.Equal("i + j + k + l", add.ToString()); + flowAnalysis = model.AnalyzeDataFlow(add); + Assert.Equal("i, j, k, l", GetSymbolNamesJoined(flowAnalysis.ReadInside)); + + add = binOps[1]; + Assert.Equal("i + j + k", add.ToString()); + flowAnalysis = model.AnalyzeDataFlow(add); + Assert.Equal("i, j, k", GetSymbolNamesJoined(flowAnalysis.ReadInside)); + + add = binOps[2]; + Assert.Equal("i + j", add.ToString()); + flowAnalysis = model.AnalyzeDataFlow(add); + Assert.Equal("i, j", GetSymbolNamesJoined(flowAnalysis.ReadInside)); + } } } From 753862a196bbad8f3beed7f84a5bbccc79357144 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 13 Dec 2024 13:02:40 -0800 Subject: [PATCH 3/7] Add extract method test --- .../ExtractMethodCodeRefactoringTests.cs | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/Features/CSharpTest/ExtractMethod/ExtractMethodCodeRefactoringTests.cs b/src/Features/CSharpTest/ExtractMethod/ExtractMethodCodeRefactoringTests.cs index d88137fcd0c5a..ab777c113f97c 100644 --- a/src/Features/CSharpTest/ExtractMethod/ExtractMethodCodeRefactoringTests.cs +++ b/src/Features/CSharpTest/ExtractMethod/ExtractMethodCodeRefactoringTests.cs @@ -6048,4 +6048,35 @@ struct S1 } """); } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/38087")] + public async Task TestPartialSelectionOfArithmeticExpression() + { + await TestInRegularAndScript1Async( + """ + class C + { + private void Repro() + { + int i = 1, j = 2; + int k = [|i + j|] + 1; + } + } + """, + """ + class C + { + private void Repro() + { + int i = 1, j = 2; + int k = {|Rename:NewMethod|}(i, j) + 1; + } + + private static int NewMethod(int i, int j) + { + return i + j; + } + } + """); + } } From 802ce1e387da9cf789cfb9cc5c6683605eb3a5e6 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 13 Dec 2024 17:08:45 -0800 Subject: [PATCH 4/7] add VB test --- .../FlowAnalysis/RegionAnalysisTests.vb | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/Compilers/VisualBasic/Test/Semantic/FlowAnalysis/RegionAnalysisTests.vb b/src/Compilers/VisualBasic/Test/Semantic/FlowAnalysis/RegionAnalysisTests.vb index 7525a6f8f50b8..40c0be1a60959 100644 --- a/src/Compilers/VisualBasic/Test/Semantic/FlowAnalysis/RegionAnalysisTests.vb +++ b/src/Compilers/VisualBasic/Test/Semantic/FlowAnalysis/RegionAnalysisTests.vb @@ -9994,6 +9994,38 @@ End Module capturedOutside) End Sub + + + Public Sub NestedBinaryOperator() + Dim compilation = CreateCompilationWithMscorlib40AndVBRuntime( + + +Module Program + Sub M(i As Integer, j As Integer, k As Integer, l As Integer) + Dim x = i + j + k + l + End Sub +End Module + + ) + + Dim tree = compilation.SyntaxTrees.First() + Dim model = compilation.GetSemanticModel(tree) + Dim nodes = tree.GetRoot().DescendantNodes().OfType(Of BinaryExpressionSyntax)().ToArray() + Assert.Equal(3, nodes.Length) + + Assert.Equal("i + j + k + l", nodes(0).ToString()) + Dim dataFlowResults = model.AnalyzeDataFlow(nodes(0)) + Assert.Equal("i, j, k, l", GetSymbolNamesJoined(dataFlowResults.ReadInside)) + + Assert.Equal("i + j + k", nodes(1).ToString()) + dataFlowResults = model.AnalyzeDataFlow(nodes(1)) + Assert.Equal("i, j, k", GetSymbolNamesJoined(dataFlowResults.ReadInside)) + + Assert.Equal("i + j", nodes(2).ToString()) + dataFlowResults = model.AnalyzeDataFlow(nodes(2)) + Assert.Equal("i, j", GetSymbolNamesJoined(dataFlowResults.ReadInside)) + End Sub + #End Region End Class From 0fce68c84ba414c6fcfc5232f8aa89b810105365 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Fri, 13 Dec 2024 17:33:24 -0800 Subject: [PATCH 5/7] Fix formatting --- .../Test/Semantic/FlowAnalysis/RegionAnalysisTests.vb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compilers/VisualBasic/Test/Semantic/FlowAnalysis/RegionAnalysisTests.vb b/src/Compilers/VisualBasic/Test/Semantic/FlowAnalysis/RegionAnalysisTests.vb index 40c0be1a60959..00673802748a4 100644 --- a/src/Compilers/VisualBasic/Test/Semantic/FlowAnalysis/RegionAnalysisTests.vb +++ b/src/Compilers/VisualBasic/Test/Semantic/FlowAnalysis/RegionAnalysisTests.vb @@ -10016,11 +10016,11 @@ End Module Assert.Equal("i + j + k + l", nodes(0).ToString()) Dim dataFlowResults = model.AnalyzeDataFlow(nodes(0)) Assert.Equal("i, j, k, l", GetSymbolNamesJoined(dataFlowResults.ReadInside)) - + Assert.Equal("i + j + k", nodes(1).ToString()) dataFlowResults = model.AnalyzeDataFlow(nodes(1)) Assert.Equal("i, j, k", GetSymbolNamesJoined(dataFlowResults.ReadInside)) - + Assert.Equal("i + j", nodes(2).ToString()) dataFlowResults = model.AnalyzeDataFlow(nodes(2)) Assert.Equal("i, j", GetSymbolNamesJoined(dataFlowResults.ReadInside)) From c640fb15606f597f92ec29ed03a841df189c09cf Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 16 Dec 2024 09:49:15 -0800 Subject: [PATCH 6/7] exercise LHS cond-access code path --- .../Emit3/FlowAnalysis/RegionAnalysisTests.cs | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/Compilers/CSharp/Test/Emit3/FlowAnalysis/RegionAnalysisTests.cs b/src/Compilers/CSharp/Test/Emit3/FlowAnalysis/RegionAnalysisTests.cs index 40546661734d6..f8a6f0b71d51c 100644 --- a/src/Compilers/CSharp/Test/Emit3/FlowAnalysis/RegionAnalysisTests.cs +++ b/src/Compilers/CSharp/Test/Emit3/FlowAnalysis/RegionAnalysisTests.cs @@ -14226,5 +14226,49 @@ private static void Repro() flowAnalysis = model.AnalyzeDataFlow(add); Assert.Equal("i, j", GetSymbolNamesJoined(flowAnalysis.ReadInside)); } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/38087")] + public void BinaryOpConditionalAccess() + { + var comp = CreateCompilation(""" + class C + { + public bool M(out int x) { x = 0; return false; } + + private static void Repro(C c) + { + const bool y = true; + const bool z = true; + int x; + _ = c?.M(out x) == y == z; + } + } + """); + comp.VerifyEmitDiagnostics(); + + var tree = comp.CommonSyntaxTrees[0]; + var model = comp.GetSemanticModel(tree); + + var decl = tree.GetRoot().DescendantNodes().OfType().Last(); + Assert.Equal("_ = c?.M(out x) == y == z;", decl.ToString()); + var flowAnalysis = model.AnalyzeDataFlow(decl); + Assert.Equal("c, y, z", GetSymbolNamesJoined(flowAnalysis.ReadInside)); + Assert.Equal("x", GetSymbolNamesJoined(flowAnalysis.WrittenInside)); + + var binOps = tree.GetRoot().DescendantNodes().OfType().ToArray(); + Assert.Equal(2, binOps.Length); + + var binOp = binOps[0]; + Assert.Equal("c?.M(out x) == y == z", binOp.ToString()); + flowAnalysis = model.AnalyzeDataFlow(binOp); + Assert.Equal("c, y, z", GetSymbolNamesJoined(flowAnalysis.ReadInside)); + Assert.Equal("x", GetSymbolNamesJoined(flowAnalysis.WrittenInside)); + + binOp = binOps[1]; + Assert.Equal("c?.M(out x) == y", binOp.ToString()); + flowAnalysis = model.AnalyzeDataFlow(binOp); + Assert.Equal("c, y", GetSymbolNamesJoined(flowAnalysis.ReadInside)); + Assert.Equal("x", GetSymbolNamesJoined(flowAnalysis.WrittenInside)); + } } } From 5a5869bda01d8854eb474b82b461d78306160d5d Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 16 Dec 2024 11:11:00 -0800 Subject: [PATCH 7/7] Assert that nullable analysis does not track regions --- src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 96a50c5f7daf6..03f68521c636f 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -462,6 +462,7 @@ private NullableWalker( bool isSpeculative = false) : base(compilation, symbol, node, EmptyStructTypeCache.CreatePrecise(), trackUnassignments: true) { + Debug.Assert(!TrackingRegions); Debug.Assert(!useDelegateInvokeParameterTypes || delegateInvokeMethodOpt is object); Debug.Assert(baseOrThisInitializer is null or { MethodKind: MethodKind.Constructor });