Skip to content

Fix S1854 FP: Throw should visit finally #9530

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 7 commits into from
Jul 19, 2024
Merged

Fix S1854 FP: Throw should visit finally #9530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a10a4ea
Fix S1854 FP: Throw should visit finally
sebastien-marichal Jul 16, 2024
ee3754d
Address comment
sebastien-marichal Jul 18, 2024
8755120
LvaWalker improvement
sebastien-marichal Jul 19, 2024
18fc5c9
Only connect the last catch to the next finally if any
sebastien-marichal Jul 19, 2024
69456ec
Add support of throw statements
sebastien-marichal Jul 19, 2024
442b7d5
Address comment
sebastien-marichal Jul 19, 2024
ae25c51
Add more tests
sebastien-marichal Jul 19, 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
2 changes: 1 addition & 1 deletion analyzers/src/SonarAnalyzer.CFG/CfgSerializer/CfgSerializer.RoslynLvaWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ protected override void WriteEdges(BasicBlock block)
{
foreach (var predecessor in lva.BlockPredecessors[block.Ordinal].Where(x => !block.Predecessors.Any(y => y.Source == x)))
{
writer.WriteEdge(BlockId(predecessor), BlockId(block), "LVA");
writer.WriteEdge(BlockId(predecessor), BlockId(block), "LVA\" fontcolor=\"blue\" penwidth=\"2\" color=\"blue");
}
base.WriteEdges(block);
}
Expand Down
15 changes: 10 additions & 5 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 @@ -140,9 +140,9 @@ private void BuildBranches(BasicBlock block)
AddPredecessorsOutsideRegion(finallyBlock);
}
}
if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.Any(x => x.Semantics == ControlFlowBranchSemantics.Rethrow))
if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.Any(x => x.Semantics is ControlFlowBranchSemantics.Rethrow or ControlFlowBranchSemantics.Throw))
{
BuildBranchesNestedCatchRethrow(block);
BuildBranchesRethrow(block);
}

void AddPredecessorsOutsideRegion(BasicBlock destination)
Expand All @@ -167,13 +167,18 @@ private void BuildBranchesFinally(BasicBlock source, ControlFlowRegion finallyRe
}
}

private void BuildBranchesNestedCatchRethrow(BasicBlock block)
private void BuildBranchesRethrow(BasicBlock block)
{
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)))
var reachableHandlerRegions = block.EnclosingRegion(ControlFlowRegionKind.TryAndCatch).NestedRegion(ControlFlowRegionKind.Try).ReachableHandlers();
var reachableCatchAndFinallyBlocks = reachableHandlerRegions.Where(x => x.FirstBlockOrdinal > block.Ordinal).SelectMany(x => x.Blocks(Cfg));
foreach (var catchBlock in reachableCatchAndFinallyBlocks.Where(x => x.EnclosingRegion.Kind is ControlFlowRegionKind.Catch))
{
AddBranch(block, catchBlock);
}
if (reachableCatchAndFinallyBlocks.FirstOrDefault() is { EnclosingRegion.Kind: ControlFlowRegionKind.Finally } finallyBlock)
{
AddBranch(block, finallyBlock);
}
}

private void AddBranch(BasicBlock source, BasicBlock destination)
Expand Down
31 changes: 16 additions & 15 deletions analyzers/tests/SonarAnalyzer.Test/CFG/Roslyn/RoslynLvaSerializerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,14 +85,14 @@ void Use(int v) {}
cfg0_block0 [shape=record label="{ENTRY #0}"]
cfg0_block5 [shape=record label="{EXIT #5}"]
cfg0_block1 -> cfg0_block2
cfg0_block2 -> cfg0_block3 [label="LVA"]
cfg0_block1 -> cfg0_block3 [label="LVA"]
cfg0_block2 -> cfg0_block4 [label="LVA"]
cfg0_block3 -> cfg0_block4 [label="LVA"]
cfg0_block2 -> cfg0_block3 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block1 -> cfg0_block3 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block2 -> cfg0_block4 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block3 -> cfg0_block4 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block4 -> NoDestination_cfg0_block4 [label="StructuredExceptionHandling"]
cfg0_block0 -> cfg0_block1
cfg0_block4 -> cfg0_block5 [label="LVA"]
cfg0_block4 -> cfg0_block5 [label="LVA"]
cfg0_block4 -> cfg0_block5 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block4 -> cfg0_block5 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block2 -> cfg0_block5
cfg0_block3 -> cfg0_block5
}
Expand Down Expand Up @@ -160,13 +160,14 @@ void Use(int v) {}
cfg0_block0 [shape=record label="{ENTRY #0}"]
cfg0_block5 [shape=record label="{EXIT #5}"]
cfg0_block1 -> cfg0_block2
cfg0_block2 -> cfg0_block3 [label="LVA"]
cfg0_block1 -> cfg0_block3 [label="LVA"]
cfg0_block2 -> cfg0_block3 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block1 -> cfg0_block3 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block3 -> NoDestination_cfg0_block3 [label="Rethrow"]
cfg0_block2 -> cfg0_block4 [label="LVA"]
cfg0_block2 -> cfg0_block4 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block3 -> cfg0_block4 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block4 -> NoDestination_cfg0_block4 [label="StructuredExceptionHandling"]
cfg0_block0 -> cfg0_block1
cfg0_block4 -> cfg0_block5 [label="LVA"]
cfg0_block4 -> cfg0_block5 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block2 -> cfg0_block5
}

Expand Down Expand Up @@ -265,17 +266,17 @@ void Use(int v) {}
cfg0_block3 -> cfg0_block4 [label="Else"]
cfg0_block2 -> cfg0_block3
cfg0_block4 -> cfg0_block3
cfg0_block3 -> cfg0_block5 [label="LVA"]
cfg0_block3 -> cfg0_block5 [label="LVA"]
cfg0_block2 -> cfg0_block5 [label="LVA"]
cfg0_block4 -> cfg0_block5 [label="LVA"]
cfg0_block3 -> cfg0_block5 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block3 -> cfg0_block5 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block2 -> cfg0_block5 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block4 -> cfg0_block5 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block5 -> cfg0_block6 [label="Else"]
cfg0_block5 -> cfg0_block7 [label="WhenTrue"]
cfg0_block6 -> cfg0_block7
cfg0_block7 -> NoDestination_cfg0_block7 [label="StructuredExceptionHandling"]
cfg0_block1 -> cfg0_block2
cfg0_block0 -> cfg0_block1
cfg0_block7 -> cfg0_block8 [label="LVA"]
cfg0_block7 -> cfg0_block8 [label="LVA" fontcolor="blue" penwidth="2" color="blue"]
cfg0_block3 -> cfg0_block8 [label="WhenFalse"]
}

Expand Down
Loading