[romainbrenguier]

Summary

S9149 (static methods should not hide methods from superclasses) raised on hiding that is required by the design of the parent class. The rule no longer raises in two such cases:

• Covariant static factories: the hiding method returns a strict subtype of the hidden method's return type (e.g. Guava's ImmutableBiMap.of() / copyOf() / builder() hiding ImmutableMap's).
• JUnit 3 public static Test suite(): the JUnit 3 runner looks this method up on each test class, so every subclass has to redeclare it.

Hiding with the same or an unrelated return type, including unknown types, is still reported.

Ruling / autoscan

• guava: all 37 issues removed (all covariant factories).
• commons-beanutils: the 2 suite() issues in test classes removed; the LocaleBeanUtils issues remain.
• autoscan: expecting 1 more false positive, because junit.framework.Test cannot be resolved without bytecode. This is a prediction and not verified locally.

Not addressed

The ticket also suggests not raising when the hidden method comes from an external library. This was not done: the semantic model has no reliable way to tell library classes apart from project classes compiled in other files. Those issues are still reported (e.g. hiding Thread.sleep).

🤖 Generated with Claude Code

Agent workflow

Tool link: https://github.com/SonarSource/languages-experimental-tooling/tree/romain/my-tickets/personal/romain-brenguier
PR created using uv run create_with_claude.py SONARJAVA-7242.txt

Addressed review comments in pr_report_6400.md using uv run address_reviews.py pr_report_6400.md

[hashicorp-vault-sonar-prod]

SONARJAVA-7242

[github-actions]

Ruling Diff Summary

Detected changes in 2 rule files: 39 issues removed, 0 issues added.

S9149 (java) on commons-beanutils - 2 issues removed, 0 issues added

Removed src/test/java/org/apache/commons/beanutils2/BeanUtils2TestCase.java (line 57)

        52 | 
        53 | 
        54 |     /**
        55 |      * Return the tests included in this test suite.
        56 |      */
>>>     57 |     public static Test suite() {
        58 |         return new TestSuite(BeanUtils2TestCase.class);
        59 |     }
        60 | 
        61 |     /**
        62 |      * Tear down instance variables required by this test case.

Removed src/test/java/org/apache/commons/beanutils2/WrapDynaBeanTestCase.java (line 75)

        70 | 
        71 | 
        72 |     /**
        73 |      * Return the tests included in this test suite.
        74 |      */
>>>     75 |     public static Test suite() {
        76 | 
        77 |         return new TestSuite(WrapDynaBeanTestCase.class);
        78 | 
        79 |     }
        80 | 

S9149 (java) on guava - 37 issues removed, 0 issues added - deleted ruling file

Removed src/com/google/common/collect/ImmutableBiMap.java (line 41)

        36 |   /**
        37 |    * Returns the empty bimap.
        38 |    */
        39 |   // Casting to any type is safe because the set will never hold any elements.
        40 |   @SuppressWarnings("unchecked")
>>>     41 |   public static <K, V> ImmutableBiMap<K, V> of() {
        42 |     return (ImmutableBiMap<K, V>) RegularImmutableBiMap.EMPTY;
        43 |   }
        44 | 
        45 |   /**
        46 |    * Returns an immutable bimap containing a single entry.

Removed src/com/google/common/collect/ImmutableBiMap.java (line 48)

        43 |   }
        44 | 
        45 |   /**
        46 |    * Returns an immutable bimap containing a single entry.
        47 |    */
>>>     48 |   public static <K, V> ImmutableBiMap<K, V> of(K k1, V v1) {
        49 |     return new SingletonImmutableBiMap<K, V>(k1, v1);
        50 |   }
        51 | 
        52 |   /**
        53 |    * Returns an immutable map containing the given entries, in order.

Removed src/com/google/common/collect/ImmutableBiMap.java (line 57)

        52 |   /**
        53 |    * Returns an immutable map containing the given entries, in order.
        54 |    *
        55 |    * @throws IllegalArgumentException if duplicate keys or values are added
        56 |    */
>>>     57 |   public static <K, V> ImmutableBiMap<K, V> of(K k1, V v1, K k2, V v2) {
        58 |     return RegularImmutableBiMap.fromEntries(entryOf(k1, v1), entryOf(k2, v2));
        59 |   }
        60 | 
        61 |   /**
        62 |    * Returns an immutable map containing the given entries, in order.

Removed src/com/google/common/collect/ImmutableListMultimap.java (line 51)

        46 |     implements ListMultimap<K, V> {
        47 | 
        48 |   /** Returns the empty multimap. */
        49 |   // Casting is safe because the multimap will never hold any elements.
        50 |   @SuppressWarnings("unchecked")
>>>     51 |   public static <K, V> ImmutableListMultimap<K, V> of() {
        52 |     return (ImmutableListMultimap<K, V>) EmptyImmutableListMultimap.INSTANCE;
        53 |   }
        54 | 
        55 |   /**
        56 |    * Returns an immutable multimap containing a single entry.

Removed src/com/google/common/collect/ImmutableListMultimap.java (line 58)

        53 |   }
        54 | 
        55 |   /**
        56 |    * Returns an immutable multimap containing a single entry.
        57 |    */
>>>     58 |   public static <K, V> ImmutableListMultimap<K, V> of(K k1, V v1) {
        59 |     ImmutableListMultimap.Builder<K, V> builder = ImmutableListMultimap.builder();
        60 |     builder.put(k1, v1);
        61 |     return builder.build();
        62 |   }
        63 | 

Removed src/com/google/common/collect/ImmutableListMultimap.java (line 67)

        62 |   }
        63 | 
        64 |   /**
        65 |    * Returns an immutable multimap containing the given entries, in order.
        66 |    */
>>>     67 |   public static <K, V> ImmutableListMultimap<K, V> of(K k1, V v1, K k2, V v2) {
        68 |     ImmutableListMultimap.Builder<K, V> builder = ImmutableListMultimap.builder();
        69 |     builder.put(k1, v1);
        70 |     builder.put(k2, v2);
        71 |     return builder.build();
        72 |   }

Removed src/com/google/common/collect/ImmutableSetMultimap.java (line 59)

        54 |     implements SetMultimap<K, V> {
        55 | 
        56 |   /** Returns the empty multimap. */
        57 |   // Casting is safe because the multimap will never hold any elements.
        58 |   @SuppressWarnings("unchecked")
>>>     59 |   public static <K, V> ImmutableSetMultimap<K, V> of() {
        60 |     return (ImmutableSetMultimap<K, V>) EmptyImmutableSetMultimap.INSTANCE;
        61 |   }
        62 | 
        63 |   /**
        64 |    * Returns an immutable multimap containing a single entry.

Removed src/com/google/common/collect/ImmutableSetMultimap.java (line 66)

        61 |   }
        62 | 
        63 |   /**
        64 |    * Returns an immutable multimap containing a single entry.
        65 |    */
>>>     66 |   public static <K, V> ImmutableSetMultimap<K, V> of(K k1, V v1) {
        67 |     ImmutableSetMultimap.Builder<K, V> builder = ImmutableSetMultimap.builder();
        68 |     builder.put(k1, v1);
        69 |     return builder.build();
        70 |   }
        71 | 

Removed src/com/google/common/collect/ImmutableSetMultimap.java (line 77)

        72 |   /**
        73 |    * Returns an immutable multimap containing the given entries, in order.
        74 |    * Repeated occurrences of an entry (according to {@link Object#equals}) after
        75 |    * the first are ignored.
        76 |    */
>>>     77 |   public static <K, V> ImmutableSetMultimap<K, V> of(K k1, V v1, K k2, V v2) {
        78 |     ImmutableSetMultimap.Builder<K, V> builder = ImmutableSetMultimap.builder();
        79 |     builder.put(k1, v1);
        80 |     builder.put(k2, v2);
        81 |     return builder.build();
        82 |   }

Removed src/com/google/common/collect/ImmutableSortedMap.java (line 85)

        80 |    * Returns the empty sorted map.
        81 |    */
        82 |   @SuppressWarnings("unchecked")
        83 |   // unsafe, comparator() returns a comparator on the specified type
        84 |   // TODO(kevinb): evaluate whether or not of().comparator() should return null
>>>     85 |   public static <K, V> ImmutableSortedMap<K, V> of() {
        86 |     return (ImmutableSortedMap<K, V>) NATURAL_EMPTY_MAP;
        87 |   }
        88 | 
        89 |   /**
        90 |    * Returns an immutable map containing a single entry.

Removed src/com/google/common/collect/ImmutableSortedMap.java (line 180)

       175 |    *         comparable
       176 |    * @throws NullPointerException if any key or value in {@code map} is null
       177 |    * @throws IllegalArgumentException if any two keys are equal according to
       178 |    *         their natural ordering
       179 |    */
>>>    180 |   public static <K, V> ImmutableSortedMap<K, V> copyOf(Map<? extends K, ? extends V> map) {
       181 |     // Hack around K not being a subtype of Comparable.
       182 |     // Unsafe, see ImmutableSortedSetFauxverideShim.
       183 |     @SuppressWarnings("unchecked")
       184 |     Ordering<K> naturalOrder = (Ordering<K>) NATURAL_ORDER;
       185 |     return copyOfInternal(map, naturalOrder);

Removed src/com/google/common/collect/ImmutableSortedMap.java (line 218)

       213 |    * @throws IllegalArgumentException if any two keys are equal according to the
       214 |    *         comparator
       215 |    * @since 19.0
       216 |    */
       217 |   @Beta
>>>    218 |   public static <K, V> ImmutableSortedMap<K, V> copyOf(
       219 |       Iterable<? extends Entry<? extends K, ? extends V>> entries) {
       220 |     // Hack around K not being a subtype of Comparable.
       221 |     // Unsafe, see ImmutableSortedSetFauxverideShim.
       222 |     @SuppressWarnings("unchecked")
       223 |     Ordering<K> naturalOrder = (Ordering<K>) NATURAL_ORDER;

Removed src/com/google/common/collect/ImmutableSortedMultiset.java (line 63)

        58 | 
        59 |   /**
        60 |    * Returns the empty immutable sorted multiset.
        61 |    */
        62 |   @SuppressWarnings("unchecked")
>>>     63 |   public static <E> ImmutableSortedMultiset<E> of() {
        64 |     return (ImmutableSortedMultiset) NATURAL_EMPTY_MULTISET;
        65 |   }
        66 | 
        67 |   /**
        68 |    * Returns an immutable sorted multiset containing a single element.

Removed src/com/google/common/collect/ImmutableSortedMultiset.java (line 171)

       166 |    * comparable.
       167 |    *
       168 |    * @throws ClassCastException if the elements are not mutually comparable
       169 |    * @throws NullPointerException if any of {@code elements} is null
       170 |    */
>>>    171 |   public static <E> ImmutableSortedMultiset<E> copyOf(Iterable<? extends E> elements) {
       172 |     // Hack around E not being a subtype of Comparable.
       173 |     // Unsafe, see ImmutableSortedMultisetFauxverideShim.
       174 |     @SuppressWarnings("unchecked")
       175 |     Ordering<E> naturalOrder = (Ordering<E>) Ordering.<Comparable>natural();
       176 |     return copyOf(naturalOrder, elements);

Removed src/com/google/common/collect/ImmutableSortedMultiset.java (line 189)

       184 |    * comparable.
       185 |    *
       186 |    * @throws ClassCastException if the elements are not mutually comparable
       187 |    * @throws NullPointerException if any of {@code elements} is null
       188 |    */
>>>    189 |   public static <E> ImmutableSortedMultiset<E> copyOf(Iterator<? extends E> elements) {
       190 |     // Hack around E not being a subtype of Comparable.
       191 |     // Unsafe, see ImmutableSortedMultisetFauxverideShim.
       192 |     @SuppressWarnings("unchecked")
       193 |     Ordering<E> naturalOrder = (Ordering<E>) Ordering.<Comparable>natural();
       194 |     return copyOf(naturalOrder, elements);

[romainbrenguier]

/pvf S9149

[hashicorp-vault-sonar-prod]

📊 Performance Validation complete
🆕 0 new · 🗑️ 177 lost issues
View dashboard

[nathsou]

Approving: the Guava and suite() false positives are handled and the ruling looks right. Two points on the exemption's boundaries, both reproduced locally with CheckVerifier probes against this branch.

[nathsou]

This exemption has no factory condition. Since JLS 8.4.8.3 requires a hiding method's return type to be return-type-substitutable for the hidden one, any difference in return types in compiling code means "exempt", so the rule now only fires when the return types are identical. For example, static String helper() hiding static Object helper() is no longer reported.

Could we restrict it to factory-like return types, e.g. the declaring class, a subtype of it, or a type nested in it? That should still cover the Guava cases (ImmutableBiMap, ImmutableBiMap.Builder), to be confirmed against the ruling.

[romainbrenguier]

Done: the exemption now only applies when the covariant return type is the declaring class, a subtype of it, or a type nested in it. static String helper() hiding static Object helper() is reported again, and so is List<String> copyOf(...) hiding Collection<String> copyOf(...). A nested Builder case was added. The CI ruling will confirm the Guava baseline.

[nathsou]

The JUnit 3 runner (BaseTestRunner.getTest) accepts any suite() whose return type implements Test, and public static TestSuite suite() is common. With .is(...), a parent and child both declaring TestSuite suite() are still reported. isSubtypeOf("junit.framework.Test") would cover it.

[romainbrenguier]

Done: switched to isSubtypeOf("junit.framework.Test") and added a TestSuite suite() test case.

[gitar-bot]

💡 Edge Case: New TestSuite fixture adds an autoscan FP; diff_S9149.json still says 1

This commit adds ChildTestSuiteCase.suite() (marked // Compliant - JUnit 3 suite) to the default sample. Autoscan runs without bytecode, so junit.framework.TestSuite and junit.framework.Test don't resolve. Both suite() return types then look identical, so isCovariantFactory returns false, and isSubtypeOf("junit.framework.Test") against an unknown class type also returns false. The rule therefore reports this method too. That is the same mechanism the PR description gives for the existing ChildTestCase.suite() false positive, so autoscan should now see 2 false positives. its/autoscan/src/test/resources/autoscan/diffs/diff_S9149.json was not updated and still has "falsePositives": 1. The PR description's "1 more false positive" claim is now out of date as well.

Bump the expected autoscan false-positive count to account for the new unresolvable suite() fixture:

{
  "ruleKey": "S9149",
  "hasTruePositives": true,
  "falseNegatives": 0,
  "falsePositives": 2
}

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review 👍 Approved with suggestions 1 closed / 2 findings

🟡 Medium risk · Broadened S9149 exemptions for covariant factories and JUnit 3 suite() methods could suppress actionable static-method-hiding findings in edge cases.

Restricts the S9149 exemption to covariant static factories and JUnit 3 suite() methods returning junit.framework.Test subtypes, so Guava factories and the commons-beanutils test suite() methods no longer raise. One open minor finding: the new ChildTestSuiteCase.suite() fixture likely adds a second autoscan false positive, but diff_S9149.json still records 1. The renamed SameReturnParent/SameReturnChild fixture addresses the earlier unrelated-return-type test concern.

💡 Edge Case: New TestSuite fixture adds an autoscan FP; diff_S9149.json still says 1

📄 java-checks-test-sources/default/src/main/java/checks/StaticMethodHidingCheckSample.java:426-436 📄 java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java:105-119 📄 its/autoscan/src/test/resources/autoscan/diffs/diff_S9149.json

This commit adds ChildTestSuiteCase.suite() (marked // Compliant - JUnit 3 suite) to the default sample. Autoscan runs without bytecode, so junit.framework.TestSuite and junit.framework.Test don't resolve. Both suite() return types then look identical, so isCovariantFactory returns false, and isSubtypeOf("junit.framework.Test") against an unknown class type also returns false. The rule therefore reports this method too. That is the same mechanism the PR description gives for the existing ChildTestCase.suite() false positive, so autoscan should now see 2 false positives. its/autoscan/src/test/resources/autoscan/diffs/diff_S9149.json was not updated and still has "falsePositives": 1. The PR description's "1 more false positive" claim is now out of date as well.

Bump the expected autoscan false-positive count to account for the new unresolvable suite() fixture

{
  "ruleKey": "S9149",
  "hasTruePositives": true,
  "falseNegatives": 0,
  "falsePositives": 2
}

✅ 1 closed

✅ Quality: "Unrelated return type" test actually uses identical return types

📄 java-checks-test-sources/default/src/main/java/checks/StaticMethodHidingCheckSample.java:370-380
UnrelatedReturnParent.value() and UnrelatedReturnChild.value() both return Integer, so this fixture repeats the same-return-type case that ImmutableSetLike.create() already covers (Object vs Object). The PR description says hiding with an "unrelated return type" is still reported, but no test checks that. Java (JLS 8.4.8.3) also rejects hiding with a return type that is not return-type-substitutable, so that case can only happen in non-compiling code. Either rename the fixture to say it covers the same return type, or move a real unrelated-return case (e.g. Integer vs String) into files/non-compiling/checks/StaticMethodHidingCheckSample.java so the stated behavior is actually tested.

🤖 Prompt for agents

Code Review: Restricts the S9149 exemption to covariant static factories and JUnit 3 `suite()` methods returning `junit.framework.Test` subtypes, so Guava factories and the commons-beanutils test `suite()` methods no longer raise. One open minor finding: the new `ChildTestSuiteCase.suite()` fixture likely adds a second autoscan false positive, but `diff_S9149.json` still records 1. The renamed `SameReturnParent`/`SameReturnChild` fixture addresses the earlier unrelated-return-type test concern.

1. 💡 Edge Case: New TestSuite fixture adds an autoscan FP; diff_S9149.json still says 1
   Files: java-checks-test-sources/default/src/main/java/checks/StaticMethodHidingCheckSample.java:426-436, java-checks/src/main/java/org/sonar/java/checks/StaticMethodHidingCheck.java:105-119, its/autoscan/src/test/resources/autoscan/diffs/diff_S9149.json

   This commit adds `ChildTestSuiteCase.suite()` (marked `// Compliant - JUnit 3 suite`) to the default sample. Autoscan runs without bytecode, so `junit.framework.TestSuite` and `junit.framework.Test` don't resolve. Both `suite()` return types then look identical, so `isCovariantFactory` returns false, and `isSubtypeOf("junit.framework.Test")` against an unknown class type also returns false. The rule therefore reports this method too. That is the same mechanism the PR description gives for the existing `ChildTestCase.suite()` false positive, so autoscan should now see 2 false positives. `its/autoscan/src/test/resources/autoscan/diffs/diff_S9149.json` was not updated and still has `"falsePositives": 1`. The PR description's "1 more false positive" claim is now out of date as well.

   Fix (Bump the expected autoscan false-positive count to account for the new unresolvable suite() fixture):
   {
     "ruleKey": "S9149",
     "hasTruePositives": true,
     "falseNegatives": 0,
     "falsePositives": 2
   }

Verification Rules 2/2 met Repos 4 inspected · PRs 1 inspected

Functional validation No results

Rules 2 passed 1 not applicable

✅ Internal artifact registries
✅ Repository license compliance

Broader impact Repos 4 inspected PRs 1 inspected

Cross-repo 4 repositories inspected 0 open impacts

SonarSource/sonarqube Inspected
SonarSource/sonarlint-vscode Inspected
SonarSource/sonar-java-symbolic-execution Inspected
SonarSource/sonarlint-intellij Inspected

Cross-PR 1 PR inspected 0 open impacts

PR 6416: SONARJAVA-7252 Decouple the Java frontend from Eclipse JDT core Inspected

Auto-approval Not enabled

Set up

_{For more verification details, comment gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
88.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube