Skip to content

Commit

Permalink
SONARJAVA-4440 Changes from feedback
Browse files Browse the repository at this point in the history 
* Update issue message to match RSPEC
* Ignore enums with a single constant built with one ore more parameters
* Add private constructor as secondary location
* Add assignments as secondaries
* Refactoring
* Raise when class contains static fields of non-relevant types
* Improve coverage
* Cover cases where instance members are all private
* Cover cases where an enum has more than one constant
* Cover cases where the single constructor is public and takes parameters
  • Loading branch information
@dorian-burihabwa-sonarsource
dorian-burihabwa-sonarsource committed Apr 7, 2023
1 parent 680c13f commit f28ba8c
Show file tree
Hide file tree
Showing 3 changed files with 236 additions and 41 deletions.
160 changes: 157 additions & 3 deletions java-checks-test-sources/src/main/java/checks/SingletonUsageCheckSample.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// TODO: check code snippet license
public class SingletonUsageCheckSample {

public static class EagerInitializedSingleton { // Noncompliant [[sc=23;ec=48;secondary=+2]]
public static class EagerInitializedSingleton { // Noncompliant [[sc=23;ec=48;secondary=+2,+5]] {{A Singleton implementation was detected. Make sure the use of the Singleton pattern is required and the implementation is the right one for the context.}}

private static final EagerInitializedSingleton instance = new EagerInitializedSingleton();

Expand All @@ -21,7 +21,7 @@ public boolean foo() {
}
}

public static class StaticBlockSingleton { // Noncompliant
public static class StaticBlockSingleton { // Noncompliant [[sc=23;ec=43;secondary=+2,+4,+9]]

private static StaticBlockSingleton instance;

Expand Down Expand Up @@ -99,7 +99,7 @@ public boolean foo() {
}
}

public enum EnumSingleton { // Noncompliant
public enum EnumSingleton { // Noncompliant [[sc=15;ec=28]] {{An Enum-based Singleton implementation was detected. Make sure the use of the Singleton pattern is required and an Enum-based implementation is the right one for the context.}}

INSTANCE;

Expand Down Expand Up @@ -133,4 +133,158 @@ protected Object readResolve() {

}

public static class MultipleStaticFieldsOfDifferentTypesDoNotPreventDetection { // Noncompliant
public static final MultipleStaticFieldsOfDifferentTypesDoNotPreventDetection ONE = new MultipleStaticFieldsOfDifferentTypesDoNotPreventDetection();
public static final String MESSAGE = "Hello, World!";
private int value;

private MultipleStaticFieldsOfDifferentTypesDoNotPreventDetection() {
this.value = 1;
}

public int getValue() {
return value;
}
}

public static class PublicConstructorWithParameters { // Compliant the constructor is public and takes parameters
public static final PublicConstructorWithParameters INSTANCE = new PublicConstructorWithParameters(42);
private int value;

public PublicConstructorWithParameters(int value) {
this.value = value;
}

public int getValue() {
return value;
}
}

enum MoreThanOneConstant {
ONE(),
THE_SAME();

private int value;

private MoreThanOneConstant() {
value = 42;
}

public int getValue() {
return value;
}
}

interface WithSides {
int sides();
}

enum Shape implements WithSides { // Compliant because single enum constants are not singletons
TRIANGLE(3),;
private int sides;

Shape(int sides) {
this.sides = sides;
}

@Override
public int sides() {
return sides;
}
}

enum EnumWithPrivateInstanceMethodAndFields { // Compliant there are non-private fields or methods
INSTANCE();
private int value;
private EnumWithPrivateInstanceMethodAndFields() {
value = 42;
}

private int increment() {
return value++;
}
}

static class ClassWithPrivateInstanceMethodAndFields { // Compliant there are non-private fields or methods
public static final ClassWithPrivateInstanceMethodAndFields INSTANCE = new ClassWithPrivateInstanceMethodAndFields();
private int value;
private ClassWithPrivateInstanceMethodAndFields() {
value = 42;
}

private int increment() {
return value++;
}
}

public static class TooManyConstructors { // Compliant
public static final TooManyConstructors INSTANCE = new TooManyConstructors();
private int field;

private TooManyConstructors() {
field = 42;
}

private TooManyConstructors(int value) {
field = value;
}
}

public static class SinglePrivateConstructorWithParameter { // Compliant
public static final SinglePrivateConstructorWithParameter INSTANCE = new SinglePrivateConstructorWithParameter(42);
private int field;

private SinglePrivateConstructorWithParameter(int value) {
field = value;
}

public int value() {
return field;
}
}

public static class LackNonPublicFieldOrInstanceMethod { // Compliant
public static final LackNonPublicFieldOrInstanceMethod INSTANCE = new LackNonPublicFieldOrInstanceMethod();
private int field;

private LackNonPublicFieldOrInstanceMethod() {
field = 42;
}
}

public static class Numbers { // Compliant because there are multiple constant instances of the same type and not a singleton
public static final Numbers ONE = new Numbers();
public static final Numbers POSITIVE_ONE = new Numbers();
private int value;

private Numbers() {
this.value = 1;
}

public int getValue() {
return value;
}
}

public static class MultipleReassignmentsPossible { // Compliant
private static MultipleReassignmentsPossible INSTANCE = new MultipleReassignmentsPossible();
private int value;

private MultipleReassignmentsPossible() {
value = 0;
}

public int increment() {
return value++;
}

public int getValue() {
return value;
}

public MultipleReassignmentsPossible reset() {
INSTANCE = new MultipleReassignmentsPossible();
return INSTANCE;
}
}
}
116 changes: 79 additions & 37 deletions java-checks/src/main/java/org/sonar/java/checks/SingletonUsageCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,35 @@
*/
package org.sonar.java.checks;

import java.util.AbstractMap;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import javax.annotation.CheckForNull;
import javax.annotation.Nullable;
import org.sonar.check.Rule;
import org.sonar.java.checks.helpers.ExpressionsHelper;
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.AssignmentExpressionTree;
import org.sonar.plugins.java.api.tree.ClassTree;
import org.sonar.plugins.java.api.tree.EnumConstantTree;
import org.sonar.plugins.java.api.tree.IdentifierTree;
import org.sonar.plugins.java.api.tree.MethodTree;
import org.sonar.plugins.java.api.tree.Tree;
import org.sonar.plugins.java.api.tree.VariableTree;

@Rule(key = "S6548")
public class SingletonUsageCheck extends IssuableSubscriptionVisitor {
private static final String MESSAGE = "A Singleton implementation was detected." + " " +
"Make sure the use of the Singleton pattern is required and the implementation is the right one for the context.";
private static final String MESSAGE_FOR_ENUMS = "An Enum-based Singleton implementation was detected." + " " +
"Make sure the use of the Singleton pattern is required and an Enum-based implementation is the right one for the context.";

@Override
public List<Tree.Kind> nodesToVisit() {
Expand All @@ -54,47 +66,23 @@ public void visitNode(Tree tree) {

private void visitEnum(ClassTree classTree) {
var enumConstants = classTree.members().stream().filter(member -> member.is(Tree.Kind.ENUM_CONSTANT)).collect(Collectors.toList());
if (enumConstants.size() == 1 && hasNonPrivateInstanceMethodsOrFields(classTree)) {
reportIssue(classTree.simpleName(), "Enum singleton pattern detected",
Collections.singletonList(new JavaFileScannerContext.Location("Single enum", enumConstants.get(0))), null);
if (enumConstants.size() == 1) {
EnumConstantTree constant = (EnumConstantTree) enumConstants.get(0);
if (isInitializedWithParameterFreeConstructor(constant) &&
hasNonPrivateInstanceMethodsOrFields(classTree)) {
reportIssue(classTree.simpleName(), MESSAGE_FOR_ENUMS,
Collections.singletonList(new JavaFileScannerContext.Location("Single enum", constant)), null);
}
}
}

private void visitClass(ClassTree classTree) {
ClassTree wrappingClass = null;
final var parent = classTree.parent();
if (parent != null && parent.is(Tree.Kind.CLASS)) {
wrappingClass = (ClassTree) parent;
}

VariableTree singletonField = null;
ClassTree singletonClass = null;
for (var member : classTree.members()) {
if (!(member.is(Tree.Kind.VARIABLE))) continue;
var classAndInstance = collectClassAndField(classTree);

final var varTree = (VariableTree) member;
final var fieldSymbol = varTree.symbol();
if (classAndInstance == null) return;

if (!fieldSymbol.isStatic()) continue;

if (fieldSymbol.type().equals(classTree.symbol().type())) {
singletonClass = classTree;
} else if (wrappingClass != null && fieldSymbol.type().equals(wrappingClass.symbol().type())) {
singletonClass = wrappingClass;
} else {
continue;
}

if (isEffectivelyFinal(fieldSymbol)) {
if (singletonField != null) {
return;
} else {
singletonField = varTree;
}
}
}

if (singletonField == null) return;
ClassTree singletonClass = classAndInstance.getKey();
VariableTree singletonField = classAndInstance.getValue();

var allConstructors = singletonClass.members().stream()
.filter(member -> member.is(Tree.Kind.CONSTRUCTOR))
Expand All @@ -110,19 +98,65 @@ private void visitClass(ClassTree classTree) {
if (singletonClass != classTree) {
flows.add(new JavaFileScannerContext.Location("Singleton helper", classTree.simpleName()));
}
allConstructors.forEach(constructor -> {
IdentifierTree methodName = allConstructors.get(0).simpleName();
flows.add(new JavaFileScannerContext.Location("Private constructor", methodName));
});
extractAssignments(singletonField).forEach(assignment -> flows.add(new JavaFileScannerContext.Location("Value assignment", assignment)));

reportIssue(singletonClass.simpleName(), "This looks like a singleton class", flows, null);
reportIssue(singletonClass.simpleName(), MESSAGE, flows, null);
}
}

@CheckForNull
private static Map.Entry<ClassTree, VariableTree> collectClassAndField(ClassTree classTree) {
ClassTree wrappingClass = null;
final var parent = classTree.parent();
if (parent != null && parent.is(Tree.Kind.CLASS)) {
wrappingClass = (ClassTree) parent;
}

List<VariableTree> staticFields = collectStaticFields(classTree, wrappingClass);
if (staticFields.size() != 1) return null;

var field = staticFields.get(0);

final var fieldSymbol = field.symbol();
ClassTree singletonClass = null;
if (fieldSymbol.type().equals(classTree.symbol().type())) {
singletonClass = classTree;
} else {
singletonClass = wrappingClass;
}

if (!isEffectivelyFinal(fieldSymbol)) return null;

return new AbstractMap.SimpleEntry<>(singletonClass, field);
}

private static List<VariableTree> collectStaticFields(ClassTree classTree, @Nullable ClassTree wrappingClass) {
Type type = classTree.symbol().type();
Type wrappingType = wrappingClass != null ? wrappingClass.symbol().type() : null;
return classTree.members().stream()
.filter(member -> member.is(Tree.Kind.VARIABLE) && ((VariableTree) member).symbol().isStatic())
.map(VariableTree.class::cast)
.filter(field -> {
Type fieldType = field.symbol().type();
return fieldType.equals(type) || (wrappingType != null && fieldType.equals(wrappingType));
}).collect(Collectors.toList());
}

private static boolean isEffectivelyFinal(Symbol symbol) {
return symbol.isFinal() ||
(symbol.isPrivate() && ExpressionsHelper.getSingleWriteUsage(symbol) != null);
}

private static boolean isInitializedWithParameterFreeConstructor(EnumConstantTree constant) {
return constant.initializer().methodSymbol().parameterTypes().isEmpty();
}

private static boolean hasNonPrivateInstanceMethodsOrFields(ClassTree classTree) {
return classTree.members().stream().anyMatch(member -> {
boolean isPrivateOrStatic = true;
if (member.is(Tree.Kind.METHOD)) {
var symbol = ((MethodTree) member).symbol();
return !symbol.isPrivate() && !symbol.isStatic();
Expand All @@ -134,4 +168,12 @@ private static boolean hasNonPrivateInstanceMethodsOrFields(ClassTree classTree)
}
});
}

private static List<AssignmentExpressionTree> extractAssignments(VariableTree variable) {
return variable.symbol().usages().stream()
.map(Tree::parent)
.filter(usage -> usage.is(Tree.Kind.ASSIGNMENT))
.map(AssignmentExpressionTree.class::cast)
.collect(Collectors.toList());
}
}
1 change: 0 additions & 1 deletion java-checks/src/test/java/org/sonar/java/checks/SingletonUsageCheckTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,4 @@ void test() {
.withCheck(new SingletonUsageCheck())
.verifyIssues();
}

}

0 comments on commit f28ba8c

Please sign in to comment.