Reintroducing unnecessary modifiers not detected unless other issues present in same file

[shrugalic]

Setup

We just added cleanthat to our Gradle build of a Java 17 project, via spotless, with default settings:

cleanthat().sourceCompatibility("17")

By doing a ./gradlew spotlessApply It nicely cleaned up various things, including unnecessary modifiers in interfaces:

interface Test {
-    public static final String "TEST";
+    String "TEST";
}

As well as issues like this one:

List<String> list = List.of("test");
-if (list.size() == 0) {
+if (list.isEmpty()) {

So far, so good.

Issue

To check if ./gradlew spotlessCheck would detect any cleanthat issues, we reverted the first of the above changes – that is, we reintroduced the unnecessary modifiers of the constants within the interface.

spotlessCheck did not detect any issues, nor did spotlessApply fix it.

However, if we also reintroduced the second issue (list.size() == 0) in the same file, then spotlessCheck did detect the issue, and spotlessApply did fix it.

If the second issue was instead reintroduced into another (second) file, it would detect only that issue, and continue to ignore the unnecessary modifiers in the first file, as long as that issue was the only on there.

Expectation

It would be great if unnecessary modifiers would be detected even if no other issues are present in a file.

[blacelle]

I feel like this is a spotless effect (maybe not issue, but limitation), and not specific to ~spotless~ cleanthat. Typically related to ratchetFrom option, and detection of which files are to be considered.

Is this reproducible in an open-source project?

[shrugalic]

I feel like this is a spotless effect (maybe not issue, but limitation), and not specific to spotless.

Did you maybe mean not specific to cleanthat?

Is this reproducible in an open-source project?

Hmm, I don't have one at hand. Did you mean any random Java open source project using Gradle and thus calling cleanthat via spotless, rather than by other means?

[blacelle]

I feel like this is a spotless effect (maybe not issue, but limitation), and not specific to spotless.

Did you maybe mean not specific to cleanthat?

yes

---

Did you mean any random Java open source project using Gradle and thus calling cleanthat via spotless, rather than by other means?

Any clearer reproduction case would be helpful. You provided a scenario with OK steps and KO steps.

spotlessCheck did not detect any issues, nor did spotlessApply fix it.

I understand spotlessApply did not clean as expected a .java file with only unnecessary modifiers.

If the second issue was instead reintroduced into another (second) file, it would detect only that issue, and continue to ignore the unnecessary modifiers in the first file, as long as that issue was the only on there.

Here, I underatand spotlessApply did not cleaned as expected a .java file with unnecessary modifiers given other issues.

Could you provide a .java file which is not cleaned as expected?

(Your report suggest the rules kind of conflicts, which would be an unusual bug).

[shrugalic]

Could you provide a .java file which is not cleaned as expected?

Sure.

How to reproduce

The following interface will not be touched by spotlessCheck or spotlessApply, even though it has unnecessary public static final modifiers:

public interface TopLevelInterface {
    public static final String TEST = "value"; // issue #1: unnecessary modifiers
}

Adding another issue to the same file will result in both issues being fixed. Here's the file before spotlessApply:

import java.util.List;

public interface TopLevelInterface {
    public static final String TEST = "value"; // issue #1: unnecessary modifiers

    default void method() {
        if (List.of().size() == 0) { // issue #2: .isEmpty() would be more idiomatic
            // nop
        }
    }
}

Output of ./gradlew spotlessCheck for this file:

* What went wrong:
Execution failed for task 'spotlessJavaCheck'.
> The following files had format violations:
      src/TopLevelInterface.java
          @@ -3,10 +3,10 @@
           import·java.util.List;

           public·interface·TopLevelInterface·{
          -\tpublic·static·final·String·TEST·=·"value";
          +\tString·TEST·=·"value";

           \tdefault·void·method()·{
          -\t\tif·(List.of().size()·==·0)·{
          +\t\tif·(List.of().isEmpty())·{
           \t\t\t//·nop
           \t\t}
           \t}

And here's the result of ./gradlew spotlessApply (great, both issues fixed):

package ch.ergon.taifun.dis;

import java.util.List;

public interface TopLevelInterface {
    String TEST = "value"; // issue #1 fixed

    default void method() {
        if (List.of().isEmpty()) { // issue #2 fixed
            // nop
        }
    }
}

If I were to add the unnecessary public static final modifiers back in, neither spotlessCheck nor spotlessApply will notice:

package ch.ergon.taifun.dis;

import java.util.List;

public interface TopLevelInterface {
    public static final String TEST = "value"; // this issue #1 seems to be invisible

    default void method() {
        if (List.of().isEmpty()) {
            // nop
        }
    }
}

[shrugalic]

If the second issue was instead reintroduced into another (second) file, it would detect only that issue, and continue to ignore the unnecessary modifiers in the first file, as long as that issue was the only on there.

Here, I understand spotlessApply did not cleaned as expected a .java file with unnecessary modifiers given other issues.

Yes, but let me try to clarify. Assume we have two separate java files. One has unnecessary modifiers, and the other one has List.of().size() == 0. spotlessCheck will only see the latter file, and consequently spotlessApply will only fix the latter file. The issue in the first file is ignored.

A workaround would be to introduce an issue like List.of().size() == 0 into the first file, then both issues in this file would be fixed (as in the examples from my previous comment).

[blacelle]

I reproduce. No insight yet, but certainly not due to spotless.

[blacelle]

This ws actually a bug in the rule ModifierOrder, corrupting the AST around modifiers. This is fixed in 2.19.

[shrugalic]

I successfully verified that the issue was fixed, thanks a lot. 🙏