From f6a3a67a35c235daca3dfbf4790c011b36e7dbde Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 14 Oct 2026 16:13:07 +0200 Subject: [PATCH 1/3] Add rule metadata --- .../org/sonar/l10n/java/rules/java/S9437.html | 106 ++++++++++++++++++ .../org/sonar/l10n/java/rules/java/S9437.json | 24 ++++ .../main/resources/profiles/Sonar_way/S9437 | 0 3 files changed, 130 insertions(+) create mode 100644 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9437.html create mode 100644 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9437.json create mode 100644 sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9437 diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9437.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9437.html new file mode 100644 index 00000000000..b7e6af5fa01 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9437.html @@ -0,0 +1,106 @@ +

Why is this an issue?

+

A static method named getInstance is the conventional entry point of the singleton pattern. Its name promises the +instance of the class, so callers assume that every call hands back the same object and that any state the class holds is shared.

+

A getInstance method whose body is an unconditional return new Foo(); breaks that promise. No static field is read or +written, so nothing is cached and each call creates a separate object. Caches, connection pools, counters and registries owned by the class are then +duplicated per call site, and every write performed through one instance is invisible to the holders of the others.

+

Because each object works perfectly in isolation, the defect does not fail loudly. It surfaces much later, as state that silently goes missing +between two unrelated parts of the application. This shape is typically introduced when the caching logic is dropped during a refactoring, or when a +class is turned into a singleton by renaming its factory method without adding the field that backs it.

+

This rule raises an issue when a static method named getInstance unconditionally returns a newly created instance of its own class, +instead of a cached one.

+

The issue is always worth reviewing, but it is not always worth fixing as reported. If the class is stateless and the name is the only problem, +renaming the method is enough. If callers are documented as receiving a fresh, independent object, the name is misleading but the behaviour may be +deliberate, and renaming is again the fix rather than caching.

+

Exceptions

+

No issue is raised on a lazily initialised getInstance, which assigns the static field before returning it, because it does cache the +instance:

+
+public static synchronized FeatureFlags getInstance() {
+  if (instance == null) {
+    instance = new FeatureFlags();
+  }
+  return instance;
+}
+
+

No issue is raised on factory methods that are not named getInstance, such as create, newInstance, +of or valueOf. A method called newInstance returning a fresh object is doing exactly what its name says, and +of or valueOf are conventionally free to return either a cached or a new value.

+

Overloads of getInstance that take arguments are also exempt. A parameterised factory such as getInstance(Locale locale) +cannot have a single instance, and the established JDK convention, for example Calendar.getInstance(TimeZone) or +MessageDigest.getInstance(String), is to key or build the result from the arguments. Only the no-argument form carries the "there is +exactly one" promise, so only that form is reported.

+

How to fix it

+

Decide which instance the method should return, then make it return that one.

+

If the class is meant to be a singleton, store the instance in a private static final field and return the field. Also make the +constructor private so that no caller can bypass the accessor. Prefer this eager form unless the construction is expensive or needs +configuration that is not available at class-loading time; if it does, use a lazily initialised field guarded against concurrent access, such as a +holder class or a synchronized accessor.

+

If the class is not meant to be a singleton and callers genuinely need their own object, keep the new and rename the method to +create or newInstance. This fixes the contract instead of the implementation and is the right choice when the class has no +state to share.

+

Both fixes change how many objects exist at runtime, so check the callers before applying either. Sharing an instance that was previously +per-caller requires the class to be safe to use from several threads, and code that mutates the object after obtaining it, or that relies on object +identity, may need to be adapted.

+

Code examples

+

Noncompliant code example

+
+public class FeatureFlags {
+
+  private final Map<String, Boolean> flags = new HashMap<>();
+
+  public static FeatureFlags getInstance() {
+    return new FeatureFlags(); // Noncompliant, every caller gets its own empty map
+  }
+
+  public void enable(String name) {
+    flags.put(name, true);
+  }
+
+  public boolean isEnabled(String name) {
+    return flags.getOrDefault(name, false);
+  }
+}
+
+// FeatureFlags.getInstance().enable("dark-mode");
+// FeatureFlags.getInstance().isEnabled("dark-mode"); // returns false, the flag was set on another object
+
+

Compliant solution

+
+public class FeatureFlags {
+
+  private static final FeatureFlags INSTANCE = new FeatureFlags();
+
+  private final Map<String, Boolean> flags = new ConcurrentHashMap<>();
+
+  private FeatureFlags() {
+  }
+
+  public static FeatureFlags getInstance() {
+    return INSTANCE;
+  }
+
+  public void enable(String name) {
+    flags.put(name, true);
+  }
+
+  public boolean isEnabled(String name) {
+    return flags.getOrDefault(name, false);
+  }
+}
+
+// FeatureFlags.getInstance().enable("dark-mode");
+// FeatureFlags.getInstance().isEnabled("dark-mode"); // returns true
+
+

Resources

+

Documentation

+ +

Related rules

+ + diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9437.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9437.json new file mode 100644 index 00000000000..872b2908695 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9437.json @@ -0,0 +1,24 @@ +{ + "title": "\"getInstance()\" methods should not return a new instance on every call", + "type": "BUG", + "code": { + "impacts": { + "RELIABILITY": "HIGH", + "MAINTAINABILITY": "MEDIUM" + }, + "attribute": "LOGICAL" + }, + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "15min" + }, + "tags": [ + "design" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-9437", + "sqKey": "S9437", + "scope": "All", + "quickfix": "infeasible" +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9437 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9437 new file mode 100644 index 00000000000..e69de29bb2d From 1c0c92b2d7d0639aa56785b128df4fc78104999a Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 14 Oct 2026 16:13:07 +0200 Subject: [PATCH 2/3] Add failing reproducer --- ...stanceReturningNewInstanceCheckSample.java | 11 + ...stanceReturningNewInstanceCheckSample.java | 211 ++++++++++++++++++ .../GetInstanceReturningNewInstanceCheck.java | 36 +++ ...InstanceReturningNewInstanceCheckTest.java | 54 +++++ 4 files changed, 312 insertions(+) create mode 100644 java-checks-test-sources/default/src/main/files/non-compiling/checks/GetInstanceReturningNewInstanceCheckSample.java create mode 100644 java-checks-test-sources/default/src/main/java/checks/GetInstanceReturningNewInstanceCheckSample.java create mode 100644 java-checks/src/main/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheck.java create mode 100644 java-checks/src/test/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheckTest.java diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/GetInstanceReturningNewInstanceCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/GetInstanceReturningNewInstanceCheckSample.java new file mode 100644 index 00000000000..d52913eb5be --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/GetInstanceReturningNewInstanceCheckSample.java @@ -0,0 +1,11 @@ +package checks; + +class GetInstanceReturningNewInstanceCheckSample { + + static class UnresolvedCreatedType { + + static UnresolvedCreatedType getInstance() { + return new Unknown(); + } + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/GetInstanceReturningNewInstanceCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/GetInstanceReturningNewInstanceCheckSample.java new file mode 100644 index 00000000000..20b02af339c --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/GetInstanceReturningNewInstanceCheckSample.java @@ -0,0 +1,211 @@ +package checks; + +import java.util.HashMap; +import java.util.Locale; +import java.util.Map; + +class GetInstanceReturningNewInstanceCheckSample { + + static class NoCache { + + private final Map flags = new HashMap<>(); + + public static NoCache getInstance() { +// ^^^^^^^^^^^> + return new NoCache(); // Noncompliant {{Rename this method, as "getInstance" is conventionally reserved for singletons, or make it return a cached instance instead of a new one.}} +// ^^^^^^^^^^^^^ + } + + void enable(String name) { + flags.put(name, true); + } + } + + static class NoCacheWithArguments { + + private NoCacheWithArguments(String name) { + } + + static NoCacheWithArguments getInstance() { +// ^^^^^^^^^^^> + return new NoCacheWithArguments("default"); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + } + + static class GenericNoCache { + + static GenericNoCache getInstance() { +// ^^^^^^^^^^^> + return new GenericNoCache<>(); // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^ + } + } + + static class RawNoCache { + + static RawNoCache getInstance() { +// ^^^^^^^^^^^> + return new RawNoCache(); // Noncompliant +// ^^^^^^^^^^^^^^^^ + } + } + + static class EagerlyCached { + + private static final EagerlyCached INSTANCE = new EagerlyCached(); + + private EagerlyCached() { + } + + static EagerlyCached getInstance() { + return INSTANCE; + } + } + + static class LazilyCached { + + private static LazilyCached instance; + + private LazilyCached() { + } + + static LazilyCached getInstance() { + if (instance == null) { + instance = new LazilyCached(); + } + return instance; + } + } + + static class SynchronizedLazilyCached { + + private static SynchronizedLazilyCached instance; + + static synchronized SynchronizedLazilyCached getInstance() { + if (instance == null) { + instance = new SynchronizedLazilyCached(); + } + return instance; + } + } + + static class HolderCached { + + private static class Holder { + private static final HolderCached INSTANCE = new HolderCached(); + } + + static HolderCached getInstance() { + return Holder.INSTANCE; + } + } + + static class NotStatic { + + NotStatic getInstance() { + return new NotStatic(); + } + } + + static class ParameterizedFactory { + + static ParameterizedFactory getInstance(Locale locale) { + return new ParameterizedFactory(); + } + + static ParameterizedFactory getInstance(Locale locale, String variant) { + return new ParameterizedFactory(); + } + } + + static class DifferentlyNamedFactories { + + static DifferentlyNamedFactories newInstance() { + return new DifferentlyNamedFactories(); + } + + static DifferentlyNamedFactories create() { + return new DifferentlyNamedFactories(); + } + + static DifferentlyNamedFactories of() { + return new DifferentlyNamedFactories(); + } + + static DifferentlyNamedFactories valueOf() { + return new DifferentlyNamedFactories(); + } + } + + static class ReturnsAnotherType { + + static Map getInstance() { + return new HashMap<>(); + } + } + + static class ReturnsAnonymousSubclass { + + static ReturnsAnonymousSubclass getInstance() { + return new ReturnsAnonymousSubclass() { + }; + } + } + + static class DoesMoreThanCreate { + + private static int calls; + + static DoesMoreThanCreate getInstance() { + calls++; + return new DoesMoreThanCreate(); + } + } + + static class ReturnsNull { + + static ReturnsNull getInstance() { + return null; + } + } + + static class NativeAccessor { + + static native NativeAccessor getInstance(); + } + + static class Throwing { + + static Throwing getInstance() { + throw new UnsupportedOperationException(); + } + } + + static class ReturnsNothing { + + static void getInstance() { + return; + } + } + + interface WithoutBody { + + static WithoutBody getInstance() { + return null; + } + } + + static abstract class AbstractGetInstance { + + abstract AbstractGetInstance getInstance(); + } + + enum EnumSingleton { + INSTANCE; + + static EnumSingleton getInstance() { + return INSTANCE; + } + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheck.java b/java-checks/src/main/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheck.java new file mode 100644 index 00000000000..63a494e059e --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheck.java @@ -0,0 +1,36 @@ +/* + * 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.List; +import org.sonar.check.Rule; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.tree.Tree; + +@Rule(key = "S9437") +public class GetInstanceReturningNewInstanceCheck extends IssuableSubscriptionVisitor { + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.METHOD); + } + + @Override + public void visitNode(Tree tree) { + // Implemented in the next step. + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheckTest.java new file mode 100644 index 00000000000..b82a3ebc47a --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheckTest.java @@ -0,0 +1,54 @@ +/* + * 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; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; + +class GetInstanceReturningNewInstanceCheckTest { + + private static final String SAMPLE = "checks/GetInstanceReturningNewInstanceCheckSample.java"; + private static final GetInstanceReturningNewInstanceCheck CHECK = new GetInstanceReturningNewInstanceCheck(); + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath(SAMPLE)) + .withCheck(CHECK) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath(SAMPLE)) + .withCheck(CHECK) + .withoutSemantic() + .verifyIssues(); + } + + @Test + void test_non_compiling() { + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath(SAMPLE)) + .withCheck(CHECK) + .verifyNoIssues(); + } +} From e344aeedac878bdf23732e7201e7c090b5ddae37 Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 14 Oct 2026 16:13:07 +0200 Subject: [PATCH 3/3] Implement rule --- .../GetInstanceReturningNewInstanceCheck.java | 64 ++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/java-checks/src/main/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheck.java b/java-checks/src/main/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheck.java index 63a494e059e..f8cfe0c4d12 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/GetInstanceReturningNewInstanceCheck.java @@ -17,13 +17,34 @@ package org.sonar.java.checks; import java.util.List; +import javax.annotation.CheckForNull; 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.MethodMatchers; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.BlockTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.NewClassTree; +import org.sonar.plugins.java.api.tree.ReturnStatementTree; +import org.sonar.plugins.java.api.tree.StatementTree; import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.java.model.ExpressionUtils; @Rule(key = "S9437") public class GetInstanceReturningNewInstanceCheck extends IssuableSubscriptionVisitor { + private static final String MESSAGE = "Rename this method, as \"getInstance\" is conventionally reserved for singletons," + + " or make it return a cached instance instead of a new one."; + private static final String SECONDARY_MESSAGE = "This name promises a single shared instance."; + + private static final MethodMatchers GET_INSTANCE = MethodMatchers.create() + .ofAnyType() + .names("getInstance") + .addWithoutParametersMatcher() + .build(); + @Override public List nodesToVisit() { return List.of(Tree.Kind.METHOD); @@ -31,6 +52,47 @@ public List nodesToVisit() { @Override public void visitNode(Tree tree) { - // Implemented in the next step. + MethodTree method = (MethodTree) tree; + if (!method.symbol().isStatic() || !GET_INSTANCE.matches(method)) { + return; + } + NewClassTree instantiation = onlyStatementInstantiation(method); + if (instantiation != null && instantiatesOwnClass(instantiation, method)) { + List secondary = + List.of(new JavaFileScannerContext.Location(SECONDARY_MESSAGE, method.simpleName())); + reportIssue(instantiation, MESSAGE, secondary, null); + } + } + + /** + * Returns the instance creation of a method whose body is nothing but {@code return new Something(…);}, or {@code null} + * when the body is anything else. A lazily initialised accessor therefore never matches, because testing and assigning + * the cached field takes more than that single statement. + */ + @CheckForNull + private static NewClassTree onlyStatementInstantiation(MethodTree method) { + BlockTree body = method.block(); + if (body == null || body.body().size() != 1) { + return null; + } + StatementTree statement = body.body().get(0); + if (!statement.is(Tree.Kind.RETURN_STATEMENT)) { + return null; + } + ExpressionTree returned = ((ReturnStatementTree) statement).expression(); + if (returned == null) { + return null; + } + ExpressionTree expression = ExpressionUtils.skipParentheses(returned); + if (!expression.is(Tree.Kind.NEW_CLASS)) { + return null; + } + NewClassTree instantiation = (NewClassTree) expression; + return instantiation.classBody() == null ? instantiation : null; + } + + private static boolean instantiatesOwnClass(NewClassTree instantiation, MethodTree method) { + Type createdType = instantiation.symbolType(); + return !createdType.isUnknown() && createdType.erasure().equals(method.symbol().owner().type().erasure()); } }