False positive SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR with reused instances

[rovarga]

Singleton detection seems to be over-eager. A bit of nest-private hierarchy, like demonstrated in https://github.com/opendaylight/yangtools/blob/2e257731e354e6cc2fd45a6f6eec79bd01490c07/common/yang-common/src/main/java/org/opendaylight/yangtools/yang/common/CanonicalValueViolation.java triggers three singleton errors:

  [ERROR] Medium: Instance-getter method of class using singleton design pattern (org.opendaylight.yangtools.yang.common.CanonicalValueViolation) is not synchronized. [org.opendaylight.yangtools.yang.common.CanonicalValueViolation] At CanonicalValueViolation.java:[line 98] SING_SINGLETON_GETTER_NOT_SYNCHRONIZED
[ERROR] Medium: Class (org.opendaylight.yangtools.yang.common.CanonicalValueViolation) using singleton design pattern has non-private constructor. [org.opendaylight.yangtools.yang.common.CanonicalValueViolation] At CanonicalValueViolation.java:[line 32] SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR
[ERROR] Medium: Class (org.opendaylight.yangtools.yang.common.CanonicalValueViolation) using singleton design pattern directly or indirectly implements Serializable interface. [org.opendaylight.yangtools.yang.common.CanonicalValueViolation] At CanonicalValueViolation.java:[lines 32-158] SING_SINGLETON_IMPLEMENTS_SE

The problem is that CanonicalValueViolation is assumed to be a singleton, whereas it only uses two constants for common instantiateions.

[rovarga]

In this particular case, the fact that the class is NOT a singleton can easily be inferred:

1. the base class is abstract, but defines hashCode()/equals() methods which are final. The designer was clearly thinking about subclassing and defined the equality contract

2. the implementation of equals() performs instanceof checks (as opposed to .getClass() equality check), further reinforcing the fact the class is supposed to be subclassed

[xzel23]

What heuristics does SpotBugs use to detect singleton classes? After upgrading to the new release, I also get several warnings of this type for classes that are clearly not singletons. For example one class has several factory methods that each return new instances. And there are several package private constructors declared.

Also AFAIK synchronization is not always needed, i. e. if the instance is held in a static field that is unconditionally initialized either at declaration or in a static initializer block.

This rule needs some rework.

[JuditKnoll]

@rovarga Thank you for the example and your explanation. I proposed a PR fixing this FP.

@xzel23 Yes, you are right about the synchronization. It was a bug in the detector in SpotBugs, which was reported in #2932 and the PR fixing it was already merged to master, so it will be in the next release.
If you could share some (hopefully minimal) examples, that could help identifying the underlying issues.

[xzel23]

@JuditKnoll Here is an example. Even if you say the error reported by spotbugs is already fixed, the important thing is: this simply is not a singleton class, so none of the singleton rules should apply. This class provides public factory methods to create new instances, this should be detected and the class not be marked as singleton:

package issues.spotbugs;

public class A {

    private static final A INSTANCE = new A("1");

    private final String text;

    private A(String text) {
        this.text = text;
    }

    public static A instance1() {
        return INSTANCE;
    }

    public static A create(String text) {
        return new A("text");
    }

    public String getText() {
        return text;
    }
}

[JuditKnoll]

@xzel23 Thank you for your example. The #2951 modifies the logic for identifying Singletons as well, which was indeed not strict enough. Your example (named NotSingletonWithFactoryMethod in the PR) isn't be considered Singleton in it.

[xzel23]

@JuditKnoll Thank you for your fixes. This works in most cases and I was able to remove the exclusions in most projects. The problem persists with classes that have implement an SPI service (because a public no args constructor must be present). But I will change this so that these classes can be real singletons and SPI loads a provider that simply provides the singleton instance. That's the correct way to do this anyway.

[JuditKnoll]

Thank you @xzel23 for checking it and getting back about this 😄