rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard

OrderImports breaks the build by collapsing enum import + whitespace inconsistencies

Open timo-a opened this issue 1 year ago • 2 comments

What version of OpenRewrite are you using?

I am using

  • Maven/Gradle plugin v5.29.0

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project.

  <profiles>
    <profile>
      <id>openrewrite</id>
      <!-- `mvn -P openrewrite org.openrewrite.maven:rewrite-maven-plugin:run` -->
      <build>
        <plugins>
          <plugin>
            <groupId>org.openrewrite.maven</groupId>
            <artifactId>rewrite-maven-plugin</artifactId>
            <version>5.29.0</version>
            <configuration>
              <activeRecipes>
                <recipe>org.openrewrite.java.OrderImports</recipe>
              </activeRecipes>
              <activeStyles>
                <style>com.yourorg.JacksonImportStyle</style>
              </activeStyles>
              <failOnDryRunResults>true</failOnDryRunResults>
            </configuration>
            <dependencies>
              <dependency>
                <groupId>com.yourorg</groupId>
                <artifactId>rewrite-recipe-starter</artifactId>
                <version>0.1.0-SNAPSHOT</version>
              </dependency>
            </dependencies>
          </plugin>
        </plugins>
      </build>
    </profile>
  </profiles>

What is the ~smallest, simplest~ way to reproduce the problem?

This example is not minimal, but it is reproducible.

  1. Checkout https://github.com/timo-a/rewrite-recipe-starter/commit/297e72f259f17cae8d62dd7c29b8481f426a37ca which adds a custom style:
    public static ImportLayoutStyle importLayout() {
        return ImportLayoutStyle.builder()
                .importAllOthers()
                .blankLine()
                .importPackage("com.fasterxml.jackson.core.*")
                .importPackage("tools.jackson.*")
                .blankLine()
                .importStaticAllOthers()
                .build();
    }
  1. publish this style locally with ./gradlew publishToMavenLocal
  2. checkout https://github.com/timo-a/jackson-core/commit/4e44f2d2ca9b98f8fd2d094d9982b793ddf60121
  3. run mvn -P openrewrite org.openrewrite.maven:rewrite-maven-plugin:run

What did you see?

this change set: https://github.com/timo-a/jackson-core/commit/c2970c0192090c3cb35f7bd32332d5dd49cde76f

  1. Collapsing enum imports into a wildcard breaks the build.
import com.fasterxml.jackson.core.util.DefaultIndenter;
import com.fasterxml.jackson.core.util.DefaultPrettyPrinter;
import com.fasterxml.jackson.core.util.MinimalPrettyPrinter;
import com.fasterxml.jackson.core.util.Separators;
import com.fasterxml.jackson.core.util.Separators.Spacing;

are replaced with

import com.fasterxml.jackson.core.util.*;

in PrettyPrinterTest. As a consequence Spacing.NONE in line 271 can no longer be resolved. I don't know what issue the compiler has here, but apparently there is an edge case concerning enum imports. Other files are affected as well, but it is the same pattern there.

  1. extends on new line is indented Example: https://github.com/timo-a/jackson-core/commit/c2970c0192090c3cb35f7bd32332d5dd49cde76f#diff-88fe5e2a29aa76eab8d3ec510f7aeb1ed3505a08c183f74e01543ff398ef7d0d The docs only speak of ordering imports:

Groups and orders import statements. If a style has been defined, this recipe will order the imports according to that style. If no style is detected, this recipe will default to ordering imports in the same way that IntelliJ IDEA does.

so this is unexpected.

  1. Unwanted blank lines Based on my style specification, I expect imports to be split in at most three groups: other, my explicitly defined package and static imports. However there is always (?) a blank line after java imports while not necessarily above it, example: https://github.com/timo-a/jackson-core/commit/c2970c0192090c3cb35f7bd32332d5dd49cde76f#diff-fc3028f1ae776b0c8a46f8cacd73ecad36c879df456fb575d69ff48f5a2c3291

Are you interested in contributing a fix to OpenRewrite?

no, just reporting

timo-a avatar May 02 '24 23:05 timo-a

Hi! Thanks for reporting in such detail with reproduction samples; looking above this seems at least closely related to this issue:

  • https://github.com/openrewrite/rewrite/issues/3283

Would you say those are the same case? Or do you think your use of a custom style factors in here as well?

What I like about that other issue is that we have a fairly simple unit test to replicate the issue there: https://github.com/openrewrite/rewrite/issues/3283#issuecomment-1927091821 We just haven't yet gotten around to a fix of this particular issue, as it's hard to get to everything.

Do appreciate you calling this out! Definitely something we should fix to have a good experience running OrderImports.

timtebeek avatar May 03 '24 09:05 timtebeek

Yeah, I think that PR is similar to my first observation.

timo-a avatar May 06 '24 22:05 timo-a

@timo-a we have recently made some improvements to the RemoveImports recipe that might also have an effect on the problem you've described here. Could you re-evaluate whether the behavior you've described still occurs?

Laurens-W avatar Sep 10 '24 09:09 Laurens-W

Specifically, these changes were made just now, and are available in the latest snapshot ; a release will go out tomorrow:

  • https://github.com/openrewrite/rewrite/issues/3283

timtebeek avatar Sep 10 '24 09:09 timtebeek

I just had another look at it with rewrite-maven-plugin 5.40.2. Observation 1: still stands, but I managed to write a test case for it, see above, so you can verify this by yourselves in the future. Observation 2: extends on new line is still indented Observation 3: There are still unwanted lines, but maybe I am defining my style wrong? needs to be examined first: https://github.com/timo-a/rewrite-recipe-starter/pull/3

timo-a avatar Sep 13 '24 06:09 timo-a

Thanks a lot for the runnable test! That should really help pinpoint a cause and get it fixed. I've asked Laurens to have a look.

timtebeek avatar Sep 13 '24 08:09 timtebeek

  • Tentatively fixed in https://github.com/openrewrite/rewrite/pull/4483

timtebeek avatar Sep 16 '24 18:09 timtebeek

Observation 3 has resolved itself, but observation 2 has not. I've opened https://github.com/openrewrite/rewrite/pull/4548 as a reproducible example.

timo-a avatar Oct 03 '24 18:10 timo-a