Issue #18843: Indentation Check Handlers should not have reference to… by smita1078 · Pull Request #18754 · checkstyle/checkstyle

smita1078 · 2026-01-25T09:42:40Z

Changes

Initialize incorrectIndentationLines eagerly to fix SpotBugs UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR warning
Changed to private final Set<Integer> incorrectIndentationLines = new HashSet<>()
Updated beginTree() to use clear() instead of reassignment
Remove SpotBugs suppression for this violation

vivek-0509 · 2026-01-25T14:35:02Z

+    <mutator>org.pitest.mutationtest.engine.gregor.mutators.VoidMethodCallMutator</mutator>
+    <description>removed call to java/util/Set::clear</description>
+    <lineContent>incorrectIndentationLines.clear();</lineContent>
+  </mutation>


Please do not add the pitest suppression. Instead, you can move the incorrectIndentationLines.clear() call into the clearState() method, since it serves the same purpose as the other method calls inside that method, clearState() is excluded from pitest so the mutation will not be generated and no suppression is needed.

vivek-0509 · 2026-01-25T17:59:04Z

                <param>clearState</param>
+                <param>logOncePerLine</param>
              </excludedMethods>


why do we need to add logOncePerLine in the excludedMethods, i think its not needed , what pitest mutation made you do so ??

We can not simply exclude from pitest, we need to cover by tests.

Yes removed, from previous run VoidMethodCallMutator was there, now its not coming anymore

romani · 2026-01-27T12:01:54Z

@vivek-0509 , please finish review, I will review after you.

vivek-0509 · 2026-01-29T21:32:53Z

@romani
I have reviewed the PR.
From what I see, this fixes the SpotBugs warning and removes handlers reliance on a public behavioral method by making the logging method package-private.
One point I wanted to clarify, is the intent of the issue to eliminate the class-level reference from handlers to
IndentationCheck entirely, or to ensure handlers don't depend on the check's public API contract ?
This PR keeps the reference but limits interaction to package-private scope and read-only configuration access. Just wanted to confirm that this matches the intended scope since issue feels little unclear to me.

romani · 2026-01-30T05:11:40Z

Goal is to avoid cycle dependencies between classes.
Check can be aware of all handlers and orchestrate them. Handlers should not know anything about Check and just give all data/behavior to Check.

vivek-0509 · 2026-01-30T17:48:01Z

Currently, this PR addresses the SpotBugs warning but does not remove the cyclic dependency between IndentationCheck
and handlers. To completely remove the cyclic dependency, we would need significant refactoring.

From my overview, I think we could create an immutable context record containing all the configuration details that handlers need (basicOffset, caseIndent, throwsIndent, etc.). Handlers would use this config to perform their indentation checks. Additionally, we can have a violation context where handlers log violations into a list, then return that list. The Check would then collect these violations and log them.

However, I need to think more about how to achieve this properly.
@smita1078 Please think in this direction, or if you have a better approach, please share it. We can discuss and then move forward with the refactoring.

smita1078 · 2026-01-30T19:09:37Z

Currently, this PR addresses the SpotBugs warning but does not remove the cyclic dependency between IndentationCheck and handlers. To completely remove the cyclic dependency, we would need significant refactoring.

From my overview, I think we could create an immutable context record containing all the configuration details that handlers need (basicOffset, caseIndent, throwsIndent, etc.). Handlers would use this config to perform their indentation checks. Additionally, we can have a violation context where handlers log violations into a list, then return that list. The Check would then collect these violations and log them.

However, I need to think more about how to achieve this properly. @smita1078 Please think in this direction, or if you have a better approach, please share it. We can discuss and then move forward with the refactoring.

@vivek-0509
Thanks for the detailed analysis! I agree with your approach of using an immutable context record for configuration.
For the violation handling, instead of having handlers return a list of violations, I'd think we can use a callback interface pattern.
Create a ViolationReporter interface with a single method: logOncePerLine then IndentationCheck would implement this interface. Handlers would receive this interface via constructor: Handler(IndentationConfig config, ViolationReporter reporter, ...) this would break circular dependency as handlers depend on interface, not Check

romani · 2026-02-03T01:45:39Z

Ok, sounds like this issue is epic, and will not be easy, but doable.

Let's split it in few issues and make sure we can reliably do refactoring.

For this PR, let's make special issue to describe small goal and merge this update, as step forward.

vivek-0509 · 2026-02-03T02:01:08Z

@smita1078 Please create an issue for this PR (the current SpotBugs fix) so we can merge it as a step forward. We need to decide on the approach before proceding with refactoring ,will continue the discussion at #14121.

smita1078 · 2026-02-03T18:32:43Z

@smita1078 Please create an issue for this PR (the current SpotBugs fix) so we can merge it as a step forward. We need to decide on the approach before proceding with refactoring ,will continue the discussion at #14121.

@vivek-0509 Created issue for this PR. #18843
The scope is limited to:
Fix SpotBugs UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR warning
Reduce public API surface (public → package-private for logging method)
This doesn't fully resolve #14121. Will continue the architectural refactoring discussion at #14121.

romani · 2026-02-03T18:36:35Z

@vivek-0509 , please finish review

vivek-0509 · 2026-02-04T01:22:10Z

+     * <p>For use by indentation handlers in this package.</p>
     *
-     * @see java.text.MessageFormat
+     * @param ast the AST node for which the error is logged
+     * @param key the message key describing the violation
+     * @param args optional arguments for the message
     */
-    public void indentationLog(DetailAST ast, String key, Object... args) {
-        if (!incorrectIndentationLines.contains(ast.getLineNo())) {
-            incorrectIndentationLines.add(ast.getLineNo());
+    /* package */ void logOncePerLine(DetailAST ast, String key, Object... args) {
+        if (incorrectIndentationLines.add(ast.getLineNo())) {


Please revert the visibility/rename changes to indentationLog method. This PR should only fix the SpotBugs warning for the scope of issue #18843. Refctoring changes will be done separately once we agree on the approach.

vivek-0509 · 2026-02-04T11:00:14Z

        }
    }
-


please revert any unrelated change

Done, Sry! missed it earlier

…ference to check itself

vivek-0509 · 2026-02-04T13:45:56Z

@romani PR looks good ok to merge

romani · 2026-02-04T18:20:21Z

@vivek-0509 , please finish review

vivek-0509 · 2026-02-04T19:14:45Z

@romani Already done #18754 (comment)

romani · 2026-02-05T01:02:27Z

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

github-actions · 2026-02-05T01:58:25Z

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

romani

Thanks a lot

smita1078 force-pushed the FixIndentation branch 3 times, most recently from a18a792 to d4074df Compare January 25, 2026 14:33

vivek-0509 suggested changes Jan 25, 2026

View reviewed changes

smita1078 force-pushed the FixIndentation branch 2 times, most recently from 44fb36a to 631fcca Compare January 25, 2026 15:59

vivek-0509 reviewed Jan 25, 2026

View reviewed changes

smita1078 force-pushed the FixIndentation branch from 631fcca to 09d342e Compare January 26, 2026 03:38

smita1078 force-pushed the FixIndentation branch from 09d342e to 9fe3060 Compare February 3, 2026 18:29

smita1078 changed the title ~~Issue #14121: Indentation Check Handlers should not have reference to…~~ Issue #18843: Indentation Check Handlers should not have reference to… Feb 3, 2026

vivek-0509 suggested changes Feb 4, 2026

View reviewed changes

smita1078 force-pushed the FixIndentation branch 5 times, most recently from ff698e1 to 3cfdf29 Compare February 4, 2026 06:58

vivek-0509 suggested changes Feb 4, 2026

View reviewed changes

Issue checkstyle#18843: Indentation Check Handlers should not have re…

2d99490

…ference to check itself

smita1078 force-pushed the FixIndentation branch from 3cfdf29 to 2d99490 Compare February 4, 2026 12:58

vivek-0509 approved these changes Feb 4, 2026

View reviewed changes

romani approved these changes Feb 5, 2026

View reviewed changes

romani merged commit c1f7520 into checkstyle:master Feb 5, 2026
121 checks passed

vivek-0509 mentioned this pull request Apr 18, 2026

Issue #14121: removed inner Handlers reference to Check itself #19704

Open

Uh oh!

Conversation

smita1078 commented Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

romani commented Jan 27, 2026

Uh oh!

vivek-0509 commented Jan 29, 2026

Uh oh!

romani commented Jan 30, 2026

Uh oh!

vivek-0509 commented Jan 30, 2026

Uh oh!

smita1078 commented Jan 30, 2026

Uh oh!

romani commented Feb 3, 2026

Uh oh!

vivek-0509 commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smita1078 commented Feb 3, 2026

Uh oh!

romani commented Feb 3, 2026

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!

smita1078 Feb 4, 2026 • 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!

vivek-0509 commented Feb 4, 2026

Uh oh!

romani commented Feb 4, 2026

Uh oh!

vivek-0509 commented Feb 4, 2026

Uh oh!

romani commented Feb 5, 2026

Uh oh!

github-actions Bot commented Feb 5, 2026

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smita1078 commented Jan 25, 2026 •

edited

Loading

vivek-0509 commented Feb 3, 2026 •

edited

Loading

smita1078 Feb 4, 2026 •

edited

Loading