Limitierungen statischer Codeanalyse?

[Zrebna]

H!

Mittlerweile kann ich endlich selber vermehrt Dinge in SonarQube ausprobieren und bin manchmal schon überrascht.
Es biete umfassende Dashboards und alles ist super intuitiv, aber Code Smell Hints meiner Meinung nach schon oft irrelevant und es gibt auch viele "false Positives".

Es gibt z.B. den Hint der vorschlägt statt einem try-catch das try-with-ressources zu nutzen, damit eine Ressource auch wirklich geschlossen wird. Nur den try-catch zu nutzen führt aber nur zu Problemen, wenn man vergisst die Resource auch zu schließen.
Klar, das kann schnell passieren und dann ist so ein Hint auch gerechtfertigt, aber es wäre cool, wenn SonarQube auch einbeziehen würde, ob nun die Ressource geschlossen wurde oder nicht. Und je nach Fall einen anderen Hint mit unterschiedlicher 'Severity'.
In Wirklichkeit gibt es aber in beiden Fällen den selben Hint.
Das bedeutet, dass statische Codeanalyse "nur" auf unterster Ebene stattfindet, und zwar dem Quellcode. Und hier noch nicht mal "allumfassend", sondern Zeile für Zeile basierend auf Mustererkennung. Also zum Beispiel wird nicht ein ganzer Block Code betrachtet und analysiert sowie letztlich korrekt verstanden und interpretiert, sondern wirklich nur "stumpf" Zeile für Zeile und es geht "rot" an, wenn eine bestehende Sonar-Regel verletzt ist.

Sehe ich das so richtig?

Den Thread bitte nicht falsch verstehen. Ich finde statische Codeanalyse super und bin ein jetzt schon ein Fan. Jedoch möchte ich auch bewusst die Grenzen und Limitierungen erfassen bzw. verstehen.

Seht ihr in der Praxis noch weitere Limitierungen, Grenzen oder Schwachstellen statischer Codeanalyse?

Lg
Zrebna

 

[KonradN]

Grob gesehen hast Du Recht. Aber es werden durchaus mehr als eine Zeile analysiert aber ganz klar: Es geht um Muster die halt in Regeln definiert wurden.

Daher ist es natürlich ganz klar stark limitiert aber es führt meiner Meinung nach dazu, dass Du Code schreibst, der deutlich weniger fehleranfällig ist. Und da geht um teilweise Kleinigkeiten ... Unter dem Strich ist es eine Erweiterung der Coding Guidelines, die man für ein Team festlegt.

 

[Oneixee5]

Die SonarQube-Analyse bezieht Bytecode mit ein. Falls dieser nicht vorhanden ist wird eine entsprechende Fehlermeldung ausgegeben.
Dein Beispiel: try-catch -> try-with-ressources würde ich selbst auch "anmeckern". try-catch ist kein Fehler aber try-with-ressources ist lesbarer und besser zu erfassen. Schlecht lesbarer Code ist auf jeden Fall ein Problem - wenn auch meistens kein entscheidendes. Programmcode ist aber für Menschen deshalb muss er gut lesbar sein, Maschinen brauchen den nicht. Ein schlecht geschriebenes Buch würdest du auch schlecht bewerten. Das Gleiche passiert hier.

 

[Zrebna]

Ja, das stimmt absolut!
Vor allem auch, weil try-with-resources grantiert, dass eine Ressource geschlossen wird. Wenn ich eine Ressource vor dem try-Block deklariere und dann z.B. in einem finally-Block erst schließe, kann ja vor dem finally-Block was "schief gehen", sodass die Ressource evtl. nicht garantiert geschlossen wird.
Der andere wesentliche Punkt von dir stimmt ganz klar auch und nicht umsonst bin ich mittlerweile ein Fan von 'Uncle Bob' und will lernen gut leserlichen Code zu schreiben und mich in Sachen "Clean Code" verbessern.

Ich möchte mal ein konkretes Beispiel von einem Sonar-Hint zeigen - dazu füge ich ein Bild ein, dass den Warnhinweis zeigt:



Dieses Warning ging letztlich aufgrund dieses Codes an (ich habe die Bezeichnungen geändert, um nicht öffentlich Unternehmenscode zu posten und der Code macht da inhaltlich auch keinen Sinn, aber das ist nicht der Kernpunkt) :

@Override
public User clone() {
        User clone = new User();
        clone.setFirstName(this.getFirstName());
        clone.setAgeGroup(this.getAgeGroup());
    
        // Do something with clone-object
}

In dem Bild scheint das Warning bzgl. den blau-unterstrichenen Stellen davon auszugehen, dass das obige "clone"-Objekt quasi keinen eigenen State hat. Das heißt, dass SonarQube hier nicht realisiert hat, dass in diesem Fall das "clone"-Objekt nicht wirklich ein Klon ist, sondern eine eigene Instanz, die mittels Konstruktor erzeugt worden ist.
Das bedeutet, dass wenn ich im this-Objekt z.B. den Vornamen von Peter auch Michael ändere, der Vorname im "clone"-Objekt immer noch Peter bleibt - also eigener State.
Soweit alles korrekt?


Jetzt ist das sicherlich keine gute Praxis in einer überschriebenen 'clone'-Methode eine Implementierung zu verwenden, die nichts mit einem Klonen zu tun hat, sondern ein eigenes und unabhängiges Objekt erzeugt.
Daher kann man aus dieser Sichtweise das Warning klar verstehen.
Jedoch ist das Warning doch sehr generisch und scheint gemäß dieser Regel immer dann anzugehen, wenn die clone-Methode überschrieben wird, auch wenn die dortige Implementierung sicher ist und keinerlei Probleme verursachen wird (siehe blaue unterstrichene Stelle).

Welche Art von "Limitierungen" kann man daraus ableiten bzw. was stellt das Beispiel für euch da?
- Ist das bereits eine Art von "false Positive" ?
Hier dazu die offizielle Definition von Sonar:
Ein falsch positives Ergebnis liegt vor, wenn unerwartet ein Problem bei Code auftritt, der kein Problem auslösen sollte,
oder wenn die vorgeschlagene Aktion für den Code keinen Sinn ergibt.

Gemäß dieser Definition würde ich schon sagen, dass es sich um eine Art "false Psoitive" handelt, weil das Warning (in der Übersetzung sind wohl mit 'Problem', Warnhinweise gemeint) an einer Stelle auftretet, an der aufgrund der scheinbar nicht berücksichtigten (?) Implementierung der überschriebenen clone-Methode, das in dem Warnhinweis hervorgehobene Problem gar nicht auftreten kann.
Was denkt ihr?

- Falls es für euch kein false Postive ist, was ist es dann?
Also ideal ist es ja definitiv nicht, wenn ein Warnhinweis aufkommt, der ein mögliches Problem anspricht, dass aufgrund der eigentlichen Implementierung gar nicht auftreten kann - letztlich wurde ja die komplette Situation durch SonarQube an dieser Stelle nicht vollständig erfasst, denn die eigentliche Implementierung der überschriebenen clone-Methode (was direkt immer zur Warnung führt) wurde an dieser Stelle nicht berücksichtigt.

 

[KonradN]

Wenn ich eine Ressource vor dem try-Block deklariere und dann z.B. in einem finally-Block erst schließe, kann ja vor dem finally-Block was "schief gehen", sodass die Ressource evtl. nicht garantiert geschlossen wird.

Nein, wenn Du das Pattern richtig anwendest, dann ist das Schließen garantiert! Das Pattern besagt dabei:

MyClosableType closableType;
try {
    closableType = .....; // Initialisierung z.B. new MyClosableType()
    // Whatever
} finally {
    if (closableType != null) {
        try {
            closableType.close();
        } catch (Exception ex) {}
    }
}

Das finally wird ja immer garantiert. Das wird auch ausgeführt, wenn eine Exception geworfen wird. Bitte führe Dir das vor Augen - also z.B. folgender Code:

public class FinallyTest {
    public static void main(String[] args) {
        try {
            throw new RuntimeException("Exit here ...");
        } finally {
            System.out.println("finally block!");
        }
    }
}

Und da siehst Du: Der finally Block wird ausgeführt, wenn der try Block verlassen wird (hier durch die RuntimeException, hätte auch ein return sein können oder so).

Ausgabe:

finally block!
Exception in thread "main" java.lang.RuntimeException: Exit here ...
    at FinallyTest.main(FinallyTest.java:4)

 

[Oneixee5]

Wenn ein Object geklont wird, dann ist der Klone eine eigene Instanz. Es gibt verschiedene Möglichkeiten Klone zu erzeugen. Die Verwendung des Interfaces Cloneable und das Überschreiben der Mehtode #clone() ist nicht der einzige Weg. Eine mögliche Varianten wäre z.B. ein Copy-Konstruktor, Copy-Factory oder man kann ein Object auch serialisieren und dann später wieder deserialisieren. Dabei entstehen ebenfalls Klone. Immer sind das eigene Instanzen.
Referenzen sind ein anderes Thema und haben mit Klonen nichts zu tun.

Konkret zu dem Fehler: "clone should be not overridden" - das finde ich sehr hilfreich und ist ein Bug in deinem Programm - ohne jetzt die Beschreibung nachzuschlagen. Irgendwo wird von User geerbt - BlindUser. Dabei wird eine neue Methode eingeführt - Was wäre denn jetzt das Ergebnis deiner clone-Methode? Ein Object vom Type User, obwohl du BlindUser geklont hast! Das ist ein Bug und ich finde sogar ein schwerwiegender. Denn ein Cast von deinem Klone zu BlindUser würde einen Fehler verursachen.

 

[KonradN]

Nun noch etwas zu den anderen Punkten:

In dem Bild scheint das Warning bzgl. den blau-unterstrichenen Stellen davon auszugehen, dass das obige "clone"-Objekt quasi keinen eigenen State hat. Das heißt, dass SonarQube hier nicht realisiert hat, dass in diesem Fall das "clone"-Objekt nicht wirklich ein Klon ist, sondern eine eigene Instanz, die mittels Konstruktor erzeugt worden ist.

Auch bei Clone hast Du immer eine eigene / neue Instanz. Das Problem bei Deinem Code ist aber einfach, dass Du ein massives Problem mit Vererbung hast. Wer sagt denn, dass das Objekt, bei dem Clone aufgerufen wurde, vom Typ User war? Evtl. gibt es ja Employee das von User erbt. Und dann willst Du natürlich, dass ein clone() Aufruf darauf auch funktioniert. Und das geht da nicht. Daher ist das natürlich kein false positive sondern ein ernsthaftes Problem.

Jetzt ist das sicherlich keine gute Praxis in einer überschriebenen 'clone'-Methode eine Implementierung zu verwenden, die nichts mit einem Klonen zu tun hat, sondern ein eigenes und unabhängiges Objekt erzeugt.

Es ist nicht nur keine gute Praxis sondern es ist schlicht falsch. Man mag jetzt hier über das "falsch" diskutieren wollen, aber es gibt grundlegende Regeln bei objektorientierten Sprachen, die über die reinen Regeln der Sprache hinaus gehen. SOLID Principles z.B. - und dagegen zu verstoßen ist ein nicht unerhebliches Problem daher greife ich zu der Aussage: Das ist falsch!

Wenn Du etwas ableitest, dann hast Du den Contract einzuhalten! Das ist das L von SOLID: Liskov Substitution Principle. Dein User erbt von Object und daher hat dein clone() den Contract von Object.clone() zu erfüllen. Und das ist nicht der Fall, denn bei abgeleiteten Klassen gilt eben nicht mehr x.clone().getClass() == x.getClass().
Und der Text sagt es auch direkt:

By convention, the returned object should be obtained by calling super.clone. If a class and all of its superclasses (except Object) obey this convention, it will be the case that x.clone().getClass() == x.getClass().

Das ist aber nicht das Problem, das Sonar damit hat. Sonar wendet sich direkt gegen das clone(), da es diesbezüglich sehr viele Fehler gab. Sowohl bei Sun als auch außerhalb von Sun. Darüber hinaus ist das Kernproblem aber, dass Du kein Deep Copy hast. Beispiel: Du hast eine Klasse User. Diese Klasse User hält eine Instanz der Klasse Adress. Nun erstellst Du ein clone von einem user. Jetzt haben beide user Instanzen eine Referenz auf die gleiche Adress Instanz. Wenn Du nun also die Adress von dem einen user Objekt verändert, dann hast sich auch die Adress des zweiten User Objekts geändert (Sie teilen sich ja eine Instanz).

Und aus genau diesem Grund ist das Best Practice, dass man kein clone nutzt sondern statt dessen Factory Methoden (oder einen Factory Konstruktor) erstellt. Damit hast Du die Freiheit, es so zu implementieren, wie Du es möchtest ohne dass er Erwartungen bezüglich des Resultats gibt.

 

[Zrebna]

Nein, wenn Du das Pattern richtig anwendest, dann ist das Schließen garantiert! Das Pattern besagt dabei:

MyClosableType closableType;
try {
    closableType = .....; // Initialisierung z.B. new MyClosableType()
    // Whatever
} finally {
    if (closableType != null) {
        try {
            closableType.close();
        } catch (Exception ex) {}
    }
}

Das finally wird ja immer garantiert. Das wird auch ausgeführt, wenn eine Exception geworfen wird. Bitte führe Dir das vor Augen - also z.B. folgender Code:

public class FinallyTest {
    public static void main(String[] args) {
        try {
            throw new RuntimeException("Exit here ...");
        } finally {
            System.out.println("finally block!");
        }
    }
}

Und da siehst Du: Der finally Block wird ausgeführt, wenn der try Block verlassen wird (hier durch die RuntimeException, hätte auch ein return sein können oder so).

Ausgabe:

finally block!
Exception in thread "main" java.lang.RuntimeException: Exit here ...
    at FinallyTest.main(FinallyTest.java:4)

Danke, das hat sehr geholfen!

Das heißt, dass auf funktionaler Ebene beide Ansätze gleichwertig sind.
Der try-with-resource-Ansatz verbessert jedoch die Lesbarkeit und könnte den Vorteil haben, dass ein Entwickler nicht vergessen kann am Ende eine Resource zu schließen.

 

[KonradN]

Das heißt, dass auf funktionaler Ebene beide Ansätze gleichwertig sind.
Der try-with-resource-Ansatz verbessert jedoch die Lesbarkeit

Ganz genau.

 

[Zrebna]

Nun noch etwas zu den anderen Punkten:

Auch bei Clone hast Du immer eine eigene / neue Instanz. Das Problem bei Deinem Code ist aber einfach, dass Du ein massives Problem mit Vererbung hast. Wer sagt denn, dass das Objekt, bei dem Clone aufgerufen wurde, vom Typ User war? Evtl. gibt es ja Employee das von User erbt. Und dann willst Du natürlich, dass ein clone() Aufruf darauf auch funktioniert. Und das geht da nicht. Daher ist das natürlich kein false positive sondern ein ernsthaftes Problem.

Ich glaube bis hierhin verstehe ich das Problem.
Also z.B., wenn 'PremiumUser extens User' und dann ein Objekt von 'PremiumUser' diese 'clone'-Methode in meinem Beispiel aufruft, dann will er ein Objekt von 'PremiumUser' - also müsst er statt

User clone = new User();

folgendes machen:

PremiumUser clone = new PremiumUser();

bzw. müsste man halt auf die analoge Weise 'clone' überschreiben - wenn man das tut, dann funtional kein Problem?
Aber das Problem liegt darin, dass man das schnell mal vergessen kann - ist das dein Grundgedanke?

Es ist nicht nur keine gute Praxis sondern es ist schlicht falsch. Man mag jetzt hier über das "falsch" diskutieren wollen, aber es gibt grundlegende Regeln bei objektorientierten Sprachen, die über die reinen Regeln der Sprache hinaus gehen. SOLID Principles z.B. - und dagegen zu verstoßen ist ein nicht unerhebliches Problem daher greife ich zu der Aussage: Das ist falsch!

Wenn Du etwas ableitest, dann hast Du den Contract einzuhalten! Das ist das L von SOLID: Liskov Substitution Principle. Dein User erbt von Object und daher hat dein clone() den Contract von Object.clone() zu erfüllen. Und das ist nicht der Fall, denn bei abgeleiteten Klassen gilt eben nicht mehr x.clone().getClass() == x.getClass().
Und der Text sagt es auch direkt:


Das ist aber nicht das Problem, das Sonar damit hat. Sonar wendet sich direkt gegen das clone(), da es diesbezüglich sehr viele Fehler gab. Sowohl bei Sun als auch außerhalb von Sun. Darüber hinaus ist das Kernproblem aber, dass Du kein Deep Copy hast. Beispiel: Du hast eine Klasse User. Diese Klasse User hält eine Instanz der Klasse Adress. Nun erstellst Du ein clone von einem user. Jetzt haben beide user Instanzen eine Referenz auf die gleiche Adress Instanz. Wenn Du nun also die Adress von dem einen user Objekt verändert, dann hast sich auch die Adress des zweiten User Objekts geändert (Sie teilen sich ja eine Instanz).

In meinem Beispiel haben aber die Instanzen 'clone' und 'this' einen uabhängigen State?
Denn 'clone' wurde ja mittels Konstrutkor erzeugt und ist somit eine eigne Instanz mit eigener Speicherzelle.
Macht man sowas wie

clone.setAge(this.getAge())   // age = 32

dann haben beide den selben Wert '32' aber die Werte sind in unterscheidlichen Speicherzellen platziert.
Also wenn man nun den Wert bei 'clone' ändert, dann zeigt er auf die entsprechende Speicherzelle und der dort enthaltene Wert wird verändert. Der Wert bei 'this' bleibt unberührt?

Und aus genau diesem Grund ist das Best Practice, dass man kein clone nutzt sondern statt dessen Factory Methoden (oder einen Factory Konstruktor) erstellt. Damit hast Du die Freiheit, es so zu implementieren, wie Du es möchtest ohne dass er Erwartungen bezüglich des Resultats gibt.

 

[KonradN]

Ok, dann doch einmal etwas ausführlicher, da es nun doch notwendig ist.

Implementierung clone()

Wenn eine Klasse Cloneable implementiert, dann ist das für Object.clone() das Zeichen, dass es ok ist, alle Werte einfach 1:1 zu kopieren.
==> Du musst also nichts von Hand kopieren!

Es reicht also
a) implements Cloneable auf der Klasse
b) die Methode public machen:

    @Override
    public Object clone() throws CloneNotSupportedException {
        return super.clone();
    }

Das war es schon!

Siehe dazu:
Cloneable (Java SE 17 & JDK 17) (oracle.com)

A class implements the Cloneable interface to indicate to the Object.clone() method that it is legal for that method to make a field-for-field copy of instances of that class.
Invoking Object's clone method on an instance that does not implement the Cloneable interface results in the exception CloneNotSupportedException being thrown.

Contract clone()

Object (Java SE 17 & JDK 17) (oracle.com)

By convention, the object returned by this method should be independent of this object (which is being cloned). To achieve this independence, it may be necessary to modify one or more fields of the object returned by super.clone before returning it. Typically, this means copying any mutable objects that comprise the internal "deep structure" of the object being cloned and replacing the references to these objects with references to the copies. If a class contains only primitive fields or references to immutable objects, then it is usually the case that no fields in the object returned by super.clone need to be modified.

Das bedeutet, dass Du mehr machen musst, wenn Du Referenzen zu veränderbaren Objekten hast. Dann musst Du auch für diese ein Clone erzeugen. Also wäre das dann etwas wie:

    @Override
    public Object clone() throws CloneNotSupportedException {
        MyClass result = (MyClass) super.clone();
        result.element = (TypeOfElement) result.element.clone();
        return result;
    }

Mit MyClass der aktuellen Klasse in der diese clone Methode ist und TypeOfElement einer Klasse mit veränderlichem State und element eine Instanzvariable von diesem Typ. TypeOfElement muss natürlich auch Cloneable sein, da sonst clone() protected wäre und Object.clone() die CloneNotSupportedException werfen würde.

Warum clone() vermeiden?

Jetzt ist die genaue Frage: Was ist daran schlecht?

a) Semantische Unsicherheit - Wird wirklich ein Deep Copy erzeugt? Der Contract fordert dies aber das ist leider oft eben explizit nicht der Fall (einfach weil viele Typen nicht Cloneable sind!) In dem Zusammenhang sind dann auch die Bemerkungen bezüglich Brechen der Verträge...

b) Wartbarkeit - clone() zu warten kann umfangreich werden. Es erzwingt oft, dass auch andere Klassen Cloneable werden. Und jede Klasse, die von einer Cloneable Klasse erbt, wird damit auch Cloneable. Wenn man das nicht will? Dann wirft man in clone() die Exception? Aber das kann man auch nicht abfragen oder so. Du führst einen Contract ein, der damit unter dem Strich die weitere Verwendung einschränkt.

Das sind ein paar einfache Punkte und da es einfach Alternativen gibt, greift man lieber auf diese zurück.

Ein Artikel, der das auch etwas erläutert:
Java Practices->Avoid clone

 

[LimDul]

Echte False Positives von einem sauber eingestellten Sonar sind relativ selten, die größeren Probleme sind eher, dass man sich entscheiden muss, wie viel Code Smell ok ist. Probleme, die wir z.B. im Projekt hatten - wir haben einiges an generierten Code. Da meckert Sonar Dinge im generierten Code an, die wir schlicht nicht verändern können.

Das andere sind halt Code Smells, wo die Behebung relativ aufwendig ist und man daher lieber mit dem Code Smell lebt. Deswegen ist meines Erachtens neben den aufgezählten technischen Limitierungen die größte Limitierung der Faktor Mensch & Zeit.

 

[Zrebna]

Ok, dann doch einmal etwas ausführlicher, da es nun doch notwendig ist.

Implementierung clone()

Wenn eine Klasse Cloneable implementiert, dann ist das für Object.clone() das Zeichen, dass es ok ist, alle Werte einfach 1:1 zu kopieren.
==> Du musst also nichts von Hand kopieren!

Es reicht also
a) implements Cloneable auf der Klasse
b) die Methode public machen:

    @Override
    public Object clone() throws CloneNotSupportedException {
        return super.clone();
    }

Das war es schon!

Siehe dazu:
Cloneable (Java SE 17 & JDK 17) (oracle.com)


Contract clone()

Object (Java SE 17 & JDK 17) (oracle.com)


Das bedeutet, dass Du mehr machen musst, wenn Du Referenzen zu veränderbaren Objekten hast. Dann musst Du auch für diese ein Clone erzeugen. Also wäre das dann etwas wie:

    @Override
    public Object clone() throws CloneNotSupportedException {
        MyClass result = (MyClass) super.clone();
        result.element = (TypeOfElement) result.element.clone();
        return result;
    }

Mit MyClass der aktuellen Klasse in der diese clone Methode ist und TypeOfElement einer Klasse mit veränderlichem State und element eine Instanzvariable von diesem Typ. TypeOfElement muss natürlich auch Cloneable sein, da sonst clone() protected wäre und Object.clone() die CloneNotSupportedException werfen würde.

Warum clone() vermeiden?

Jetzt ist die genaue Frage: Was ist daran schlecht?

a) Semantische Unsicherheit - Wird wirklich ein Deep Copy erzeugt? Der Contract fordert dies aber das ist leider oft eben explizit nicht der Fall (einfach weil viele Typen nicht Cloneable sind!) In dem Zusammenhang sind dann auch die Bemerkungen bezüglich Brechen der Verträge...

b) Wartbarkeit - clone() zu warten kann umfangreich werden. Es erzwingt oft, dass auch andere Klassen Cloneable werden. Und jede Klasse, die von einer Cloneable Klasse erbt, wird damit auch Cloneable. Wenn man das nicht will? Dann wirft man in clone() die Exception? Aber das kann man auch nicht abfragen oder so. Du führst einen Contract ein, der damit unter dem Strich die weitere Verwendung einschränkt.

Das sind ein paar einfache Punkte und da es einfach Alternativen gibt, greift man lieber auf diese zurück.

Ein Artikel, der das auch etwas erläutert:
Java Practices->Avoid clone

Also hier verstehe ich die Problematik schon und vor allem auch den Aufwand und was alles schief gehen kann (Attribut eines komplexen Datentyps muss ebenfalls das Cloneable-IF implementieren), aber das ist ja nicht so, wie die Implementierung, die ich oben gepostet habe.

Ich vermute mal, dass die von mir gepostete Implementierung die clone-Methode in dem Sinne zweckentfremdet nutzt, als dass sie diese eigentlich gar nicht nutzt, sondern einfach mittels Konstruktor eine neue und unabhängige (?) Instanz erzeugt und dann Werte setzt...

Ich denke immer noch, dass die Instanz dort unabhängig ist (?), siehe meine Ausführung am Ende meines letzten Posts.

 

[KonradN]

Also hier verstehe ich die Problematik schon und vor allem auch den Aufwand und was alles schief gehen kann (Attribut eines komplexen Datentyps muss ebenfalls das Cloneable-IF implementieren), aber das ist ja nicht so, wie die Implementierung, die ich oben gepostet habe.

Wenn Du die Meldung durchliest, dann siehst Du, dass nicht die konkrete Implementation bewertet wurde sondern generell die Nutzung von clone. Das ist in dem Bildschirmfoto doch ganz genau zu erkennen. Schon die Überschrift: "clone should not be overriden" ist da doch sehr deutlich.

Also alles, worauf da sonarcube reagiert hat, ist die Signatur der Methode.

 

[Zrebna]

Wenn Du die Meldung durchliest, dann siehst Du, dass nicht die konkrete Implementation bewertet wurde sondern generell die Nutzung von clone. Das ist in dem Bildschirmfoto doch ganz genau zu erkennen. Schon die Überschrift: "clone should not be overriden" ist da doch sehr deutlich.

Also alles, worauf da sonarcube reagiert hat, ist die Signatur der Methode.

Genau, das ist der Punkt.

SonarQube reagiert sofort mit diesem generischen Warning, sobald man eine clone-Methode überschreibt und berücksichtigt nicht, dass diese meiner Meinung nach zweckentfremdet wurde (?) und dort (also in der Implementierung) gar nichts "geklont" wird, sondern eine unabhängige Instanz erzeugt wird. Die ist ja unabhängig mit einem eigenen Zustand (ändere ich was in der "geklonten" bzw. eigentlich einfach nur neu erzeugten Instanz 'clone', hat das keinerlei Auswirkung auf this this-Instanz), oder sehe ich das falsch (siehe letzter Abschnitt vorletzter Post)?

Deswegen trifft das blau Unterstrichene in dem gepostetem Bild (Gefahr, dass beide Objekte sich selben Zustand teilen: Ändere ich in einem den Wert eines Attributs, dann ist er auch im anderem geändert...keine Unabhängigkeit) in diesem Fall doch gar nicht zu, oder?

Ich will da das generische Warning von Sonar, dass standardmäßig "stumpf" losgeht, sobald es den Code scannt und liest, dass eine clone-Methode überschrieben wird, ohne die dortige Implementierung zu berücksichtigen, nicht kritisieren. Das hat sicher andere gute Gründe, dass man diese clone-Methode nicht zweckentfremden soll.
Aber in dem Warnhinweis stehen halt trotzdem auch Gefahren drin, die halt hier meiner Ansicht nach nicht zutreffen.
Und ich weiß nicht, wie man das benennen kann, denn ein false Postive ist es nicht, aber trotzdem trifft das Warning nicht den Kern.

Intuitiv ist das schon etwas, was da sehr oberflächlich ist und in einer Art und Weise für die mir die Worte fehlen nicht optimal:
Es geht stumpf nach Trigger X ein generisches Warning an, dass Inhlate undd Hinweise enthält, die an der Stelle gar nicht relevant sind, da der gesamte Kontext (hier die zweckentfremdete Implementierung von der clone-Methode - man sollte diese hier logischerweise gar nicht verwenden, um ein neues Objekt zu erzeugen und diese Methode da einfach anders benennen, ka z.B. 'createInstanceOutOfOriginal(<T> original)), nicht mit in Betracht gezogen wird.

Hypothetisch, von einer Super-SonarAI würde hier eben ein anderes Warning losgehen, wie etwas 'Don't misuse the clone method' oder sowas in der Art. Hoffe, dass nun klarer ist, worum es mir geht.

 

[Oneixee5]

Du musst nicht einsehen das du falsch liegst. Es bleibt trotzdem einfach ein Fakt, dass #clone() nicht überschrieben werden soll. Eine Zweckentfremdung der Methode macht den Sachverhalt nicht besser und ist ein zweiter Bug, welcher von SonarCube nicht erkannt wurde. Es würde auch niemand, der deinen Code liest davon ausgehen, dass du gar nicht #clone() meinst, es aber trotzdem missbrauchst. So denkt kein Programmierer und ein Automat schon gar nicht. Jeder der diesen Code liest geht von einem Fehler aus.

 

[KonradN]

SonarQube reagiert sofort mit diesem generischen Warning, sobald man eine clone-Methode überschreibt und berücksichtigt nicht, dass diese meiner Meinung nach zweckentfremdet wurde (?)

Und genau das darf doch nicht sein! Verträge müssen eingehalten werden! Damit verhält sich Deine Klasse nicht so wie Object. Und damit hast Du einen klaren Verstoß gegen das Liskov Substitution Principle.

Das ist etwas, das da bei Sonarcube nicht erwartet wird. Da wird davon ausgegangen, dass ein Entwickler sich schon an die SOLID Principles halten wird. Du beschwerst Dich hier unter dem Strich, dass Du nur eine Code Smell Warnung bekommst statt direkt öffentlich ausgepeitscht zu werden (oder so ähnlich) ...

Bei so Code Analysen werden Hinweise vergeben, dass Stellen analysiert werden müssen. Und unter der Prämisse ist es egal, was in clone ist. Jede clone Methode wird angemeckert mit dem (berechtigten) Hinweis. Tiefere Analysen müssen nicht sein.

Das ist also wie ein Hinweis auf ein Verbrechen. Da reicht ein einfacher Hinweis ("Ich glaube da ist ein Verbrechen geschehen"). Dann startet die Strafverfolgung und dann kann im Rahmen dieser festgestellt werden, wogegen alles verstoßen wurde. Du kannst sagen: "Da ist aus Deiner Sicht ein Mord geschehen" und dann wird das analysiert und nein, es war kein Mord. Es war ein Totschlag mit Raub und Hausfriedensbruch und und und ...

Diese Details sind aber egal - es geht bei dem Hinweis ja nur darum, dass da ein Prozess gestartet wird. Und das sehe ich hier ebenso. Und bei Dir kommt heraus: Sonarcube meckert einen Hausfriedensbruch an und dann stellt sich heraus: Du hast den Liskov ermordet

Edit: Typo / Ausdruck verbessert.

 

[Zrebna]

Und genau das darf doch nicht sein! Verträge müssen eingehalten werden! Damit verhält sich Deine Klasse nicht so wie Object. Und damit hast Du einen klaren Verstoß gegen das Liskov Substitution Principle.

Ja, das passt schon und hat auf jedec Fall seinen Sinn.

Das ist etwas, das da bei Sonarcube nicht erwartet wird. Da wird davon ausgegangen, dass ein Entwickler sich schon an die SOLID Principles halten wird. Du beschwerst Dich hier unter dem Strich, dass Du nur eine Code Smell Warnung bekommst statt direkt öffentlich ausgepeitscht zu werden (oder so ähnlich) ...

Bei so Code Analysen werden Hinweise vergeben, dass Stellen analysiert werden müssen. Und unter der Prämisse ist es egal, was in clone ist. Jede clone Methode wird angemeckert mit dem (berechtigten) Hinweis. Tiefere Analysen müssen nicht sein.

Passt auch, versteh ich - die clone() sollte da überhaupt nicht verwendet werden.

Aber nochmal: Zumindest folgender Punkt bleibt meiner Meinung nach:
Die Implementierung wird berücksichtigt und deswegen passen die Hinweise in der Hint-Beschreibung bzgl. was da passieren kann in diesem Fall nicht 100%. Sofern ich recht habe, und das dort erzeuge "clone-Objekt" völlig unabhängig vom this-Objekt ist. Das habe ich ja oben jetzt schon paar mal dargelegt, incl. Beispiel im vorletztem Post von mir.

Wenn SonarQube ein "perfektes Orakel" wäre, dann könnte ja statt dem erhaltenen Warn-Hinweis eine anderer kommen, der die Implementierung berücksichtigt, der dann etwas so ginge:
"Don't override and misue clone(). Rename method to better describe the implementation". Natürlich besser formuliert.

Das ist also wie ein Hinweis auf ein Verbrechen. Da reicht ein einfacher Hinweis ("Ich glaube da ist ein Verbrechen geschehen"). Dann startet die Strafverfolgung und dann kann im Rahmen dieser festgestellt werden, wogegen alles verstoßen wurde. Du kannst sagen: "Da ist aus Deiner Sicht ein Mord geschehen" und dann wird das analysiert und nein, es war kein Mord. Es war ein Totschlag mit Raub und Hausfriedensbruch und und und ...

Ja, das geht mir ein. Aus einer Perspektive ist genau das keine Begrenzung von solchen Tools. Aber es ist auch denkbar, dass sich Jemand auf die Seite stellt und sagt: "Ich hätte lieber Hinweise die mehr Kontext des Quellcodes berücksichtigen, also mehr und differenziertere Warnings, damit ich weniger Aufwand habe herauszufinden gegen was konkret hier verstoßen wurde und gegen was nicht." So ungefähr...

Diese Details sind aber egal - es geht bei dem Hinweis ja nur darum, dass da ein Prozess gestartet wird. Und das sehe ich hier ebenso. Und bei Dir kommt heraus: Sonarcube meckert einen Hausfriedensbruch an und dann stellt sich heraus: Du hast den Liskov ermordet

Edit: Typo / Ausdruck verbessert.

 

[Zrebna]

Du musst nicht einsehen das du falsch liegst. Es bleibt trotzdem einfach ein Fakt, dass #clone() nicht überschrieben werden soll. Eine Zweckentfremdung der Methode macht den Sachverhalt nicht besser und ist ein zweiter Bug, welcher von SonarCube nicht erkannt wurde. Es würde auch niemand, der deinen Code liest davon ausgehen, dass du gar nicht #clone() meinst, es aber trotzdem missbrauchst. So denkt kein Programmierer und ein Automat schon gar nicht. Jeder der diesen Code liest geht von einem Fehler aus.

Also ich persönlich habe null Anreiz dran hier richtig oder falsch zu legen. Sondern zu differenzieren und habe mir die Aufgabe gesetzt bewusst nach Grenzen statischer Codeanalyse zu suchen, obwohl ich sich 1 A finde.

 

[KonradN]

Es wird bei der Analyse nach gewissen Mustern gesucht. Und ein Muster, nach dem hier offensichtlich geschaut wird, ist das Überschreiben von clone().

Und dann ist immer die Frage: gibt es false positives? Und das sehe ich hier nicht. Deine Methode erzeugt auch ein clone. Das Ergebnis von Deiner Methode entspricht dem Ergebnis von super.clone(), so du wirklich alle Felder kopiert hast. Das erklärt evtl. dass wir Probleme haben, Deinen Punkt zu verstehen. Aber selbst wenn Du da irgend etwas anderes machen würdest wie eine Ausgabe von "clone" und dann ein return null - das würde da vermutlich erst einmal nichts ändern.

Ja, die Meldung könnte dann evtl. etwas differenzierter sein, aber
a) Was sollte denn dann da das Pattern sein, nach dem geschaut wird?
b) Was für eine Priorität hätte es?

Es hat einfach keine Priorität, denn du bekommst eine Meldung und das ist richtig. Du würdest Dir ja selbst nur eine etwas modifizierte Meldung wünschen so wie ich dich verstanden habe. (Oder wir konnten Dich diesbezüglich hoffentlich überzeugen.)

Man kann da bestimmt Verbesserungsvorschläge unterbreiten - evtl. wird dem gefolgt. Aber da ist dann tatsächlich die Frage, was überhaupt an Änderungen Sinn macht (nach was soll geschaut werden?) und dann ist da immer noch die Frage der Priorität - es gibt bestimmt genug andere Verbesserungen, die eine höhere Priorität haben.

Das wäre da so eine Sicht darauf. Aber ganz wichtig: Wir sind alle nur Aussenseiter. Wir arbeiten nicht für diesen Anbieter. Daher können wir nur unsere Sicht benennen und die kann durchaus von Deiner abweichen und das heisst nicht, dass Deine Sicht falsch ist oder so.

Ansonsten gilt natürlich: Wenn Du der Meinung bist, dass das Überschreiben von clone() Sinn macht, dann kannst Du das natürlich machen. Du kannst garantiert spezifizieren, dass Du diese Regel einfach komplett nicht haben willst. Das ist üblich. Bei PMD habe ich eine ganze Reihe an Regeln ausgeschlossen, weil diese einfach keinen Sinn machen. Also z.B. ist es bei einer Konsolen-Anwendung notwendig, System.out zu nutzen für Ausgaben. Aber auch die Regel von Demeter hatte bei mir Stream-Ausdrücke angemeckert und daher habe ich die Regeln diesbezüglich komplett ausgeschlossen ....

Also unter dem Strich is Sonarcube nur ein Tool und Du musst schauen, ob und wie Du es für Dich sinnvoll einsetzen kannst. Nicht alles muss sinnvoll sein für Dich.