Add support for `case null, default` in switch and fix concrete syntax model for new switch syntax by johannescoetzee · Pull Request #4364 · javaparser/javaparser

johannescoetzee · 2024-04-03T17:44:46Z

Related issue: #4361

Note: This does not address any of the pattern matching-related changes. I decided to start small to build familiarity with the codebase.

This PR mainly addresses my comment regarding the new case null, default possibility for switch labels by introducing an isDefault field for the switch entry, setting this where appropriate and using this value to distinguish cases for PP and LPP.

While testing the LPP, I realized that the new -> syntax for switch expressions wasn't supported in the ConcreteSyntaxModel, so I've updated that to work for what's included here (which excludes the patterns case).

I see Intellij also made some formatting changes, but I use the style config from this project, so the changes should be correct.

For the code generation part, I added the isDefault field, ran the code generators, then fixed the constructors by hand.

javaparser-core/src/main/java/com/github/javaparser/printer/ConcreteSyntaxModel.java

...ser-core/src/main/java/com/github/javaparser/printer/concretesyntaxmodel/CsmConditional.java

codecov · 2024-04-03T18:21:44Z

Codecov Report

Attention: Patch coverage is 74.54545% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 51.929%. Comparing base (afc5c75) to head (40ab63a).
Report is 10 commits behind head on master.

Additional details and impacted files

@@              Coverage Diff              @@
##            master     #4364       +/-   ##
=============================================
+ Coverage   51.860%   51.929%   +0.069%     
=============================================
  Files          497       503        +6     
  Lines        28326     28369       +43     
  Branches      4916      4920        +4     
=============================================
+ Hits         14690     14732       +42     
+ Misses       11595     11591        -4     
- Partials      2041      2046        +5

Flag	Coverage Δ
AlsoSlowTests	`51.929% <74.545%> (+0.069%)`	⬆️
javaparser-core	`51.929% <74.545%> (+0.069%)`	⬆️
javaparser-symbol-solver	`51.929% <74.545%> (+0.069%)`	⬆️
jdk-10	`51.926% <74.545%> (+0.069%)`	⬆️
jdk-11	`51.926% <74.545%> (+0.069%)`	⬆️
jdk-12	`51.926% <74.545%> (+0.069%)`	⬆️
jdk-13	`51.926% <74.545%> (+0.069%)`	⬆️
jdk-14	`51.926% <74.545%> (+0.069%)`	⬆️
jdk-15	`51.926% <74.545%> (+0.069%)`	⬆️
jdk-16	`51.926% <74.545%> (+0.069%)`	⬆️
jdk-17	`51.926% <74.545%> (+0.069%)`	⬆️
jdk-18	`51.926% <74.545%> (+0.076%)`	⬆️
jdk-8	`51.924% <74.545%> (+0.069%)`	⬆️
jdk-9	`51.926% <74.545%> (+0.069%)`	⬆️
macos-latest	`51.919% <74.545%> (+0.065%)`	⬆️
ubuntu-latest	`51.922% <74.545%> (+0.069%)`	⬆️
windows-latest	`51.908% <74.545%> (+0.069%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ava/com/github/javaparser/ParserConfiguration.java	`86.231% <100.000%> (+0.305%)`	⬆️
...or/language_level_validations/Java19Validator.java	`100.000% <100.000%> (ø)`
...r/language_level_validations/Java1_0Validator.java	`97.142% <100.000%> (+0.219%)`	⬆️
...or/language_level_validations/Java20Validator.java	`100.000% <100.000%> (ø)`
...or/language_level_validations/Java21Validator.java	`100.000% <100.000%> (ø)`
.../validator/postprocessors/Java19PostProcessor.java	`100.000% <100.000%> (ø)`
.../validator/postprocessors/Java20PostProcessor.java	`100.000% <100.000%> (ø)`
.../validator/postprocessors/Java21PostProcessor.java	`100.000% <100.000%> (ø)`
...thub/javaparser/metamodel/JavaParserMetaModel.java	`99.460% <100.000%> (+0.002%)`	⬆️
...hub/javaparser/metamodel/SwitchEntryMetaModel.java	`100.000% <ø> (ø)`
... and 10 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6efbbae...40ab63a. Read the comment docs.

johannescoetzee · 2024-04-03T19:33:33Z

I see there's a single test failing with

Codecov: Failed to write uploader binary: EPERM: operation not permitted, open 'D:\a\_actions\codecov\codecov-action\v4.1.1\dist\codecov.exe'

which looks unrelated to this change, but I don' have permission to rerun the action.

javaparser-core/src/main/java/com/github/javaparser/metamodel/SwitchEntryMetaModel.java

javaparser-core/src/main/java/com/github/javaparser/metamodel/JavaParserMetaModel.java

javaparser-core/src/main/java/com/github/javaparser/ast/stmt/SwitchEntry.java

javaparser-core/src/main/java/com/github/javaparser/ast/observer/ObservableProperty.java

javaparser-core/src/main/java/com/github/javaparser/printer/DefaultPrettyPrinterVisitor.java

javaparser-core/src/main/java/com/github/javaparser/printer/PrettyPrintVisitor.java

...-generators/src/main/java/com/github/javaparser/generator/core/other/TokenKindGenerator.java

...ser-core/src/main/java/com/github/javaparser/printer/concretesyntaxmodel/CsmConditional.java

...ser-core/src/main/java/com/github/javaparser/printer/lexicalpreservation/changes/Change.java

javaparser-core/src/main/javacc/java.jj

jlerbsc · 2024-04-04T07:10:09Z

It's a great job. Thank you very much. Your approach is exactly what I'm looking for. Let's take things one step at a time, because I don't necessarily have the time or the skills to analyse very large PR files. I also prefer it when you first explain what you want to do and what the alternatives are (which you've done perfectly on the other related subjects) because that gives me a slightly clearer idea of the content of the PR.

I'll let you answer my questions before I integrate your work. However, I think that the validation part of the new syntax is missing (validator because it should only be authorised from a certain version of Java). It also seems to me that tests on the LPP are missing.

Just so you know, before integrating this PR, I'm going to publish a new version of JP.

...arser/printer/lexicalpreservation/transformations/ast/body/StatementTransformationsTest.java

johannescoetzee · 2024-04-04T11:31:52Z

Thank you for the quick and thorough review!

Regarding LPP tests, I did add some simple ones (I added a comment to those so that you can see them in the conversation history). If you'd like me to add more involved ones, please let me know and I'll be happy to do so.

I didn't add the validation part to avoid conflicts with #4357, but if this is going to go in first, I'll add that here and then we can deal with conflicts later.

Regarding a new version, that's no problem at all! I don't know what you have in mind for the JavaParser release schedule, but in my opinion releasing all of the non-preview java 21 features at once would be ideal!

jlerbsc · 2024-04-04T16:12:05Z

I didn't add the validation part to avoid conflicts with #4357, but if this is going to go in first, I'll add that here and then we can deal with conflicts later.

I'm not going to validate the PR #4357 because it contains too many unrelated changes and I'm therefore unable to validate its content. There are also points that need to be reworked. It's a big job but I don't want to risk destabilising the project.

jlerbsc · 2024-04-04T16:14:21Z

Regarding a new version, that's no problem at all! I don't know what you have in mind for the JavaParser release schedule, but in my opinion releasing all of the non-preview java 21 features at once would be ideal!

I agree with this.

johannescoetzee · 2024-04-04T18:46:42Z

I'm not going to validate the PR #4357

Good to know. I'll update this PR with the validation tomorrow then.

javaparser-core/src/main/javacc/java.jj

johannescoetzee · 2024-04-05T15:35:45Z

And just a quick general update: another task came up that took up my time today, hence no update here. I'll be working on this PR again on Tuesday, however.

And thanks again for the quick feedback and open discussion!

jlerbsc · 2024-04-05T15:51:37Z

And just a quick general update: another task came up that took up my time today, hence no update here. I'll be working on this PR again on Tuesday, however.

And thanks again for the quick feedback and open discussion!

There's no hurry, I've got other things to do too.

johannescoetzee · 2024-04-10T16:15:10Z

I've updated the PR with changes that address the feedback given and I've mostly mentioned specific changes in the conversation threads where they're relevant.

There are a few extra additions from code generation, however (specifically the *Visitor changes). The codegen scripts output is formatted differently from the checked-in source and since the project formatting is inconsistent, I couldn't fix these with automated formatting. After running the generators, I went through the diff manually to separate formatting changes from actual generated code and missed these in the first iteration.

If there's a better way to do this, please let me know! The eventual fix would be to automate formatting so that the project as a whole is consistent, but that's a bigger effort.

johannescoetzee · 2024-04-10T16:20:53Z

...rser-core-testing/src/test/java/com/github/javaparser/ast/validator/Java14ValidatorTest.java

            void recordDeclaration() {
                String s = "record X() { }";
                ParseResult<CompilationUnit> result = javaParser.parse(COMPILATION_UNIT, provider(s));
-                TestUtils.assertProblems(result, "(line 1,col 1) Record Declarations are not supported. Pay attention that this feature is supported starting from 'JAVA_14' language level. If you need that feature the language level must be configured in the configuration before parsing the source files.");


This was Intellij's auto formatting. I could change it back, but the change is more consistent with how other tests do it.

javaparser-core-testing/src/test/java/com/github/javaparser/printer/PrettyPrinterTest.java

jlerbsc · 2024-04-10T18:47:24Z

...rser-core-testing/src/test/java/com/github/javaparser/ast/validator/Java21ValidatorTest.java

+import org.junit.jupiter.api.Test;
+
+import static com.github.javaparser.ParseStart.EXPRESSION;
+import static com.github.javaparser.ParserConfiguration.LanguageLevel.JAVA_21_INCOMPLETE;


Can you rename this to JAVA_21.

jlerbsc · 2024-04-10T19:01:01Z

javaparser-core/src/main/javacc/java.jj

-			","
-			"default" { isDefault = true; }
-         |
+			LOOKAHEAD({!(getToken(2).image == "," && getToken(3).image == "default")})


Can you explain this new rule?

I added an explanation as a comment in code as well, but to repeat here for convenience:

Since null is also a conditional expression, a lookahead is necessary to determine
whether this is the "null, default" case, or whether the null is a regular
conditional expression. The check is negated to maintain the ordering in the JLS.

In the "null, default" case, the next 3 tokens will be "null" "," "default". Since
"default" can only be preceded by "," in this specific case (as "default" cannot
appear in a list elsewhere), it is sufficient to check that "," "default" appear
in the correct order. Since the only label that can appear before the "default"
is the single-token "null", the "," token must also be the second token.

jlerbsc · 2024-04-11T12:47:40Z

Do you have anything else to add? If not, I'll merge your PR.

johannescoetzee · 2024-04-11T14:22:37Z

If you're happy with the changes here, then I think this PR is done. There are things like the validators, lookahead logic for the different cases, and the CSM that I'll have to change again when adding switch pattern support, but I think it's better to do that when actually adding the relevant features.

jlerbsc · 2024-04-11T18:38:15Z

Thank you for this development.

johannescoetzee added 4 commits April 2, 2024 15:45

Add SwitchEntry.isDefault and metamodel gen

ae1bec2

Add switch entry codegen

9621f4a

Add PrettyPrinter support and tests for null/default switch cases

60dcf2e

Fix concrete syntax model for switch

09fdbe0

johannescoetzee commented Apr 3, 2024

View reviewed changes

javaparser-core/src/main/java/com/github/javaparser/printer/ConcreteSyntaxModel.java Outdated Show resolved Hide resolved

johannescoetzee commented Apr 3, 2024

View reviewed changes

...ser-core/src/main/java/com/github/javaparser/printer/concretesyntaxmodel/CsmConditional.java Outdated Show resolved Hide resolved

Merge branch 'master' into johannes/switch-null-default

92f3a37