ignore parse errors below imports (to allow the plugin to work with Java > 18) #83
Conversation
… > 18) - unfortunately the used https://github.com/javaparser/javaparser is stuck with Java 18 (in latest version 3.25.7) - but it turned out that even in case of a parse error somewhere in a top level declaration the imports are correctly parsed and available - introduced <ignoreIrrelevantParseErrors> which is `false` for now but allows to ignore the parse errors in the top level declarations (they shouldn't affect import sorting at all) - introduced a test case with Java 21 record deconstruction
|
To give it a bit more context: I had a general look how to make the plugin work with Java 21. Then I discovered that the imports are parsed despite parse errors for not yet known Java syntax. |
There was a problem hiding this comment.
Overall, I think this is a great feature. Thanks for contributing it @janScheible ! I have some suggestions, and some questions.
In addition to those I said inline already, I'm curious how this interacts with the removeUnused. If parsing stops because of errors right away, does that cause the plugin to believe that all imports are unused, and cause all imports to be removed? We may want to warn, or explicitly prevent, the use of removeUnused option if this new option is used, if so.
src/main/java/net/revelc/code/impsort/maven/plugin/AbstractImpSortMojo.java
Outdated
Show resolved
Hide resolved
| // should not happen but let's simply give up and return the problems as they are | ||
| return problems; |
There was a problem hiding this comment.
This could happen if a file contained only imports, but no class declaration, right?
What happens if the error is in the package declaration above the imports? Would this feature just ignore the imports that come after?
There was a problem hiding this comment.
Yes, I tested it. The parser is happy with a file without a top level declaration. I changed the comment in the code accordingly.
If the package declaration would cause a parse error the plugin would still fail (as it did before my changes).
| static LanguageLevel getLanguageLevel(String compliance, boolean ignoreIrrelevantParseErrors) { | ||
| if (compliance == null || compliance.trim().isEmpty() || ignoreIrrelevantParseErrors) { |
There was a problem hiding this comment.
I'm not sure why the presence of this new option means that the parser should ignore the user-specified compliance level.
There was a problem hiding this comment.
The motivation is to just have <ignoreParseErrorsBelowImports>true</ignoreParseErrorsBelowImports> in the pom.xml and never look back. That means that Java versions that would otherwise make the plugin fail because the version is not yet known to the parser, must be dealt with (fall back to LanguageLevel.POPULAR).
There was a problem hiding this comment.
I think it's reasonable to fall back to the POPULAR one when one is specified that is not recognized (which guarantees errors) AND you have this option set.
However, it should not fall back to POPULAR and override the user configuration just because you have this option set, when there's a recognized version.
There was a problem hiding this comment.
This is exactly how it is implemented after my rework:
private static LanguageLevel parseLanguageLevel(String langLevel,
boolean ignoreParseErrorsBelowImports) {
return Stream.of(LanguageLevel.values()).filter(ll -> ll.name().equals(langLevel)).findFirst()
.or(() -> ignoreParseErrorsBelowImports ? Optional.of(LanguageLevel.POPULAR)
: Optional.empty())
.orElseThrow(() -> new IllegalArgumentException("No enum constant "
+ LanguageLevel.class.getName().replace('$', '.') + "." + langLevel));
}… > 18) - ignoreParseErrorsBelowImports is much more descriptive than ignoreIrrelevantParseErrors - LanguageLevel.POPULAR is now only used when ignoreParseErrorsBelowImports=true and the used Java version would be unsupported by the parser - Result now also contains the problems (mainly for the unit test)
You're absolutely right. It conflicts with |
janScheible
changed the title
|
Am I missing something here? If you take your project and run with java 21 as a build time, it works fine now. The moment you stay I want to target jdk 19 or above runtime, it fails enum check and javaparser then does not work at all. So what exactly is this doing for me? Just ignoring it? I can skip the plugin and already started globally doing so. As long as Javaparser team is so reluctant to just let the enums be and work its a stalemate in my opinion and I don't see why trying to do something like this is beneficial that I don't get by already turning it off. And lets face it, teams that are targeting these higher jdks are certainly not the ones that need help making sure their code is formatted. I'm sure I'm missing something here but that is my take. Do note, I actually started working with Javaparser team due to this and have already had many PRs merged but they want actual logic for jdk 18 preview, and all for 19, 20, 21, and 22 before any of that is even allowed but at same time said they lost most contributors to help and I also don't have time either. If you really want impsort on those higher versions, just use spotless and possibly open rewrite. Or better yet, go to the source and help them get it compliant. |
|
Summary of my understanding of the problem:
What I intend to achieve with my pull request:
Limitations of my pull request:
Potential solutions:
From a user's point of view I think that doing nothing would be really bad for the plugin. Users will be forced to either turn it off and/or switch to another solution. My pull request (a) has the advantage of being comparable simple. |
|
@janScheible Thank you for incredible detail. This is helpful to understand full motivation. For my use cases I've went full on just disabling it and IMO that is a worst case scenario. I think this sounds ok now. Up to @ctubbsii to agree at this point and seems that was close, just a question that is outstanding you answered and has not been accepted yet. I went ahead and let the build run here. |
|
I'm going to revisit this after the new year. I think there's some good stuff in here, but I want to examine the edge cases a bit more closely to understand the user experience under a few common scenarios. Thanks, @janScheible for putting all this work in to this so far. |
Hi! Any news regarding the merge request? |
|
@janScheible Sorry, not yet. I hope to get to it soon. In the meantime, can you resolve the conflicts? |
I took care of this conflict. |
Thank you @hazendaz. :-) |
hazendaz
dismissed
ctubbsii’s stale review
Issue was addressed with prevention from having both flags set.
|
Gave my thumbs up here. I believe all outstanding concerns have been addressed but will wait a bit longer for @ctubbsii to get back on this. |
|
I know @ctubbsii hasn't been around much lately. I see he is back doing some reviews last few days. I'll let this sit until Sunday. If no word by then I'd like to go ahead and merge and move for a release on this. Although I had hoped javaparser sped up some they really haven't and are still only to java 18 when I checked last week. So I think this is important as many are moving really fast to java 21. |
|
@hazendaz Sunday has passed already, any chance to get it merged and release? |
|
When will you deploy it to Maven Central? |
|
taking care of the release in a few minutes. |
|
I'm still getting parser errors with v1.10.0 <plugin>
<groupId>net.revelc.code</groupId>
<artifactId>impsort-maven-plugin</artifactId>
<version>1.10.0</version>
<configuration>
<removeUnused>true</removeUnused>
<staticGroups>*</staticGroups>
<groups>java.,javax.,org.,com.,*</groups>
</configuration>
<executions>
<execution>
<goals>
<goal>sort</goal>
</goals>
</execution>
</executions>
</plugin> |
|
Oh and |
|
@delanym you have to add |
|
@delanym One other thing, if you set impsort.compliance (assuming in properties section) to 17 and your code failed, you likely have a bad class that you should look at or introduced code > 17 that cannot work. The plugin on version 1.9.0 worked fine with java 21 even targeting java 21 so long as the compliance level was set to 17 and there was in fact no java 21 code being used. As @janScheible noted, if you have in fact added java 19+ (javaparser supports to 18 in 1.10.0), then you need the other flag as it is not a default of true. |
|
I am using 21 language constructs. I'm sorry to say it still fails to parse with this config: <properties>
<impsort.compliance>21</impsort.compliance>
</properties>
<plugin>
<groupId>net.revelc.code</groupId>
<artifactId>impsort-maven-plugin</artifactId>
<version>1.10.0</version>
<configuration>
<ignoreParseErrorsBelowImports>true</ignoreParseErrorsBelowImports>
<!-- <removeUnused>true</removeUnused>-->
<staticGroups>*</staticGroups>
<groups>java.,javax.,org.,com.,*</groups>
</configuration>
<executions>
<execution>
<goals>
<goal>sort</goal>
</goals>
</execution>
</executions>
</plugin> |
|
Be nice if I could ask |
|
I should mention the plugin sorts import statements in files in earlier modules before failing on the same file every time in a subsequent reactor module. So I tried skipping that module, but then the fail occurs in the same way on another module further down the DAG. Proprietary code. There's nothing particularly striking about these files though. |
|
Will be releasing this again tomorrow as javaparser has now released java 21 support. In the meantime, try overriding javaparser to the latest release as a dependency to the plugin. See if that fixes the issue. If not, could you possibly create a small reproducible project to cause the issue? |
|
@hazendaz It works. Without the javaparser's latest version it fails in this kind of code: Can we have a new version please? |
|
Yes will get it out tomorrow evening. Thanks for checking.
Sent from my Verizon, Samsung Galaxy smartphone
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Amir Moradabadi ***@***.***>
Sent: Tuesday, June 4, 2024 6:00:52 PM
To: revelc/impsort-maven-plugin ***@***.***>
Cc: Jeremy Landis ***@***.***>; Mention ***@***.***>
Subject: Re: [revelc/impsort-maven-plugin] ignore parse errors below imports (to allow the plugin to work with Java > 18) (PR #83)
@hazendaz<https://github.com/hazendaz> It works.
<plugin>
<groupId>net.revelc.code</groupId>
<artifactId>impsort-maven-plugin</artifactId>
<version>1.10.0</version>
<configuration>
<compliance>21</compliance>
</configuration>
<dependencies>
<dependency>
<groupId>com.github.javaparser</groupId>
<artifactId>javaparser-core</artifactId>
<version>3.26.0</version>
</dependency>
</dependencies>
</plugin>
Without the javaparser's latest version it fails in this kind of code:
var person = switch (myEnum){
case A, B -> new Person("A B");
case null, default -> new Person("Default");
};
Can we have a new version please?
—
Reply to this email directly, view it on GitHub<#83 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAHODI5N7E6KJ7K7NILQ3WTZFY2JJAVCNFSM6AAAAABAH7PZL2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBYGQ4DIMBWHE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Javaparser 3.26.0 can't handle a normal switch statement and its killing impsort plugin. |
Linking javaparser ticket javaparser/javaparser#4455 |
|
new release 1.11.0 has been pushed to central. |
<ignoreIrrelevantParseErrors>which isfalsefor now but allows to ignore the parse errors in the top level declarations (they shouldn't affect import sorting at all)