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

Remove redundant break expressions in switch-case statements. #829

Merged
merged 3 commits into from
Feb 11, 2022

Conversation

Yozer
Copy link
Member

@Yozer Yozer commented Feb 8, 2022

Fixes #432 (or at least most common scenarios).

  1. Consider Throw/Break/Return/Continue as keywords that will exit from case.
  2. Check if the previous 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.

@GrahamTheCoder
Copy link
Member

Roslyn does provide a way to do control flow analysis, e.g.

if (!node.Statements.IsEmpty())
controlFlowAnalysis =
ModelExtensions.AnalyzeControlFlow(_semanticModel, node.Statements.First(), node.Statements.Last());
bool mayNeedReturn = controlFlowAnalysis?.EndPointIsReachable != false;
if (mayNeedReturn) {
var csReturnExpression = csReturnVariableOrNull ??
(ExpressionSyntax)SyntaxFactory.DefaultExpression(returnType);
postBodyStatements.Add(SyntaxFactory.ReturnStatement(csReturnExpression));
}

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.
When doing imperfect analyses such as these, I try to name the method to try to hint whether it's an under or overapproximation using the word "definitely" to emphasize which thing is known for sure

Here's an example of a very similar analysis:

private bool DefinitelyExecutedAfterPreviousStatement(VBSyntax.InvocationExpressionSyntax invocation)
{
SyntaxNode parent = invocation;
while (true) {
parent = parent.Parent;
switch (parent)
{
case VBSyntax.ParenthesizedExpressionSyntax _:
continue;
case VBSyntax.BinaryExpressionSyntax binaryExpression:
if (binaryExpression.Left == invocation) continue;
else return false;
case VBSyntax.ArgumentSyntax argumentSyntax:
// Being the leftmost invocation of an unqualified method call ensures no other code is executed. Could add other cases here, such as a method call on a local variable name, or "this.". A method call on a property is not acceptable.
if (argumentSyntax.Parent.Parent is VBSyntax.InvocationExpressionSyntax parentInvocation && parentInvocation.ArgumentList.Arguments.First() == argumentSyntax && FirstArgDefinitelyEvaluated(parentInvocation)) continue;
else return false;
case VBSyntax.ElseIfStatementSyntax _:
case VBSyntax.ExpressionSyntax _:
return false;
case VBSyntax.StatementSyntax _:
return true;
}
}
}

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:

public static bool IsDefinitelyAssignedBeforeRead(ISymbol localSymbol, DataFlowAnalysis methodFlow)
{
if (!methodFlow.ReadInside.Contains(localSymbol)) return true;
var unassignedVariables = methodFlow.GetVbUnassignedVariables();
return unassignedVariables != null && !unassignedVariables.Contains(localSymbol);
}

@@ -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())) {
Copy link
Member

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:
image

Copy link
Member

@GrahamTheCoder GrahamTheCoder left a 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?

@Yozer
Copy link
Member Author

Yozer commented Feb 10, 2022

Thanks for the review and comments - I changed the method name as suggested.
I will try to take a look if there is a way to run the built-in analyzer in the simplification stage.
The only thing I'm worried about it is limiting the scope where "Unreachable code" simplification is applied. I'm not sure if the CodeConverter should be too aggressive with it.

@GrahamTheCoder GrahamTheCoder merged commit ac30790 into icsharpcode:master Feb 11, 2022
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.

VB -> C#: Switch statement generates unreachable code
2 participants