Issue #16786: Ensure enum constants accessibility is properly evaluated #16787

JackPGreen · 2025-04-07T15:52:49Z

Extends change in #16049, in the case of an enum constant, it's accessibility will be derived based on the enclosing enum (which could be private) rather than being assumed to be implicitly public.

Test(s) added to validate.

Fixes: #16786
Closes: #17112

romani · 2025-04-13T19:21:03Z

@JackPGreen , CI is very red, please address it.

JackPGreen · 2025-04-13T23:32:04Z

@JackPGreen , CI is very red, please address it.

Thanks, fixed.

f44a879

romani · 2025-04-19T14:01:15Z

@JackPGreen , please squash commits in one, CI does not like it, we need green CI

JackPGreen · 2025-04-19T15:30:32Z

@JackPGreen , please squash commits in one, CI does not like it, we need green CI

Done :)

romani · 2025-04-20T15:46:16Z

@kkoutsilis , @mahfouz72 , please review update.
and please review my comment in issue, I placed soft approval, but if you do not think so, we can discuss it a bit more.

Main point here is not Java spec, but more about javadoc tool behavior and user experience on documentations, no need to document what is not placed in html pages of javadoc. If user do care so much on doc on enum, he should put it at top level class.

JackPGreen · 2025-04-20T16:05:28Z

Main point here is not Java spec, but more about javadoc tool behavior and user experience on documentations, no need to document what is not placed in html pages of javadoc. If user do care so much on doc on enum, he should put it at top level class.

I think there are two separate issues here:

Issue #9280: New property 'accessModifiers' as substitution of 'scope' and 'excludeScope' in JavadocVariableCheck #16049 caused an unintended change in behaviour for enums only in a patch release (unexpected) which this PR addresses (which prevented me from upgrading Checkstyle in my project)
it appears that inaccessible public fields (whether enum, class etc) aren't being handled as well as they could be, and a wider fix could fix all the issues at once.

It's up to you whether (1) is worth addressing in isolation.

mahfouz72 · 2025-04-21T10:49:30Z

@kkoutsilis , @mahfouz72 , please review update.
and please review my comment in issue

I am ok with the update as it addresses the enum case. However, this issue is broader. We should verify whether a variable is truly accessible. So if we are going to fix this, we should handle all such cases, not just enums.

Docs need to be updated also because right now it doesn't tell anything about true accesibility, it just talks about the direct access modifier of the variable

I noticed that @kkoutsilis tried to handle such cases by using concepts like surroundingAccessModifier.

But he got these reviews.

if we need advanced filtering based on outer access modifier, let's do it as a separate step, when we have real use cases

I think we should look at class/type's fields own visibility, completely ignoring all upper types visibility/modifiers., Let's not consider any non target visibility. On edge cases, users might use suppressions.

@kkoutsilis Will it be easy to bring back your work on trying to handle the surrounding/outer scope issue?

romani · 2025-04-22T13:17:56Z

I recommend to remove violation as hot fix, by merge of this PR, and in separate issue to do better support.
For now it is better to remove false-positive quicker.
@MohanadKh03 , do you agree ?

MohanadKh03 · 2025-04-23T10:41:22Z

For now it is better to remove false-positive quicker.

I agree as well the changes in this PR looks good but I just wonder what about other tokens in the future ?

This is long and heavy problem to define scope/visibility based on all upper classes.
I think we should look at class/type's fields own visibility, completely ignoring all upper types visibility/modifiers.

At least for the current issue of enums I see this PR is good to go but what about other tokens ? should they be handled separately in different PRs ? I think that this is not a big issue and user could just document the whole thing in an upper class for most of the tokens in the future but after taking a quick look on codebases that have inner types (classes, enums, etc..) they mostly document the inner class not just within the upper class e.g: Spring's codebase is full of this pattern

To make it even more complicated, modern java has a lot not explicit visibilities, we have separate Check on this, and it is very complicated by its own.

Since it is complicated on its own we might as well just handle enums for now and start discussing other tokens if needed to handle them at all

kkoutsilis · 2025-04-23T17:47:54Z

Hey, to give my two cents here, this PR looks good for addressing the enums problem @JackPGreen faced, but this makes the check a bit confusing, as in all other cases it checks for direct access modifier, though for enums it checks the surrounding access modifier.

The docs should be updated to clarify this behaviour before merging this PR.

Overall, we probably need to decide if we keep the direct access modifier or refactor to use the surrounding access modifier for all cases. I don't have a strong opinion right now on one or the other, I just prefer consistency for all cases overall when possible.

A good indication would be user feedback. If there are more confused or blocked users by this check's behaviour, then it's worth spending time on it. If there are no further complaints and users are happy, maybe it's not worth the discussion.

Hope this makes sense.

@kkoutsilis Will it be easy to bring back your work on trying to handle the surrounding/outer scope issue?

@mahfouz72 If we decide to go with it, I can have a look. As you see from the previous PR I faced some issues with it 😅, but it should be possible.

james-baker-aera · 2025-05-01T10:17:51Z

@kkoutsilis

A good indication would be user feedback. If there are more confused or blocked users by this check's behaviour, then it's worth spending time on it. If there are no further complaints and users are happy, maybe it's not worth the discussion.

I'm here to say that this is stopping us upgrading to a more recent version of Checkstyle (without turning the JavadocVariableCheck off). We have a lot of package-private enums without Javadoc that 10.22.0 and after flag as violations.

(I found this PR while looking into the violations.)

romani · 2025-05-02T15:37:52Z

Thanks a lot for feedback.
Please stay on previous version, we will merge this PR soon, we just need to align on expected behavior. I will do release right after this fix is merged.

@james-baker-aera, can you check on your project that this fix is covering all your false positives?
If you have open source project, please share link we will put it in testing efforts to make sure no regression.

romani · 2025-05-02T15:46:01Z

The docs should be updated to clarify this behaviour before merging this PR.

@JackPGreen , please update documentation to mention that we still consider interpretation of javadoc tool , when evaluating accessibility.
We can add small enum to examples, and make a comment above it, that javadoc tool doesn't consider it public, so Check does the same

JackPGreen · 2025-05-02T22:29:36Z

The docs should be updated to clarify this behaviour before merging this PR.

@JackPGreen , please update documentation to mention that we still consider interpretation of javadoc tool , when evaluating accessibility. We can add small enum to examples, and make a comment above it, that javadoc tool doesn't consider it public, so Check does the same

I'm not sure that's sensible in it's current state - with this change, accessibility is only "properly" evaluated (i.e. considering parents) for enums - but other scenarios (e.g. a public member of a private class), it isn't - so it's not accurate to say that it's consistent with Javadoc tooling.

I think improving the documentation would be better once ^ that ^ is properly addressed and it's simpler to explain.

romani · 2025-05-03T14:02:27Z

if we extend documentation to claim that we consider parents visiblity (no mentioning of javadoc tool), it will be enough.
and we can create new issue on fields of classes, as you described at #16786 (comment) , so it will be know defect in Check and we can address it in separate PR, as looks like it less bother people than enum.
Parents visibility matters for javadoc tool, one of the major usages for people who write javadoc.

On future, there will be group of people who wants to not consider javadoc tool at all, as they use only IDE abilities to render javadoc during navigation over code. We will think in future on how to reconcile such use cases, lets proceed with focus on javadoc tool.

JackPGreen · 2025-05-03T15:45:43Z

if we extend documentation to claim that we consider parents visiblity (no mentioning of javadoc tool), it will be enough. and we can create new issue on fields of classes, as you described at [#16786 (comment)]

Happy to add documentation, but struggling with the wording.

I updating the docs, changing:

It also accounts for special cases where fields have implicit modifiers, such as public static final for interface fields and public static for enum constants.

To:

It also accounts for special cases where fields have implicit modifiers, such as public static final for interface fields and public static for enum constants, or where the enclosing types' accessibility is more restrictive and hides the field.

Might help? Suggestions welcomed.

romani · 2025-05-03T20:06:33Z

or where the nesting types accessibility is more restrictive and hides the nested field.

Please add to all examples your junit test code.

public enum PublicEnum {
        CONSTANT // violation, 'Missing a Javadoc comment'
    }

    private enum PrivateEnum {
        CONSTANT
    }

JackPGreen · 2025-05-03T22:22:38Z

or where the nesting types accessibility is more restrictive and hides the nested field.

Please add to all examples your junit test code.
public enum PublicEnum {
        CONSTANT // violation, 'Missing a Javadoc comment'
    }

    private enum PrivateEnum {
        CONSTANT
    }

PR updated.

@JackPGreen , please squash commits in one, CI does not like it, we need green CI

But squashed into one commit because of ^ this ^

romani · 2025-05-03T22:32:17Z

[ERROR] [checkstyle] [ERROR] C:\projects\checkstyle\src\site\xdoc\checks\javadoc\javadocvariable.xml:241: Trailing whitespace is not allowed [noTrailingWhitespace]

romani · 2025-05-03T22:46:12Z

Before pushing, run on local mvn clean verify

JackPGreen · 2025-05-03T23:07:31Z

Before pushing, run on local mvn clean verify

Thanks, was testing locally but with the wrong goal (checkstyle:checkstyle). Hopefully all fixed now:

[INFO] BUILD SUCCESS

romani · 2025-05-03T23:18:10Z

Add to commit results on test execution, test phase generate actual xdoc pages

Patch at https://dev.azure.com/romanivanovjr/romanivanovjr/_build/results?buildId=29287&view=logs&j=c902ebb4-c9f8-5f09-4e17-ff78fbbc842e&t=9ca98c81-ff64-58f0-9d03-a23ac1c4a111&l=1850

JackPGreen · 2025-05-05T08:17:31Z

Add to commit results on test execution, test phase generate actual xdoc pages

Not sure I follow, sorry?

There were some checkstyle/CI issues with the docs I was unable to reproduce locally but all addressed now.

romani

If you would like to add more text to website
Add it to https://github.com/checkstyle/checkstyle/blob/master/src/site/xdoc/checks/javadoc/javadocvariable.xml.template
Run tests, it will update xdoc file , add it to commit.

Items:

romani · 2025-05-05T15:30:04Z

src/main/java/com/puppycrawl/tools/checkstyle/utils/CheckUtil.java

@@ -314,20 +314,27 @@ public static boolean isReceiverParameter(DetailAST parameterDefAst) {
     * @return the access modifier of the method/constructor.
     */
    public static AccessModifierOption getAccessModifierFromModifiersToken(DetailAST ast) {


This method is used in lot of Checks , lets not extend this fix to others
https://github.com/search?q=repo%3Acheckstyle%2Fcheckstyle%20getAccessModifierFromModifiersToken&type=code

Let's keep this fix isolated to single Check.
You can copy this util method to Check, and apply change to Check special method.

This method is used in lot of Checks , lets not extend this fix to others https://github.com/search?q=repo%3Acheckstyle%2Fcheckstyle%20getAccessModifierFromModifiersToken&type=code

Let's keep this fix isolated to single Check. You can copy this util method to Check, and apply change to Check special method.

The PR that exposed this issue changed ^ that ^ method so I think updating it again makes sense in case there are other places the same issue crept in.

It might be great miss in review of that PR.

If we change common method, now we need to re check all affected Checks that this new logic is good for them.
Most of them are not javadoc so, same though might be not applicable.

Javadoc check affected is only JavadocMethod.
https://checkstyle.org/checks/javadoc/javadocmethod.html
That use that same property.

Ok, lets do copy of method , keep in Until class, but new method will be used for javadoc only. In other case, we need to do diff testing for all affected Checks. I hope to do diff testing only for 2 javadoc Checks.

It might be great miss in review of that PR.

If we change common method, now we need to re check all affected Checks that this new logic is good for them. Most of them are not javadoc so, same though might be not applicable.

I understand your concern, but those checks have already been affected in an unexpected way without checking.

usage is big:
RecordComponentNumberCheck
UnusedLocalVariableCheck
ParameterNameCheck

JavadocVariableCheck
JavadocMethodCheck

and it might be concern for non-javadoc Check, as we are conflict of interpretation for javadoc and non-javadoc.
Functionality before this PR was focused on java only, and we naively thought that it will work for javadoc, that is why we have this issue.
Now we are clear that javadoc has nuance on it, and only javadoc related checks should be affected by this update.

does it make sense?

We already can not predict and find by diff testinng ease sceanrios.
Whole reason of this issue exists, as we did not predicted it, diff showed nothing.

We can merge this, and in few moth one more person will be frustrated that we did not account him,

Before this change, if I remember correctly, this method was raising an error as it could not evaluate the access modifier of an ENUM_CONSTANT_DEF. Based on that, I don't believe any other checks are affected, unless a scenario is not tested.

If I understand correctly, the conflict of opinions here is whether we should copy the method with the new changes to be used only for Javadoc checks, or update the existing method, which is used in a plethora of checks.

As I see it we could make a copy of the method for javadoc checks with the new updates, and revert the method to be used in other checks before these changes, this will probably satisfy both sides.

In my mind, it does not make a big difference, as the access modifier of ENUM_CONSTANT_DEF does not seem to be evaluated anywhere else apart from this check, but just to be safe, the approach of copy for javadoc and revert for the rest should be the safer option.

@romani @JackPGreen Did I get that correct? What do you think?

As I see it we could make a copy of the method for javadoc checks with the new updates, and revert the method to be used in other checks before these changes, this will probably satisfy both sides.

In my mind, it does not make a big difference, as the access modifier of ENUM_CONSTANT_DEF does not seem to be evaluated anywhere else apart from this check, but just to be safe, the approach of copy for javadoc and revert for the rest should be the safer option.

@romani @JackPGreen Did I get that correct? What do you think?

I think adding another path with the previous undesirable behaviour just makes everything more complicated... but if it's two project maintainers vs one contributor I guess it is what it is.

@JackPGreen , can you create new PR with split usage for method. Creation of new method might result in coverage problem, that might be not easy to cover, especially pitest.

But let give it a try, and we might be lucky. So we will have two PRs to merge one or another.

This PR is in good state, let's keep it without changes.

I will estimate risk vs efforts to make subjective decisions based on my experience.

@JackPGreen , can you create new PR with split usage for method. Creation of new method might result in coverage problem, that might be not easy to cover, especially pitest.

But let give it a try, and we might be lucky. So we will have two PRs to merge one or another.

#17112

...mples/resources/com/puppycrawl/tools/checkstyle/checks/javadoc/javadocvariable/Example6.java

romani · 2025-05-17T15:08:18Z

Github, generate report for UnusedLocalVariable/all-examples-in-one

github-actions · 2025-05-17T15:56:01Z

Report for UnusedLocalVariable/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a019bb4c950971ed3e1c7677b60f523da5cb3776_2025155548/reports/diff/index.html

romani · 2025-05-17T16:34:05Z

Github, generate report for ParameterName/all-examples-in-one

github-actions · 2025-05-17T17:35:25Z

Report for ParameterName/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a019bb4c950971ed3e1c7677b60f523da5cb3776_2025173248/reports/diff/index.html

romani · 2025-05-17T19:10:16Z

Github, generate report for RecordComponentNumber/all-examples-in-one

github-actions · 2025-05-17T20:07:15Z

Report for RecordComponentNumber/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a019bb4c950971ed3e1c7677b60f523da5cb3776_2025200701/reports/diff/index.html

romani · 2025-05-19T02:27:53Z

Github, generate report for JavadocVariable/all-examples-in-one

romani · 2025-05-19T02:28:45Z

Github, generate report for JavadocMethod/all-examples-in-one

github-actions · 2025-05-19T03:11:27Z

Report for JavadocVariable/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a019bb4c950971ed3e1c7677b60f523da5cb3776_2025031054/reports/diff/index.html

github-actions · 2025-05-19T03:19:27Z

Report for JavadocMethod/all-examples-in-one:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a019bb4c950971ed3e1c7677b60f523da5cb3776_2025031910/reports/diff/index.html

romani · 2025-05-20T01:56:11Z

All reports are empty on diff except for JavadocVariable, JavadocVariable diff report looks good.

The only concern is vague possibility on affecting other Checks, that is not common code at all, as we don't see it in diff.

…ly evaluated

romani · 2025-05-22T01:30:04Z

Successor PR #17112 (comment)

JackPGreen force-pushed the javadoc-reproducer branch from a312e2d to d962f2f Compare April 7, 2025 16:02

JackPGreen force-pushed the javadoc-reproducer branch 2 times, most recently from 9d985ee to 7d6d054 Compare April 19, 2025 15:30

romani mentioned this pull request May 3, 2025

private enums being treated as public in JavadocVariableCheck #16786

Closed

JackPGreen force-pushed the javadoc-reproducer branch from 7d6d054 to 5c5ce75 Compare May 3, 2025 22:21

JackPGreen force-pushed the javadoc-reproducer branch from 5c5ce75 to 5afb835 Compare May 3, 2025 22:36

JackPGreen force-pushed the javadoc-reproducer branch from 5afb835 to 95174e0 Compare May 3, 2025 23:06

JackPGreen force-pushed the javadoc-reproducer branch from 95174e0 to c28aff5 Compare May 4, 2025 23:25

JackPGreen force-pushed the javadoc-reproducer branch 2 times, most recently from 953eb04 to 2199c06 Compare May 5, 2025 07:40

romani requested changes May 5, 2025

View reviewed changes

JackPGreen force-pushed the javadoc-reproducer branch from 2199c06 to a019bb4 Compare May 6, 2025 22:58

JackPGreen requested a review from romani May 11, 2025 22:10

JackPGreen mentioned this pull request May 12, 2025

Issue #17061: Replace REPOSITORY_OWNER CI ENV var with built-in GITHUB_REPOSITORY_OWNER #17055

Merged

Issue checkstyle#16786: Ensure enum constants accessibility is proper…

59d2702

…ly evaluated

JackPGreen force-pushed the javadoc-reproducer branch from a019bb4 to 59d2702 Compare May 21, 2025 11:22

JackPGreen mentioned this pull request May 21, 2025

Issue #16786: Ensure enum constants accessibility is properly evaluated (addressing JavadocVariableCheck _only_) #17112

Merged

romani closed this in #17112 May 21, 2025

Uh oh!

Issue #16786: Ensure enum constants accessibility is properly evaluated #16787

Issue #16786: Ensure enum constants accessibility is properly evaluated #16787

Uh oh!

Conversation

JackPGreen commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Apr 13, 2025

Uh oh!

JackPGreen commented Apr 13, 2025

Uh oh!

romani commented Apr 19, 2025

Uh oh!

JackPGreen commented Apr 19, 2025

Uh oh!

romani commented Apr 20, 2025

Uh oh!

JackPGreen commented Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahfouz72 commented Apr 21, 2025

Uh oh!

romani commented Apr 22, 2025

Uh oh!

MohanadKh03 commented Apr 23, 2025

Uh oh!

kkoutsilis commented Apr 23, 2025

Uh oh!

james-baker-aera commented May 1, 2025

Uh oh!

romani commented May 2, 2025

Uh oh!

romani commented May 2, 2025

Uh oh!

JackPGreen commented May 2, 2025

Uh oh!

romani commented May 3, 2025

Uh oh!

JackPGreen commented May 3, 2025

Uh oh!

romani commented May 3, 2025

Uh oh!

JackPGreen commented May 3, 2025

Uh oh!

romani commented May 3, 2025

Uh oh!

romani commented May 3, 2025

Uh oh!

JackPGreen commented May 3, 2025

Uh oh!

romani commented May 3, 2025

Uh oh!

JackPGreen commented May 5, 2025

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackPGreen commented Apr 7, 2025 •

edited

Loading

JackPGreen commented Apr 20, 2025 •

edited

Loading

romani May 17, 2025 •

edited

Loading

romani May 20, 2025 •

edited

Loading