Skip to content
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: Value used after catch #9531

Closed
wants to merge 4 commits into from

Conversation

mary-georgiou-sonarsource
Copy link
Contributor

@mary-georgiou-sonarsource mary-georgiou-sonarsource commented Jul 17, 2024

Fixes #9467

Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

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

I don't think the approach is sound.

@@ -124,7 +125,7 @@ private void BuildBranches(BasicBlock block)
if (block.EnclosingNonLocalLifetimeRegion() is { Kind: ControlFlowRegionKind.Try } tryRegion)
{
var catchesAll = false;
foreach (var catchOrFilterRegion in block.Successors.SelectMany(CatchOrFilterRegions))
foreach (var catchOrFilterRegion in block.Successors.Union(Cfg.Blocks[tryRegion.LastBlockOrdinal].Successors).SelectMany(CatchOrFilterRegions))
Copy link
Contributor

Choose a reason for hiding this comment

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

For me this approach looks more natural:

Suggested change
foreach (var catchOrFilterRegion in block.Successors.Union(Cfg.Blocks[tryRegion.LastBlockOrdinal].Successors).SelectMany(CatchOrFilterRegions))
foreach (var catchOrFilterRegion in CatchOrFilterRegions(tryRegion.EnclosingRegion(ControlFlowRegionKind.TryAndCatch)))

This is only to show the idea, the implementation needs to be a bit different (this currently throws in a using statement). But the idea would be for every block to look at the enclosing try-catch region and add branches to all catch regions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand.
I need the leaving regions of the try region.

@mary-georgiou-sonarsource mary-georgiou-sonarsource force-pushed the mary/fp-after-catch branch 4 times, most recently from 7a275bd to ff769a1 Compare July 19, 2024 14:53
Copy link

sonarcloud bot commented Jul 23, 2024

Copy link

sonarcloud bot commented Jul 23, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix S1854 FP: Value used after catch
2 participants