Skip to content

Conversation

@Anmol202005
Copy link
Collaborator

Fixes: #122

@Anmol202005 Anmol202005 marked this pull request as draft October 28, 2025 23:09
@Anmol202005 Anmol202005 marked this pull request as ready for review October 29, 2025 13:25
@Anmol202005 Anmol202005 marked this pull request as draft October 29, 2025 13:26
@Anmol202005
Copy link
Collaborator Author

@romani @rdiachenko This PR currently covers most of the cases (all cases for sarif report) but there are some failing edge cases for xml report.

Issue: XML reports only mention the check ID, which creates ambiguous matching when the ID value matches another check's class name, such as:

<module name="Checker">
  <module name="TreeWalker">
    <module name="com.puppycrawl.tools.checkstyle.checks.UpperEllCheck"/>
    <module name="com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck">
      <property name="id" value="com.puppycrawl.tools.checkstyle.checks.UpperEllCheck"/>
    </module>
  </module>
</module>

No such issue with sarif since it mentions both the check name as well as the id.
for the above case the sarif report provides the source as:
com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck#com.puppycrawl.tools.checkstyle.checks.UpperEllCheck

Can we make a similar thing for xml report in the main checkstyle repo otherwise there will be uncertainty with the logic always.

@rdiachenko
Copy link
Member

@Anmol202005 please create an issue in the main checkstyle repo and include examples and expected output.

Regarding:

    <module name="com.puppycrawl.tools.checkstyle.checks.coding.FinalLocalVariableCheck">
      <property name="id" value="com.puppycrawl.tools.checkstyle.checks.UpperEllCheck"/>
    </module>

I think this is more theoretical than practical case. Users are in control of their configs, and I can't imagine the case when they want to set id pointing to a different module. This can be done by some odd refactoring or mistake.

If the above case happens, what's the result of autofix execution? Will it fail or do nothing? We should just ignore such cases, because you have violation belonging to a different module id, which means if you try to execute recipe, nothing should change, right?

@romani
Copy link
Member

romani commented Oct 30, 2025

If people wanted to shoot in their own leg by such config, ok , it is their problem.
There are a lot of places where you can do stupid stuff in our config, we will not support it.

@Anmol202005
Copy link
Collaborator Author

If the above case happens, what's the result of autofix execution? Will it fail or do nothing? We should just ignore such cases, because you have violation belonging to a different module id, which means if you try to execute recipe, nothing should change, right?

Yes nothing will change :)

@Anmol202005 Anmol202005 marked this pull request as ready for review October 30, 2025 19:36
@Anmol202005
Copy link
Collaborator Author

@romani this PR is ready to review.
but IMO we should update the violation report to mention both check as well as the id as source (If it doesn't break anything else). It will make the logic much straight forward here.

Should i create an issue for same in the main checkstyle repo ?

@romani
Copy link
Member

romani commented Oct 31, 2025

Please create , but please use CLI to show a problem, to let us see all details.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support multiple instances of the same Checkstyle check with different IDs

3 participants