-
Notifications
You must be signed in to change notification settings - Fork 222
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
Remove redundant break expressions in switch-case statements. #829
Conversation
Roslyn does provide a way to do control flow analysis, e.g. CodeConverter/CodeConverter/CSharp/DeclarationNodeVisitor.cs Lines 1083 to 1092 in 97ea8f3
However, you'll see it's a full semantic analysis, which means a) It's generally quite expensive to run b) It needs to run on a semantic model i.e. either the existing VB one, or even more expensively constructing a custom one So I think this solution is pretty sensible and will cover a lot of cases cheaply. Here's an example of a very similar analysis: CodeConverter/CodeConverter/CSharp/ExpressionNodeVisitor.cs Lines 1097 to 1120 in 068c7dd
The reason I use definitely is because of its use for the concept of "definite assignment" in C#, which there's also actually an example of here: CodeConverter/CodeConverter/CSharp/DefiniteAssignmentAnalyzer.cs Lines 10 to 15 in 97ea8f3
|
@@ -765,8 +765,7 @@ public override async Task<SyntaxList<StatementSyntax>> VisitSelectBlock(VBSynta | |||
} | |||
|
|||
var csBlockStatements = (await ConvertStatementsAsync(block.Statements)).ToList(); | |||
if (csBlockStatements.LastOrDefault() | |||
?.IsKind(SyntaxKind.ReturnStatement, SyntaxKind.BreakStatement) != true) { | |||
if (!WillLastStatementExit(csBlockStatements.LastOrDefault())) { |
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.
This is an example of where the advice from contributing.md applies:
Always try to convert directly between the VB and C# model, avoid converting then post-processing the converted result. This prevents the code getting tangled interdependencies, and means you have the full semantic model available to make an accurate conversion.
So in general, I'd have written the analysis method against the Visual Basic syntax tree. That way if you decide there's an interesting case to add that does need the semantic model, you can easily grab it.
That said, what you've written is actually closer to something that could be a wholly independent rosyln analysis/fixer on a C# tree independent of all else. Interestingly a built-in one already exists, so I do wonder if it could be run as part of the general simplification phase to keep the complexity out of here:
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.
I'd say this is a definite (no pun intended) improvement, so I'm happy to merge it, and we can always consider trying cleverer things with mass code actions separately. Are you happy to tweak the method name to something like DefinitelyExits
or MayReachStatementAfter
?
Thanks for the review and comments - I changed the method name as suggested. |
Fixes #432 (or at least most common scenarios).
case
.if
statement will exit on all code paths.I'm not sure if there is some kind of built-in way in Roslyn to check it. My solution will cover only simple ifs. Nested switch/if statements will still generate
break
keyword.