Skip to content

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

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.

Sign up for GitHub

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

Merged
mary-georgiou-sonarsource merged 2 commits into from
Jul 18, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4a64cf
fix Fp
mary-georgiou-sonarsource Jul 16, 2024
2eb4bc4
apply comments
mary-georgiou-sonarsource Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ private void BuildBranches(BasicBlock block)
AddPredecessorsOutsideRegion(finallyBlock);
}
}
if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.Any(x => x.Semantics == ControlFlowBranchSemantics.Rethrow))
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mary-georgiou-sonarsource mary-georgiou-sonarsource Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed here #9530
Thanks @sebastien-marichal

{
BuildBranchesNestedCatchRethrow(block);
}

void AddPredecessorsOutsideRegion(BasicBlock destination)
{
Expand All @@ -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();
Copy link
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mary-georgiou-sonarsource mary-georgiou-sonarsource Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :/

Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mary-georgiou-sonarsource mary-georgiou-sonarsource Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@mary-georgiou-sonarsource mary-georgiou-sonarsource Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#9539

foreach (var catchBlock in reachableHandlers.Where(x => x.Kind == ControlFlowRegionKind.Catch && x.FirstBlockOrdinal > block.Ordinal).SelectMany(x => x.Blocks(Cfg)))
{
AddBranch(block, catchBlock);
}
}

private void AddBranch(BasicBlock source, BasicBlock destination)
{
blockSuccessors[source.Ordinal].Add(destination);
Expand Down
118 changes: 118 additions & 0 deletions ...SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,124 @@ public void Catch_Loop_Propagates_LiveIn()
context.Validate("Method(5);");
}

[TestMethod]
public void Throw_NestedCatch_LiveOut()
{
var code = """
var value = 100;
try
{
try
{
Method(value);
Method(0);
}
catch
{
value = 200;
throw;
}
}
catch
{
Method(value);
Method(1);
}
""";
var context = CreateContextCS(code);
context.ValidateEntry();
context.Validate("Method(0);", LiveIn("value"), LiveOut("value"));
context.Validate("value = 200;", LiveOut("value"));
context.Validate("Method(1);", LiveIn("value"));
context.ValidateExit();
}

[TestMethod]
public void Throw_NestedCatch_LiveInInConsecutiveOuterCatch()
{
var code = """
var value = 100;
try
{
try
{
Method(value);
Method(0);
}
catch
{
value = 200;
throw;
}
}
catch (ArgumentNullException)
{
Method(value);
Method(1);
}
catch (IOException)
{
Method(value);
Method(2);
}
catch (NullReferenceException)
{
Method(value);
Method(3);
}
""";
var context = CreateContextCS(code);
context.ValidateEntry();
context.Validate("Method(0);", LiveIn("value"), LiveOut("value"));
context.Validate("value = 200;", LiveOut("value"));
context.Validate("Method(1);", LiveIn("value"));
context.Validate("Method(2);", LiveIn("value"));
context.Validate("Method(3);", LiveIn("value"));
context.ValidateExit();
}

[TestMethod]
public void Throw_NestedCatch_OuterCatchRethrows_LiveOutOuterCathc()
{
var code = """
var value = 100;
try
{
try
{
try
{
Method(value);
Method(0);
}
catch
{
value = 200;
throw;
}
}
catch // Outer catch
{
Method(value);
Method(1);
throw;
}
}
catch
{
Method(value);
Method(2);
}
""";
var context = CreateContextCS(code);
context.ValidateEntry();
context.Validate("Method(0);", LiveIn("value"), LiveOut("value"));
context.Validate("value = 200;", LiveOut("value"));
context.Validate("Method(1);", LiveIn("value"), LiveOut("value"));
context.Validate("Method(2);", LiveIn("value"));
context.ValidateExit();
}

[TestMethod]
public void Finally_LiveIn()
{
Expand Down
2 changes: 1 addition & 1 deletion analyzers/tests/SonarAnalyzer.Test/TestCases/DeadStores.RoslynCfg.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1612,7 +1612,7 @@ public void NestedCatchAndRethrow()
}
catch
{
value = 200; // Noncompliant FP
value = 200; // Compliant, catch rethrows and moves to the next catch.
throw;
}
}
Expand Down