-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix S1854 FP: Throw should visit finally #9530
Conversation
b39bcb2
to
0efe47a
Compare
0efe47a
to
071ba89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some refactoring
...ts/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed I'm not sure why you get the first or default catch or throw.
Please take a look.
5364a23
to
69456ec
Compare
{ | ||
AddBranch(block, catchBlock); | ||
} | ||
if (reachableCatchAndFinallyBlocks.Any() && reachableCatchAndFinallyBlocks.First() is { EnclosingRegion.Kind: ControlFlowRegionKind.Finally } finallyBlock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (reachableCatchAndFinallyBlocks.Any() && reachableCatchAndFinallyBlocks.First() is { EnclosingRegion.Kind: ControlFlowRegionKind.Finally } finallyBlock) | |
if (reachableCatchAndFinallyBlocks.FirstOrDefault() is { EnclosingRegion.Kind: ControlFlowRegionKind.Finally } finallyBlock) |
Quality Gate passed for 'Sonar .NET Java Plugin'Issues Measures |
Quality Gate passed for 'SonarAnalyzer for .NET'Issues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -65,7 +65,7 @@ public void Using_LiveInUntilTheEnd(string usingStatement, string suffix) | |||
[TestMethod] | |||
public void Using_Nested_Block_LiveInUntilTheEnd() | |||
{ | |||
var code = """ | |||
const string code = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cons string
has no performance benefit over var
:
https://stackoverflow.com/a/49224799
This creates unnecessary visual noise.
Fixes #9466
Depends on #9528