Lombok: JavaParser fails with StringIndexOutOfBoundsException when lombok processor is enabled

[amishra-u]

What version of OpenRewrite are you using?

• OpenRewrite 8.41.2

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

I haven't debugged the exact root cause yet. the issue seems to occur when annotations are added or removed in the source code. This appears to result in incorrect character positions for the cursor.
(Will add more detail about the issue after debugging)

  @Getter
  @Builder
  public static class Request {
    @Nonnull private final List<TransportJobId> transportJobIds;
    @Nullable private final UnixTimeMillis estimatedReadyAt;
    @Nonnull private final RegionId regionId;
    @Nonnull private final boolean wasTriggeredFromFulfillmentOrder;
  }

What did you expect to see?

A valid compilation unit should be generated for the source file.

What did you see instead?

A ParseError instance is generated for the source file instead.

What is the full stack trace of any errors you encountered?

java.lang.StringIndexOutOfBoundsException: begin 5327, end 5315, length 5434
  java.base/java.lang.String.checkBoundsBeginEnd(String.java:3319)
  java.base/java.lang.String.substring(String.java:1874)
  org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitAnnotation(ReloadableJava17ParserVisitor.java:143)
  org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.visitAnnotation(ReloadableJava17ParserVisitor.java:72)
  com.sun.tools.javac.tree.JCTree$JCAnnotation.accept(JCTree.java:2601)
  com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  org.openrewrite.java.isolated.ReloadableJava17ParserVisitor.convert(ReloadableJava17ParserVisitor.java:1604)
  ...

Are you interested in [contributing a fix to OpenRewrite]

Yes.

[knutwannheden]

This is basically the reason we haven't enabled the Lombok processor yet. It appears like it doesn't always adjust the offsets in the AST correctly. I suspect that us a bug in Lombok. So either we need to get it fixed there or we need to adjust our parser so that it tracks the cursor in the file without relying on the AST.

[knutwannheden]

I think we are actually very close to have this working, we just didn't find the time to complete it.

[knutwannheden]

If you feel like taking a look at it, I believe that this is where we need to fix things to no longer rely on the position from the AST:

rewrite/rewrite-java-17/src/main/java/org/openrewrite/java/isolated/ReloadableJava17ParserVisitor.java

Line 1682 in abb80e1

String prefix = source.substring(cursor, Math.max(cursor, getActualStartPosition((JCTree) t)));

[amishra-u]

Able to reproduce this bug with below unit test, @nonnull to field to cause the error.

    @Test
    void lombokBug() {
        rewriteRun(
          java(
            """
              import lombok.Getter;
              import javax.annotation.Nonnull;

              @Getter
              public class Foo {
                @Nonnull private final String foo;
              }
              """
          )
        );
    }

[timtebeek]

Thanks for the runnable example @amishra-u ! That should help zero in on the issue. Did you by chance look at a fix as well?

[amishra-u]

Thanks for the runnable example @amishra-u ! That should help zero in on the issue. Did you by chance look at a fix as well?

Yes, I’ve identified the root cause. It’s not an OpenRewrite bug, but rather an issue with the Lombok annotation processor. I added a workaround in OpenRewrite, which resolved the problem. I’m also looking into fixing the underlying issue in Lombok itself and will publish a PR soon.

Lombok incorrectly updates the annotation type’s position on a field to the variable’s position in certain scenarios. If you closely look after image it has correct position for annotation but only the annotation type position is updated to the variable’s position.
Before annotation processor:

After annotation processor:

[timtebeek]

Awesome work! Thanks for diving in and posting the outcome here. Keep us posted on any progress with regards to a workaround or fix upstream.

[amishra-u]

This method is buggy, it doesn't make deep copy.
https://github.com/projectlombok/lombok/blob/master/src/core/lombok/javac/handlers/JavacHandlerUtil.java#L2111

[knutwannheden]

This is also what I observed, but I never analyzed it in detail. Fixing this upstream would be awesome! Hopefully that will then address all such errors in Lombok.

[amishra-u]

Publish the draft PR with the unit test.
@timtebeek, could you please help verify it with OpenRewrite, even before this change is released?

[timtebeek]

hi @amishra-u ; sure! I don't quite know how to install a custom build of Lombok to your local Maven repository, but perhaps you already have that part figured out from your work there.

The version of Lombok we use is set (and can be overwritten) in the rewrite-build-gradle-plugin:
https://github.com/openrewrite/rewrite-build-gradle-plugin/blob/a6a7fd4ef41b0e123df09bc036a20c697b8cc485/src/main/java/org/openrewrite/gradle/RewriteJavaPlugin.java#L72-L77
If you then publish the rewrite-build-gradle-plugin to Maven local you can update the versions used here to use latest.integration:

rewrite/build.gradle.kts

Lines 1 to 5 in a8946ac

	plugins {
	id("org.openrewrite.build.root") version("latest.release")
	id("org.openrewrite.build.java-base") version("latest.release")
	id("org.owasp.dependencycheck") version("latest.release")
	}

I hope that helps you already there. Feel free to share the steps you did for the local installation of Lombok here as well.

[amishra-u]

Hi @timtebeek,I verified the fix with our Uber monorepo and discovered a couple more issues, which I’ve addressed with fixes in OpenRewrite (PR to be published soon). After running the FindMissingTypes recipe, I didn’t find any missing types.
Finally we got Lombok working! 🚀

[timtebeek]

Awesome, thanks a lot! I've merged & released your two most recent PRs, and look forward to the further fixes whenever ready. 🙏🏻

[amishra-u]

Awesome, thanks a lot! I've merged & released your two most recent PRs, and look forward to the further fixes whenever ready. 🙏🏻

Thank you!

[timtebeek]

Also let us know when you think we can close this issue; I think the initial issue has been addressed, and we're waiting on:

• fix: Don't alter the AST node positions of original code. projectlombok/lombok#3800

Anything left to do here then before we close this issue? Or can we close and monitor that upstream PR?

[amishra-u]

No, once #4835 and projectlombok/lombok#3800 is merged. We will have lombok working for everything except few experimental ones.

There was some earlier activity on the PR from @Rawi01, but there hasn't been any recent activity—he might be on vacation. I'll post the PR to the Lombok forum for review next week.

[timtebeek]

Great to see your PR merged! A release might take a little while still, but at least there's good progress. I think from our side of things we could now look at adding the same Lombok handling to the Java 8 and 21 parsers as well; did you have that use case there too? Or should we pick that up on our end? Mostly to avoid any duplicate work

[amishra-u]

We don't have any usecase for java-8 also not for java-21 yet. If there is no urgency I will take care this once I have some bandwidth.

[jevanlingen]

I will give it a go today for java 8 and 21 (I am at GMT+1).