projectlombok / lombok

Very spicy additions to the Java programming language.
https://projectlombok.org/
Other
12.93k stars 2.4k forks source link

[regression] No implementation was created for the mapper element. #3777

Closed rgrunber closed 1 week ago

rgrunber commented 2 weeks ago

Copying from https://github.com/projectlombok/lombok/pull/3744#issuecomment-2462409609 .

Hi all, I think https://github.com/projectlombok/lombok/pull/3744 introduced a regression in Lombok.

I have the following steps to reproduce :

  1. git clone https://github.com/dxaraujo/vscode-java/ (this is a sample project that shows the issue)
  2. Enter the project and set the lombok.version property to 1.18.35 in the pom.xml.
  3. Now go into https://github.com/projectlombok/lombok and build against the commit before this one, 268b6145 (ant dist).
  4. Run mvn install:install-file -Dfile=dist/lombok-1.18.35.jar -DgroupId=org.projectlombok -DartifactId=lombok -Dversion=1.18.35 -Dpackaging=jar, which should install the buld artifact into the local Maven repository.
  5. Go into the "vscode-java" sample project above and run mvn clean verify. It will use the Lombok we just built, and succeed.
  6. Go back to the lombok project and checkout and build this commit, 04c9755b (ant dist).
  7. Run mvn install:install-file -Dfile=dist/lombok-1.18.31.jar -DgroupId=org.projectlombok -DartifactId=lombok -Dversion=1.18.35 -Dpackaging=jar (Note the jar is lombok-1.18.31.jar but we'll treat it as 1.18.35 to simplify things)
  8. Perform (5) again, and you'll see the build fails with :
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /home/rgrunber/sample-projects/issue-3836/src/main/java/com/vscode/java/mapper/UserMapper.java:[9,8] No implementation was created for UserMapper due to having a problem in the erroneous element java.util.ArrayList. Hint: this often means that some other annotation processor was supposed to process the erroneous element. You can also enable MapStruct verbose mode by setting -Amapstruct.verbose=true as a compilation argument.
[INFO] 1 error
[INFO] -------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
filiphr commented 2 weeks ago

Thanks for looking into this @rgrunber. Looking at #3744 seems like a new release of org.projectlombok:lombok-mapstruct-binding is needed. The current version (0.2.0) depends on the now removed AnnotationProcessorHider. AstModificationNotifierData

filiphr commented 2 weeks ago

OK, checking a bit more about it I realized that The changes done in #3744 are not released yet (at least not on Maven Central), so the fact that this is happening is OK and most likely when the next release to Maven Central is done the lombok-mapstruct-binding will be released with it as well.

Rawi01 commented 2 weeks ago

I ran some tests and it seems like the new code doesn't work at all. Calling getAnnotationMirrors on the TypeMirror always returns an empty list. I tried to fix that by using getAnnotationMirrors on the TypeElement but it seems like lombok doesn't clean up this list and it still contains the annotations after lombok handled the class.

kanchev1 commented 2 weeks ago

@Rawi01 I've been using the annotationMirrors code from the current NotifierHider.java for over a year, and it has worked amazingly well. That's why I decided to finally upstream it.

With Lombok versions >= 1.8.28, I simply add my custom AstModifyingAnnotationProcessor implementation in a subproject that gets compiled before my main project, so that the former class can be used as an APT processor by javac. I've been using Java Temurin >= 11 versions, but I doubt that APT processing behaves differently on older versions.

How did you actually test it? Do you have a test project? Maybe I can help resolve the issue.

Rawi01 commented 1 week ago

@kanchev1 Yes, I created a test case and a small script that run it (to add it to our test suite). I just pushed it here: https://github.com/Rawi01/lombok/tree/mapstructIntegration

There is a block you can use to download and use the latests lombok + mapstruct binding release version and that passes the test.

rzwitserloot commented 1 week ago

This... is gearing up to be a weird one. Muchos gracias @rgrunber @rawi01 for the reproduction formula, and @kanchev1 for the feedback. We're looking into it now.

rzwitserloot commented 1 week ago

@kanchev1 We don't use mapstruct daily so this is just guesstimating what's happening, but:

  1. Our previous implementation (the one you 'fixed') works like this:

A. Mapstruct happens to run first and asks lombok if it is done. Lombok says no.

B. Lombok is invoked and immediately sets a flag to answer yes forevermore to mapstruct next time it asks.

C. Lombok has its own round system, but it doesn't wait for that to finish; it starts answering 'yes' right away. Let alone what's going to happen if the AP processing system itself has multiple rounds because other APs that produce new source files are forcing new rounds.

  1. Your implementation throws it all out the window and goes with a new way to determine if a type is 'done': Does it have lombok annotations.

A. Apparently, this simply doesn't work at all.

B. Separate from whether it works in the first place, the concept is broken; the fact that there are no lombok annotations present in the AST tree does not imply lombok is 'done'.

Hence, for now, a straight revert; fixing this issue needs to use a different tack entirely. Possibly as simple as going with the 'vibe' of our strategy (lombok tracks whether it has 'run' or not and answers the question "is this type done" based on whether it has or not), but delay the act of answering 'yeah we are done' until lombok has completed all its rounds. I'm not 100% sure that will actually work; I would expect MapStruct doesn't run at all while we go through our rounds. But, your strategy, separate from apparently not working for some, has fundamental problems.

rzwitserloot commented 1 week ago

@Rawi01 but it sounds like you're saying in your last comment: Just update the binding to 0.3.0 and kanchev's update works great...

rzwitserloot commented 1 week ago

I think I understand it a little better.

  1. The current impl that lombok provides for the mapstruct-binding is not capable of dealing with multirounds.
  2. kanchev's fix tries to address this, but the way chosen to fix it, is a dead end. We will revert it, see below.
  3. ... but it works great... for simple cases. It's a dead end because it doesn't cover complex cases and doesn't look like a good basis that it ever will. However, even for the simple cases, what we pushed last week is also broken.
  4. ... that's because it's a 2-parter: There's the lombok-mapstruct binding which kanchev's PR updated and which we'd need to push out, which we didn't... and separately it removes a bunch of bookkeeping that made the 'old' binding work. So, this push breaks the old binding, and whilst it contains the code for the new one, we did not push the artefact.

The obvious fix, as @Rawi01 's script does, of pushing a new artefact doesn't actually get us anywhere because as far as we can tell @kanchev1 's fix is actually fundamentally broken. So we just revert and leave this bug open, for now. Details on why kanchev's fix is fundamentally broken will be posted in the appropriate PR: #3744

the PR that caused this has been reverted in commit 033870592900e47d9855d190924d0addecddad61.