PatternExpr -> TypePatternExpr refactor in preparation for record pattern implementation #4387

johannescoetzee · 2024-04-24T16:21:09Z

See #4385 for details

This is a WIP PR and is still has some TODOs:

Documentation in comments (I intend to rewrite history and add those with rebasing, so for the final PR each commit will be properly documented)
A comprehensive review of where TypePatternExpr is used
Stylistic cleanup (e.g. AbstractJavaParserContext copyright comment)
Possibly some changes to the ConcreteSyntaxModel, although this might only be true once the record pattern PR is in.

…ion methods

codecov · 2024-04-24T16:42:03Z

Codecov Report

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

Project coverage is 51.902%. Comparing base (7df03e3) to head (b59fc92).
Report is 15 commits behind head on master.

❗ Current head b59fc92 differs from pull request most recent head 2a364d8. Consider uploading reports for the commit 2a364d8 to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##            master     #4387       +/-   ##
=============================================
- Coverage   51.912%   51.902%   -0.011%     
=============================================
  Files          503       505        +2     
  Lines        28442     28461       +19     
  Branches      4934      4934               
=============================================
+ Hits         14765     14772        +7     
- Misses       11625     11636       +11     
- Partials      2052      2053        +1

Flag	Coverage Δ
AlsoSlowTests	`51.902% <48.543%> (-0.011%)`	⬇️
javaparser-core	`51.902% <48.543%> (-0.011%)`	⬇️
javaparser-symbol-solver	`51.902% <48.543%> (-0.011%)`	⬇️
jdk-10	`51.888% <48.543%> (-0.021%)`	⬇️
jdk-11	`51.899% <48.543%> (-0.011%)`	⬇️
jdk-12	`51.888% <48.543%> (-0.021%)`	⬇️
jdk-13	`51.888% <48.543%> (-0.021%)`	⬇️
jdk-14	`51.888% <48.543%> (-0.021%)`	⬇️
jdk-15	`51.899% <48.543%> (-0.011%)`	⬇️
jdk-16	`51.899% <48.543%> (-0.011%)`	⬇️
jdk-17	`51.888% <48.543%> (-0.021%)`	⬇️
jdk-18	`51.899% <48.543%> (-0.011%)`	⬇️
jdk-8	`51.897% <48.543%> (-0.011%)`	⬇️
jdk-9	`51.888% <48.543%> (-0.021%)`	⬇️
macos-latest	`51.895% <48.543%> (-0.011%)`	⬇️
ubuntu-latest	`51.885% <48.543%> (-0.011%)`	⬇️
windows-latest	`51.881% <48.543%> (-0.011%)`	⬇️

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

Files	Coverage Δ
...aparser/ast/visitor/GenericListVisitorAdapter.java	`40.047% <ø> (ø)`
.../javaparser/ast/visitor/GenericVisitorAdapter.java	`39.086% <ø> (ø)`
...parser/ast/visitor/GenericVisitorWithDefaults.java	`98.058% <ø> (ø)`
...github/javaparser/ast/visitor/HashCodeVisitor.java	`8.490% <ø> (ø)`
...vaparser/ast/visitor/NoCommentHashCodeVisitor.java	`67.289% <ø> (ø)`
...thub/javaparser/ast/visitor/NodeFinderVisitor.java	`3.915% <ø> (ø)`
...arser/ast/visitor/ObjectIdentityEqualsVisitor.java	`99.029% <ø> (ø)`
...ser/ast/visitor/ObjectIdentityHashCodeVisitor.java	`97.087% <ø> (ø)`
...hub/javaparser/ast/visitor/VoidVisitorAdapter.java	`99.043% <ø> (ø)`
...avaparser/ast/visitor/VoidVisitorWithDefaults.java	`98.029% <ø> (ø)`
... and 17 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 b311fd5...2a364d8. Read the comment docs.

jlerbsc · 2024-04-25T06:35:36Z

javaparser-core/src/main/java/com/github/javaparser/ast/expr/Expression.java

@@ -881,4 +877,19 @@ public final boolean elidesTypeArguments() {
        NodeWithTypeArguments nwta = (NodeWithTypeArguments) this;
        return scope.elidesTypeArguments() && (!nwta.getTypeArguments().isPresent() || nwta.isUsingDiamondOperator());
    }
+


This appears to be generated by the TypeCastingGenerator, whereas it seems to have been added manually.

I think the same applies as the comment I left below regarding formatting and project configuration. I think this is generated formatting which just hasn't been included before.

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

jlerbsc · 2024-04-25T06:46:31Z

javaparser-core/src/main/java/com/github/javaparser/resolution/Context.java


    /**
     * @return The parent context, if there is one. For example, a method exists within a compilation unit.
     */
    Optional<Context> getParent();

-


Can you adapt the project configuration in your IDE so as not to multiply the code modifications?

I'm using the included dev-files/JavaParser-idea.xml for my project configuration, so my IDE formatting should be up to project standards. I suspect this is a change from code generator output. When I run the code generators, it effectively rewrites every source file, even if there isn't anything new being added, resulting in a lot of style changes that aren't relevant to anything being done. I've been cherry-picking files that are actually relevant, so with this being new, it wouldn't have been included before.

The proper fix is to set up automatic formatting and run it as a commit hook, which would avoid any unnecessary style changes, but I'm not sure what the best way is to do that, both in which tools to use and how to minimise impact on open PRs. Until some kind of automatic formatting exists, I think manually removing generated formatting changes from PRs is more effort than it's worth, especially since these would have to be repeated for every new codegen PR.

javaparser-core/src/main/java/com/github/javaparser/ast/expr/PatternExpr.java

jlerbsc · 2024-04-25T06:52:08Z

javaparser-core/src/main/java/com/github/javaparser/ast/expr/PatternExpr.java

+    public PatternExpr() {
+    }
+
+    @Override


Is it normal that there is no @generated annotation? What will happen with subsequent generations? Isn't there a risk of inconsistency in the code?

This is something I've noticed as well. Sometimes the @Generated annotation is added and sometimes not. It's also not entirely consistent across runs: I've seen @Generated added on a second or later codegen run, but I've never seen it removed.

As far as I can tell, this annotation doesn't have any effect on the generators themselves, so the risk is that contributors will edit generated code without realising it. This is not an ideal situation, but I don't know what the solution is yet. I'm still learning how the generators work myself, so I'm hoping that adding the RecordPatternExpr class later will answer some of the questions I have as well.

jlerbsc · 2024-04-25T07:06:36Z

javaparser-core-testing/src/test/java/com/github/javaparser/ast/expr/InstanceOfExprTest.java

-        TypePatternExpr typePatternExpr = expr.getPattern().get();
+        PatternExpr patternExpr = expr.getPattern().get();
+        assertInstanceOf(TypePatternExpr.class, patternExpr);
+        TypePatternExpr typePatternExpr = (TypePatternExpr) patternExpr;


Can you use the asTypePatternExpr() method to avoid casts?

jlerbsc · 2024-04-25T07:07:01Z

javaparser-core-testing/src/test/java/com/github/javaparser/ast/expr/InstanceOfExprTest.java

-        TypePatternExpr typePatternExpr = expr.getPattern().get();
+        PatternExpr patternExpr = expr.getPattern().get();
+        assertInstanceOf(TypePatternExpr.class, patternExpr);
+        TypePatternExpr typePatternExpr = (TypePatternExpr) patternExpr;


jlerbsc · 2024-04-25T07:08:54Z

javaparser-core/src/main/java/com/github/javaparser/ast/expr/InstanceOfExpr.java

@@ -142,7 +142,10 @@ public Optional<SimpleName> getName() {
        if (pattern == null) {
            return Optional.empty();
        }
-        return Optional.of(pattern.getName());
+        if (!(pattern instanceof TypePatternExpr)) {


Can you use the isTypePatternExpr() method to avoid instanceof?

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

javaparser-core/src/main/java/com/github/javaparser/ast/expr/InstanceOfExpr.java

+        if (!(pattern instanceof TypePatternExpr)) {
+            return Optional.empty();
+        }
+        return Optional.of(((TypePatternExpr) pattern).getName());


Can you use the asTypePatternExpr() method?

javaparser-core/src/main/java/com/github/javaparser/resolution/Context.java

jlerbsc · 2024-04-25T07:13:02Z

...a/com/github/javaparser/symbolsolver/javaparsermodel/contexts/AbstractJavaParserContext.java

- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU Lesser General Public License for more details.
- */
+///*


This modification is strange.

I agree. It's probably something I added by accident with some IDE shortcut, but I've fixed it now.

...a/com/github/javaparser/symbolsolver/javaparsermodel/contexts/AbstractJavaParserContext.java

jlerbsc · 2024-04-25T07:24:45Z

Very good work. Thank you very much.

Based on your experience, you could create documentation in the wiki for using the generators (when, how, why use them?).
For the moment, to my knowledge, only this documentation is available (https://javaparser.org/code-generation-and-maven-in-javaparser/). It's not enough to ensure the project's long-term future, and it's a part that I'm not familiar with...

johannescoetzee · 2024-05-02T10:38:38Z

Based on your experience, you could create documentation in the wiki for using the generators (when, how, why use them?). For the moment, to my knowledge, only this documentation is available (https://javaparser.org/code-generation-and-maven-in-javaparser/). It's not enough to ensure the project's long-term future, and it's a part that I'm not familiar with...

I had the same idea when I started with this, so I kept detailed notes of everything I did for code generation for this PR. Since I'm going to add a new node type in the follow-up PR, I want to use those notes as a reference to confirm that they're correct. Once I've confirmed that, I'll update the wiki page with everything I know about code generation.

johannescoetzee · 2024-05-02T11:06:48Z

@jlerbsc I've updated the PR to address feedback given here (although I left the whitespace changes in with a discussion about why; please let me know if you disagree). I've also included a PatternExprTest class which should make codecov bot happy while providing some minimal tests for the auto-generated methods.

At this point, I no longer consider the PR a WIP since I think it makes more sense to postpone further logic changes to the record patterns pr.

johannescoetzee · 2024-05-02T11:09:43Z

There's something going on with the github runners:

Run actions/setup-java@v4
  with:
    distribution: zulu
    java-version: 12
    java-package: jdk
    check-latest: false
    server-id: github
    server-username: GITHUB_ACTOR
    server-password: GITHUB_TOKEN
    overwrite-settings: true
    job-status: success
    token: ***
  env:
    OS: macos-latest
    JDK: 12
Installed distributions
  Trying to resolve the latest version from remote
  Error: Could not find satisfied version for semver 12. 
  Available versions: 22.0.1+8, 22.0.0+36, 21.0.3+9, 21.0.2+13, 21.0.1+12, 20.0.2+9, 19.0.2+7, 18.0.2+1, 17.0.11+9, 17.0.10+7, 17.0.3+7, 16.0.2+7, 15.0.10+5, 13.0.14+5, 11.0.23+9, 11.0.15+10, 8.0.412+8, 8.0.332+9, 22.0.0+36, 21.0.2+13, 21.0.0+35, 20.0.1+9, 20.0.0+36, 19.0.1+10, 19.0.0+36, 18.0.2+9, 18.0.1+10, 18.0.0+37, 17.0.10+7, 17.0.9+8, 17.0.8+1, 17.0.8+7, 17.0.7+7, 17.0.6+10, 17.0.5+8, 17.0.4+1, 17.0.4+8, 17.0.2+8, 17.0.1+12, 17.0.0+35, 16.0.1+9, 16.0.1+9, 16.0.0+36, 15.0.9+5, 15.0.8+4, 15.0.7+4, 15.0.6+5, 15.0.5+3, 15.0.4+5, 15.0.3+3, 15.0.2+7, 15.0.1+9, 15.0.1+9, 13.0.13+5, 13.0.12+4, 13.0.11+4, 13.0.10+5, 13.0.9+3, 13.0.8+5, 13.0.7+5, 13.0.6+5, 13.0.5+1, 13.0.5+1, 13.0.5+1, 11.0.22+7, 11.0.21+9, 11.0.20+1, 11.0.20+8, 11.0.19+7, 11.0.18+10, 11.0.17+8, 11.0.16+1, 11.0.16+8, 11.0.14+1, 11.0.14+9, 11.0.13+8, 11.0.12+7, 11.0.11+9, 11.0.10+9, 11.0.9+1, 11.0.9+1, 11.0.9+1, 8.0.402+6, 8.0.392+8, 8.0.382+5, 8.0.372+7, 8.0.362+9, 8.0.362+8, 8.0.352+8, 8.0.345+1, 8.0.342+7, 8.0.322+6, 8.0.312+7, 8.0.302+8, 8.0.292+10, 8.0.282+8, 8.0.275+1

johannescoetzee · 2024-05-02T11:11:05Z

It looks like java versions 9, 10, 12, and 14 are no longer installed correctly on the macos runners. I'll have a look and see if I can fix it

johannescoetzee · 2024-05-02T12:23:10Z

I think https://github.blog/changelog/2024-04-01-macos-14-sonoma-is-generally-available-and-the-latest-macos-runner-image/ is the culprit. The new macos-latest runners are M1 (therefore ARM) based (https://github.com/actions/runner-images#available-images), and there aren't azul JDK ARM releases for the failing versions: https://www.azul.com/downloads/?os=macos&architecture=arm-64-bit&package=jdk&show-old-builds=true#zulu

jlerbsc · 2024-05-02T12:57:54Z

I've opened an exit on the project. We may have to change distrition soon.

jlerbsc · 2024-05-02T12:59:19Z

Do you have anything else to add to this PR?

johannescoetzee · 2024-05-02T13:02:20Z

Do you have anything else to add to this PR?

If you are satisfied with the changes here, then I don't have anything further to add for now. Everything else would be better suited to the record pattern pr

johannescoetzee added 11 commits April 24, 2024 14:42

Intellij refactor: PatternExpr -> TypePatternExpr

c04439b

Refactor: PatternExpr -> TypePatternExpr in java.jj

42405a2

Add TypePatternExpr codegen

bbc8f26

Remove now-unused PatternExprMetaModel and Expression.XPatternExpress…

4b5e2fd

…ion methods

Add PatternExpr abstract class and add it to the MetaModelGenerator

ca9f594

Add PatternExpr codegen

8ef52f9

Make TypePatternExpr a subclass of PatternExpr with codegen

07dd530

Update grammar to reflect new pattern inheritance structure

0667001

Move TypePatternExpr type to PatternExpr

7611bf5

Expect PatternExpr instead of TypePatternExpr in instanceof

d3fb640

Replace TypePatternExpr with PatternExpr in javaparsermodel.contexts

b59fc92

jlerbsc reviewed Apr 25, 2024

View reviewed changes

javaparser-core/src/main/java/com/github/javaparser/metamodel/TypePatternExprMetaModel.java Outdated Show resolved Hide resolved

jlerbsc reviewed Apr 25, 2024

View reviewed changes

javaparser-core/src/main/java/com/github/javaparser/ast/expr/PatternExpr.java Outdated Show resolved Hide resolved

jlerbsc reviewed Apr 25, 2024

View reviewed changes

javaparser-core/src/main/java/com/github/javaparser/resolution/Context.java Outdated Show resolved Hide resolved

jlerbsc reviewed Apr 25, 2024

View reviewed changes

...a/com/github/javaparser/symbolsolver/javaparsermodel/contexts/AbstractJavaParserContext.java Outdated Show resolved Hide resolved

johannescoetzee marked this pull request as draft April 29, 2024 11:28

johannescoetzee added 4 commits April 29, 2024 17:02

Replace TypePatternExpr cast and instanceof where possible

c33390a

Fix copyright feedback

c2f3eea

Remove unused imports

2a364d8

Merge branch 'master' into johannes/record-patterns

3c506b3

Add documentation for PatternExpr

3f33cef

Add tests for generated pattern methods

bf90de2

johannescoetzee marked this pull request as ready for review May 2, 2024 11:03

johannescoetzee changed the title ~~[WIP] PatternExpr -> TypePatternExpr refactor in preparation for record pattern implementation~~ PatternExpr -> TypePatternExpr refactor in preparation for record pattern implementation May 2, 2024

Merge branch 'master' into johannes/record-patterns

aab474b

jlerbsc merged commit 3dcb1e0 into javaparser:master May 2, 2024
34 of 38 checks passed

jlerbsc added this to the next release milestone May 2, 2024

jlerbsc added the PR: Added A PR that introduces new behaviour (e.g. functionality, tests) label May 2, 2024

This was referenced May 2, 2024

Spotless formatting implementation proposal #4408

Closed

Fix missed generated code from PatternExpr refactor #4414

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PatternExpr -> TypePatternExpr refactor in preparation for record pattern implementation #4387

PatternExpr -> TypePatternExpr refactor in preparation for record pattern implementation #4387

johannescoetzee commented Apr 24, 2024

codecov bot commented Apr 24, 2024 •

edited

Loading

jlerbsc Apr 25, 2024

johannescoetzee May 2, 2024

jlerbsc Apr 25, 2024

johannescoetzee May 2, 2024

jlerbsc Apr 25, 2024

johannescoetzee May 2, 2024

jlerbsc Apr 25, 2024

jlerbsc Apr 25, 2024

jlerbsc Apr 25, 2024

jlerbsc Apr 25, 2024

jlerbsc Apr 25, 2024

johannescoetzee May 2, 2024

jlerbsc commented Apr 25, 2024

johannescoetzee commented May 2, 2024

johannescoetzee commented May 2, 2024

johannescoetzee commented May 2, 2024

johannescoetzee commented May 2, 2024

johannescoetzee commented May 2, 2024

jlerbsc commented May 2, 2024

jlerbsc commented May 2, 2024

johannescoetzee commented May 2, 2024

PatternExpr -> TypePatternExpr refactor in preparation for record pattern implementation #4387

PatternExpr -> TypePatternExpr refactor in preparation for record pattern implementation #4387

Conversation

johannescoetzee commented Apr 24, 2024

codecov bot commented Apr 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlerbsc commented Apr 25, 2024

johannescoetzee commented May 2, 2024

johannescoetzee commented May 2, 2024

johannescoetzee commented May 2, 2024

johannescoetzee commented May 2, 2024

johannescoetzee commented May 2, 2024

jlerbsc commented May 2, 2024

jlerbsc commented May 2, 2024

johannescoetzee commented May 2, 2024

codecov bot commented Apr 24, 2024 •

edited

Loading