Fix S1854 FP: Throw should connect to outer catch #9528

mary-georgiou-sonarsource · 2024-07-16T12:45:35Z

sebastien-marichal · 2024-07-16T15:49:09Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

@@ -138,6 +138,10 @@ private void BuildBranches(BasicBlock block)
                AddPredecessorsOutsideRegion(finallyBlock);
            }
        }
+        if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is { })


Suggested change

if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is { })

if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is not null)

It's probably an FN for T0007.

sebastien-marichal

LGTM!
We discussed it together and it seems good for me.
I only added a comment about is not null usage

Tim-Pohlmann

LGTM only cosmetic suggestions.

Tim-Pohlmann · 2024-07-18T14:12:44Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

+    {
+        var tryRegion = block.EnclosingRegion(ControlFlowRegionKind.TryAndCatch).NestedRegion(ControlFlowRegionKind.Try);
+        foreach (var catchBlock in tryRegion.ReachableHandlers()
+                                            .Where(x => x.Kind == ControlFlowRegionKind.Catch && x != block.EnclosingRegion && x.FirstBlockOrdinal > block.Ordinal)


Suggested change

.Where(x => x.Kind == ControlFlowRegionKind.Catch && x != block.EnclosingRegion && x.FirstBlockOrdinal > block.Ordinal)

.Where(x => x.Kind == ControlFlowRegionKind.Catch && x.FirstBlockOrdinal > block.Ordinal)

The middle condition is already covered by the last condition.

Tim-Pohlmann · 2024-07-18T14:18:44Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

+        foreach (var catchBlock in tryRegion.ReachableHandlers()
+                                            .Where(x => x.Kind == ControlFlowRegionKind.Catch && x != block.EnclosingRegion && x.FirstBlockOrdinal > block.Ordinal)
+                                            .SelectMany(x => x.Blocks(Cfg)))


I would separate this into an extra variable. foreach with multiple lines is always awkward to read.

Tim-Pohlmann · 2024-07-18T14:28:43Z

...ts/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs

+    {
+        var code = """
+            var value = 100;
+            try{


Tim-Pohlmann · 2024-07-18T14:34:45Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

@@ -138,6 +138,10 @@ private void BuildBranches(BasicBlock block)
                AddPredecessorsOutsideRegion(finallyBlock);
            }
        }
+        if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is { })


Suggested change

if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is { })

if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.Any(x => x.Semantics == ControlFlowBranchSemantics.Rethrow))

sonarcloud · 2024-07-18T14:51:31Z

Quality Gate passed for 'Sonar .NET Java Plugin'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarcloud · 2024-07-18T14:56:29Z

Quality Gate passed for 'SonarAnalyzer for .NET'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

pavel-mikula-sonarsource · 2024-07-19T06:34:42Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

@@ -138,6 +138,10 @@ private void BuildBranches(BasicBlock block)
                AddPredecessorsOutsideRegion(finallyBlock);
            }
        }
+        if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.Any(x => x.Semantics == ControlFlowBranchSemantics.Rethrow))


What about Throw semantics? All the UTs have throw, but throw new Excetion("Whaaat?", ex) should behave the same

fixed here #9530
Thanks @sebastien-marichal

pavel-mikula-sonarsource · 2024-07-19T06:36:33Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

@@ -161,6 +165,15 @@ private void BuildBranchesFinally(BasicBlock source, ControlFlowRegion finallyRe
        }
    }

+    private void BuildBranchesNestedCatchRethrow(BasicBlock block)
+    {
+        var reachableHandlers = block.EnclosingRegion(ControlFlowRegionKind.TryAndCatch).NestedRegion(ControlFlowRegionKind.Try).ReachableHandlers();


Doesn't ReachableHandlers also connect back to it's own Catch in this case? Or more precisely, all neighboring catches too? That would be wrong

@pavel-mikula-sonarsource I'm after asking for x.FirstBlockOrdinal > block.Ordinal) - all the catch with greater ordinal that the one I'm in.
Isn't that working?

var reachableHandlers = block.EnclosingRegion(ControlFlowRegionKind.TryAndCatch).NestedRegion(ControlFlowRegionKind.Try).ReachableHandlers(); foreach (var catchBlock in reachableHandlers.Where(x => x.Kind == ControlFlowRegionKind.Catch && x.FirstBlockOrdinal > block.Ordinal).SelectMany(x => x.Blocks(Cfg))) {}

Also not sure what you mean neighbouring catches :/

Try Catch Ex As IOException Throw 'You are here, with your block.Ordinal Catch Ex As FormatException 'This is a neighbour that you don't want to connect to. And it has FirstBlockOrdinal > block.Ordinal End Try

Go escape, you'd need to store your TryAndCatch region, and be bigger than its LastBlockOrdinal

Got It!
I'm opening a PR to fix this.

fix Fp

a4a64cf

mary-georgiou-sonarsource force-pushed the mary/lva-connect-throw branch from 8354430 to a4a64cf Compare July 16, 2024 12:46

mary-georgiou-sonarsource requested a review from sebastien-marichal July 16, 2024 12:54

sebastien-marichal reviewed Jul 16, 2024

View reviewed changes

sebastien-marichal approved these changes Jul 17, 2024

View reviewed changes

sebastien-marichal mentioned this pull request Jul 17, 2024

Fix S1854 FP: Throw should visit finally #9530

Merged

mary-georgiou-sonarsource requested review from pavel-mikula-sonarsource and Tim-Pohlmann and removed request for pavel-mikula-sonarsource July 17, 2024 09:01

Tim-Pohlmann approved these changes Jul 18, 2024

View reviewed changes

apply comments

2eb4bc4

mary-georgiou-sonarsource enabled auto-merge (squash) July 18, 2024 14:52

mary-georgiou-sonarsource merged commit a9af306 into master Jul 18, 2024
17 checks passed

mary-georgiou-sonarsource deleted the mary/lva-connect-throw branch July 18, 2024 14:57

pavel-mikula-sonarsource reviewed Jul 19, 2024

View reviewed changes

mary-georgiou-sonarsource mentioned this pull request Jul 22, 2024

LVA: Don't connect from throw in catch to neighbouring catch #9539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix S1854 FP: Throw should connect to outer catch #9528

Fix S1854 FP: Throw should connect to outer catch #9528

mary-georgiou-sonarsource commented Jul 16, 2024

sebastien-marichal Jul 16, 2024

sebastien-marichal left a comment

Tim-Pohlmann left a comment

Tim-Pohlmann Jul 18, 2024

Tim-Pohlmann Jul 18, 2024

Tim-Pohlmann Jul 18, 2024

Tim-Pohlmann Jul 18, 2024

sonarcloud bot commented Jul 18, 2024

sonarcloud bot commented Jul 18, 2024

pavel-mikula-sonarsource Jul 19, 2024

mary-georgiou-sonarsource Jul 19, 2024

pavel-mikula-sonarsource Jul 19, 2024 •

edited

Loading

mary-georgiou-sonarsource Jul 19, 2024

pavel-mikula-sonarsource Jul 19, 2024

pavel-mikula-sonarsource Jul 19, 2024 •

edited by mary-georgiou-sonarsource

Loading

mary-georgiou-sonarsource Jul 22, 2024

mary-georgiou-sonarsource Jul 22, 2024

	if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is { })
	if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is not null)

	.Where(x => x.Kind == ControlFlowRegionKind.Catch && x != block.EnclosingRegion && x.FirstBlockOrdinal > block.Ordinal)
	.Where(x => x.Kind == ControlFlowRegionKind.Catch && x.FirstBlockOrdinal > block.Ordinal)

Fix S1854 FP: Throw should connect to outer catch #9528

Fix S1854 FP: Throw should connect to outer catch #9528

Conversation

mary-georgiou-sonarsource commented Jul 16, 2024

Choose a reason for hiding this comment

sebastien-marichal left a comment

Choose a reason for hiding this comment

Tim-Pohlmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 18, 2024

Quality Gate passed for 'Sonar .NET Java Plugin'

sonarcloud bot commented Jul 18, 2024

Quality Gate passed for 'SonarAnalyzer for .NET'

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-mikula-sonarsource Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-mikula-sonarsource Jul 19, 2024 • edited by mary-georgiou-sonarsource Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-mikula-sonarsource Jul 19, 2024 •

edited

Loading

pavel-mikula-sonarsource Jul 19, 2024 •

edited by mary-georgiou-sonarsource

Loading