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

[java] Make AtLeastOneConstructor Lombok-aware #667

Closed
GiantCrocodile opened this Issue Oct 14, 2017 · 16 comments

Comments

Projects
None yet
4 participants
@GiantCrocodile

GiantCrocodile commented Oct 14, 2017

We already have Lombok-aware rules, but AtLeastOneConstructor is currently not among them.

The rule should be extended to consider classes annotated as @Data, @Value, @Builder, @NoArgsConstructor, @RequiredArgsConstructor and @AllArgsConstructorAtLeastOneConstructor

@GiantCrocodile GiantCrocodile changed the title from Make PMD compatible with Lombok to [java] Make PMD compatible with Lombok Oct 14, 2017

@adangel

This comment has been minimized.

Member

adangel commented Oct 16, 2017

Thanks for the suggestion! We have already some limited support for the rules UnusedPrivateField, ImmutableField and SingularField.

For missing constructors, which rule are you referring to? Is it AtLeastOneConstructor? This could be probably extended to be less strict, when Lombok is used.

Are there other rules, you would see, that could benefit from "Lombok-Awareness"?

@GiantCrocodile

This comment has been minimized.

GiantCrocodile commented Oct 18, 2017

Hey @adangel. I think that some specific rules should respect the @Data annotation of Lombok and the @Entity annotation . Right now I have these warnings in my Lombok models:

  • UnusedPrivateField (unlikely with Entity annotation)
  • AtLeastOneConstructor (unlikely with Lombok)

These two warnings are very unlikely to be true if they are in a class which has these two annotations.

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 18, 2017

@GiantCrocodile@Entity is not a lombok annotation as far as I know... are you referring to javax.persistence.Entity?

If so, I'm not sure it would be ok to whitelist it... it simply grants serialization / deserialization from the DB, but no way to access the field in application code / no usage in hashCode, equals, toString... Putting / retrieving stuff from the db when never used in application logic seems unnecessary, and usually points to a dead code issue... I see only 2 reasons to do this:

  1. the db is shared with another application that requires that field. A very specific scenario which is better handled by @SuppressWarning / an XPath suppression rule.
  2. altering the db schema is a costly operation we want to avoid / defer to downtime. Again, an either transient or very specific scenario.

What's your scenario? I may be missing a point here...

As for @Data, we should already be handling it, at least for the UnusedPrivateField. If so, this is a false-positive we should definitely address.

AtLeastOneConstructor should definitely be relaxed to avoid issues with classes annotated as @Data, @Value, @Builder, @NoArgsConstructor, @RequiredArgsConstructor, @AllArgsConstructor

@GiantCrocodile

This comment has been minimized.

GiantCrocodile commented Oct 18, 2017

Yes I mean the @Entity annotation you mentioned. Although I suspected it to do more and this is why I mentioned it here too. Honestly I'm not sure where I saw a relation between this entity and Lombok. Maybe because I knew it is used somewhere else but it is impossible to detect that I think. Otherwise you would have to have a logical interpretation of the whole source code to see if this model's variable is used somewhere. Mentioning it wasn't related to any of the two scenarios you thought of.

I agree on the other points you mentioned afterwards. Regarding UnusedPrivateField, either this is a false-positive or an outdated rule.

@jsotuyod jsotuyod changed the title from [java] Make PMD compatible with Lombok to [java] Make AtLeastOneConstructor Lombok-aware Oct 29, 2017

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 29, 2017

@GiantCrocodile I reworked the report according to our discussion.

I'd like to ask you to confirm the UnusedPrivateField false positive under PMD 5.8.1, and if so, open a new issue to track it separately. Thanks in advanced.

@GiantCrocodile

This comment has been minimized.

GiantCrocodile commented Oct 29, 2017

@jsotuyod PMD 5.8.1 was released 01-July-2017. I think I'm already using 5.8.1 because when I reported this the version was already out some months. Are you sure you mean 5.8.1 or am I wrong somehow?

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 29, 2017

@GiantCrocodile yes, 5.8.1 is the last release before the upcoming 6.0.0. If you are certain, I'd please ask you to submit a new issue with the code snippet triggering the issue, since it's not being covered by our current test case

@GiantCrocodile

This comment has been minimized.

GiantCrocodile commented Oct 29, 2017

@jsotuyod I just noticed that I was wrong. I forgot to put @Data annotation above one of my classes and thus violation of rule UnusedPrivateField was triggered.

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 29, 2017

@GiantCrocodile it happens! Awesome then, we just need to work on the AtLeastOneConstructor rule

@GiantCrocodile

This comment has been minimized.

GiantCrocodile commented Nov 29, 2017

What about this one for v6.0.0 @jsotuyod?

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Nov 29, 2017

@GiantCrocodile I don't think so. We are too close to 6.0.0 to change it's scope. But definitely doable for 6.1.0. After 6.0.0 we should return to our one month release cycles.

@jsotuyod jsotuyod added this to the 6.1.0 milestone Nov 29, 2017

@adangel adangel modified the milestones: 6.1.0, 6.2.0 Feb 25, 2018

@adangel adangel modified the milestones: 6.2.0, 6.3.0 Mar 26, 2018

@GiantCrocodile

This comment has been minimized.

GiantCrocodile commented Apr 22, 2018

Any news on this :)?

@adangel adangel modified the milestones: 6.3.0, 6.4.0 Apr 28, 2018

@bke2027

This comment has been minimized.

bke2027 commented May 10, 2018

@UtilityClass also doesn't quiet the UseUtilityClass rule

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented May 10, 2018

@bke2027 added #1094 to track that unrelated improvement.

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented May 30, 2018

@GiantCrocodile this has long been overdue. I've finally submitted a PR to fix this for 6.5.0. Along the way, I found and fixed a couple of FNs.

Please, check it out and provide your feedback.

jsotuyod added a commit to Monits/pmd that referenced this issue May 30, 2018

[java] AtLeastOneConstructor supports Lombok
 - Lombok annotations that create a constructor whitelist the class
 - Take the chance to cover a bunch of other FNs
 - Improve documentation on the rule
 - Some other minor improvements
 - Fixes pmd#667
@GiantCrocodile

This comment has been minimized.

GiantCrocodile commented Jun 2, 2018

Thank you @jsotuyod!

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