Allow MapStruct to be used together with Project Lombok #510

Closed
panov-andy opened this Issue Mar 20, 2015 · 62 comments

Projects

None yet
@panov-andy

Hi.
I'm using Project Lombok and unable generate mappers with maven.
I use @Data annotation (lombok) if it make sense.

@gunnarmorling
Member

There has been a discussion about the combination of MapStruct + Lombok on the Google group recently: https://groups.google.com/forum/#!topic/mapstruct-users/91fBwD4hNZE. An example for using both together is here: https://github.com/mapstruct/mapstruct-examples

If this does not help, it'd be good to know what the exact issue is and how your set-up does look like.

@panov-andy

Suddenly it's not an option for me to split project to modules.
Anyway I'll thing about it.
Thanks.

@dave-lo
dave-lo commented May 27, 2015

I was able to get the mapstruct working with Lombok on versions < 1.16.0 because the annotationprocessor was still exposed. Starting 1.16.0+ the annotation processor is hidden so the maven compiler can't use it and thus this solution won't work

@agudian
Member
agudian commented May 28, 2015

That's too bad - it's likely that there is not much that we can do on our side. Perhaps the devs at lombok can take care of supporting annotation-processors again...

@rzwitserloot

We can't do much without more info on how @dave-lo fixed this issue. I don't even know what the issue is :/

@gunnarmorling
Member

Hey @rzwitserloot, thanks for dropping by. Yes, it would help to know what @dave-lo did do, e.g. it's not clear to me what "Starting 1.16.0+ the annotation processor is hidden" means. I suppose somehow he managed to establish an ordering of Lombok and then MapStruct.

Generally, the issue is that MapStruct generates code based on accessors of JavaBeans properties which don't exist yet in the source code when using Lombok's @Data annotation. I'm not sure whether an annotation processor such as MapStruct can reliably "see" bytecode generated by another participant of the same compilation (as that case generally is not foreseen by the compiler environment).

Another idea for making the integration better would be to let MapStruct take fields into account as well (we want that anyways) but let it generate accessor-based code instead of field-accessing code by means of some configuration (or e.g. when spotting the @Data annotation). That'd by my preferred approach as it should be more robust and e.g. also work in IDEs.

@rzwitserloot

@gunnarmorling Yes, MapStruct should be able to reliably see the stuff that lombok generates, if lombok runs first. Lombok tries to run first but it can't always make this happen, so apparently here MapStruct runs first and thus can't see the stuff lombok adds.

The 'hiding' that is being referred to, is that lombok's annotation processors are squirreled away in public inner classes of private classes, which make them visible for the various command line and jar launcher mechanisms of java (such as MANIFEST.MF's Main-Class key), but invisible to java code. This to avoid having a bunch of lombok internals 'taint' a project's namespace. I haven't yet figured out what part of this change is making it harder to make lombok run first.

For what it's worth, lombok forces (or, it should, perhaps that is the bug?) a new annotation processing round after it changes things, so that other APs can always pick up on the changes even if MS ran first.

The way the annotation processor API design solves this problem is that the flow should be either:

  1. javac parses
  2. javac fires a round
  3. lombok gets to go first and adds some accessor methods
  4. MapStruct gets to go second and sees them, and generates some stuff.
  5. javac is done with the round, but because lombok said stuff changed, another round starts.
  6. lombok fires again but doesn't change anything as everything available has already been inspected.
  7. MapStruct should draw the same conclusion.
  8. javac is done with the round, notices nothing changed, and fires a final no-changes round.
  9. lombok picks this up, does nothing.
  10. MapStruct picks this up, dos nothing.
  11. the final round is done without error and thus javac moves on to bytecode generation.

Or, alternatively, if the order is reversed:

  1. javac parses
  2. javac fires a round
  3. MapStruct gets to go first and doesn't see the accessor methods yet.
  4. lombok goes second and adds them. Also marks to javac that more rounds are needed.
  5. javac is done with the round, but because lombok said stuff changed, another round starts.
  6. MapStruct gets to go again and should pick up on the changes here, modifying what it has done so far / doing more.
  7. lombok fires again but doesn't change anything as everything available has already been inspected.
  8. javac is done with the round, notices nothing changed, and fires a final no-changes round.
  9. MapStruct picks this up, dos nothing.
  10. lombok picks this up, does nothing.
  11. the final round is done without error and thus javac moves on to bytecode generation.

An alternate take is that MS doesn't do anything but gather statistics in all the rounds until the final round and then generates it all, this is the most flexible design for a lot of AP processing (basically, if you don't ever generate any new sources, it's probably the right choice: Just make a list of what you need to do in all the rounds, until the final round, then do everything the list tells you to).

Perhaps MapStruct doesn't do this. If you only ever use 1 Annotation Processor, just doing all the work in the first round and ignoring all further rounds works fine. It only breaks when multiple processors are involved.

So far, either:

  1. Lombok is buggy and doesn't trigger another round.
  2. MapStruct has a flaw in the design where it doesn't stick to the appropriate rules about how to deal with rounds.
  3. MapStruct DOES deal with rounds properly, but that part is bugged. They often can be; it rarely comes up!
  4. Something else is going on that I don't yet understand.

Any guesses on your side of idea 2 or idea 3 might be relevant here?

@rspilker
  1. Something goes wrong with identifying lomboks annotation processor. The correct processor is lombok.launch.AnnotationProcessorHider$AnnotationProcessor. Possibly the $ is a . or the old processor is used, the one shipped in pre 1.16.0 versions.
@rmannibucau

A workaround based on maven - but making IDEs a bit lost - is to delombokize the sources (build config):

<sourceDirectory>${project.build.directory}/generated-sources/delombok</sourceDirectory>

<plugins>
  <plugin> <!-- normally optional but here we use mapstruct so we need this step -->
    <groupId>org.projectlombok</groupId>
    <artifactId>lombok-maven-plugin</artifactId>
    <version>1.16.6.1</version>
    <executions>
      <execution>
        <phase>generate-sources</phase>
        <goals>
          <goal>delombok</goal>
        </goals>
        <configuration>
          <sourceDirectory>src/main/java/</sourceDirectory>
          <addOutputDirectory>true</addOutputDirectory>
        </configuration>
      </execution>
    </executions>
  </plugin>
  <plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-compiler-plugin</artifactId>
    <version>3.3</version>
    <executions>
      <execution>
        <id>default-compile</id>
        <goals>
          <goal>compile</goal>
        </goals>
        <configuration>
          <annotationProcessors>
            <annotationProcessor>org.mapstruct.ap.MappingProcessor</annotationProcessor>
          </annotationProcessors>
        </configuration>
      </execution>
    </executions>
  </plugin>
@gunnarmorling
Member

@rzwitserloot, @rspilker Thanks for that thorough explanation; I am aware of the round-based environment for annotation processors and I don't know of any apparent issues in MapStruct due to this.

So far I assumed the processing environment could not handle altered types (as that's not foreseen traditionally) and thus might not call for another round if Lombok has added getters/setters, but your description indicates otherwise. If I find a minute, I'll set up a test project for debugging what's going on.

@rmannibucau Thanks for chiming in and providing that workaround!

@sjaakd
Contributor
sjaakd commented Nov 26, 2015

but your description indicates otherwise

Hmm. I wander what happens when we have a mixed scenario. Partially Lombok partially not. Will MapStruct be able to cope with that scenario. Generating mappers, waiting for Lombok and then?

@rspilker
rspilker commented Dec 6, 2015

If think the problem is the $ in the class name. On the command line, that might be interpreted as a variable. I was able to run the following command successfully:

javac -processor 'lombok.launch.AnnotationProcessorHider$AnnotationProcessor' -cp lombok.jar Test.java

However, without the single quotes it would give me an error.

@rspilker
rspilker commented Dec 7, 2015

The code also suggests that somehow switching the compiler to in-process would be a workaround.Is that possible?

@rmannibucau

It is but not a reliable option for existing builds.

@rspilker
rspilker commented Dec 7, 2015

I just wanted to verify our assumptions regarding the cause. If I understand the code, it generates a file that's passed to javac using `@arguments-file-name'.

I find it a bit strange that you would still need to escape entries in that file. Why would they be interpreted, unless javac does it. And if javac does it, possibly we can always add the single quotes. Having them on the command line does not work in Windows. Possible we can use double quotes and an escape symbol.

But I do agree that either the Plexus compiler or javac does not handle the $ sign in program arguments correctly.

@rspilker
rspilker commented Dec 8, 2015

I think the problem is that Maven itself interprets it as a property. Possibly the following works:

lombok.launch.AnnotationProcessorHider&#36;AnnotationProcessor
@gunnarmorling
Member

An alternate take is that MS doesn't do anything but gather statistics in all the rounds until the final round and then generates it all, this is the most flexible design for a lot of AP processing

Hey @rzwitserloot, @rspilker, I tried that, and indeed this lets MapStruct see the Lombok-generated accessors. The problem though is that the compiler issues a warning saying "File for type XYZ created in the last round will not be subject to annotation processing". I take that as that it's a bad practice to generate new files in the last round. And really this makes sense as it isn't friendly towards other processors.

So I'd have to somehow postpone file creation to that additional round triggered by Lombok but I don't see a reliable way for doing so. Specifically, in the first round - where MapStruct is called for its @Mapper annotation - I don't know whether there will be another non-final round (as triggered by Lombok) or not. I also tried to generate the files in the first round and the second one - if there is one - but then javac complains about the same file being generated twice.

Atm. I am left behind a bit clueless, the only reliable approach I can see is to anticipate that there will be accessors when Lombok's annotations are present and generate my code based on that anticipation. But I'd dislike the fact to rely on such knowledge and assumptions about another processor.

@rmannibucau

What about allowing mapstruct to know other processors by conf and let mapstruct handles their lifecycle - kind of delegate/decorator pattern?

@agudian
Member
agudian commented Dec 12, 2015

What about allowing mapstruct to know other processors by conf and let mapstruct handles their lifecycle - kind of delegate/decorator pattern?

That shouldn't be the responsibility of MapStruct. Plus, each processor is only called for those types that carry the annotation(s) that the processor feels responsible for. So adding an uber-processor isn't something that the APT API considers.

Making sure that lombok is executed before mapstruct by passing the list of processors accordingly doesn't do it?

          <annotationProcessors>
            <annotationProcessor>lombok.launch.AnnotationProcessorHider$AnnotationProcessor</annotationProcessor>
            <annotationProcessor>org.mapstruct.ap.MappingProcessor</annotationProcessor>
          </annotationProcessors>
@rmannibucau

@agudian agree but the point is making mapstruct usable. Doesnt work as mentionned cause of a mvn issue. Waiting or hacking the mvn fix is surely the best option.

@agudian
Member
agudian commented Dec 12, 2015

Waiting or hacking the mvn fix is surely the best option.

I'm touching those parts of the code right now anyway and can take a closer look into https://issues.apache.org/jira/browse/MCOMPILER-256 while I'm at it.

@gunnarmorling
Member

Making sure that lombok is executed before mapstruct by passing the list of processors accordingly doesn't do it?

The issue is that both processors are called in the same round - first Lombok, then MapStruct - but within that same round the TypeElement s examined by MapStruct do not yet expose the accessors added by Lombok from what I can see. There will be another non-final round triggered by Lombok in which I can see the Lombok generated stuff, so if I wait for that round to write out the stuff from MapStruct it works. It's only that I don't know whether there will be such round or not (in case Lombok is not present).

Essentially, one would have to wait until all source and target types of MapStruct-generated mappers are stable - i.e. not modified by Lombok or amended by generating super-types - and only then generate the mapper implementations. I am not sure though I can identify that point of time.

@gunnarmorling
Member

I've thought about a conceptually similar case that actually is supported officially by JSR 269: The generation of super-classes.

We can make this case work as we can identify types with super-types not yet generated (because they are to be generated in a subsequent round or by another processor in the same round); the super-type elements will have kind ERROR in this case. This allows to defer handling of the affected sub-types to another round. I think that's the way to do it correctly as envisioned by the JSR 269 creators.

The issue is that I don't see how it could be done with Lombok-modified entities. I can't find out whether a type has been amended by Lombok in the same round, not allowing for the decision whether to defer handling of the types or not.

I think this is one of the cases where Lombok's modification of types really causes trouble, because it makes collobaration with other processors as intended by the JSR 269 impossible from what I can say. I'd be more than happy to be convinced otherwise, though.

@rmannibucau

Well you can also state the jsr 269 was badly designed ;) - just trying to be fair from both point of view.

More seriously do you care bringing these explanations there rzwitserloot/lombok#973 ? Think working between both projects will be more efficient and a solution seems possible with this last comment.

@gunnarmorling
Member

Added a comment over there; Not sure how it could look like, but maybe @rzwitserloot et al. have a good idea.

@rmannibucau

Thanks Gunnar!

@fwonce
fwonce commented Jan 11, 2016 edited

Anyway, when speaking of code/bytecode generation, getter/setter generation is cheaper (at a user's side) than bean mapper generation, because any popular IDE can do the former one in a blink of an eye . So it's an easy decision for me to make when mapstruct and lombok conflicts.

EDIT:
Seen some thumbs down on this comment and I want to add a few words to make it clear. I can realize it's cool to have both work - every one here loves both for sure, but what I'm pointing out is the core value of these two great libraries are not equal. It's about a trade-off when the problem remains unsolved. With all due respect, lombok just introduces some language candies which Java language fail to provide, just like its slogan suggests - spicing up your Java code - great to have, but not a disaster w/o them. Lombok eases the work for the one who writes down the code. Nonetheless, vanilla Java doesn't harm the readability of code anyway.
On the other hand, managed bean mapping is a programming convention worth being encouraged, as boring manual bean mapping code scattered all around the project sucks. Managed bean mapping vastly boost the readability, and mapstruct-like libraries simplify the task. Of course a must-have is favored over a great-to-have.

@5im5im
5im5im commented Mar 12, 2016

Will there be a out of the box support for lombok in the near future?

@agudian
Member
agudian commented Mar 12, 2016

As explained in the earlier comments, this is nothing that we can solve on our side due to the nature of how lombok operates. They have some ideas on how to not interfere with other annotation-processors, though - so you could follow up over there.

@ahus1
ahus1 commented Apr 24, 2016

I took the example from @rmannibucau one step further to not confuse the IDE. The solution for me was to set addOutputDirectory to false and use the output of de-lombok only for creating the mappers.

See an example in my forked mapstruct examples project. Please comment, I'm happy to provide a pull request.

<!-- first de-lombok the sources to make getters/setters visible for mapstruct,
but *DON'T'* add the output directory to the sources, as we will only need it for mapstruct processing -->
<plugin>
    <groupId>org.projectlombok</groupId>
    <artifactId>lombok-maven-plugin</artifactId>
    <version>${org.projectlombok.maven.version}</version>
    <executions>
        <execution>
            <phase>generate-sources</phase>
            <goals>
                <goal>delombok</goal>
            </goals>
            <configuration>
                <sourceDirectory>src/main/java</sourceDirectory>
                <addOutputDirectory>false</addOutputDirectory>
                <outputDirectory>${project.build.directory}/delombok</outputDirectory>
            </configuration>
        </execution>
    </executions>
</plugin>

<!-- use the de-lomobok files to create the necessary mappers -->
<plugin>
    <groupId>org.bsc.maven</groupId>
    <artifactId>maven-processor-plugin</artifactId>
    <version>2.2.4</version>
    <configuration>
        <defaultOutputDirectory>
            ${project.build.directory}/generated-sources/mapstruct
        </defaultOutputDirectory>
        <processors>
            <processor>org.mapstruct.ap.MappingProcessor</processor>
        </processors>
        <sourceDirectory>
            ${project.build.directory}/delombok
        </sourceDirectory>
    </configuration>
    <executions>
        <execution>
            <id>process</id>
            <phase>generate-sources</phase>
            <goals>
                <goal>process</goal>
            </goals>
        </execution>
    </executions>
</plugin>

<!-- now take the original sources together with the created mappers to the compiler -->
<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-compiler-plugin</artifactId>
    <version>3.0</version>
    <configuration>
        <annotationProcessors>
            <annotationProcessor>lombok.launch.AnnotationProcessorHider$AnnotationProcessor</annotationProcessor>
        </annotationProcessors>
    </configuration>
</plugin>
@sjaakd
Contributor
sjaakd commented Apr 25, 2016 edited

@ahus1 , there are many questions around the combination, so I'll reckon: a working example is very welcome. @agudian , @gunnarmorling : WYDT?

Q. Why do you need both the maven-compiler-plugin and the maven-processor-plugin? I think we have an example with the maven-compiler-plugin as well nowadays. So perhaps better to use one technology.

Now, on the example. I'm not sure whether I onderstand it completely. What's this lombok.launch.AnnotationProcessorHider$AnnotationProcessor doing? I thought lombok designed its own processor. I this something Lombok internal, or actually exposed by Lombok?

@ahus1
ahus1 commented Apr 26, 2016

I've used the processor plugin as I wanted to create only the generated sources. This way I avoid any irritation for my IDE (IntelliJ). If I would use the compiler plugin, I would need to put the delombok-sources into the sourcepath, and that would conflict with the other original sources. AFAIK I can't specify an alternative sources folder for the compiler like I can do for the processor.

The lombok.launch.AnnotationProcessorHider$AnnotationProcessor is the lombok processor that is usually auto-discovered. But as I don't want the Mapstruct processor to run again, I have to specify the Lombok processor manually (specifying the processors manually will disable autodetection of annotation processors).

I currently don't see any further optimizations - suggestions for optimizations are welcome.

@dslivka
dslivka commented May 3, 2016

@agudian: what about the @gunnarmorling 's idea?

Another idea for making the integration better would be to let MapStruct take fields into account as well (we want that anyways) but let it generate accessor-based code instead of field-accessing code by means of some configuration (or e.g. when spotting the @Data annotation). That'd by my preferred approach as it should be more robust and e.g. also work in IDEs.

Sounds simple and elegant. You can consider making configuration similar to @XmlAccessorType(XmlAccessType.FIELD), in this case it would be @Mapper(accessType = MapperAccessType.FIELD)

@dslivka
dslivka commented May 9, 2016 edited

In addition to my previous post: source and target beans may have different character, e.g. source is generated by CXF/JAXB and its getter/setter names do not necessarily follow the Lombok conventions, the manually created target may use Lombok. Therefore I suggest annotation @Mapper(sourceAccessType = MapperAccessType.PROPERTY, targetAccessType = MapperAccessType.FIELD)

@jvz
jvz commented Jun 29, 2016 edited

FWIW, the Lombok/MapStruct combo worked just fine for me in Gradle.

Edit: that was because I had one jar that used lombok, and a different one used mapstruct, so the order of execution of annotation processors was not an issue.

@lukas0krupa

@ahus1 Thank you for sharing your code. I managed to get it finally working with Maven.

Then I had some time and I managed to get it working with maven-compiler-plugin and multiple executions with specific configuration (you can have different configurations per executions).

Long-story-short, 3 executions:

  • turning default compiler off
  • compiling all sources except mappers
  • compiling mappers with extra sources compiled in step above

So far working nicely ! And I didn't have to delombok.
Not sure if more efficient or optimal.

pom.xml snippet

           <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-compiler-plugin</artifactId>
                <executions>
                    <execution>
                        <id>default-compile</id>
                        <phase>none</phase>
                    </execution>
                    <execution>
                        <id>Normal compilation (excluding mappers)</id>
                        <goals>
                            <goal>compile</goal>
                        </goals>
                        <configuration>
                            <excludes>
                                <exclude>**/**/mapper/**/*</exclude>
                            </excludes>
                        </configuration>
                    </execution>
                    <execution>
                        <id>MapStruct mappers</id>
                        <goals>
                            <goal>compile</goal>
                        </goals>
                        <configuration>
                            <compilerArgs>
                                <compilerArg>
                                    -Amapstruct.defaultComponentModel=spring
                                </compilerArg>
                            </compilerArgs>
                            <includes>
                                <include>**/**/mapper/**/*</include>
                                <include>${project.build.directory}/classes/**/*</include>
                            </includes>
                            <annotationProcessors>
                                <processor>org.mapstruct.ap.MappingProcessor</processor>
                            </annotationProcessors>
                        </configuration>
                    </execution>
                </executions>
            </plugin>
@ravy
ravy commented Sep 29, 2016

I am using maven, lombok, mapstruct.
figured out i am having this issue too.

@AntwaneB
AntwaneB commented Oct 12, 2016 edited

I don't know if it belongs here (feel free to remove / move this comment), but in order to use the solution given by @ahus1, make sure to run your IDE / maven with the JDK and not the JRE, or you'll get an error trying to delombok the sources saying that the class com/sun/tools/javac/util/Context (which is located in tools.jar, only in the JDK) is missing.

Also, if you want to generate mappers for Spring, don't forget to add the following lines in the configuration tag for the 2nd plugin:

<options>
    <mapstruct.defaultComponentModel>spring</mapstruct.defaultComponentModel>
</options>
@anilbharadia

any update on when this will be resolved ? or any official workaround ?

@rzwitserloot

I've been pointed at this thread by other parties, and I now understand more thoroughly why this is an issue.

Here's a crazy idea which might not even work: What if lombok modifies the typemirrors that mapstruct ends up looking at?

Before I investigate that (it might not be possible, but I have high hopes), let me double check some assumptions first:

  • If mapstruct runs before lombok does, then mapstruct is 'lost at sea'; it cannot tell that the typemirrors its looking at aren't stable yet and it can't assume a second round will occur. All it can feasibly do is find a @Data annotation or similar and divert its work to another round in the assumption there should be one. That's.. inelegant, to say the least.

  • If mapstruct runs AFTER lombok does, mapstruct STILL doesn't see the generated code, because it's going off of typemirrors which will get an update in the next round, but we haven't gotten there yet. However, if lombok modifies the mirrors as part of the AST rewrite, then this should work fine.

If the above 2 assumptions are true, then, presumably, if this happens:

  • We find a way to reliably tell maven etc to run lombok first and mapstruct after, which is complicated due to bugs related to how maven handles $, but, those are bugs that those tools should eventually fix or have already fixed, AND

  • We modify those typemirrors,

then you can use lombok and mapstruct together.

Am I missing anything?

@gunnarmorling
Member

Hey @rzwitserloot, thanks for stopping by!

I don't think there should any assumption be made on the order of processor execution. Even if you manage to get it to work for Maven, there are other build tools and also execution within IDEs. It seems like opening a can of worms.

By now, MapStruct can deal with types whose hierarchy gets expanded by other processors. While JSR 269 does not allow to modify a type itself, processors can generate super-types of the compiled classes. In such a case one will see type mirrors of Kind.ERROR (because the super-class to be generated does not exist yet when visiting the sub-type from which it gets generated in the first round). MapStruct will then defer processing of the affected mappers to the next round (or defer it again if yet another super-type is generated from that intermediate super-type). I.e. MapStruct will wait with processing of mappers until the hierarchies of the types under compilation have stabilized (or, if a referenced super-type never gets generated, this is a regular compile error in which case we don't need to care about generating mappers).

It's my understanding that this is how conforming JSR 269 processors are supposed to behave.

So if Lombok somehow managed to let types which it hasn't amended yet have type kind ERROR, MapStruct should be able to do the right thing (we'll fetch new type mirrors for types deferred to later rounds, so we'll see their latest state). But I guess that brings us back to the initial question of ordering, you probably cannot do this when Lombok runs after other processors such as MapStruct. Essentially it all boils down to the fact that Lombok is doing things - altering types - that simply have not been foreseen by the JSR 269 infrastructure.

I'd be happy to be proven wrong of course!

@rzwitserloot

Allright, I spent the evening trying to figure this all out, and I have relatively good news to report.

I think we can fix this!

This branch: https://github.com/rzwitserloot/lombok/tree/playNiceWithOtherAP

contains my proof-of-concept work.

Just check that out and run 'ant testAp', which makes an example processor that looks for a generated (by lombok) getter. The ant task runs the compile job of an example file twice, once with the ordering: First lombok, THEN the other AP, and then in reverse.

For the first case (first lombok, THEN the other AP), it now works. This lombok version is handicapped (probably just blows up if you try to do anything with it other than make it generate getters on fields), as I said, proof-of-concept only.

I also added code to print the ordering of annotation processors, but, extremely unfortunately, javac will init and then immediately proceed to round 1 per annotation handler, so the very first call lombok ever gets is already too late. Lombok DOES notice that this happened (it figures out that some other AP ran earlier), so, assuming lombok gets invoked at all even if the first AP errored out (something I'll test later), lombok can at least error/warn advising that it really should be explicitly told to run first. That's a separate issue (how do we ensure lombok runs first?).

@agudian
Member
agudian commented Dec 13, 2016

Okay, that looks interesting - basically you see that for any AST modifications you're doing, the corresponding modifications to the Elements/Mirrors are being added, right? That may be something to build on...

We find a way to reliably tell maven etc to run lombok first and mapstruct after, which is complicated due to bugs related to how maven handles $, but, those are bugs that those tools should eventually fix or have already fixed, AND ...

I heard about that bug in the maven-compiler-plugin before, but I couldn't figure out what's actually wrong there (nor was there a project to reproduce it). Do you have any details on that?
But loosely speaking, the maven-compiler-plugin just hands classpath, processorpath and the list of annotation processors as given in the pom.xml to javax.tools.JavaCompiler (forking a new javac process isn't usually done anymore, only in very specific cases), so if the JDK can handle it, Maven should be fine.

Anyway, regarding the processor order. Is that deterministic in Eclipse-builds as well? Perhaps even in the order as detected in the .factorypath?

@rzwitserloot

Sounds like the issues with manually ordering annotation processors by explicitly listing them has been solved. That's nice; we can in theory do some really crazy hackery to try to fix the ordering ourselves (which involves being an SPI provider for charsets, thus getting called earlier, and then 'fixing' things by injecting ourselves as agent into the running process to rewrite javac so that it prioritizes lombok. Yeah, hackery).... but it'd be a lot simpler to just rely on the buildprocess authors setting it up so that lombok runs first.

Processor order in eclipse is irrelevant; lombok runs before APs in eclipse, so it should work out regardless.

But, good point, I will try to test if lombok+mapstruct in eclipse works.

@agudian
Member
agudian commented Jan 10, 2017

@rzwitserloot Did you get to check combination of MapStruct and your proof-of-concept in Eclipse?

Would be great to get an idea if your approach works out (which would be cool, as it's not limited to solve the problem for MapStruct users only 😉) or if we have to come up with an alternative that might need to be specific for the combination of MapStruct and Lombok.

@gunnarmorling
Member

if we have to come up with an alternative

To shed some light on this, we've been considering the idea of an SPI which would to make it pluggable either to determine whether a type is "complete" (i.e. it will not be amend by other processors). Lombok than could implement such SPI, telling other processors such as MapStruct whether it has "done its thing". MapStruct would query that SPI and defer processing such type until a later round (in which the SPI implementation has stated that the type is stable).

An alternative SPI would make the determination of properties of given types pluggable. Then Lombok could provide an implementation returning all the artificial property (getters and setters) it creates.

Both SPIs would solve the ordering problem, as it they either would allow other processors such as MapStruct to defer their processing until Lombok is done or they could act based on the presence of properties as Lombok will generate them.

@gunnarmorling
Member

Such SPI could live in the namespace of MapStruct in the first iteration, but we even may consider to extract it into some more "neutral" place, akin to the AOP alliance's interceptor JAR from a while ago.

@rzwitserloot

@rspilker and I have worked on turning the proof-of-concept solution (which makes it all work if lombok is 'first in the AP ordering') into a proper releasable commit. That work's pretty much done at this point.

Next step is to investigate how we can make things work when the APs are ordered such that mapstruct is first:

  • I'm pretty sure that annotation processors have their own classpaths, so an SPI solution would look fairly hacky.
  • @rspilker and I think the solution to this would probably a snippet of code (we could turn it into a library too, but it wouldn't be a particularly large snippet) which detects that lombok is in the lineup of APs, but will run later, therefore the AP this snippet is included in should simply do nothing and wait for the next round (which lombok will guarantee will occur). You'd have to include this in mapstruct but it should be fairly low impact. If it's not, there's always the option that mapstruct emits a NOTE or WARNING via the procEnv.getMessager() that the AP ordering should be tuned so that lombok runs first.

Once that's done, we'll release a new version of lombok and update this issue.

@gunnarmorling
Member

I'm pretty sure that annotation processors have their own classpaths, so an SPI solution would look fairly hacky.

Actually I think an SPI is the one solution that isn't hacky at all as it's not based on any assumptions about the processor order :)

It's not that each processor has their on classpath, instead there is one shared processor path (alternatively, a processor module path in JDK 9). We already have SPIs defined in MapStruct and pick up user-provided implementations from the processor path to make certain things in MapStruct customizable. This works like a charm.

@gunnarmorling gunnarmorling added a commit to gunnarmorling/mapstruct that referenced this issue Jan 14, 2017
@gunnarmorling gunnarmorling #510 Adding experimental SPI for letting AST-modifying annotation pro…
…cessors tell us about future modifications
60823f0
@gunnarmorling
Member
gunnarmorling commented Jan 14, 2017 edited

Hey @rzwitserloot, @rspilker, I've been moving forward with the SPI we discussed. You can find it in my MapStruct fork here: #1042

Do you think you can give it a try to implement this in Lombok so we can see whether it resolves the situation? That's what you'd have to do:

  • Build MapStruct as per my branch above (run mvn clean install -pl processor -am -DskipTests).

  • Create an implementation of the new SPI:

      LombokProcessor implements AstModifyingAnnotationProcessor {
    
          boolean isTypeComplete(TypeMirror type) {
              // return true if Lombok has amended the type's AST, false otherwise.
          }
      }
    
  • Add a file to the Lombok JAR, named META-INF/services/org.mapstruct.ap.spi.AstModifyingAnnotationProcessor with the FQN of your SPI implementation as the sole contents

  • Use your new Lombok JAR with this test project that makes use of MapStruct and Lombok within one compilation unit: mapstruct/mapstruct-examples#15 (cd mapstruct-lombok && mvn clean install)

  • Let us know whether it works or not :)

Looking forward to hearing from you, it'd be awesome if we could make MapStruct and Lombok work together nicely once and for all!

@rzwitserloot
@rzwitserloot

I accidentally a punctuation. What I meant to type was: We will work on this tomorrow (thursday) evening :)

@gunnarmorling
Member

Cool, looking forward to the outcome. Let us know in case there's anything we can help with.

@gunnarmorling gunnarmorling added a commit to gunnarmorling/mapstruct that referenced this issue Jan 21, 2017
@gunnarmorling gunnarmorling #510 Adding experimental SPI for letting AST-modifying annotation pro…
…cessors such as Lombok tell us about future modifications
31cfbdd
@gunnarmorling gunnarmorling added a commit to gunnarmorling/mapstruct that referenced this issue Jan 31, 2017
@gunnarmorling gunnarmorling #510 Adding experimental SPI for letting AST-modifying annotation pro…
…cessors such as Lombok tell us about future modifications
754877c
@gunnarmorling gunnarmorling added a commit to gunnarmorling/mapstruct that referenced this issue Jan 31, 2017
@gunnarmorling gunnarmorling #510 Addressing review remarks 16898e0
@gunnarmorling gunnarmorling added a commit that referenced this issue Jan 31, 2017
@gunnarmorling gunnarmorling #510 Adding experimental SPI for letting AST-modifying annotation pro…
…cessors such as Lombok tell us about future modifications
5e59f34
@gunnarmorling gunnarmorling added this to the 1.2.0.Beta1 milestone Jan 31, 2017
@gunnarmorling
Member

The SPI has been merged to master. Thanks @rzwitserloot, @rspilker and everyone else for making it happen!

@gunnarmorling gunnarmorling changed the title from fail if Project Lombok is used (projectlombok.org) to Allow MapStruct to be used together with Project Lombok Jan 31, 2017
@msymonov
msymonov commented Feb 6, 2017

Hi everyone, when do we expect this feature to be released? What is actually required? Will we need both artifacts to be released? And thanks for everyone's efforts

@gunnarmorling
Member
gunnarmorling commented Feb 6, 2017 edited

You'll need MapStruct 1.2.0.Beta1 (should be out very soon, we hope to do it next week) and the latest "Edge" release of Lombok. @rzwitserloot, @rspilker, any indication when there will be a regular Lombok release with your change?

@rspilker
rspilker commented Feb 7, 2017

I expect a new release within a week.

@rzwitserloot

Lombok release 1.16.14 is out with this feature.

@fabiohbarbosa

Problem resolved with new lombok version!
Thanks

@Maaartinus

Cool! Could someone update the http://mapstruct.org/faq/?

@gunnarmorling
Member

@Maaartinus Maybe you could do it? Just click the "Edit on GitHub" link on the upper right of the FAQ page and you could prepare a PR. We've planned the Beta1 release for Monday next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment