SONARJAVA-6779 Implement new rule S9343: Methods should not increase accessibility when overriding or hiding - #5941
SONARJAVA-6779 Implement new rule S9343: Methods should not increase accessibility when overriding or hiding#5941romainbrenguier wants to merge 7 commits into
Conversation
Add rule S9343 which detects methods that increase accessibility when overriding instance methods or hiding static methods in parent classes.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
🤖 Generated with GitHub Actions
|
❌ Ruling needs updating. A fix PR has been created: #5944 Please review and merge it into your branch. |
…tests - Fix FP: skip package-private static methods in different packages (not inherited/hidden) - Fix FN: filter out interface owners when checking overridden symbols so that superclass override is detected even when interface entry comes first - Remove unnecessary cast to MethodSymbol in accessLevel call - Extract common access comparison into reportIfAccessIncreased to reduce duplication - Use switch expression for accessLevelName - Add test cases: interface+superclass override, different param types, private parent static method, multi-level static hierarchy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ruling Diff SummaryDetected changes in 5 rule files: 0 issues removed, 181 issues added. S9343 (
|
🤖 Generated with GitHub Actions
|
❌ Ruling needs updating. A fix PR has been created: #5947 Please review and merge it into your branch. |
…cleanup, and test coverage - Remove invalid `implements Runnable2` from SuperWithProtected (interface methods are implicitly public, so declaring execute2() as protected caused a compilation error) - Guard against unknown access levels in reportIfAccessIncreased to prevent false positives on unresolved symbols - Eliminate duplicate cast to Symbol.MethodSymbol by introducing a local variable in checkStaticMethodHiding - Add non-compiling test for unknown parameter types and partial semantics - Add bytecode parent test (ClassLoader.findClass) to cover the null declaration branch in reportIfAccessIncreased Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract hasSameParameterTypes into MethodTreeUtils to eliminate cross-file duplication with StaticMethodHidingCheck. Replace local getPackage/samePackage with JUtils.getPackage. Add test cases for static hiding edge cases (field name collision, instance vs static, parameter count mismatch) and interface+class hierarchy overrides. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…erage Move illegal static-hiding-instance test case to non-compiling samples to fix compilation error that broke annotation resolution across all test files (causing S9149 StaticMethodHidingCheckTest to fail with 4 false positives). Extract findHiddenStaticMethod helper into MethodTreeUtils to eliminate duplication between S9343 and S9149 static method hiding traversal logic. Add additional edge-case test scenarios for coverage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review ✅ Approved 3 resolved / 3 findingsImplements new rule S9343 to detect methods that inappropriately increase accessibility, addressing the static hiding path, instance override inspection, and unknown parent access level issues. No issues found. ✅ 3 resolved✅ Edge Case: Static hiding path ignores package visibility across packages
✅ Edge Case: Instance override only inspects overriddenSymbols.get(0)
✅ Edge Case: Unknown parent access level can trigger "from unknown" report
Implementation Status ✅ 1 / 1 issues implemented✅ SONARJAVA-6779 — 1 / 1 objectivesThe PR successfully implements the new rule S9343 to detect when methods increase accessibility during overriding or hiding, complete with check logic, tests, documentation, and ruling results. ✅ 1 complete
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
nathsou
left a comment
nathsou
left a comment
There was a problem hiding this comment.
Requesting changes: these three blockers create unavoidable false positives or publish rule assets that diverge from the S9343 RSPEC. Please address them before merge.
| } | ||
|
|
||
| private void checkInstanceMethodOverride(MethodTree methodTree, Symbol.MethodSymbol methodSymbol) { | ||
| List<Symbol.MethodSymbol> overriddenSymbols = methodSymbol.overriddenSymbols(); |
There was a problem hiding this comment.
[P1] Exclude java.lang.Object#clone() from this rule. Object.clone() is protected, while a public override is the conventional usable clone API and is the compliant style documented by S2157. This lookup selects the non-interface Object#clone() and reports a protected-to-public increase; the new ruling baseline already contains that false positive at BeanMap.java:209.
| private void checkInstanceMethodOverride(MethodTree methodTree, Symbol.MethodSymbol methodSymbol) { | ||
| List<Symbol.MethodSymbol> overriddenSymbols = methodSymbol.overriddenSymbols(); | ||
| overriddenSymbols.stream() | ||
| .filter(s -> !s.owner().isInterface()) |
There was a problem hiding this comment.
[P1] Do not discard a public superinterface before deciding whether the access increase is actionable. In the new AbstractDoer / ConcreteDoer sample, ConcreteDoer#doIt must be public to implement Doer#doIt; keeping the protected superclass visibility does not compile. Filtering interfaces here makes the rule report an impossible-to-fix issue. Compare against the most accessible overridden declaration (or skip when an interface requires the child visibility).
| "func": "Constant\/Issue", | ||
| "constantCost": "5min" | ||
| }, | ||
| "tags": [ |
There was a problem hiding this comment.
[P1] Regenerate the plugin rule assets from the S9343 RSPEC branch instead of committing hand-written copies. This directory's README declares these files generated; draft RSPEC #7894 has tags encapsulation, design, and pitfall, plus MODULAR (not CLEAR). The committed HTML also omits RSPEC's potential-impact section and other explanatory text. Generate from rule/add-RSPEC-S9343 and commit the resulting JSON/HTML/profile assets.
2 participants
Add rule S9343 which detects methods that increase accessibility when overriding instance methods or hiding static methods in parent classes.