From 63a598ad41ba2d84bf7067200da9ba20eec2d8a2 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Tue, 18 Aug 2026 10:43:46 +0200 Subject: [PATCH 1/7] Implement new rule S9343 Add rule S9343 which detects methods that increase accessibility when overriding instance methods or hiding static methods in parent classes. --- ...ethodOverrideAccessibilityCheckSample.java | 222 ++++++++++++++++++ .../MethodOverrideAccessibilityCheck.java | 147 ++++++++++++ .../MethodOverrideAccessibilityCheckTest.java | 43 ++++ .../org/sonar/l10n/java/rules/java/S9343.html | 59 +++++ .../org/sonar/l10n/java/rules/java/S9343.json | 24 ++ .../main/resources/profiles/Sonar_way/S9343 | 0 6 files changed, 495 insertions(+) create mode 100644 java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java create mode 100644 java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java create mode 100644 java-checks/src/test/java/org/sonar/java/checks/MethodOverrideAccessibilityCheckTest.java create mode 100644 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9343.html create mode 100644 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9343.json create mode 100644 sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9343 diff --git a/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java new file mode 100644 index 00000000000..ef3d0190da3 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java @@ -0,0 +1,222 @@ +package checks; + +class MethodOverrideAccessibilityCheckSample { + + // --- Instance override: protected -> public --- + + static class Parent1 { + protected void process() {} +// ^^^^^^^> + } + + static class ChildProtectedToPublic extends Parent1 { + @Override + public void process() {} // Noncompliant {{Increase of accessibility from "protected" to "public" when overriding method.}} +// ^^^^^^^ + } + + // --- Instance override: package-private -> public --- + + static class Parent2 { + void handle() {} +// ^^^^^^> + } + + static class ChildPackageToPublic extends Parent2 { + @Override + public void handle() {} // Noncompliant {{Increase of accessibility from "package-private" to "public" when overriding method.}} +// ^^^^^^ + } + + // --- Instance override: package-private -> protected --- + + static class Parent3 { + void doWork() {} +// ^^^^^^> + } + + static class ChildPackageToProtected extends Parent3 { + @Override + protected void doWork() {} // Noncompliant {{Increase of accessibility from "package-private" to "protected" when overriding method.}} +// ^^^^^^ + } + + // --- Static hiding: protected -> public --- + + static class Parent4 { + protected static void compute(int x) {} +// ^^^^^^^> + } + + static class ChildStaticProtectedToPublic extends Parent4 { + public static void compute(int x) {} // Noncompliant {{Increase of accessibility from "protected" to "public" when hiding method.}} +// ^^^^^^^ + } + + // --- Static hiding: package-private -> protected --- + + static class Parent5 { + static void resolve(String s) {} +// ^^^^^^^> + } + + static class ChildStaticPackageToProtected extends Parent5 { + protected static void resolve(String s) {} // Noncompliant {{Increase of accessibility from "package-private" to "protected" when hiding method.}} +// ^^^^^^^ + } + + // --- Static hiding: package-private -> public --- + + static class Parent6 { + static void transform(int y) {} +// ^^^^^^^^^> + } + + static class ChildStaticPackageToPublic extends Parent6 { + public static void transform(int y) {} // Noncompliant {{Increase of accessibility from "package-private" to "public" when hiding method.}} +// ^^^^^^^^^ + } + + // --- Abstract method implementation with increased access --- + + static abstract class AbstractParent1 { + abstract void doTask(); +// ^^^^^^> + } + + static class ConcretePackageToProtected extends AbstractParent1 { + @Override + protected void doTask() {} // Noncompliant {{Increase of accessibility from "package-private" to "protected" when overriding method.}} +// ^^^^^^ + } + + static abstract class AbstractParent2 { + abstract void perform(); +// ^^^^^^^> + } + + static class ConcretePackageToPublic extends AbstractParent2 { + @Override + public void perform() {} // Noncompliant {{Increase of accessibility from "package-private" to "public" when overriding method.}} +// ^^^^^^^ + } + + static abstract class AbstractParent3 { + protected abstract void execute(); +// ^^^^^^^> + } + + static class ConcreteProtectedToPublic extends AbstractParent3 { + @Override + public void execute() {} // Noncompliant {{Increase of accessibility from "protected" to "public" when overriding method.}} +// ^^^^^^^ + } + + // --- Multi-level hierarchy --- + + static class GrandParent { + protected void action() {} +// ^^^^^^> + } + + static class MiddleClass extends GrandParent { + @Override + protected void action() {} // Compliant - same access level + } + + static class GrandChild extends GrandParent { + @Override + public void action() {} // Noncompliant {{Increase of accessibility from "protected" to "public" when overriding method.}} +// ^^^^^^ + } + + // GrandChild via already-widened parent: compliant since direct parent is public + static class AlreadyPublicParent { + public void method() {} + } + + static class GrandChildCompliant extends AlreadyPublicParent { + @Override + public void method() {} // Compliant - same as direct parent + } + + // --- Compliant: same access level maintained --- + + static class Parent7 { + protected void keep() {} + } + + static class SameProtected extends Parent7 { + @Override + protected void keep() {} // Compliant + } + + static class Parent8 { + void maintain() {} + } + + static class SamePackagePrivate extends Parent8 { + @Override + void maintain() {} // Compliant + } + + static class Parent9 { + public void stay() {} + } + + static class SamePublic extends Parent9 { + @Override + public void stay() {} // Compliant + } + + // --- Compliant: same access level for static hiding --- + + static class Parent10 { + protected static void staticKeep(int x) {} + } + + static class SameStaticProtected extends Parent10 { + protected static void staticKeep(int x) {} // Compliant + } + + // --- Compliant: interface implementation --- + + interface Processor { + void run(); + } + + static class ProcessorImpl implements Processor { + @Override + public void run() {} // Compliant - interface methods are implicitly public + } + + // --- Compliant: constructors --- + + static class ParentWithConstructor { + protected ParentWithConstructor() {} + } + + static class ChildWithConstructor extends ParentWithConstructor { + public ChildWithConstructor() {} // Compliant - constructors don't override + } + + // --- Compliant: method not overriding anything --- + + static class Parent11 { + protected void existing() {} + } + + static class NewMethod extends Parent11 { + public void newMethod() {} // Compliant - not overriding + } + + // --- Compliant: private methods cannot be overridden --- + + static class BaseWithPrivate { + private void secret() {} + } + + static class ChildOfPrivate extends BaseWithPrivate { + public void secret() {} // Compliant - not overriding, private methods are not visible + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java new file mode 100644 index 00000000000..2b8e3e1df57 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java @@ -0,0 +1,147 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import java.util.Collections; +import java.util.List; +import org.sonar.check.Rule; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.JavaFileScannerContext; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.Tree; + +@Rule(key = "S9343") +public class MethodOverrideAccessibilityCheck extends IssuableSubscriptionVisitor { + + @Override + public List nodesToVisit() { + return Collections.singletonList(Tree.Kind.METHOD); + } + + @Override + public void visitNode(Tree tree) { + if (context.getSemanticModel() == null) { + return; + } + MethodTree methodTree = (MethodTree) tree; + Symbol.MethodSymbol methodSymbol = methodTree.symbol(); + if (methodSymbol.isStatic()) { + checkStaticMethodHiding(methodTree, methodSymbol); + } else { + checkInstanceMethodOverride(methodTree, methodSymbol); + } + } + + private void checkInstanceMethodOverride(MethodTree methodTree, Symbol.MethodSymbol methodSymbol) { + List overriddenSymbols = methodSymbol.overriddenSymbols(); + if (overriddenSymbols.isEmpty()) { + return; + } + Symbol.MethodSymbol overriddenSymbol = overriddenSymbols.get(0); + if (overriddenSymbol.owner().isInterface()) { + return; + } + int childLevel = accessLevel(methodSymbol); + int parentLevel = accessLevel(overriddenSymbol); + if (childLevel > parentLevel) { + reportAccessibilityIssue(methodTree, overriddenSymbol, parentLevel, childLevel, "overriding"); + } + } + + private void checkStaticMethodHiding(MethodTree methodTree, Symbol.MethodSymbol methodSymbol) { + Symbol.TypeSymbol owner = (Symbol.TypeSymbol) methodSymbol.owner(); + Type superClass = owner.superClass(); + while (superClass != null) { + for (Symbol symbol : superClass.symbol().lookupSymbols(methodSymbol.name())) { + if (symbol.isMethodSymbol() && symbol.isStatic() && !symbol.isPrivate() + && hasSameParameterTypes(methodSymbol, (Symbol.MethodSymbol) symbol)) { + int childLevel = accessLevel(methodSymbol); + int parentLevel = accessLevel((Symbol.MethodSymbol) symbol); + if (childLevel > parentLevel) { + reportAccessibilityIssue(methodTree, (Symbol.MethodSymbol) symbol, parentLevel, childLevel, "hiding"); + } + return; + } + } + superClass = superClass.symbol().superClass(); + } + } + + private void reportAccessibilityIssue(MethodTree methodTree, Symbol.MethodSymbol parentMethod, + int parentLevel, int childLevel, String verb) { + String message = String.format("Increase of accessibility from \"%s\" to \"%s\" when %s method.", + accessLevelName(parentLevel), accessLevelName(childLevel), verb); + MethodTree declaration = parentMethod.declaration(); + if (declaration != null) { + reportIssue(methodTree.simpleName(), message, + Collections.singletonList(new JavaFileScannerContext.Location("Parent method", declaration.simpleName())), + null); + } else { + reportIssue(methodTree.simpleName(), message); + } + } + + private static int accessLevel(Symbol symbol) { + if (symbol.isPrivate()) { + return 0; + } else if (symbol.isPackageVisibility()) { + return 1; + } else if (symbol.isProtected()) { + return 2; + } else if (symbol.isPublic()) { + return 3; + } + return -1; + } + + private static String accessLevelName(int level) { + switch (level) { + case 0: + return "private"; + case 1: + return "package-private"; + case 2: + return "protected"; + case 3: + return "public"; + default: + return "unknown"; + } + } + + private static boolean hasSameParameterTypes(Symbol.MethodSymbol method, Symbol.MethodSymbol candidate) { + List methodParams = method.parameterTypes(); + List candidateParams = candidate.parameterTypes(); + if (methodParams.size() != candidateParams.size()) { + return false; + } + for (int i = 0; i < methodParams.size(); i++) { + Type methodParam = methodParams.get(i); + Type candidateParam = candidateParams.get(i); + if (methodParam.isUnknown() || candidateParam.isUnknown()) { + return false; + } + if (!methodParam.erasure().equals(candidateParam.erasure())) { + return false; + } + } + return true; + } + +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/MethodOverrideAccessibilityCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/MethodOverrideAccessibilityCheckTest.java new file mode 100644 index 00000000000..18beac40041 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/MethodOverrideAccessibilityCheckTest.java @@ -0,0 +1,43 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class MethodOverrideAccessibilityCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/MethodOverrideAccessibilityCheckSample.java")) + .withCheck(new MethodOverrideAccessibilityCheck()) + .verifyIssues(); + } + + @Test + void withoutSemantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/MethodOverrideAccessibilityCheckSample.java")) + .withCheck(new MethodOverrideAccessibilityCheck()) + .withoutSemantic() + .verifyNoIssues(); + } + +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9343.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9343.html new file mode 100644 index 00000000000..4761e132f60 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9343.html @@ -0,0 +1,59 @@ +

This rule raises an issue when a method in a subclass has broader accessibility than the method it overrides in the parent class.

+

Why is this an issue?

+

In object-oriented languages, access modifiers control the visibility of methods, typically ranging from most restrictive (accessible only within the +defining class) through intermediate levels (accessible within packages or to subclasses) to most accessible (publicly available to all code).

+

When a subclass overrides or redefines a method from a parent class, it should not make the method more accessible than it was in the parent class. +Doing so breaks the encapsulation principle and can lead to several problems:

+
    +
  • Exposes internal implementation: A method that was intentionally kept at a restricted visibility level in the parent class may + have been designed for internal use only. Making it more broadly accessible in a subclass exposes internal details that clients shouldn't depend + on.
    • +
  • Violates the Liskov Substitution Principle: This principle states that objects of a subclass should be substitutable for objects + of the parent class without breaking the application. When you increase accessibility, you're adding new capabilities to the subclass's public API + that weren't part of the parent class's contract.
    • +
  • Creates maintenance issues: If the parent class's method accessibility was intentionally limited, increasing it in a subclass + makes it harder to maintain and evolve the class hierarchy.
    • +
+

In Java, this applies both to instance methods (which are overridden) and static methods (which are hidden).

+

How to fix it

+

Keep the same access modifier as the parent class method. If the method needs to be more accessible, reconsider whether the parent class design +should be changed instead.

+

Code examples

+

Noncompliant code example

+
+class Parent {
+    protected void process() {
+        // Internal processing logic
+    }
+}
+
+class Child extends Parent {
+    @Override
+    public void process() { // Noncompliant - increased from protected to public
+        super.process();
+    }
+}
+
+

Compliant solution

+
+class Parent {
+    protected void process() {
+        // Internal processing logic
+    }
+}
+
+class Child extends Parent {
+    @Override
+    protected void process() { // Compliant - same access level
+        super.process();
+    }
+}
+
+

Resources

+

Documentation

+ diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9343.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9343.json new file mode 100644 index 00000000000..8b947703b87 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9343.json @@ -0,0 +1,24 @@ +{ + "title": "Methods should not increase accessibility when overriding or hiding", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "design", + "inheritance" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-9343", + "sqKey": "S9343", + "scope": "All", + "quickfix": "unknown", + "code": { + "impacts": { + "MAINTAINABILITY": "MEDIUM" + }, + "attribute": "CLEAR" + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9343 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9343 new file mode 100644 index 00000000000..e69de29bb2d From c32e89948627871708a361537a3a7ed58f768b98 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 18 Aug 2026 09:32:27 +0000 Subject: [PATCH 2/7] Update ruling results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🤖 Generated with GitHub Actions --- .../commons-beanutils/java-S9343.json | 157 ++++++++++++++++++ .../resources/eclipse-jetty/java-S9343.json | 81 +++++++++ .../src/test/resources/guava/java-S9343.json | 77 +++++++++ .../resources/sonar-server/java-S9343.json | 21 +++ 4 files changed, 336 insertions(+) create mode 100644 its/ruling/src/test/resources/commons-beanutils/java-S9343.json create mode 100644 its/ruling/src/test/resources/eclipse-jetty/java-S9343.json create mode 100644 its/ruling/src/test/resources/guava/java-S9343.json create mode 100644 its/ruling/src/test/resources/sonar-server/java-S9343.json diff --git a/its/ruling/src/test/resources/commons-beanutils/java-S9343.json b/its/ruling/src/test/resources/commons-beanutils/java-S9343.json new file mode 100644 index 00000000000..2410b1a10a6 --- /dev/null +++ b/its/ruling/src/test/resources/commons-beanutils/java-S9343.json @@ -0,0 +1,157 @@ +{ +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/BeanMap.java": [ +209 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/BasicDynaBeanTestCase.java": [ +102, +161 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/BeanComparatorTestCase.java": [ +63, +83 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/BeanUtilsBenchCase.java": [ +84, +159 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/BeanUtilsTestCase.java": [ +128, +168 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/BeanificationTestCase.java": [ +68, +87 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/ConstructorUtilsTestCase.java": [ +55, +71 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/ConvertUtilsTestCase.java": [ +67, +86 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/DynaBeanMapDecoratorTestCase.java": [ +91, +113 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/DynaBeanUtilsTestCase.java": [ +106, +176 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/DynaPropertyUtilsTestCase.java": [ +104, +173 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/DynaResultSetTestCase.java": [ +83, +104 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/DynaRowSetTestCase.java": [ +86, +107 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/LazyDynaBeanTestCase.java": [ +77, +87 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/LazyDynaClassTestCase.java": [ +56, +71 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/LazyDynaListTestCase.java": [ +82, +89 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/LazyDynaMapTestCase.java": [ +78, +87 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/MappedPropertyTestCase.java": [ +53, +67 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/MethodUtilsTestCase.java": [ +56, +71 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/PropertyUtilsBenchCase.java": [ +82, +148 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/PropertyUtilsTestCase.java": [ +204, +239 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/ArrayConverterTestCase.java": [ +52, +57 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/BigDecimalConverterTestCase.java": [ +45, +58 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/BigIntegerConverterTestCase.java": [ +44, +57 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/ByteConverterTestCase.java": [ +43, +56 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/CharacterConverterTestCase.java": [ +51, +56 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/ClassConverterTestCase.java": [ +51, +56 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/DateConverterTestCase.java": [ +50, +55 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/DoubleConverterTestCase.java": [ +43, +56 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/FileConverterTestCase.java": [ +47, +56 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/FloatConverterTestCase.java": [ +43, +56 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/IntegerConverterTestCase.java": [ +44, +57 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/LongConverterTestCase.java": [ +43, +56 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/ShortConverterTestCase.java": [ +43, +56 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/converters/URLConverterTestCase.java": [ +47, +56 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/locale/LocaleBeanUtilsTestCase.java": [ +52, +69 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/locale/LocaleBeanificationTestCase.java": [ +77, +96 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/locale/LocaleConvertUtilsTestCase.java": [ +68, +94 +], +"commons-beanutils:commons-beanutils:src/test/java/org/apache/commons/beanutils2/locale/converters/BaseLocaleConverterTestCase.java": [ +77, +120 +] +} diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S9343.json b/its/ruling/src/test/resources/eclipse-jetty/java-S9343.json new file mode 100644 index 00000000000..6b7a3625deb --- /dev/null +++ b/its/ruling/src/test/resources/eclipse-jetty/java-S9343.json @@ -0,0 +1,81 @@ +{ +"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/ByteArrayEndPoint.java": [ +127, +137, +448 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/SocketChannelEndPoint.java": [ +200 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java": [ +384, +1240, +1347 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/test/java/org/eclipse/jetty/io/SocketChannelEndPointTest.java": [ +711 +], +"org.eclipse.jetty:jetty-project:jetty-jmx/src/main/java/org/eclipse/jetty/jmx/ConnectorServer.java": [ +115, +164 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java": [ +757, +921 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java": [ +1516 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/ServerConnector.java": [ +380 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java": [ +98 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpInputInterceptor.java": [ +82 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionCache.java": [ +91, +102, +192 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionCache.java": [ +66, +73, +80 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/HttpInputAsyncStateTest.java": [ +138 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/ProxyConnectionTest.java": [ +376, +395 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/ClassLoadingObjectInputStream.java": [ +73 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/Scanner.java": [ +545, +611 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/Utf8StringBuffer.java": [ +56 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/Utf8StringBuilder.java": [ +56 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/compression/CompressionPool.java": [ +118 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/security/Constraint.java": [ +106 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/thread/ReservedThreadExecutor.java": [ +155, +163 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/test/java/org/eclipse/jetty/util/IteratingCallbackTest.java": [ +319 +] +} diff --git a/its/ruling/src/test/resources/guava/java-S9343.json b/its/ruling/src/test/resources/guava/java-S9343.json new file mode 100644 index 00000000000..ff009fa1532 --- /dev/null +++ b/its/ruling/src/test/resources/guava/java-S9343.json @@ -0,0 +1,77 @@ +{ +"com.google.guava:guava:src/com/google/common/base/Splitter.java": [ +184, +200, +236, +241, +299, +305 +], +"com.google.guava:guava:src/com/google/common/collect/AbstractMapBasedMultimap.java": [ +1270 +], +"com.google.guava:guava:src/com/google/common/collect/ConcurrentHashMultiset.java": [ +491 +], +"com.google.guava:guava:src/com/google/common/collect/ConsumingQueueIterator.java": [ +43 +], +"com.google.guava:guava:src/com/google/common/collect/ForwardingNavigableMap.java": [ +284 +], +"com.google.guava:guava:src/com/google/common/collect/Maps.java": [ +776, +823, +2724, +2756 +], +"com.google.guava:guava:src/com/google/common/collect/Multimaps.java": [ +128, +210, +288, +368, +599, +620, +1687 +], +"com.google.guava:guava:src/com/google/common/collect/Sets.java": [ +723 +], +"com.google.guava:guava:src/com/google/common/collect/SortedLists.java": [ +115, +126, +156, +174 +], +"com.google.guava:guava:src/com/google/common/collect/StandardTable.java": [ +714, +779 +], +"com.google.guava:guava:src/com/google/common/hash/Crc32cHashFunction.java": [ +112 +], +"com.google.guava:guava:src/com/google/common/hash/Murmur3_128HashFunction.java": [ +163 +], +"com.google.guava:guava:src/com/google/common/hash/Murmur3_32HashFunction.java": [ +184 +], +"com.google.guava:guava:src/com/google/common/hash/SipHashFunction.java": [ +146 +], +"com.google.guava:guava:src/com/google/common/reflect/TypeResolver.java": [ +241 +], +"com.google.guava:guava:src/com/google/common/util/concurrent/AbstractScheduledService.java": [ +128, +149 +], +"com.google.guava:guava:src/com/google/common/util/concurrent/Futures.java": [ +2085 +], +"com.google.guava:guava:src/com/google/common/util/concurrent/SettableFuture.java": [ +48, +52, +58 +] +} diff --git a/its/ruling/src/test/resources/sonar-server/java-S9343.json b/its/ruling/src/test/resources/sonar-server/java-S9343.json new file mode 100644 index 00000000000..129656c98e6 --- /dev/null +++ b/its/ruling/src/test/resources/sonar-server/java-S9343.json @@ -0,0 +1,21 @@ +{ +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/computation/task/projectanalysis/formula/coverage/LinesAndConditionsWithUncoveredVariationCounter.java": [ +34 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/platform/Platform.java": [ +115 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/platform/platformlevel/PlatformLevel1.java": [ +73 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/plugins/StaticResourcesServlet.java": [ +50 +], +"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/ws/ServletRequest.java": [ +91 +], +"org.sonarsource.sonarqube:sonar-server:src/test/java/org/sonar/server/platform/ServerTesterPlatform.java": [ +33, +41 +] +} From bb5b065628581f25576d602331679c23b07c9caa Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Tue, 18 Aug 2026 11:54:55 +0200 Subject: [PATCH 3/7] Fix S9343 bugs, remove unnecessary cast, reduce duplication, and add 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 --- ...ethodOverrideAccessibilityCheckSample.java | 53 ++++++++++++ .../MethodOverrideAccessibilityCheck.java | 83 ++++++++++--------- 2 files changed, 96 insertions(+), 40 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java index ef3d0190da3..a070be036e5 100644 --- a/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java @@ -219,4 +219,57 @@ private void secret() {} static class ChildOfPrivate extends BaseWithPrivate { public void secret() {} // Compliant - not overriding, private methods are not visible } + + // --- Interface + superclass override: should still detect when interface comes first --- + + interface Runnable2 { + void execute2(); + } + + static class SuperWithProtected implements Runnable2 { + protected void execute2() {} +// ^^^^^^^^> + } + + static class ChildImplementsAndOverrides extends SuperWithProtected { + @Override + public void execute2() {} // Noncompliant {{Increase of accessibility from "protected" to "public" when overriding method.}} +// ^^^^^^^^ + } + + // --- Compliant: static method with different parameter types (no hiding match) --- + + static class ParentStaticDiffParams { + static void process(int x) {} + } + + static class ChildStaticDiffParams extends ParentStaticDiffParams { + public static void process(String x) {} // Compliant - different parameter types, not hiding + } + + // --- Compliant: static method with private parent (not inherited) --- + + static class ParentStaticPrivate { + private static void hidden() {} + } + + static class ChildStaticPrivate extends ParentStaticPrivate { + public static void hidden() {} // Compliant - private parent method not inherited + } + + // --- Static hiding: protected -> public in multi-level hierarchy --- + + static class StaticGrandParent { + protected static void calc(int x) {} +// ^^^^> + } + + static class StaticMiddle extends StaticGrandParent { + protected static void calc(int x) {} // Compliant - same access + } + + static class StaticGrandChild extends StaticGrandParent { + public static void calc(int x) {} // Noncompliant {{Increase of accessibility from "protected" to "public" when hiding method.}} +// ^^^^ + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java index 2b8e3e1df57..cd1bb7769c8 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java @@ -50,18 +50,10 @@ public void visitNode(Tree tree) { private void checkInstanceMethodOverride(MethodTree methodTree, Symbol.MethodSymbol methodSymbol) { List overriddenSymbols = methodSymbol.overriddenSymbols(); - if (overriddenSymbols.isEmpty()) { - return; - } - Symbol.MethodSymbol overriddenSymbol = overriddenSymbols.get(0); - if (overriddenSymbol.owner().isInterface()) { - return; - } - int childLevel = accessLevel(methodSymbol); - int parentLevel = accessLevel(overriddenSymbol); - if (childLevel > parentLevel) { - reportAccessibilityIssue(methodTree, overriddenSymbol, parentLevel, childLevel, "overriding"); - } + overriddenSymbols.stream() + .filter(s -> !s.owner().isInterface()) + .findFirst() + .ifPresent(overriddenSymbol -> reportIfAccessIncreased(methodTree, methodSymbol, overriddenSymbol, "overriding")); } private void checkStaticMethodHiding(MethodTree methodTree, Symbol.MethodSymbol methodSymbol) { @@ -70,12 +62,9 @@ private void checkStaticMethodHiding(MethodTree methodTree, Symbol.MethodSymbol while (superClass != null) { for (Symbol symbol : superClass.symbol().lookupSymbols(methodSymbol.name())) { if (symbol.isMethodSymbol() && symbol.isStatic() && !symbol.isPrivate() + && !(symbol.isPackageVisibility() && !samePackage(methodSymbol, symbol)) && hasSameParameterTypes(methodSymbol, (Symbol.MethodSymbol) symbol)) { - int childLevel = accessLevel(methodSymbol); - int parentLevel = accessLevel((Symbol.MethodSymbol) symbol); - if (childLevel > parentLevel) { - reportAccessibilityIssue(methodTree, (Symbol.MethodSymbol) symbol, parentLevel, childLevel, "hiding"); - } + reportIfAccessIncreased(methodTree, methodSymbol, (Symbol.MethodSymbol) symbol, "hiding"); return; } } @@ -83,8 +72,32 @@ && hasSameParameterTypes(methodSymbol, (Symbol.MethodSymbol) symbol)) { } } - private void reportAccessibilityIssue(MethodTree methodTree, Symbol.MethodSymbol parentMethod, - int parentLevel, int childLevel, String verb) { + private static boolean samePackage(Symbol s1, Symbol s2) { + Symbol p1 = getPackage(s1); + Symbol p2 = getPackage(s2); + if (p1 == null || p2 == null) { + return false; + } + return p1.equals(p2); + } + + private static Symbol getPackage(Symbol symbol) { + Symbol owner = symbol.owner(); + while (owner != null) { + if (owner.isPackageSymbol()) { + return owner; + } + owner = owner.owner(); + } + return null; + } + + private void reportIfAccessIncreased(MethodTree methodTree, Symbol childMethod, Symbol.MethodSymbol parentMethod, String verb) { + int childLevel = accessLevel(childMethod); + int parentLevel = accessLevel(parentMethod); + if (childLevel <= parentLevel) { + return; + } String message = String.format("Increase of accessibility from \"%s\" to \"%s\" when %s method.", accessLevelName(parentLevel), accessLevelName(childLevel), verb); MethodTree declaration = parentMethod.declaration(); @@ -98,31 +111,21 @@ private void reportAccessibilityIssue(MethodTree methodTree, Symbol.MethodSymbol } private static int accessLevel(Symbol symbol) { - if (symbol.isPrivate()) { - return 0; - } else if (symbol.isPackageVisibility()) { - return 1; - } else if (symbol.isProtected()) { - return 2; - } else if (symbol.isPublic()) { - return 3; - } + if (symbol.isPrivate()) return 0; + if (symbol.isPackageVisibility()) return 1; + if (symbol.isProtected()) return 2; + if (symbol.isPublic()) return 3; return -1; } private static String accessLevelName(int level) { - switch (level) { - case 0: - return "private"; - case 1: - return "package-private"; - case 2: - return "protected"; - case 3: - return "public"; - default: - return "unknown"; - } + return switch (level) { + case 0 -> "private"; + case 1 -> "package-private"; + case 2 -> "protected"; + case 3 -> "public"; + default -> "unknown"; + }; } private static boolean hasSameParameterTypes(Symbol.MethodSymbol method, Symbol.MethodSymbol candidate) { From 0a8d46851ecdb42887428a16c5064aefae0caded Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 18 Aug 2026 10:11:45 +0000 Subject: [PATCH 4/7] Update ruling results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🤖 Generated with GitHub Actions --- .../java-S9343.json | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S9343.json diff --git a/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S9343.json b/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S9343.json new file mode 100644 index 00000000000..53df8f25d93 --- /dev/null +++ b/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S9343.json @@ -0,0 +1,51 @@ +{ +"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/ByteArrayEndPoint.java": [ +127, +137, +448 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/SocketChannelEndPoint.java": [ +200 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java": [ +384, +1240, +1347 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/test/java/org/eclipse/jetty/io/SocketChannelEndPointTest.java": [ +711 +], +"org.eclipse.jetty:jetty-project:jetty-jmx/src/main/java/org/eclipse/jetty/jmx/ConnectorServer.java": [ +115, +164 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java": [ +757, +921 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java": [ +1516 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/ServerConnector.java": [ +380 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/handler/ResourceHandler.java": [ +98 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipHttpInputInterceptor.java": [ +82 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionCache.java": [ +91, +102, +192 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionCache.java": [ +66, +73, +80 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/HttpInputAsyncStateTest.java": [ +138 +] +} From b29af8719ae94617486ada0ccea5ba6365ed836b Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Tue, 18 Aug 2026 13:28:42 +0200 Subject: [PATCH 5/7] Fix S9343 CI failures: compilation error, unknown access guard, cast 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 --- ...ethodOverrideAccessibilityCheckSample.java | 26 ++++++++++++++++++ ...ethodOverrideAccessibilityCheckSample.java | 27 ++++++++++++++----- .../MethodOverrideAccessibilityCheck.java | 12 +++++---- .../MethodOverrideAccessibilityCheckTest.java | 9 +++++++ 4 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java new file mode 100644 index 00000000000..fb8148837ae --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java @@ -0,0 +1,26 @@ +package checks; + +class MethodOverrideAccessibilityCheckSample { + + // --- Compliant: unknown parameter types should not trigger hiding detection --- + + static class ParentWithUnknownParam { + static void process(Unknown param) {} + } + + static class ChildWithUnknownParam extends ParentWithUnknownParam { + public static void process(Unknown param) {} // Compliant - parameter type is unknown + } + + // --- Noncompliant: hiding still detected when class has partial semantics --- + + static class ParentWithKnownStatic { + protected static void compute(int x) {} + } + + static class ChildHidingWithUnknownField extends ParentWithKnownStatic { + Unknown field; + public static void compute(int x) {} // Noncompliant {{Increase of accessibility from "protected" to "public" when hiding method.}} + } + +} diff --git a/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java index a070be036e5..41911068d05 100644 --- a/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java @@ -220,13 +220,9 @@ static class ChildOfPrivate extends BaseWithPrivate { public void secret() {} // Compliant - not overriding, private methods are not visible } - // --- Interface + superclass override: should still detect when interface comes first --- + // --- Superclass override with protected -> public --- - interface Runnable2 { - void execute2(); - } - - static class SuperWithProtected implements Runnable2 { + static class SuperWithProtected { protected void execute2() {} // ^^^^^^^^> } @@ -272,4 +268,23 @@ static class StaticGrandChild extends StaticGrandParent { public static void calc(int x) {} // Noncompliant {{Increase of accessibility from "protected" to "public" when hiding method.}} // ^^^^ } + + // --- Noncompliant: overriding bytecode parent (no source declaration available) --- + + static class CustomClassLoader extends ClassLoader { + @Override + public Class findClass(String name) throws ClassNotFoundException { // Noncompliant {{Increase of accessibility from "protected" to "public" when overriding method.}} +// ^^^^^^^^^ + return super.findClass(name); + } + } + + // --- Compliant: overriding bytecode parent with same access --- + + static class CompliantClassLoader extends ClassLoader { + @Override + protected Class findClass(String name) throws ClassNotFoundException { // Compliant - same access + return super.findClass(name); + } + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java index cd1bb7769c8..665965dfcb6 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java @@ -62,10 +62,12 @@ private void checkStaticMethodHiding(MethodTree methodTree, Symbol.MethodSymbol while (superClass != null) { for (Symbol symbol : superClass.symbol().lookupSymbols(methodSymbol.name())) { if (symbol.isMethodSymbol() && symbol.isStatic() && !symbol.isPrivate() - && !(symbol.isPackageVisibility() && !samePackage(methodSymbol, symbol)) - && hasSameParameterTypes(methodSymbol, (Symbol.MethodSymbol) symbol)) { - reportIfAccessIncreased(methodTree, methodSymbol, (Symbol.MethodSymbol) symbol, "hiding"); - return; + && !(symbol.isPackageVisibility() && !samePackage(methodSymbol, symbol))) { + Symbol.MethodSymbol candidate = (Symbol.MethodSymbol) symbol; + if (hasSameParameterTypes(methodSymbol, candidate)) { + reportIfAccessIncreased(methodTree, methodSymbol, candidate, "hiding"); + return; + } } } superClass = superClass.symbol().superClass(); @@ -95,7 +97,7 @@ private static Symbol getPackage(Symbol symbol) { private void reportIfAccessIncreased(MethodTree methodTree, Symbol childMethod, Symbol.MethodSymbol parentMethod, String verb) { int childLevel = accessLevel(childMethod); int parentLevel = accessLevel(parentMethod); - if (childLevel <= parentLevel) { + if (parentLevel < 0 || childLevel < 0 || childLevel <= parentLevel) { return; } String message = String.format("Increase of accessibility from \"%s\" to \"%s\" when %s method.", diff --git a/java-checks/src/test/java/org/sonar/java/checks/MethodOverrideAccessibilityCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/MethodOverrideAccessibilityCheckTest.java index 18beac40041..fabe4d23b58 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/MethodOverrideAccessibilityCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/MethodOverrideAccessibilityCheckTest.java @@ -20,6 +20,7 @@ import org.sonar.java.checks.verifier.CheckVerifier; import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; class MethodOverrideAccessibilityCheckTest { @@ -40,4 +41,12 @@ void withoutSemantic() { .verifyNoIssues(); } + @Test + void test_non_compiling() { + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath("checks/MethodOverrideAccessibilityCheckSample.java")) + .withCheck(new MethodOverrideAccessibilityCheck()) + .verifyIssues(); + } + } From a737ceb6251339593e706f75973351bab1936155 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Tue, 18 Aug 2026 14:11:56 +0200 Subject: [PATCH 6/7] Reduce S9343 duplication, improve coverage, and add test cases 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 --- ...ethodOverrideAccessibilityCheckSample.java | 13 +++++ ...ethodOverrideAccessibilityCheckSample.java | 47 +++++++++++++++++++ .../MethodOverrideAccessibilityCheck.java | 41 ++-------------- .../java/checks/StaticMethodHidingCheck.java | 22 +-------- .../java/checks/helpers/MethodTreeUtils.java | 20 ++++++++ 5 files changed, 86 insertions(+), 57 deletions(-) diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java index fb8148837ae..4c8413e5a13 100644 --- a/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java @@ -23,4 +23,17 @@ static class ChildHidingWithUnknownField extends ParentWithKnownStatic { public static void compute(int x) {} // Noncompliant {{Increase of accessibility from "protected" to "public" when hiding method.}} } + // --- Compliant: override from unknown parent type --- + + static class ChildOfUnknownParent extends UnknownParent { + @Override + public void doSomething() {} // Compliant - parent is unknown, no overridden symbols resolved + } + + // --- Compliant: static method in class extending unknown parent --- + + static class StaticChildOfUnknownParent extends UnknownParent { + public static void staticMethod() {} // Compliant - unknown superclass, no hiding detected + } + } diff --git a/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java index 41911068d05..760e4018f06 100644 --- a/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java @@ -287,4 +287,51 @@ protected Class findClass(String name) throws ClassNotFoundException { // Com return super.findClass(name); } } + + // --- Compliant: static method with same name as a field in superclass --- + + static class ParentWithField { + protected int compute; + } + + static class ChildStaticSameAsField extends ParentWithField { + public static void compute() {} // Compliant - parent has a field, not a method + } + + // --- Compliant: static method with same name as instance method in superclass --- + + static class ParentWithInstanceMethod { + protected void process() {} + } + + static class ChildStaticSameAsInstance extends ParentWithInstanceMethod { + public static void process() {} // Compliant - parent method is not static + } + + // --- Compliant: static method with no-arg hiding parent with params --- + + static class ParentStaticWithParams { + protected static void action(int x, String y) {} + } + + static class ChildStaticNoParams extends ParentStaticWithParams { + public static void action() {} // Compliant - different parameter count + } + + // --- Instance override: protected -> public through interface + class hierarchy --- + + interface Doer { + void doIt(); + } + + static class AbstractDoer { + protected void doIt() {} +// ^^^^> + } + + static class ConcreteDoer extends AbstractDoer implements Doer { + @Override + public void doIt() {} // Noncompliant {{Increase of accessibility from "protected" to "public" when overriding method.}} +// ^^^^ + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java index 665965dfcb6..b891f299a1f 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java @@ -19,6 +19,8 @@ import java.util.Collections; import java.util.List; import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.MethodTreeUtils; +import org.sonar.java.model.JUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.Symbol; @@ -64,7 +66,7 @@ private void checkStaticMethodHiding(MethodTree methodTree, Symbol.MethodSymbol if (symbol.isMethodSymbol() && symbol.isStatic() && !symbol.isPrivate() && !(symbol.isPackageVisibility() && !samePackage(methodSymbol, symbol))) { Symbol.MethodSymbol candidate = (Symbol.MethodSymbol) symbol; - if (hasSameParameterTypes(methodSymbol, candidate)) { + if (MethodTreeUtils.hasSameParameterTypes(methodSymbol, candidate)) { reportIfAccessIncreased(methodTree, methodSymbol, candidate, "hiding"); return; } @@ -75,23 +77,7 @@ private void checkStaticMethodHiding(MethodTree methodTree, Symbol.MethodSymbol } private static boolean samePackage(Symbol s1, Symbol s2) { - Symbol p1 = getPackage(s1); - Symbol p2 = getPackage(s2); - if (p1 == null || p2 == null) { - return false; - } - return p1.equals(p2); - } - - private static Symbol getPackage(Symbol symbol) { - Symbol owner = symbol.owner(); - while (owner != null) { - if (owner.isPackageSymbol()) { - return owner; - } - owner = owner.owner(); - } - return null; + return JUtils.getPackage(s1).equals(JUtils.getPackage(s2)); } private void reportIfAccessIncreased(MethodTree methodTree, Symbol childMethod, Symbol.MethodSymbol parentMethod, String verb) { @@ -130,23 +116,4 @@ private static String accessLevelName(int level) { }; } - private static boolean hasSameParameterTypes(Symbol.MethodSymbol method, Symbol.MethodSymbol candidate) { - List methodParams = method.parameterTypes(); - List candidateParams = candidate.parameterTypes(); - if (methodParams.size() != candidateParams.size()) { - return false; - } - for (int i = 0; i < methodParams.size(); i++) { - Type methodParam = methodParams.get(i); - Type candidateParam = candidateParams.get(i); - if (methodParam.isUnknown() || candidateParam.isUnknown()) { - return false; - } - if (!methodParam.erasure().equals(candidateParam.erasure())) { - return false; - } - } - return true; - } - } diff --git a/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java b/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java index 858900a4ce6..4b704bf3e30 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java @@ -19,6 +19,7 @@ import java.util.Collections; import java.util.List; import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.MethodTreeUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.Symbol; @@ -57,7 +58,7 @@ public void visitNode(Tree tree) { private boolean checkHiding(MethodTree methodTree, Symbol.MethodSymbol methodSymbol, Type superClass) { for (Symbol symbol : superClass.symbol().lookupSymbols(methodSymbol.name())) { if (symbol.isMethodSymbol() && symbol.isStatic() && !symbol.isPrivate() - && hasSameParameterTypes(methodSymbol, (Symbol.MethodSymbol) symbol)) { + && MethodTreeUtils.hasSameParameterTypes(methodSymbol, (Symbol.MethodSymbol) symbol)) { reportHidingIssue(methodTree, methodSymbol, (Symbol.MethodSymbol) symbol); return true; } @@ -88,23 +89,4 @@ private static boolean isIntentionalHiding(Symbol.MethodSymbol methodSymbol) { || methodSymbol.metadata().isAnnotatedWith("org.jetbrains.annotations.ApiStatus$ScheduledForRemoval"); } - private static boolean hasSameParameterTypes(Symbol.MethodSymbol method, Symbol.MethodSymbol candidate) { - List methodParams = method.parameterTypes(); - List candidateParams = candidate.parameterTypes(); - if (methodParams.size() != candidateParams.size()) { - return false; - } - for (int i = 0; i < methodParams.size(); i++) { - Type methodParam = methodParams.get(i); - Type candidateParam = candidateParams.get(i); - if (methodParam.isUnknown() || candidateParam.isUnknown()) { - return false; - } - if (!methodParam.erasure().equals(candidateParam.erasure())) { - return false; - } - } - return true; - } - } diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java index 2837674eadb..2f4f6991414 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java @@ -27,6 +27,7 @@ import org.sonar.plugins.java.api.JavaVersion; import org.sonar.plugins.java.api.semantic.MethodMatchers; import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.Arguments; import org.sonar.plugins.java.api.tree.ArrayTypeTree; import org.sonar.plugins.java.api.tree.BaseTreeVisitor; @@ -259,6 +260,25 @@ public void visitLambdaExpression(LambdaExpressionTree lambdaExpressionTree) { } } + public static boolean hasSameParameterTypes(Symbol.MethodSymbol method, Symbol.MethodSymbol candidate) { + List methodParams = method.parameterTypes(); + List candidateParams = candidate.parameterTypes(); + if (methodParams.size() != candidateParams.size()) { + return false; + } + for (int i = 0; i < methodParams.size(); i++) { + Type methodParam = methodParams.get(i); + Type candidateParam = candidateParams.get(i); + if (methodParam.isUnknown() || candidateParam.isUnknown()) { + return false; + } + if (!methodParam.erasure().equals(candidateParam.erasure())) { + return false; + } + } + return true; + } + public static Optional isGetterLike(Symbol.MethodSymbol methodSymbol) { if (!methodSymbol.parameterTypes().isEmpty() || isPrivateStaticOrAbstract(methodSymbol)) { return Optional.empty(); From 0c5c2244dfad89efb3339365cbdde168ce53329f Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Tue, 18 Aug 2026 15:30:41 +0200 Subject: [PATCH 7/7] Fix S9343 compilation error, reduce duplication, and improve test coverage 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 --- ...ethodOverrideAccessibilityCheckSample.java | 10 ++++ ...ethodOverrideAccessibilityCheckSample.java | 54 +++++++++++++++---- .../MethodOverrideAccessibilityCheck.java | 19 ++----- .../java/checks/StaticMethodHidingCheck.java | 22 +------- .../java/checks/helpers/MethodTreeUtils.java | 17 ++++++ 5 files changed, 76 insertions(+), 46 deletions(-) diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java index 4c8413e5a13..3dbd0d1503b 100644 --- a/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/MethodOverrideAccessibilityCheckSample.java @@ -30,6 +30,16 @@ static class ChildOfUnknownParent extends UnknownParent { public void doSomething() {} // Compliant - parent is unknown, no overridden symbols resolved } + // --- Compliant: static method with same name as instance method in superclass (compile error in Java) --- + + static class ParentWithInstanceMethod { + protected void process() {} + } + + static class ChildStaticSameAsInstance extends ParentWithInstanceMethod { + public static void process() {} // Compliant - parent method is not static + } + // --- Compliant: static method in class extending unknown parent --- static class StaticChildOfUnknownParent extends UnknownParent { diff --git a/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java index 760e4018f06..69583507a20 100644 --- a/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/MethodOverrideAccessibilityCheckSample.java @@ -298,16 +298,6 @@ static class ChildStaticSameAsField extends ParentWithField { public static void compute() {} // Compliant - parent has a field, not a method } - // --- Compliant: static method with same name as instance method in superclass --- - - static class ParentWithInstanceMethod { - protected void process() {} - } - - static class ChildStaticSameAsInstance extends ParentWithInstanceMethod { - public static void process() {} // Compliant - parent method is not static - } - // --- Compliant: static method with no-arg hiding parent with params --- static class ParentStaticWithParams { @@ -334,4 +324,48 @@ static class ConcreteDoer extends AbstractDoer implements Doer { public void doIt() {} // Noncompliant {{Increase of accessibility from "protected" to "public" when overriding method.}} // ^^^^ } + + // --- Compliant: static method hiding same access level (protected -> protected) --- + + static class StaticParentSameAccess { + protected static void safeMethod(int x) {} + } + + static class StaticChildSameAccess extends StaticParentSameAccess { + protected static void safeMethod(int x) {} // Compliant - same access level + } + + // --- Compliant: static method hiding with reduced access (public -> protected, compiler error normally) --- + // Note: this case would normally be a compiler error, but covered as non-compiling test + + // --- Noncompliant: static hiding package-private -> public through multiple levels --- + + static class DeepStaticBase { + static void deepMethod() {} +// ^^^^^^^^^^> + } + + static class DeepStaticMiddle extends DeepStaticBase { + // does not redefine deepMethod + } + + static class DeepStaticChild extends DeepStaticMiddle { + public static void deepMethod() {} // Noncompliant {{Increase of accessibility from "package-private" to "public" when hiding method.}} +// ^^^^^^^^^^ + } + + // --- Compliant: instance method override with same access through multiple interfaces --- + + interface InterfaceA { + void doAction(); + } + + interface InterfaceB { + void doAction(); + } + + static class ImplBothInterfaces implements InterfaceA, InterfaceB { + @Override + public void doAction() {} // Compliant - implementing interface methods + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java index b891f299a1f..9a025c0415a 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MethodOverrideAccessibilityCheck.java @@ -24,7 +24,6 @@ import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.Symbol; -import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.Tree; @@ -59,21 +58,9 @@ private void checkInstanceMethodOverride(MethodTree methodTree, Symbol.MethodSym } private void checkStaticMethodHiding(MethodTree methodTree, Symbol.MethodSymbol methodSymbol) { - Symbol.TypeSymbol owner = (Symbol.TypeSymbol) methodSymbol.owner(); - Type superClass = owner.superClass(); - while (superClass != null) { - for (Symbol symbol : superClass.symbol().lookupSymbols(methodSymbol.name())) { - if (symbol.isMethodSymbol() && symbol.isStatic() && !symbol.isPrivate() - && !(symbol.isPackageVisibility() && !samePackage(methodSymbol, symbol))) { - Symbol.MethodSymbol candidate = (Symbol.MethodSymbol) symbol; - if (MethodTreeUtils.hasSameParameterTypes(methodSymbol, candidate)) { - reportIfAccessIncreased(methodTree, methodSymbol, candidate, "hiding"); - return; - } - } - } - superClass = superClass.symbol().superClass(); - } + MethodTreeUtils.findHiddenStaticMethod(methodSymbol, + symbol -> !(symbol.isPackageVisibility() && !samePackage(methodSymbol, symbol))) + .ifPresent(hidden -> reportIfAccessIncreased(methodTree, methodSymbol, hidden, "hiding")); } private static boolean samePackage(Symbol s1, Symbol s2) { diff --git a/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java b/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java index 4b704bf3e30..74abc32bb80 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java @@ -23,7 +23,6 @@ import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.Symbol; -import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.Tree; @@ -45,25 +44,8 @@ public void visitNode(Tree tree) { if (!methodSymbol.isStatic() || isIntentionalHiding(methodSymbol)) { return; } - Symbol.TypeSymbol owner = (Symbol.TypeSymbol) methodSymbol.owner(); - Type superClass = owner.superClass(); - while (superClass != null) { - if (checkHiding(methodTree, methodSymbol, superClass)) { - return; - } - superClass = superClass.symbol().superClass(); - } - } - - private boolean checkHiding(MethodTree methodTree, Symbol.MethodSymbol methodSymbol, Type superClass) { - for (Symbol symbol : superClass.symbol().lookupSymbols(methodSymbol.name())) { - if (symbol.isMethodSymbol() && symbol.isStatic() && !symbol.isPrivate() - && MethodTreeUtils.hasSameParameterTypes(methodSymbol, (Symbol.MethodSymbol) symbol)) { - reportHidingIssue(methodTree, methodSymbol, (Symbol.MethodSymbol) symbol); - return true; - } - } - return false; + MethodTreeUtils.findHiddenStaticMethod(methodSymbol, symbol -> true) + .ifPresent(hidden -> reportHidingIssue(methodTree, methodSymbol, hidden)); } private void reportHidingIssue(MethodTree methodTree, Symbol.MethodSymbol methodSymbol, Symbol.MethodSymbol hiddenMethod) { diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java index 2f4f6991414..ab0b0786bd9 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java @@ -260,6 +260,23 @@ public void visitLambdaExpression(LambdaExpressionTree lambdaExpressionTree) { } } + public static Optional findHiddenStaticMethod(Symbol.MethodSymbol methodSymbol, + Predicate additionalFilter) { + Symbol.TypeSymbol owner = (Symbol.TypeSymbol) methodSymbol.owner(); + Type superClass = owner.superClass(); + while (superClass != null) { + for (Symbol symbol : superClass.symbol().lookupSymbols(methodSymbol.name())) { + if (symbol.isMethodSymbol() && symbol.isStatic() && !symbol.isPrivate() + && additionalFilter.test(symbol) + && hasSameParameterTypes(methodSymbol, (Symbol.MethodSymbol) symbol)) { + return Optional.of((Symbol.MethodSymbol) symbol); + } + } + superClass = superClass.symbol().superClass(); + } + return Optional.empty(); + } + public static boolean hasSameParameterTypes(Symbol.MethodSymbol method, Symbol.MethodSymbol candidate) { List methodParams = method.parameterTypes(); List candidateParams = candidate.parameterTypes();