removeUnusedImports - JDK15 TextBlocks - Fails when running jdk11

[blacelle]

New occurences of #681

For now, just a reproduction case

[blacelle]

Options.instance(context).put(Option.SOURCE, "9");

in com.google.googlejavaformat.java.RemoveUnusedImports#removeUnusedImports(java.lang.String) looks dubious.

[blacelle]

The initial fix was relying on https://github.com/diffplug/spotless/pull/687/files#diff-282881b9b1bb0861937748f383b30476cf1134c56a5a33a450a2caa6ac4efabeR220 which seems to have moved elsewhere in main branch.

[blacelle]

@nedtwigg Could the issue be rooted in the runtime JDK used to execute spotless? In my case, I run a JDK11 and here I'm processing a JDK17 file

[blacelle]

This proposes an alternative removeUnusedImports based on javaParser (which can handle jdk17 syntax, even with a jdk11 runtime). (In my case, I run spotless over JDK11 as AWS Lambda is not yet JDK17 friendly aws/aws-lambda-base-images#29).

[nedtwigg]

Looks great! Part of me thinks we should just replace the GJF implementation with this one. And another part of me thinks we should do removeUnusedImports(Implementation.JavaParser/Implementation.GJF).

The synthesis is to use JavaParser by default since it should have the exact same behavior but resilient to more JDK versions. And then allow users to specify Implementaiton.GJF if they depend on the old behavior somehow.

Author's choice!

[blacelle]

In the meantime, the author added this as a Cleanthat mutator. Hence, it would also be available as a cleanthat mutator. We may also make it available through some dedicated Glue to Cleanthat, very similarly to GJF (hence, no duplicated code between Cleanthat and Spotless).

Pro GJF :

• It may be considered more stable than Javaparser (older, more users, etc)
• Typically, I reported an issue to javaparser team while working on this (while the equivalent code in CleanThat does not fail on given edge-case)

Pro Javaparser :

• Can parse source from different JDK
• May be lighter than GJF (artifacts wise)
• Through Cleanthat, it may not be this lighter anymore (e.g. Cleanthat brings Guava).
• May enable customizations (e.g. Replace static imports with non-static imports #1379)

Part of the code is inspired from Revelc (https://github.com/revelc/impsort-maven-plugin/blob/main/src/test/java/net/revelc/code/impsort/ImpSortTest.java) but simpler as it does only removing unused imports (which Revelc also implemented imports ordering, while Spotless has a solid implementation I did not plan to challenge for this round).

---

Need more time to :

1. What if this implements (now or later) import sorting/grouping? (We would introduce options? while removeUnusedImports suggests a one-size-fits-all)
2. Replace GJF implementation? No, I'm not confident enough in JavaParser to drop GFJ all-together. I would keep GJF as a fallback.
3. Rely on Cleanthat for such a core step? Happy-me, if we agree this is the way to go.

[nedtwigg]

I'd be happy to rely on whatever works best for removeUnusedImports(). Of the three options, cleanThat seems like the most actively developed option, so I'd be happy for that to become the default, but I would definitely keep GJF around as a fallback. I don't want to overwhelm the user with configuration options though. removeUnusedImports() is such a simple concept, it should just work, and it's too bad that GJF's behavior changes based on the JVM that's running.

What if this implements (now or later) import sorting/grouping

IMO our current import sorter works but has ossified such that we can't really change anything about it ever again. It underspecified some things around grouping and static imports, and now we can't fix or improve these issues without breaking the formatting of people who are "relying on the bugs".

So if this PR someday becomes the foundation of a new import cleaner that can deal with groups, static imports, etc. that sounds great. That could be a Spotless DSL, or it could just be a cleanthat option that users can activate, whatever is easy.

My sticking point is always that people solve the problems they themselves have, and that Spotless helps to keep them solved instead of needing to revisit the issue over and over. For people who have already adopted our existing import formatters, I don't want them to have to think about this ever again. For people who are coming to Spotless for the first time or starting new projects, I think it would be great to have a more comprehensive import cleaning solution. It might be worth thinking about an import cleaning solution which could be used across Java/Scala/Kotlin, etc. but mostly I think users don't care about that.

[blacelle]

Ok. I'm totally in-line. I'll release soon a cleanthat version with this, and update the PR accordingly.

---

Regarding imports sorting, I personally do not lack anything in spotless. I came to Spotless from Revelc plugins, and some options (I do not use) may be missing in Spotless. My main discomfort is about the default configuration in Spotless (which is something, while I (personally) look for Eclipse defaults.). Also, I encountered some issues (the infamous 'Why are my files not import-sorted ?'), hence digged into Spotless import-srting implementation.

I fully agree on your synthesis, and improving import-sorting is definitely a lower-value item. (I'm on this issue only because a Cleanthat user encountered this textBlock issue).

[blacelle]

https://github.com/solven-eu/cleanthat/blob/master/MUTATORS.generated.MD#unnecessaryimport

[blacelle]

Here is an iteration.

I'm not fan of:

static final String GJF = "google-java-format";
static final String CLEANTHAT = "cleanthat-javaparser-unnecessaryimport";

[taofik69]

-- BarreraVIP Kamis, 02 Maret 2023, 02:31AM +08:00 dari Benoit Lacelle ***@***.*** :

…

Here is an iteration. I'm not fan of: static final String GJF = "google-java-format"; static final String CLEANTHAT = "cleanthat-javaparser-unnecessaryimport"; — Reply to this email directly, view it on GitHub , or unsubscribe . You are receiving this because you are subscribed to this thread. Message ID: @ github . com>

[blacelle]

I leave the default to GJF.

Javaparser still encounter an issue with annotated packages (which are quite rare, but at least one known issue).

People will be free to switch, and we may switch to cleanthat at any moment.

[blacelle]

It nows handles annotated packages.

[blacelle]

@nedtwigg This is a good first version.

---

One limitation:

• Sealed classes: JEP 360: Sealed Classes (Preview) javaparser/javaparser#2888

It would fails with something like:

[WARNING] Issue parsing this. 1 problems. First problem: (line 26,col 2) Parse error. Found "abstract", expected one of  "enum" "exports" "module" "open" "opens" "provides" "record" "requires" "strictfp" "to" "transitive" "uses" "with" "yield" <IDENTIFIER>
Problem stacktrace : 
  com.github.javaparser.GeneratedJavaParser.generateParseException(GeneratedJavaParser.java:13793)
  com.github.javaparser.GeneratedJavaParser.jj_consume_token(GeneratedJavaParser.java:13638)
  com.github.javaparser.GeneratedJavaParser.Identifier(GeneratedJavaParser.java:3388)
  com.github.javaparser.GeneratedJavaParser.SimpleName(GeneratedJavaParser.java:3286)
  com.github.javaparser.GeneratedJavaParser.MethodDeclaration(GeneratedJavaParser.java:2044)
  com.github.javaparser.GeneratedJavaParser.ClassOrInterfaceBodyDeclaration(GeneratedJavaParser.java:1688)
  com.github.javaparser.GeneratedJavaParser.ClassOrInterfaceBody(GeneratedJavaParser.java:1200)
  com.github.javaparser.GeneratedJavaParser.ClassOrInterfaceDeclaration(GeneratedJavaParser.java:514)
  com.github.javaparser.GeneratedJavaParser.CompilationUnit(GeneratedJavaParser.java:152)
  com.github.javaparser.JavaParser.parse(JavaParser.java:125)
  com.github.javaparser.JavaParser.parse(JavaParser.java:305)

Other potential limitations:

• https://github.com/javaparser/javaparser/labels/Java%2015%20%28preview%29%20Support

[blacelle]

Note for myself: I could try workaround the javaParser limitations.

test. case for sealed added in the PR: it appears :

• GJF is supposedly OK with a JDK17 (Code with sealed interfaces can not be formatted google/google-java-format#697)
• GJF crashes with a pre-JDK17 (like textBlocks)
• Cleanthat leaves the file untouched over any JDK.

[blacelle]

As a side-note, while getting issues over this case before renaming some test files from .java to .test, I see:

[...]

@nedtwigg Shouldn't the faulty filename appears in the stack?

Forget this, too much code for today.

[blacelle]

@nedtwigg This is ready-for-merging

[nedtwigg]

Published in plugin-gradle 6.18.0 and plugin-maven 2.36.0.