Support ErrorProne by using javax.annotation.processing.Generated or javax.annotation.Generated

[jnehlmeier]

Ebean uses its own annotation to mark generated code. However the standard annotations for generated code are javax.annotation.processing.Generated or javax.annotation.Generated.

When using Google ErrorProne to check code for bug patterns (similar to FindBugs/SpotBugs) the UnusedMethod checker (https://errorprone.info/bugpattern/UnusedMethod) is triggered for _Ebean$ModuleInfo.otherClasses(). This happens when you do not have any AttributeConverter or similar. In that case otherClasses() returns an empty list and the method itself is never called.

Regardless of the above (which could be fixed by always calling otherClasses or not generating the method), I generally do not want to execute code checks on generated code. ErrorProne has an option to skip any code that is annotated with javax.annotation.processing.Generated or javax.annotation.Generated. However this does not work with Ebean because of non-standard annotation being used.

Expected behavior

Use standard annotations for generated code so that code analysis tools can detect it

Actual behavior

io.ebean.typequery.Generated is used.

[rbygrave]

Background

We started with javax.annotation.Generated and life was simple. Then along came Java 9+ and @Generated got moved to javax.annotation.processing.Generated. At that point the querybean-generator followed the common approach of many other annotation processors of detecting which version of java we are compiling and using the appropriate annotation.

Ultimately that was problematic (I think I remember this correctly) because if we target compiling Java 8 byte code but have say Java 11 as the JDK then. In this case the annotation processor would use javax.annotation.Generated but of course this doesn't exist in Java 11 and we'd actually have a compilation error. So using javax.annotation.Generated was at best "risky" and we were not able to use javax.annotation.processing.Generated with Java 8 compilation.

Now at the time, I felt that a lot of annotation processors hit this issue and hence started using their own @Generated and this is where I felt we were at. So my expectation is that tools detecting @Generated started detecting any @Generated because that was becoming a thing !!!

Today

However today, Ebean has a minimum Java 11 requirement. That means, we can't possibly hit that issue that we could in the past as target version and JDK must both be 11+ now. That means, we should be back to a simple world where we could just go back to using javax.annotation.processing.Generated.

That is, we should just use javax.annotation.processing.Generated now (because now we are Java 11+ only).

[rbygrave]

Ah, I forgot another problem. javax.annotation.processing.Generated

1. Lives in the java.compiler module which is problematic for use with module-path.
2. Uses @Retention(SOURCE) ... which is problematic for tools like JaCoCo which want CLASS retention (or RUNTIME retention)

That is, back in the day I ended up having this conversation at jigsaw-dev on the topic - https://www.mail-archive.com/[email protected]/msg11191.html

TLDR of that is that using javax.annotation.processing.Generated is problematic with module-path with the thinking that we don't really want people to add requires static java.compiler into their module-info.

See also: google/error-prone@4d6a0cc and discussion at google/error-prone#1863 and discussion at mapstruct/mapstruct#1528

[rbygrave]

So I think this comes back to:

• ErrorProne actually supports all @Generated - check the version of ErrorProne being used.
• Using our own @Generated with retention CLASS still looks like the right thing to do (wrt tooling like JaCoCo that is reading classes)

[jnehlmeier]

See also: google/error-prone@4d6a0cc

Interesting, but when ErrorProne supports all annotations named "Generated" then why does it trigger for ModuleInfo? Seems like something is off here. I thought it might be because your @Generated has @Retention(RUNTIME) instead of @Retention(CLASS) (why is it RUNTIME?) but after skimming through ErrorProne source I couldn't find any indication for it. Looks like it should have worked.

Luckily ErrorProne also has the possibility to exclude source paths, so I excluded the generated source root in my project. But I see this solution as workaround for something that should have already worked out of the box.

[rbygrave]

then why does it trigger for ModuleInfo?

I think this refers to _Ebean$ModuleInfo. Yes, I'd expect ErrorProne to detect this is a generated class and ignore - hmmm.

(why is it RUNTIME?)

Er, it really should be @Retention(CLASS) ... and I think I should change that !!!

[rbygrave]

Hmmm, just to note that I think ModuleInfoLoader is probably a bad choice of name and probably _Ebean$ModuleInfo is also not great given that module-info is a thing. (and in some future people will need to explicitly specify the _Ebean$ModuleInfo in the module-info to service load it.

Maybe, EntityClassRegister and GeneratedEntityRegister or something like that would be better ...

[jnehlmeier]

then why does it trigger for ModuleInfo?

I think this refers to _Ebean$ModuleInfo. Yes, I'd expect ErrorProne to detect this is a generated class and ignore - hmmm.

Just found: google/error-prone#3108

I guess that is the reason why it did not work.

(why is it RUNTIME?)

Er, it really should be @Retention(CLASS) ... and I think I should change that !!!

There are a couple of other annotations in that package that use RUNTIME as well.

[jnehlmeier]

Hmmm, just to note that I think ModuleInfoLoader is probably a bad choice of name and probably _Ebean$ModuleInfo is also not great given that module-info is a thing. (and in some future people will need to explicitly specify the _Ebean$ModuleInfo in the module-info to service load it.

Maybe, EntityClassRegister and GeneratedEntityRegister or something like that would be better ...

Yeah, wasn't there a discussion somewhere about that name? Personally I never really liked it because the name doesn't give a clue what it is used for.

Not sure about "generated" in the name just because the file is generated. I would prefer having "ebean" in the name, so I know where this file is coming from. Maybe EbeanEntityRegister or Ebean_EntityRegister to make it look more like a generated file.

[rbygrave]

Note related: #2731 Change ebean-querybean annotations from Retention RUNTIME to CLASS

[rbygrave]

Note related: #2733 - Rename ModuleInfoLoader to EntityClassRegister and _Ebean$ModuleInfo to EbeanEntityRegister

[rbygrave]

Ok, I think we can close this one now. @jnehlmeier re-open if need be.