Spotless with Eclipse Import Order has different ordering than Eclipse Formatter

[thespags]

If you are submitting a bug, please include the following:

• summary of problem
  Spotless with eclipse formatter orders static imports differently than eclipse or eclipse plugin in Intellij
• gradle or maven version
  mvn 3.3.3
• spotless version
  1.27.0
• operating system and version
  Description: Ubuntu 16.04.6 LTS
• copy-paste your full Spotless configuration block(s), and a link to a public git repo that reproduces the problem if possible

                   <java>
                       <importOrder>
                           <file>/style/myTeam.importorder</file>
                       </importOrder>
                   </java>
               </configuration>

with order

#Organize Import Order
#Fri Jun 28 15:00:43 PDT 2019
4=com.myTeam
3=java
2=javax
1=
0=\#

• copy-paste the full content of any console errors emitted by gradlew spotless[Apply/Check] --stacktrace
  N/A

If you're just submitting a feature request or question, no need for the above.

EclipseFormatter issues
krasa/EclipseCodeFormatter#200
krasa/EclipseCodeFormatter#105

Write

package somePackage;

import static somePackage.Foo.SomeEnum.Bar;
import static somePackage.Foo.SomeZClass;
import static somePackage.Foo.SomeZClass.m;

import java.util.function.Supplier;

// Use as a type argument to be a static import.
public class Foo implements Supplier<SomeZClass> {

    public void someMethod() {
        SomeEnum e = Bar;
        m(); // call m with a static import
    }

    @Override
    public SomeZClass get() {
        return null;
    }

    public enum SomeEnum {
        Bar
    }

    // Add Z this should be sorted after SomeEnum
    public static class SomeZClass {
        static void m() {}
    }
}

Eclipse Plugin will order imports:

import static somePackage.Foo.SomeZClass;
import static somePackage.Foo.SomeEnum.Bar;
import static somePackage.Foo.SomeZClass.m;

Eclipse will order imports:

import static somePackage.Foo.SomeEnum.Bar;
import static somePackage.Foo.SomeZClass.m;

import java.util.function.Supplier;

import somePackage.Foo.SomeZClass;

and spotless will order imports

import static somePackage.Foo.SomeEnum.Bar;
import static somePackage.Foo.SomeZClass;
import static somePackage.Foo.SomeZClass.m;

import java.util.function.Supplier;

Spotless using Eclipse format rules should have the same behavior as Eclipse

[nedtwigg]

Thanks for another example. Related: #167, #174

[thespags]

Sorry I forgot to look up relevant issues, would you like me to dupe this issue out?

[nedtwigg]

I think your issue adds helpful new information. Looks like krasa/EclipseCodeFormatter#105 fixed part of this problem (so they are closer to a fix than we are), and krasa/EclipseCodeFormatter#200 fixes the rest of it. If someone ever gets around to a PR for one of these import sorter issues, I'd like to resolve all of them, to minimize back-compat issues.

What I mean by back-compat is that it's important for Spotless users to have the option to freeze their formatting. Fixing this bug will change the order of imports in existing code. So it's important to have something like <importOrder><mode>legacy</mode>... for people who don't want to choose between "reorder all your imports" and "get new bugfixes and features". If we fix these bugs all at once, then there's just two modes, but if we fix them bit by bit it gets more complicated.

[ekropotin]

Hello. I have another case of inconsistent import order handling which is not related neither upper case packages(EclipseCodeFormatter#105) nor static imports(EclipseCodeFormatter#200). Do I need to file a new issue against it or it's ok to provide details here?

[nedtwigg]

Is the problem that it's different than eclipse? If so this issue is a good place for it. Otherwise a new issue is better.

[ekropotin]

@nedtwigg yes it is.
So, in a nutshell the problem is that Spotless tries to arrange imports of packages that are not specified in "importorder" config in alphabetical order with respect to other imports. Eclipse Code Fromatter, in turn, places such imports at the end of the imports block.
Below is an example:
build.gradle:

plugins {
    // Language plugins
    id 'java'
    id "com.diffplug.gradle.spotless" version "3.28.1"
}

repositories {
    maven { url "https://plugins.gradle.org/m2/" }
    mavenCentral()
    jcenter()
}

dependencies {
    implementation 'io.opencensus:opencensus-api:0.26.0'
}

spotless {
    java {
        importOrderFile 'Test.importorder'
        eclipse().configFile 'CodeStyle.xml'
    }
}

Test.importorder:

5=org
4=com
0=com.test

The file pre-formatted with ECF with Test.importorder:

package com.test;

import com.test.foo.Bar;

import org.xml.sax.SAXException;

import io.grpc.Deadline;

public class Example {

    public void test() throws SAXException {
        Deadline.getSystemTicker();
        throw new SAXException(Bar.BAR);
    }
}

Output of gradle spotlessCheck:

> Task :spotlessJava FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':spotlessJava'.
> The following files had format violations:
      src/main/java/com/test/Example.java
          @@ -2,10 +2,10 @@

           import·com.test.foo.Bar;

          +import·io.grpc.Deadline;
          +
           import·org.xml.sax.SAXException;

          -import·io.grpc.Deadline;
          -
           public·class·Example·{

           ····public·void·test()·throws·SAXException·{
  Run 'gradlew spotlessApply' to fix these violations.

[ekropotin]

@nedtwigg any update on this issue?

[nedtwigg]

I have no personal plans to work on any of the importsorter issues. I don't care what the order is, I just care that something enforces it. I agree it's a bug, and I'm happy to merge a solution from anyone.

[cimi]

So, in a nutshell the problem is that Spotless tries to arrange imports of packages that are not specified in "importorder" config in alphabetical order with respect to other imports. Eclipse Code Fromatter, in turn, places such imports at the end of the imports block.

@ekropotin we can work around this issue by adding an additional item to the .importorder file that forces unspecified imports to go at the end, for example:

#Organize Import Order
#Mon Apr 15 23:51:55 CEST 2019
0=java
1=javax
2=org
3=com
4=

This makes spotless formatting consistent with IDEA/Eclipse code formatter, so I'm able to use save actions with reformat on save without causing any conflicts.

[ekropotin]

@cimi This works like a charm! Thanks. You made my day!

[bwRavencl]

So, in a nutshell the problem is that Spotless tries to arrange imports of packages that are not specified in "importorder" config in alphabetical order with respect to other imports. Eclipse Code Fromatter, in turn, places such imports at the end of the imports block.

@ekropotin we can work around this issue by adding an additional item to the .importorder file that forces unspecified imports to go at the end, for example:

#Organize Import Order
#Mon Apr 15 23:51:55 CEST 2019
0=java
1=javax
2=org
3=com
4=

This makes spotless formatting consistent with IDEA/Eclipse code formatter, so I'm able to use save actions with reformat on save without causing any conflicts.

Good advice, however unfortunately this still doesn't seem fix the issue entirely in some cases:

Eclipse:

import com.sun.jna.platform.win32.Guid.CLSID;
import com.sun.jna.platform.win32.Guid.GUID;
import com.sun.jna.platform.win32.Guid.IID;
import com.sun.jna.platform.win32.Ole32;
import com.sun.jna.platform.win32.Variant;
import com.sun.jna.platform.win32.WinDef.BOOLByReference;
import com.sun.jna.platform.win32.WinDef.DWORD;
import com.sun.jna.platform.win32.WinDef.UINT;
import com.sun.jna.platform.win32.WinDef.UINTByReference;
import com.sun.jna.platform.win32.WinNT.HRESULT;
import com.sun.jna.platform.win32.COM.Unknown;

Spotless:

import com.sun.jna.platform.win32.COM.Unknown;
import com.sun.jna.platform.win32.Guid.CLSID;
import com.sun.jna.platform.win32.Guid.GUID;
import com.sun.jna.platform.win32.Guid.IID;
import com.sun.jna.platform.win32.Ole32;
import com.sun.jna.platform.win32.Variant;
import com.sun.jna.platform.win32.WinDef.BOOLByReference;
import com.sun.jna.platform.win32.WinDef.DWORD;
import com.sun.jna.platform.win32.WinDef.UINT;
import com.sun.jna.platform.win32.WinDef.UINTByReference;
import com.sun.jna.platform.win32.WinNT.HRESULT;

Eclipse seems to order segments consisting entirely of uppercase letters below segments with mixed case, regardless of the characters.

[Frettman]

com.sun.jna.platform.win32.COM.Unknown

This is a bit of a confusing case, as COM is not a class: COM is a package. Knowing this, Eclipse's ordering is absolutely correct, as it lists everything from the com.sun.jna.platform.win32 package, before everything from the subpackage com.sun.jna.platform.win32.COM.
Spotless is only looking at the single file it's currently formatting/validating and does not actually have access to all the referenced types. As such, it can only make best guesses. Right now it seems to only sort lexicographically. The fix provided in krasa/EclipseCodeFormatter#105 splits each import around the last . and sorts by "package" and then by class.
But consider this example:

import example.BFoo;
import example.C.CFoo;
import example.DFoo;
import example.A.AFoo;

This is what Eclipse produces and I'd consider it correct. But this result only makes sense if you know that C is a class with an inner class CFoo and A is a package containing the top-level class AFoo.
Right now, spotless would sort example.C.CFoo; correctly. But with krasa/EclipseCodeFormatter#105 (unless I'm reading that patch wrong) it would sort it below example.A.AFoo; at the very bottom, which is incorrect. And I'd rather spotless handles nested classes correctly than upper case packages.
The incorrect ordering reported in krasa/EclipseCodeFormatter#200 seems to be just that: EclipseCodeFormatter no longer sorting nested classes correctly because of krasa/EclipseCodeFormatter#105.

Regarding the fact that "Organize Imports" in Eclipse never only just sorts imports, but e.g. also turning some specific static imports into regular imports: Again, this can only be done correctly when all referenced types are available (which they're not) to determine whether Foo is a package or a class. So spotless might not optimize the imports like Eclipse, but I'd consider its sorting still correct.

Anyway, to fix the incorrect ordering of packages starting with upper case, it might be necessary to explicitly list them. Then they could be handled accordingly.

[nedtwigg]

I'm inclined to close this as "wontfix" now that we understand the problem better. As you put it @Frettman

I'd rather spotless handles nested classes correctly than upper case packages.

I like your idea

it might be necessary to explicitly list them.

Maybe a regex where if the regex matches then it is sorted as a package rather than a class?

[Frettman]

Despite me posting here, I don't actually have or use upper case package names and don't expect to. But I imagine (or at least hope) it's rare enough that a simple list of package names should suffice. If patterns would be really helpful for some reason, I'd evaluate if something simpler than regex would be enough: e.g. * matches everything except .; ** matches everything including ..
But I'll make an educated guess that Regex matching shouldn't be too diffucult to add as an additional option, once the rest of the improved sorting has been implemented. I'd probably mark them in some way (like surround them with /.../) so they're clearly recognizable as regex (we wouldn't want to treat a regular package name as regex).
So all in all I'd expect the Maven configuration to look something like this:

<treatAsPackage>
  <package>com.sun.jna.platform.win32.COM</package>
  <package>com.sun.jna.platform.*.COM</package>
  <package>/^.*\.COM$/</package>
</treatAsPackage>

[nedtwigg]

I vote for the simplest thing possible. After reading your comment, I am convinced that a list of packages is the simplest thing, and regex makes it more complicated than necessary.

[Frederick888]

Possibly another difference between Eclipse/jdt.ls and Spotless/IntelliJ, which is also related to nested classes:

import static com.example.TestFixtures.A_CONST;
import static com.example.TestFixtures.NestedFoo.B_CONST;
import static com.example.TestFixtures.Z_CONST;

While Spotless/IntelliJ (with importOrder()) will leave it untouched, Eclipse/jdt will change it to:

import static com.example.TestFixtures.A_CONST;
import static com.example.TestFixtures.Z_CONST;
import static com.example.TestFixtures.NestedFoo.B_CONST;

I think the Eclipse one looks better, though I'm atm more keen on finding a way just to make them consistent.

[nedtwigg]

The trouble is that Spotless can't tell if NestedFoo is a package or a class because Spotless doesn't have the full sourcetree, just the single file it is working on. Eclipse has the full classfiles, which is how it can know that NestedFoo is a class and put it after Z.

[jochenberger]

I think we should assume that people stick to the conventions and start their class names with an uppercase characters.

[Frettman]

But Spotless could work on a best effort basis and work under the assumption (and common convention) that package names are lower case and class names start with an upper case letter. As such, for import static, the last segment is the static field or method, and the upper case segments before that could be assumed the be (nested) simple class names.
So basically, something like chopping off the field/method, sort by fully qualified class name like for regular imports and then sort by field/method name.
Upper case packages could be handled by the same <treatAsPackage> configuration suggested above.
You know, I might actually take a stab at it (improving import static and the treatAsPackage configuration).

[Frettman]

While I'd personally consider changing the behavior according to @Frederick888's example a fix, do we need a flag to restore the old behavior anyway?

[Frettman]

I just provided a pull request. It introduces semantics-aware sorting of Java imports, i.e. imports are sorted by package, classes and static members, instead of just lexicographically. This is the new default behavior, but the old one can be restored (in Maven e.g. via false). The imports are split based on conventions: package names start with lower case, class names with upper case. For exceptions, the and configurations can be used.
Let me know what you think or if I need to improve something.

[nedtwigg]

Thanks for the PR, but I just want to reiterate in this issue the same point that I made in the PR:

The most important compatibility guarantee we make is to our existing users. I agree that for new users, semanticSort is probably the correct default behavior. But the premise of Spotless is "formatting doesn't matter, turn this on and then stop thinking about it". Anytime we change the defaults our users have to think about it, which is bad.

[thespags]

@nedtwigg would @Frettman's PR be okay then if the previous behavior was kept as default and the new behavior was opt-in?

[jhonnen]

I just tried this change (thanks!) and found a few differences compared to the eclipse formatter:

1. fields first or last? checked again, this is just a side-effect of the case-sensitive sorting, lower case fields are not sorted first
   eclipse:

   import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
   import static org.mockito.Mockito.mock;
   import static org.mockito.Mockito.when;
   

   spotless:

   import static org.mockito.Mockito.mock;
   import static org.mockito.Mockito.when;
   import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
   

2. members case-sensitive?
   eclipse:

   import static pkg.SomeClass.WARMUP_TIME_MSEC;
   import static pkg.SomeClass.ignoreCaseStrategy;
   

   spotless:

   import static pkg.SomeClass.ignoreCaseStrategy;
   import static pkg.SomeClass.WARMUP_TIME_MSEC;
   

Can you tell which of these differences are expected behavior?

[Frettman]

@jhonnen, you definitely found a bug that explains both differences. Within each group (package, class, member) sorting should only be using the natural/lexicographical order for strings, which means upper case before lower case. Just to be clear, it was my intention that Spotless produces the same order as Eclipse here (which seems straight forward enough to be used as a general mode and not just e.g. an Eclipse-compatibility mode).

@nedtwigg Should I create a new PR for the fix?

[nedtwigg]

Yes, I'd be happy to merge a PR to fix these issues. In this unique case don't add a changelog entry since we haven't released the original yet.

[nedtwigg]

Fixed in plugin-gradle 6.19.0 and plugin-maven 2.37.0 thanks to @Frettman.