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

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

Merged
merged 18 commits into from
May 2, 2024

Conversation

johannescoetzee
Copy link
Contributor

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.

Copy link

codecov bot commented Apr 24, 2024

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

Impacted file tree graph

@@              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.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public PatternExpr() {
}

@Override
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

TypePatternExpr typePatternExpr = expr.getPattern().get();
PatternExpr patternExpr = expr.getPattern().get();
assertInstanceOf(TypePatternExpr.class, patternExpr);
TypePatternExpr typePatternExpr = (TypePatternExpr) patternExpr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

TypePatternExpr typePatternExpr = expr.getPattern().get();
PatternExpr patternExpr = expr.getPattern().get();
assertInstanceOf(TypePatternExpr.class, patternExpr);
TypePatternExpr typePatternExpr = (TypePatternExpr) patternExpr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idem.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the asTypePatternExpr() method?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This modification is strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jlerbsc
Copy link
Collaborator

jlerbsc commented Apr 25, 2024

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 johannescoetzee marked this pull request as draft April 29, 2024 11:28
@johannescoetzee
Copy link
Contributor Author

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 johannescoetzee marked this pull request as ready for review May 2, 2024 11:03
@johannescoetzee
Copy link
Contributor Author

@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 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
@johannescoetzee
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@jlerbsc
Copy link
Collaborator

jlerbsc commented May 2, 2024

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

@jlerbsc
Copy link
Collaborator

jlerbsc commented May 2, 2024

Do you have anything else to add to this PR?

@johannescoetzee
Copy link
Contributor Author

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

@jlerbsc jlerbsc merged commit 3dcb1e0 into javaparser:master May 2, 2024
34 of 38 checks passed
@jlerbsc jlerbsc added this to the next release milestone May 2, 2024
@jlerbsc jlerbsc added the PR: Added A PR that introduces new behaviour (e.g. functionality, tests) label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Added A PR that introduces new behaviour (e.g. functionality, tests)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants