pull request for #15

@t1 first of all - thank you for your contribution! 👍

Referring to #15 (comment): "works out of the box" - is a good approach in general. However: I'm not sure that enough to exclude only annotated methods, in particular I'm thinking about https://projectlombok.org/features/Cleanup.html and maybe there are others, but can't tell for sure due to lack of tests and experience with Lombok on our side. And the message that will be delivered to community by addition of built-in exclusion for Lombok annotation might lead to a question why not all code that Lombok generates is excluded. By saying this, I'm not against such change, just want to make sure that will not be introduced something what will be hard to maintain (including answers on questions in mailing list and so on) in a long run. @marchof WDYT?

P.S. commit message requires change, because #15 is a bigger story - for example also about Java 7 try-with-resources and not just about Lombok, but that's minor in lights of question above

@Godin: You are completely right: This fixes only a subset of the code that Lombok can generate (and of course doesn't fix most of what has to be done for #15). And I must admit that I hadn't thought of this before.

Let's take a look at all lombok features:

- means that this annotation doesn't generate any code that affects branch or line coverage.

Features that add methods get the @lombok.Generated annotation and are skipped by this pull request. Features that add lines without branches are covered, so this might change the line statistics. I assume that this not really an issue.

Leaving us with four Lombok features that add branches: We heavily use Lombok in our team, but we've never used @Cleanup (I guess it became obsolete with ARM blocks available). We regularly use @NonNull and @SneakyThrows, and rarely @Getter(lazy=true). So this is a realistic use case.

When I look at our tests for code with @NonNull and @Getter(lazy=true), we try to cover all branches here as well! So we explicitly want that code to be covered. I'm not so sure about @Cleanup and @SneakyThrows, it may be that we don't want to cover that code, as we assume that code generated by Lombok is sufficiently safe, but I guess that's a question of target coverage check limits: If we want 100% coverage, we'd want to cover those, too.

OTOH do you have any idea of how this could be fixed? We could try to de-lombok the code ourselves, but changes in Lombok would require an update to JaCoCo. Even finding out which version of Lombok actually produced that code may not be very easy... as well as figuring out, if this code actually did come from Lombok and is not in the source code. I'd say this is not feasible.

What do you think? Is it sufficient to cleanly document that only the methods and constructors annotated as @lombok.Generated are skipped?

@t1 first of all - thank you very much for such detailed comment! 👍
And please excuse us for delays with replies.

This fixes only a subset of the code that Lombok can generate (and of course doesn't fix most of what has to be done for #15). And I must admit that I hadn't thought of this before.

So reworded commit message to not cause automatic close of #15. Also rebased on top of master.

OTOH do you have any idea of how this could be fixed? We could try to de-lombok the code ourselves, but changes in Lombok would require an update to JaCoCo. Even finding out which version of Lombok actually produced that code may not be very easy... as well as figuring out, if this code actually did come from Lombok and is not in the source code. I'd say this is not feasible.

Filtering on a level lower than classes/methods, i.e. on a level of bytecode instructions is difficult to implement even in case of bytecode that only Java compiler generates. Each "third" comment of #15 among variety of "plus ones" and "whens" tries to explain exactly this to the users - see for example #15 (comment) or #15 (comment)

Attempts to also put on a table instructions (and not just methods) generated by Lombok or JVM-based languages other than Java (Scala, Kotlin, etc) only complicate this story. And exactly from here question arises - whether it is sufficient to exclude only methods for Lombok.

What do you think? Is it sufficient to cleanly document that only the methods and constructors annotated as @lombok.Generated are skipped?

Based on your description, I'm fine to give it a try.

But anyway would like to hear opinion of @marchof . So @marchof WDYT?

@Godin I'm also not a lombok user but from @t1 's description this seems to come with great benefits for typical lombok usage. Therefore I would also vote for adopting this.

Implementation wise we should use the new filtering API which just became available (eaef191) on master ;-)

Some additional thoughts from a non-Lombok-user:

Assuming that getters and setters are used by other code, they supposed to be covered indirectly, and if they are not used, then why do they exist? 😜 so can missing coverage be an indicator of unused code? if so, then maybe the fact that filter can't be disabled might be challenged. Was always wondering about this, but never had an access to experienced Lombok user, @t1 😉

Also in contrast for example to try-with-resources statement that introduces completely unreachable and hence untestable code, they don't introduce such.

Also filtering of getters and setters generated by Lombok might cause a logical question - why handwritten getters and setters are not filtered? 😉 Aren't we vendor-neutral, @marchof ? 😆

P.S. in the lights of restrictions that modularity in Java 9 will bring, the fact that Lombok hacks into internals of compiler to do its job looks quite interesting...

@Godin When code is handwritten unit tests are required. If code is generated there is no need for unit tests for the generated code (given that someone else tested the generator for us).

Implementation wise we should use the new filtering API which just became available

@marchof maybe, however

• as of today the main purpose of a new internal API - is to filter some instructions of a method, but not all of them
• needs to be measured, but usage of this API in comparison to the current way of filtering of synthetic methods might be slower
• we haven't migrated already existing filtering of methods
• might be a way simpler/faster from an implementation point of view to use old way
• and focus more on current work-in-progress for instruction-level filters ;)

As a Lombok user, the methods I'm eager to filter from Jacoco (and sonarqube) reports are equals() and hashcode(). They are complex and difficult to get full branch coverage. I trust the generated code and don't need to get int every nook and cranny.

@Godin The new API already works for this purpose! You have all information at hand in MethodNode. I agree that there is a faster way than filtering all instructions from the generated method, but this performance issue only applies to actually filtered methods.

I would prefer to integrate this feature with the new API and also move existing filters soon. I can prepare PRs for this.

performance issue only applies to actually filtered methods

Fair enough.

also move existing filters soon. I can prepare PRs for this.

@marchof I also can do this, while definitely can't review my own changes 😝 what about deal to do a review of filter for try-with-resources before move of existing filters? 😉

I would prefer to integrate this feature with the new API

Given a fair statement about performance and upcoming refactoring of existing filters, now this looks reasonable and convincing. @t1 will you have time and wish to work on this?

This comment is "just" for motivation and encouragement purposes. You deserve some credit!

@t1 thank you so much for that amazing pull-request. My dev-team and I at Otto (GmbH & Co. KG) would love to have this feature. We are lombok-powerusers.
Excluding everything annotated by lombok/Generated is exactly what we need!

@Godin To get this contribution in I can adjust it to the new API and do the paperwork.

@t1 Is there a reason for closing this PR? Any objections that we include your contributions based of the new filtering API?

Sorry for not replying earlier. I was extremely busy last week.
I've started to work on this again this morning, but I'm not git-savvy enough to understand how I can change the fix #15 comment in an existing pull request. And it wasn't much work, yet, and redoing it based on the new API seemed easier to me, so I wanted to start from scratch.
@marchof: Do you want to take over? I wouldn't object ;-)

@t1 Done #513. Please feel free to comment on this. The new implementation assumes the annotation is runtime invisible as Lombok JavaDoc says retention is CLASS. Can you confirm?

@marchof i think you are corrent. the annotation is runtime invisible. there is an option to write findbugsannotations next to the generated annotation. the findbugsannotation is runtime visible though :)
but it seems your implementation can use the invisible annotations as well! so no need to highjack the findbugs annotation for that :D

@t1 @BeneStem guys, do you have a hello-world Maven project demonstrating how to get methods annotated with lombok.Generated ? I'd like to do end-to-end test before merging #513

@Godin If you give me a few minutes I could build a demo with gradle.

@BeneStem take your time, example with Gradle will be useful! But anyway I would really love to see how to make it work with Maven too 😉

@Godin I'll do it with maven than.

@Godin Ok. You can use this one: https://github.com/BeneStem/iaa-lecture. If you have any questions feel free to ask :)

@BeneStem sorry, but first of all I wouldn't call it "minimalistic example" 😉 secondly and most importantly: I don't see annotation lombok.Generated in generated classes and this is exactly why I asked for the example - addition of Lombok as dependency is clearly not enough.

@Godin Lombok should add the annotations without setting any properties. Look at the bottom of this page: https://projectlombok.org/features/configuration.html. There is stated that you have to disable it manually if you don't want it.

"Lombok normally adds @javax.annotation.Generated annotations to all generated nodes where possible. You can stop this with:

lombok.addGeneratedAnnotation = false
Lombok can add the @SuppressFBWarnings annotation which is useful if you want to run FindBugs on your class files. To enable this feature, make sure findbugs is on the classpath when you compile, and add the following config key:

lombok.extern.findbugs.addSuppressFBWarnings = true"

@BeneStem here is my point:
if I compile

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@MyAnnotation
class Fun {
}

@Target({ElementType.CONSTRUCTOR, ElementType.METHOD, ElementType.FIELD, ElementType.TYPE})
@Retention(RetentionPolicy.CLASS)
@interface MyAnnotation {
}

then I can clearly see MyAnnotation in class file:

javap -v -p Fun | grep MyAnnotation
  #11 = Utf8               LMyAnnotation;

but I don't see Generated in your classes:

javap -v -p -cp target/classes de.nordakademie.iaa.model.Room | grep Generated

Moreover

java -jar ~/.m2/repository/org/projectlombok/lombok/1.16.16/lombok-1.16.16.jar config -g --verbose | grep "lombok.Generated"
## Generate @lombok.Generated on all generated code (default: false).

as well as projectlombok/lombok@70f77f9 state that generation of this annotation is off by default.

@Godin You are correct. I added another dependency and activated FindbugsAnnotation to see if this one is set.
I was able to verify that this is setted.

I will try activating the generated annotation now...

@Godin I tried activating "lombok.addGeneratedAnnotation=true" the annotation. But was not able to find it in the class file... But the FB annotation is there.
Are you able to see the annotation after pulling?

@BeneStem I managed to configure my own example - was using wrong name of configuration property and the correct one is lombok.addLombokGeneratedAnnotation

@Godin Ahh, thank you. Now it is working in my example as well. Thank you and sorry for the confusion!

@Godin I stated an issue at lomboks repostiory to document the property.