Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please also add @DaggerGenerated to generated nested classes #4540

Open
aleuner opened this issue Dec 18, 2024 · 3 comments
Open

Please also add @DaggerGenerated to generated nested classes #4540

aleuner opened this issue Dec 18, 2024 · 3 comments

Comments

@aleuner
Copy link

aleuner commented Dec 18, 2024

Dagger- generated code has the @Generated and the @DaggerGenerated annotations on top level classes of all generated Java source files.

The firster nicely allows source code scanning (e.g. static analysis) tools to recognize the code as generated, and to skip those classes. Also, implying that inner (or nested) classes of such code must also be generated code, to skip them too.
Sonar Scanner for example does this just fine.

JaCoCo, which scans class files, is different and will, for example, include generated classes like *_Factory.InstanceHolder or *ComponentImpl in its scans, even when excluding their respective directly annotated *_Factory / Dagger*Component outer classes.

I made feature request jacoco/jacoco#1815 for an improvement on this, and got an explanation basically pointing out:

  • Tools scanning class files cannot infer the inner-outer relationship so easily, because outer and inner classes reside in separate files.
  • While it might be possible to detect that a class is nested inside another (or maybe that an outer class has nested classes), figuring out if the outer class has @DaggerGenerated would mean no longer being able to look at class files one-by-one.

While the code for @Modules and @Components mostly resides in packages separated from other code - and I can thus add scanning exclusions for them by their package - the factory code for @Inject annotated classes is generated next to them.
I can still apply exclusion patterns but this requires knowledge of Dagger internals, which is uglier IMHO (but here you are for jacoco-maven-plugin: <exclude>**/*Factory$InstanceHolder.class</exclude>).

So it would be much nicer if all classes generated by Dagger got the @DaggerGenerated annotation ... generated.

@JakeWharton
Copy link

Just a small note:

Tools scanning class files cannot infer the inner-outer relationship so easily, because outer and inner classes reside in separate files.

This is not what they said. Your second bullet is their only limitation.

Class files contain a very explicit and easy-to-parse pointer to the enclosing class (if any), but getting any details about that class other than the relationship does require that it also be parsed which they are choosing not to do.

@aleuner
Copy link
Author

aleuner commented Dec 18, 2024

Hmm, I didn't express myself very well then :-)

While didn't know how exactly, even I assumed that the inner-class was somehow identifying its outer class. Even static inner classes. So yes, my first bullet point doesn't really hold and was more my hallucination.
I wanted to say that looking at the inner class it is not so easy to see the whole picture. Kind of redundant to the second point.

My understanding is that an enhancement in JaCoCo would be quite a big change. This is why I made the request here - hoping for a solution which is less intrusive.

@bcorso
Copy link

bcorso commented Dec 19, 2024

JaCoCo, which scans class files, is different and will, for example, include generated classes like *_Factory.InstanceHolder or *ComponentImpl in its scans, even when excluding their respective directly annotated _Factory / DaggerComponent outer classes.

Sorry, I'm not too familiar with JaCoCo. Is JaCoCo like hard-coded to look for @DaggerGenerated, or is this like a custom exclusion rule you've written yourself?

My understanding is that an enhancement in JaCoCo would be quite a big change.

It might be worth pushing a bit in jacoco/jacoco#1815. In particular, I don't think they actually said how big the change would be. I took the comment more as just a statement of how things currently work, so perhaps they would be okay with accepting the feature request if you pushed.

This is why I made the request here - hoping for a solution which is less intrusive.

This isn't difficult, per-se, but it's a bit annoying to track all of these down and keep it up to date. If you want to give it a try we can accept a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants