[GitHub] [geode] jujoramos opened pull request #4078: GEODE-7226: Ignore @Immutable classes in PMD

["jujoramos (GitHub)" <gi...@apache.org>]

The 'StaticFieldsMustBeImmutable' rule now detects the '@Immutable'
annotation at the class level, so developers can annotate the class
directly instead of manually annotating every effectively immutable
static field individually.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [X] Is your initial contribution a single, squashed commit?

- [X] Does `gradlew build` run cleanly?

- [X] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/4078 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on issue #4078: GEODE-7226: Ignore @Immutable classes in PMD

["jujoramos (GitHub)" <gi...@apache.org>]

@onichols-pivotal: I'm not saying it's a bad thing (a bit verbose sometimes, yes, not a bad thing per se), I'm saying that the analysis **_is not working as it's intended, neither as it's documented right now_**, so it's a bug that needs to be fixed in my opinion.
To give you some more context: I opened a PR the other day and one of my classes had multiple effectively immutable fields so I annotated it directly with `@Immutable`, as described [here](https://github.com/apache/geode/blob/develop/static-analysis/pmd-rules/src/main/resources/org/apache/geode/pmd/staticimmutable.xml#L35-L36): _**you can annotate that class or the field**_. The build CI, however, failed with `All static final fields must be immutable objects. No mutable static state is allowed`; it wasn't obvious why the failure was happening until I checked the source code for `StaticFieldsMustBeImmutable` class and found the "bug"...
Sure, I can go ahead and change the `PR` to just modify the rule documentation/description of the rule so the user knows that only the field level annotation is supported but, considering that this was [discussed and approved](https://markmail.org/message/ekuvv24orjha64lu) through the dev list, I still believe it's better to change rule to behave as it was originally proposed.  

[ Full content available at: https://github.com/apache/geode/pull/4078 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] Bill commented on issue #4078: GEODE-7226: Ignore @Immutable classes in PMD

["Bill (GitHub)" <gi...@apache.org>]

IMO it is not ok for the `StaticFieldsMustBeImmutable` rule to ignore classes with the `@Immutable` annotation.

The only purpose I can see, of an `@Immutable` anno, would be to express developer intent similarly to the way `@Override` does. For the expression of intent to be most useful, it would be enforced somehow, e.g. if I mark a class as `@Immutable` but that class has a non-final field, I should get a build-time error.

Mutability of static fields is an issue orthogonal to mutability of the class that contains the fields. Whether or not a class (i.e. its instances) are mutable, it should be viewed as a design bug, if that class has any non-final static field, or any final static field referencing a mutable object type.

So I'm a -1 on this PR as it stands.

[ Full content available at: https://github.com/apache/geode/pull/4078 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] upthewaterspout commented on issue #4078: GEODE-7226: Ignore @Immutable classes in PMD

["upthewaterspout (GitHub)" <gi...@apache.org>]

I guess I'm in favor of not merging this PR.

Originally I was hoping we could just use `@Immutable` like the JCP annotation - putting it on a class would mean instances of the class are immutable. Then PMD could enforce that static final fields only to refer to `@Immutable` classes. So the fact that you can put `@Immutable` on a static field is really more of a workaround to the limitations of PMD, and maybe the fact that some static fields don't refer to immutable classes that we control.

I agree there is probably more value in using `@Immutable` on a class as documentation that the *instances* of the class are immutable. 

[ Full content available at: https://github.com/apache/geode/pull/4078 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on issue #4078: GEODE-7226: Ignore @Immutable classes in PMD

["jujoramos (GitHub)" <gi...@apache.org>]

@onichols-pivotal: agreed, and the detailed response is really appreciated 👍.
That said, just one more thing: according to the [original PR description](https://github.com/apache/geode/pull/3178):

```
Immutable - mark classes or fields that are really immutable as immutable. The pmd rule can ignore these.
```

The above (plus the fact that my class was effectively immutable and annotated with the `@Immutable` annotation) is what made me think I was actually hitting a bug in our custom rule, that's why I opened the ticket and made the changes. Deciding whether we should rename and re-think our current annotations should be addressed through another ticket, and probably after some discussion within the dev list.
If there's not enough consensus to merge this PR I'll just change the rule documentation without touching the actual implementation, at the end of the day it won't kill anyone to annotate multiple fields instead of the class 🤷‍♂.

@upthewaterspout @Bill : thoughts?.

[ Full content available at: https://github.com/apache/geode/pull/4078 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on issue #4078: GEODE-7226: Ignore @Immutable classes in PMD

["jujoramos (GitHub)" <gi...@apache.org>]

@upthewaterspout @Bill @onichols-pivotal @demery-pivotal 

Thanks all for the feedback, instead of modifying the rule's implementation I'll just update its documentation to prevent confusions when the PMD task fails.
I won't open a new PR though, will just update this one instead as the discussion thread is pretty good and might be useful in the future when looking at the ticket 👍.
Cheers.

[ Full content available at: https://github.com/apache/geode/pull/4078 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on issue #4078: GEODE-7226: Ignore @Immutable classes in PMD

["jujoramos (GitHub)" <gi...@apache.org>]

@demery-pivotal: I hear and - at some level - agree with you concern... however, this is still a bug from my perspective as the annotation and PMD analysis is not working as it should have worked since we introduced this to Geode back in February. See the [discussion](https://markmail.org/message/ekuvv24orjha64lu) and the [original's PR description](https://github.com/apache/geode/pull/3178) for further details.
I expect the PMD analysis to totally ignore the static "mutable" (not really) fields within my class if I add the `@Immutable` annotation to the class itself, by adding the annotation I'm explicitly stating that the entire class is immutable and it's my responsibility to make sure this stands true. We can certainly make mistakes, of course, but that's when the reviewers from the community should point them out while analysing the PR.
Just my 2 cents, please let me know your thoughts.

[ Full content available at: https://github.com/apache/geode/pull/4078 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on issue #4078: GEODE-7226: Ignore @Immutable classes in PMD

["jujoramos (GitHub)" <gi...@apache.org>]

@demery-pivotal: I hear and - at some level - agree with you concern... however, this is still a bug form my perspective as the annotation and PMD analysis is not working as it should have worked since we introduced this to Geode back in February. See the [discussion](https://markmail.org/message/ekuvv24orjha64lu) and the [original's PR description](https://github.com/apache/geode/pull/3178) for further details.
I expect the PMD analysis to totally ignore the static "mutable" (not really) fields within my class if I add the `@Immutable` annotation to the class itself, by adding the annotation I'm explicitly stating that the entire class is immutable and it's my responsibility to make sure this stands truth. We can certainly make mistakes, of course, but that's when the reviewers from the community should point them out while analysing the PR.
Just my 2 cents, please let me know your thoughts.

[ Full content available at: https://github.com/apache/geode/pull/4078 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] onichols-pivotal commented on issue #4078: GEODE-7226: Ignore @Immutable classes in PMD

["onichols-pivotal (GitHub)" <gi...@apache.org>]

A class-level @Immutable annotation would be useful iff PMD could leverage it to enforce that all fields are immutable or effectively immutable or annotated @Immutable. 

As it is, this PR seems to be proposing to de-automate a check currently performed by PMD, instead expecting "reviewers from the community should point them out while analysing the PR."  This seems like a step backwards.  Why is it such a bad thing that fields be analyzed and annotated individually?

[ Full content available at: https://github.com/apache/geode/pull/4078 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org