[JAVA] S2157 Add a "clone()" method to this class - False Positive - zero new fields and clone method is defined in parent class

[Adam Gabryś]

Hi,

We have a parent class which is defined in a 3rd party library. It defines a "clone" method but does not implement the "Cloneable" interface.

Next, we have our class which extends that class and implements "Cloneable". We don't introduce any new fields, so "clone" method defined in parent class provides all required logic.

Unfortunately, SonarQube reported an issue that our class requires "clone" method. If we add "clone" method which delegates all logic to the parent, then we hit another problem: S1185 - Overriding methods should do more than simply call the same method in the super class.

Code:

Environment:

• SonarQube 5.6.7
• SonarJava 4.15.0.12310

Example class:

https://github.com/agabrys/sonarqube-falsepositives/blob/master/src/main/java/biz/gabrys/agabrys/sonarqube/falsepositives/d20171218/S2157.java

Project:

https://github.com/agabrys/sonarqube-falsepositives

Build:

mvn clean package sonar

Best Regards,

Adam Gabryś

[Michael Gumowski]

Hello Adam,

I'm not quite sure that it's a FP here. I think that having a forced implementation of the clone() method as soon as you introduce the Clonable interface is not a bad thing. Now, in case where there is no new field introduced, then indeed calling super.clone() should do the job, as per definition from the clone() method in Object. We therefore could change implementation of rule RSPEC-1185 to authorize such use case, but to me it is not enough. From java8 javadoc Object.clone() method, we can read that :

Creates and returns a copy of this object. The precise meaning of "copy" may depend on the class of the object. The general intent is that, for any object x, the expression:

 x.clone() != x

will be true, and that the expression:

 x.clone().getClass() == x.getClass()

will be true, but these are not absolute requirements. While it is typically the case that:

 x.clone().equals(x)

will be true, this is not an absolute requirement.

Which is rather blurry... I particularly don't like the fact that you won't have the same instance on the other of your object after calling the subclass clone() method, and so doing "more" than just calling super.clone() sounds always a better option to me.

At worst, I would prefer to add the override of the clone method, and choose "Won't Fix" on the issue risen for S1185.

What do you think?

Cheers,

Michael

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/DB6PR0801MB12879C4910281B60A4191616980E0%40DB6PR0801MB1287.eurprd08.prod.outlook.com.
For more options, visit https://groups.google.com/d/optout.

--

Michael Gumowski | SonarSource

Software Developer, Language Team
https://www.sonarsource.com

[Adam Gabryś]

Hello Michael,
I agree with the opinion that forced the implementation is not a bad thing.

I saw that some people change a return value type to not force users of the clone method to casting, e.g.:

public class Foo implements Cloneable {
    // ...
    @Override
    public Foo clone() {
        try {
            return (Foo) super.clone();
        } catch (CloneNotSupportedException e) {
            // never
            throw new IllegalStateException(e);
        }
    }
}

But it is not very popular, so it would be great if you could extends the rule specification (RSPEC-1185). I don't like to mark issues as Won't Fix ;-)

Cheers,
Adam Gabryś