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

Option to ignore generated code #28

Closed
talios opened this issue May 5, 2015 · 14 comments
Closed

Option to ignore generated code #28

talios opened this issue May 5, 2015 · 14 comments

Comments

@talios
Copy link

talios commented May 5, 2015

A lot of code generated via tools like immutables.org trigger warnings from the plugin, it would be nice to be able to disable the warmings for these files.

In general, these are all annotated with javax.annotation.Generated, unfortunately that's flagged as @Retention(SOURCE) so can't be tracked at the .class level.

Thoughts? Maybe add an exclusions list in the plugin configuration?

@gaul
Copy link
Owner

gaul commented May 5, 2015

@talios Could you provide an example? I followed the simple example from immutables.org but could not trigger any modernizer warnings.

@talios
Copy link
Author

talios commented May 5, 2015

When compiling for JDK7 I get out of our builds:

[ERROR] /Users/amrk/.../ImmutableTrustedIpDto.java:26: Prefer java.util.Objects.requireNonNull(Object)
[ERROR] /Users/amrk/.../ImmutableTrustedIpDto.java:27: Prefer java.util.Objects.requireNonNull(Object)

The toString() generated method uses Google Guava:

@Override
public String toString() {
    return MoreObjects.toStringHelper("TrustedIpDto")
            .add("id", id)
            .add("name", name)
            .add("dateRange", dateRange)
            .toString();
}

Maybe it's more an of option to treat guava as "semi modern"?

@vladimirfx
Copy link

+1
I've use JDO enhancement for truly transient persistence. Result of modernizer check is 3 line ERROR for single entity:

BlackListEntry.java:-1: Prefer java.lang.StringBuilder
BlackListEntry.java:-1: Prefer java.lang.StringBuilder
BlackListEntry.java:-1: Prefer java.lang.StringBuilder

Exclude file can't help with that, because that violation is usefull for my handwrited code.

@gaul
Copy link
Owner

gaul commented Jul 11, 2015

I do not see an easy, general fix for this -- #3 (comment) suggests hooking the source annotation during compilation for later use in the main modernizer goal. I opened an upstream issue against immutables to use StringBuilder but could not find where JDO uses StringBuffer -- @vladimirfx can you point me to this or open an upstream issue and reference it here?

@vladimirfx
Copy link

StringBuffer is extensively used in Datanucleus enchanced classes. See class
org.datanucleus.enhancer.methods.CopyField for example and many other in same package.
Actualy, there is nothing wrong with Datanucleus Enchancer and many other bytecode manipulation libs. This libs designed for compatibility with old Java ( or Guava , etc) versions. There is nothing to fix instead of modernizer.
We need exclusion filter not only by rule but by target characteristics like FindBugs has.

@vorburger
Copy link
Contributor

immutables/immutables#749 is another issue in Immutables.org re. this... but it's probably unrealistic to expect all 3rd party code generators to adapt, so the idea of using #3 (comment) seems cool...

In the case of Immutables.org, and perhaps other code generators, I've noticed that it also adds the @edu.umd.cs.findbugs.annotations.SuppressFBWarnings annotation (probably only when its on the classpath) and that annotation happens to be RetentionPolicy.CLASS - perhaps that could be used as a "pragmatic inference" that such a class should be ignored - or is that a net cast too wide?

In the case of Immutables.org specifically, a simple exclusion / ignore by class name could work, given that they are (typically, it's customizale) all named Immutable* ... see #64.

@jhaber
Copy link

jhaber commented Mar 5, 2018

I think this can be solved as part of #3, which we're working on in our fork. The simplest thing that comes to mind is config flags like includeGeneratedClasses and includeGeneratedTestClasses (both defaulting to false)

@vladimirfx
Copy link

@jhaber
In our case classes is not generated but enhanced, i.e. bytecode of original classes is modified. This is usual practice in world of ORMs and AOP tools. And usually injected bytecode is intensionally "old" style code to stay compatible with previous platform versions.
Unfortunately all such masses (model classes, AOP enhanced services etc) of code cat not be verified by modernizer :(

@jhaber
Copy link

jhaber commented Mar 6, 2018

Interesting, is it possible to run modernizer before the bytecode is modified? You also mentioned exclusions by target characteristics, did you have something particular in mind?

@vorburger
Copy link
Contributor

@jhaber and @vladimirfx just wanted to make sure that you saw that there now IS actually already a new option to ignore/exclude by package / class name FQN reg exp, see #67. @jhaber if you have #3 working for some specific @SuppressWarnings, it perhaps shouldn't be that hard to extend that to also kick in for any javax.annotation.Generated? That was probably the initial goal of this issue.

@jhaber
Copy link

jhaber commented Mar 6, 2018

👍 for using the same mechanism to ignore javax.annotation.Generated classes. I think @vladimirfx's issue is different enough to tackle in a separate issue

@Serranya
Copy link
Contributor

Serranya commented Sep 11, 2019

Immutables added their own @Generated annotation that has a retention of CLASS. Other projects e.g. rocker did the same.

As reference jacoco recognizes all annotations that are called Generated as long as the rentention is wider than SOURCE. I think modernizer should do the same. This should already cover a great set of projects.

@gaul
Copy link
Owner

gaul commented Sep 11, 2019

This sounds like a great idea; can someone investigate implementing this? I believe it should be straightforward if you follow the existing @SuppressModernizer implementation.

@gaul
Copy link
Owner

gaul commented Oct 13, 2019

Fixed by #99.

@gaul gaul closed this as completed Oct 13, 2019
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

6 participants