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.

Sign up for GitHub

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

Jump to bottom

Issue #12099: Add ArchUnit test to ensure fields in modules and utils are immutable, and modules are correctly annotated #12135

Merged
merged 1 commit into from Sep 12, 2022
Merged

Issue #12099: Add ArchUnit test to ensure fields in modules and utils are immutable, and modules are correctly annotated #12135

merged 1 commit into from Sep 12, 2022

Conversation

Vyom-Yadav
Copy link
Member

@Vyom-Yadav Vyom-Yadav commented Aug 29, 2022 •
edited

#12099

A brief about the changes in this PR:

  • There are 4 ArchRules, ArchUnit is just like English, so the rules are easy to understand.
    • fieldsInUtilClassesShouldBeImmutable
    • fieldsInStatelessChecksShouldBeImmutable
    • classesWithImmutableFieldsShouldBeStateless
    • classesWithMutableFieldsShouldBeStateful
  • Suppression list was added and populated.

@Vyom-Yadav Vyom-Yadav force-pushed the expandArchUnit-Immutability branch 5 times, most recently from 513d326 to e7f4583 Compare August 29, 2022 10:20
@rnveach rnveach self-assigned this Aug 29, 2022
rnveach
rnveach reviewed Aug 29, 2022
View reviewed changes
src/test/java/com/puppycrawl/tools/checkstyle/internal/UtilClassesImmutabilityTest.java Outdated
"com.puppycrawl.tools.checkstyle.utils.CommonUtil.EMPTY_OBJECT_ARRAY",
"com.puppycrawl.tools.checkstyle.utils.CommonUtil.EMPTY_STRING_ARRAY",
"com.puppycrawl.tools.checkstyle.utils.TokenUtil.TOKEN_IDS",
"com.puppycrawl.tools.checkstyle.utils.XpathUtil.TOKEN_TYPES_WITH_TEXT_ATTRIBUTE"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BitSet is mutable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix violations in a separate PR, for now this can stay in suppression list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, leaving for discussion.

@Vyom-Yadav Vyom-Yadav force-pushed the expandArchUnit-Immutability branch 5 times, most recently from 2d7c1e4 to 66d782e Compare August 29, 2022 19:19
@rnveach rnveach assigned nrmancuso and unassigned rnveach Aug 30, 2022
@rnveach rnveach requested a review from nrmancuso August 30, 2022 00:36
@Vyom-Yadav Vyom-Yadav force-pushed the expandArchUnit-Immutability branch 2 times, most recently from 2a987b4 to 15451dd Compare August 31, 2022 09:21
@Vyom-Yadav Vyom-Yadav mentioned this pull request Aug 31, 2022
Closed
@Vyom-Yadav Vyom-Yadav changed the title Issue #12099: Add ArchUnit test to ensure fields in Util classes are immutable Issue #12099: Add ArchUnit test to ensure fields are modules and utils are immutable and modules are correctly annotated Aug 31, 2022
@Vyom-Yadav Vyom-Yadav requested review from rnveach and removed request for nrmancuso August 31, 2022 09:28
@rnveach rnveach requested a review from nrmancuso August 31, 2022 12:20
@Vyom-Yadav Vyom-Yadav mentioned this pull request Sep 1, 2022
Closed
@Vyom-Yadav Vyom-Yadav force-pushed the expandArchUnit-Immutability branch 2 times, most recently from 97405b9 to f28ec2d Compare September 1, 2022 05:27
nrmancuso
nrmancuso requested changes Sep 5, 2022
View reviewed changes
Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more items, we are getting close now:

src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
@Vyom-Yadav Vyom-Yadav force-pushed the expandArchUnit-Immutability branch 2 times, most recently from 3de9398 to ece76c4 Compare September 7, 2022 20:54
nrmancuso
nrmancuso requested changes Sep 8, 2022
View reviewed changes
Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Immutability is still a somewhat nebulous definition to us, as we have no concrete way to say 100% that something is or isn't immutable. Let's make this method easy to understand and maintain as time goes on and we find more conditions to check.

Please also add some whitespace in here.

Items:

src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
@Vyom-Yadav
Copy link
Member Author

@nrmancuso I enhanced the JavaDoc of the method to clarify further what are we trying to achieve.

pbludov
pbludov suggested changes Sep 9, 2022
View reviewed changes
Copy link
Member

@pbludov pbludov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit of minor changes please

src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
nrmancuso
nrmancuso requested changes Sep 10, 2022
View reviewed changes
Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vyom-Yadav please fix condition name/ logic

src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
@nrmancuso nrmancuso assigned rnveach and unassigned nrmancuso Sep 10, 2022
rnveach
rnveach requested changes Sep 10, 2022
View reviewed changes
Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we discuss going over these suppressions, either in new issue or ... ?

src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java Outdated Show resolved Hide resolved
@romani
Copy link
Member

romani commented Sep 10, 2022

I recommend to apply all easy to fix items but all other, move to separate issue.

@Vyom-Yadav Vyom-Yadav force-pushed the expandArchUnit-Immutability branch 3 times, most recently from dc72712 to 6bfd95d Compare September 11, 2022 04:39
@Vyom-Yadav Vyom-Yadav mentioned this pull request Sep 11, 2022
Open
@Vyom-Yadav Vyom-Yadav force-pushed the expandArchUnit-Immutability branch 2 times, most recently from 0a69cd1 to 7bd92a7 Compare September 12, 2022 08:46
rnveach
rnveach reviewed Sep 12, 2022
View reviewed changes
src/test/java/com/puppycrawl/tools/checkstyle/internal/ImmutabilityTest.java
"com.puppycrawl.tools.checkstyle.checks.naming.LambdaParameterNameCheck",
"com.puppycrawl.tools.checkstyle.checks.naming.LocalFinalVariableNameCheck",
"com.puppycrawl.tools.checkstyle.checks.naming.LocalVariableNameCheck",
"com.puppycrawl.tools.checkstyle.checks.naming.MemberNameCheck",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a conversation while waiting for the last item to be done ( #12135 (comment) ),

MemberNameCheck

I took a look at this, and this class has no annotation on it. It extends AbstractAccessControlNameCheck which also has no annotation on it. It also extends AbstractNameCheck which has @StatelessCheck on it.

Is this violation solely because the annotation is on parent class, which is 2 level deep?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Violation is because the class itself has immutable fields (no fields in this case, not the inherited fields) and has no annotation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has an annotation but not the main class itself. I assume that via reflection the annotation will show up, but I was curious if ArchUnit was not seeing the annotation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ArchUnit is not seeing it, it also has @Inherited, I will report this to ArchUnit team.

@Vyom-Yadav
Issue checkstyle#12099: Add ArchUnit test to ensure fields in modules…
ec8c7a3 
… and utils are immutable, and modules are correctly annotated
@rnveach rnveach merged commit 69b3269 into checkstyle:master Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pbludov pbludov pbludov approved these changes

@rnveach rnveach rnveach approved these changes

@nrmancuso nrmancuso nrmancuso approved these changes

@baratali baratali Awaiting requested review from baratali

Assignees

@rnveach rnveach

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Vyom-Yadav @romani @pbludov @rnveach @nrmancuso