Fix added switch entries printed by LPP #4679
Conversation
574c7cc to
3343a18
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4679 +/- ##
=========================================
Coverage 58.143% 58.143%
=========================================
Files 508 508
Lines 29771 29771
Branches 5251 5251
=========================================
Hits 17310 17310
Misses 10323 10323
Partials 2138 2138
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
| * SwitchLabel -> Expression ; | ||
| * }</pre} | ||
| * | ||
| * for switch entries in the JLS. The expression in the entry body is |
There was a problem hiding this comment.
Can't this be done in a Validator?
There was a problem hiding this comment.
The problem isn't that the input code is incorrect, but rather that the JavaParser representation for SwitchEntry is (slightly) incorrect. JP Assumes that the body of a SwitchEntry will be a list of statements which was true before switch expressions were added, but is no longer the case for this rule.
The workaround for this (I assume) was to wrap the expression in an ExpressionStmt node which works well, but is not entirely correct according to the JLS since the ExpressionStmt in this specific case can contain any expression, not just those which are legal expression statements according to the JLS, for example below (a lambda is not a valid expression statement as far as I can tell, but the below snippet is still legal Java code):
return switch (o) {
case String s -> (arg) -> System.out.println(arg + s);
case null, default -> (arg) -> {};
};
If I understood the situation correctly, then this means that this case does require some special handling, because JP is breaking the rules slightly.
|
|
||
| String output = LexicalPreservingPrinter.print(switchExpr); | ||
|
|
||
| assertEquals( |
There was a problem hiding this comment.
You can replace assertEquals by TestUtils.assertEqualsStringIgnoringEol
There was a problem hiding this comment.
I've updated the PR with this suggestion
3343a18 to
4090a69
Compare
|
Once again, thank you for your help. |
Fixes #4646.
This PR includes 2 changes to fix the token range of switch expression entries. Before, the token range of the
ExpressionStmt(which is added to the switch entry) was set to the token range of theExpressioncontained in it. This was incorrect, since the token range for theExpressionStmtshould include the;at the end. This could be fixed in theSwitchEntryproduction/method, but I decided to move it into a separate method is a cleaner solution overall.After fixing the token range, the LPP output was better, but still formatted strangely:
This was because of shared logic between the older switch statement entries with
:and the newer switch expression entries in theConcreteSyntaxModel. To fix this, I effectively moved a sharednewline()after the:/->to only affect the switch statement entries and added a space after the->for switch expressions. I also refactored everything a bit to avoid duplicating thespace(), token(ARROW), space()at the end of each->branch and thetoken(COLON), newline()at the end of each:branch.