Qualität ist uns wichtig. Und wir haben von PVS-Studio gehört. All dies führte zu dem Wunsch, Docotic.Pdf zu überprüfen und herauszufinden, was noch verbessert werden kann.
Docotic.Pdf ist eine Allzweckbibliothek für die Arbeit mit PDF-Dateien. Es ist in C # geschrieben, es gibt keinen unsicheren Code, keine anderen externen Abhängigkeiten als die .NET-Laufzeit. Es funktioniert sowohl unter .NET 4+ als auch unter .NET Standard 2+.
Die Bibliothek befindet sich seit etwas mehr als 10 Jahren in der Entwicklung und verfügt über 110.000 Codezeilen, ohne Tests, Beispiele und andere Dinge zu berücksichtigen. Für die statische Analyse verwenden wir ständig Code Analysis und StyleCop. Mehrere tausend automatisierte Tests schützen uns vor Regressionen. Unsere Kunden aus verschiedenen Ländern und Branchen vertrauen auf die Qualität der Bibliothek.
Welche Probleme erkennt PVS-Studio?
Installation und erster Eindruck
Ich habe die Testversion von der PVS-Studio-Website heruntergeladen. Angenehm überrascht von der geringen Größe des Installateurs. Mit Standardeinstellungen installiert: Analyse-Engines, eine separate PVS-Studio-Umgebung, Integration in Visual Studio 2017.
Nach der Installation wurde nichts gestartet und dem Startmenü wurden zwei Verknüpfungen mit denselben Symbolen hinzugefügt: Standalone und PVS-Studio. Für einen Moment überlegte ich, was ich anfangen sollte. Standalone gestartet und war von der Benutzeroberfläche unangenehm überrascht. Die für Windows festgelegte 200% -Skala wird schief unterstützt. Ein Teil des Textes ist zu klein, ein Teil des Textes passt nicht in den dafür vorgesehenen Raum. Der Name, das Einhorn und die Aktionsliste werden für jede Fenstergröße zugeschnitten. Auch bei Vollbild.
Okay, ich habe beschlossen, meine Projektdatei zu öffnen. Plötzlich fand das Menü Datei keine solche Gelegenheit. Dort wurde mir nur angeboten, einzelne Dateien zu öffnen. Danke, dachte ich, ich würde lieber eine andere Option ausprobieren. PVS-Studio gestartet - sie zeigten mir ein Fenster mit verschwommenem Text. Die Skala von 200% machte sich erneut bemerkbar. Der gemeldete Text:
Suchen Sie in Three Crowns nach mir, suchen Sie in Visual Studio nach dem PVS-Studio-Menü. Ok, öffnete das Studio.
Geöffnete Lösung. In der Tat gibt es ein PVS-Studio-Menü, mit dem das „Aktuelle Projekt“ überprüft werden kann. Er hat das Projekt, das ich brauche, aktuell gemacht und einen Check gestartet. Im Studio wurde ein Fenster mit den Ergebnissen der Analyse angezeigt. Im Hintergrund erschien ein Fenster mit dem Fortschritt des Scans, das ich jedoch nicht sofort fand. Zuerst hatte man das Gefühl, dass die Prüfung nicht begann oder sofort endete.
Erstes Prüfergebnis
Der Analysator überprüfte alle 1253 Projektdateien in ungefähr 9 Minuten und 30 Sekunden. Am Ende der Prüfung änderte sich der Dateizähler nicht so schnell wie zu Beginn. Möglicherweise besteht eine nichtlineare Abhängigkeit der Scandauer von der Anzahl der gescannten Dateien.
Informationen zu 81 hohen, 109 mittleren und 175 niedrigen Warnungen wurden im Ergebnisfenster angezeigt. Wenn Sie die Häufigkeit berechnen, erhalten Sie 0,06 hohe Warnungen / Datei, 0,09 mittlere Warnungen / Datei und 0,14 niedrige Warnungen / Datei. Oder
0,74 Hohe Warnungen pro tausend Codezeilen, 0,99 Mittlere Warnungen pro tausend Codezeilen und 1,59 Niedrige Warnungen pro tausend Codezeilen.
Hier
in diesem Artikel wird darauf
hingewiesen, dass der Analysator in CruiseControl.NET mit seinen 256.000 Codezeilen 15 Warnungen für Hoch, 151 Mittel und 32 Niedrig gefunden hat.
Es stellt sich heraus, dass in Docotic.Pdf prozentual mehr Warnungen in jeder der Gruppen ausgegeben wurden.
Was wird gefunden?
Ich habe mich entschlossen, niedrige Warnungen zu diesem Zeitpunkt zu ignorieren.
Ich habe die Warnungen nach der Spalte Code sortiert und es stellte sich heraus, dass der absolute Rekordhalter für die Häufigkeit
V3022 „Ausdruck ist immer wahr / falsch“ und
V3063 „Ein Teil des bedingten Ausdrucks ist immer wahr / falsch, wenn er ausgewertet wird“ war. Meiner Meinung nach geht es um eine Sache. Insgesamt ergeben diese beiden Warnungen 92 von 190 Fällen. Relative Häufigkeit = 48%.
Die Logik der Unterteilung in Hoch und Mittel ist nicht ganz klar. Ich hatte erwartet, dass
V3072 "Die '
A' -Klasse, die IDisposable-Mitglieder enthält, IDisposable selbst nicht implementiert" und
V3073 "Nicht alle IDisposable-Mitglieder sind ordnungsgemäß entsorgt. Rufen Sie 'Dispose' auf, wenn Sie beispielsweise 'A' class 'in der High-Gruppe entsorgen. Aber das ist natürlich Geschmack.
Überrascht, dass
V3095 „Das Objekt wurde verwendet, bevor es gegen Null verifiziert wurde. Kontrolllinien: N1, N2 ”ist doppelt so hoch und einmal so mittel markiert. Bug?
Vertrauen, aber überprüfen
Es ist Zeit zu überprüfen, wie vernünftig die Warnungen sind. Gibt es echte Fehler? Gibt es falsche Warnungen?
Ich habe die gefundenen Warnungen in die folgenden Gruppen unterteilt.
Wichtige Warnungen
Ihre Korrektur erhöhte die Stabilität, löste Probleme mit Speicherlecks usw. Echte Fehler / Unvollkommenheiten.
16 davon wurden ausgegeben, was etwa 8% aller Warnungen entspricht.
Ich werde einige Beispiele geben.
V3019 "Möglicherweise wird eine falsche Variable nach der Typkonvertierung mit dem Schlüsselwort 'as' mit null verglichen. Überprüfen Sie die Variablen 'Farbe', 'indiziert' »
public override bool IsCompatible(ColorImpl color) { IndexedColorImpl indexed = color as IndexedColorImpl; if (color == null) return false; return indexed.ColorSpace.Equals(this); }
Wie Sie sehen können, wird die variable Farbe anstelle von indiziert mit null verglichen. Dies ist falsch und kann zu NRE führen.
V3080 “Mögliche Null-Dereferenzierung. Überprüfen Sie 'cstr_index.tile_index' »
Ein kleines Fragment zur Veranschaulichung:
if (cstr_index.tile_index == null) { if (cstr_index.tile_index[0].tp_index == null) {
Offensichtlich implizierte die erste Bedingung! = Null. In der aktuellen Form löst der Code bei jedem Aufruf NRE aus.
V3083 “Unsicherer Aufruf des Ereignisses 'OnProgress', NullReferenceException ist möglich. Überlegen Sie, ob Sie einer lokalen Variablen ein Ereignis zuweisen möchten, bevor Sie es aufrufen. “
public void Updated() { if (OnProgress != null) OnProgress(this, new EventArgs()); }
Eine Warnung hat geholfen, eine mögliche Ausnahme zu beheben. Warum kann es entstehen? Stackoverflow hat eine
gute Erklärung .
V3106 „Möglicherweise ist der Index außerhalb der Grenzen. Der '0'-Index zeigt über die' v'-Grenze hinaus »
var result = new List<FontStringPair>(); for (int i = 0; i < text.Length; ++i) { var v = new List<FontStringPair>(); createPairs(text[i].ToString(CultureInfo.InvariantCulture)); result.Add(v[0]); }
Der Fehler besteht darin, dass das Ergebnis von createPairs ignoriert wird und stattdessen auf eine leere Liste zugegriffen wird. Anscheinend hat createPairs die Liste zunächst als Parameter akzeptiert, aber beim Ändern der Methode ist ein Fehler aufgetreten.
V3117 'Konstruktorparameter' validateType 'wird nicht verwendet
Für einen ähnlichen Code wurde eine Warnung ausgegeben
public SomeClass(IDocument document, bool validateType = true) : base(document, true) { m_provider = document; }
Die Warnung selbst scheint nicht wichtig zu sein. Das Problem ist jedoch schwerwiegender, als es auf den ersten Blick scheint. Beim Hinzufügen des optionalen Parameters validateType wurde vergessen, den Aufruf an den Konstruktor der Basisklasse zu korrigieren.
V3127 „Es wurden zwei ähnliche Codefragmente gefunden. Vielleicht ist dies ein Tippfehler und die Variable 'range' sollte anstelle von 'domain' verwendet werden. “
private void fillTransferFunction(PdfStreamImpl function) {
Möglicherweise wird keine Warnung ausgegeben, wenn sich die Teile des Codes geringfügig unterscheiden. In diesem Fall wurde jedoch beim Kopieren und Einfügen ein Fehler festgestellt.
Theoretische / formale Warnungen
Sie sind entweder korrekt, aber ihre Korrektur behebt keine spezifischen Fehler und beeinträchtigt nicht die Lesbarkeit des Codes. Oder sie zeigen auf Stellen, an denen möglicherweise ein Fehler vorliegt, der jedoch nicht vorhanden ist. Beispielsweise wird die Reihenfolge der Parameter absichtlich geändert. Für solche Warnungen müssen Sie nichts im Programm ändern.
Davon wurden 57 ausgegeben, was etwa 30% aller Warnungen entspricht. Ich werde Beispiele für Fälle geben, die Aufmerksamkeit verdienen.
V3013 „Es ist seltsam, dass der Hauptteil der Funktion 'BeginText' dem Hauptteil der Funktion 'EndText' (166, Zeile 171) vollständig entspricht.“
public override void BeginText() { m_state.ResetTextParameters(); } public override void EndText() { m_state.ResetTextParameters(); }
Beide Körperfunktionen sind eigentlich gleich. Aber es ist richtig. Und ist es wirklich so seltsam, wenn die Funktionskörper einer Zeile zusammenfallen?
V3106 „Möglicher negativer Indexwert. Der Wert des 'c1'-Index könnte -1 erreichen. “
freq[256] = 1;
Ich stimme zu, ich habe einen Teil des nicht so klaren Algorithmus gegeben. Aber meiner Meinung nach macht sich der Analysator in diesem Fall vergeblich Sorgen.
V3107 "Identischer Ausdruck" Nachbar "links und rechts von der zusammengesetzten Zuordnung"
Die Warnung wird durch einen ziemlich alltäglichen Code verursacht:
neighsum += neighsum;
Ja, es kann durch Multiplikation umgeschrieben werden. Aber es gibt keinen Fehler.
V3109 „Der Unterausdruck 'l_cblk.data_current_size' ist auf beiden Seiten des Operators vorhanden. Der Ausdruck ist falsch oder kann vereinfacht werden. “
if ((l_cblk.data_current_size + l_seg.newlen) < l_cblk.data_current_size) {
Ein Kommentar im Code verdeutlicht die Absicht. Wieder falscher Alarm.
Begründete Warnungen
Ihre Korrektur wirkte sich positiv auf die Lesbarkeit des Codes aus. Das heißt, es wurden unnötige Bedingungen, Überprüfungen usw. reduziert. Die Auswirkung auf die Funktionsweise des Codes ist nicht offensichtlich.
Davon wurden 103 ausgegeben, was etwa 54% aller Warnungen entspricht.
V3008 „Der Variablen 'l_mct_deco_data' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler “
if (m_nb_mct_records == m_nb_max_mct_records) { ResizeMctRecords(); l_mct_deco_data = (OPJ_INT32)m_nb_mct_records; } l_mct_deco_data = (OPJ_INT32)m_nb_mct_records;
Rechte-Analysator: Zuordnung innerhalb, falls nicht erforderlich.
V3009 „Es ist seltsam, dass diese Methode immer ein und denselben Wert zurückgibt.“
private static bool opj_dwt_decode_tile(opj_tcd_tilecomp_t tilec, OPJ_UINT32 numres) { if (numres == 1U) return true;
Auf Anraten des Analysators wurde die Methode geändert und gibt nichts mehr zurück.
V3022 "Ausdruck '! Hinzufügen' ist immer wahr"
private void addToFields(PdfDictionaryImpl controlDict, bool add) {
In der Tat hat das zweite Wenn keinen Sinn. Die Bedingung wird immer wahr sein.
V3029 "Der bedingte Ausdruck der
nebeneinander angeordneten '
if' -Anweisungen ist identisch."
if (stroke) extGState.OpacityStroke = opacity; if (stroke) state.AddReal(Field.CA, opacity);
Es ist unklar, wie dieser Code entstanden ist. Aber jetzt haben wir es behoben.
V3031 „Eine übermäßige Prüfung kann vereinfacht werden. Das '||' Der Operator ist von entgegengesetzten Ausdrücken umgeben. “
Dies ist ein Albtraum:
if (!(cp.m_enc.m_tp_on != 0 && ((!opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) && (t2_mode == J2K_T2_MODE.FINAL_PASS)) || opj_codec_t.OPJ_IS_CINEMA(cp.rsiz)))) {
Nach den Änderungen wurde es viel besser
if (!(cp.m_enc.m_tp_on != 0 && (opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) || t2_mode == J2K_T2_MODE.FINAL_PASS))) {
V3063 „Ein Teil des bedingten Ausdrucks ist immer wahr, wenn er ausgewertet wird: x! = Null“
V3022 "Ausdruck 'x! = Null' ist immer wahr"
Hier habe ich Warnungen eingefügt, dass die Überprüfung auf Null keinen Sinn macht. Ob dies richtig ist, ist eine kontroverse Frage. Im Folgenden habe ich das Wesentliche des Problems ausführlicher beschrieben.
Grundlose Warnungen
False Positives Aufgrund von Fehlern bei der Durchführung eines bestimmten Tests oder einer Art Analysatorfehler.
Davon wurden 14 ausgegeben, was etwa 7% aller Warnungen entspricht.
V3081 “Der '
i' -Zähler wird nicht in einer verschachtelten Schleife verwendet. Überprüfen Sie die Verwendung des 'j'-Zählers. “
Eine leicht vereinfachte Version des Codes, für den diese Warnung ausgegeben wurde:
for (uint i = 0; i < initialGlyphsCount - 1; ++i) { for (int j = 0; j < initialGlyphsCount - i - 1; ++j) {
Offensichtlich wird i in einer verschachtelten Schleife verwendet.
V3125 „Das Objekt wurde verwendet, nachdem es gegen Null verifiziert wurde.“
Code, für den eine Warnung ausgegeben wird:
private static int Compare_SecondGreater(cmapEncodingRecord er1, cmapEncodingRecord er2) { if (er1 == er2) return 0; if (er1 != null && er2 == null) return 1; if (er1 == null && er2 != null) return -1; return er1.CompareTo(er2); }
er1 kann nicht null sein, wenn CompareTo () aufgerufen wird.
Ein weiterer Code, für den diese Warnung ausgegeben wird:
private static void realloc(ref int[][] table, int newSize) { int[][] newTable = new int[newSize][]; int existingSize = 0; if (table != null) existingSize = table.Length; for (int i = 0; i < existingSize; i++) newTable[i] = table[i]; for (int i = existingSize; i < newSize; i++) newTable[i] = new int[4]; table = newTable; }
Tabelle kann in einer Schleife nicht null sein.
V3134 „Die Verschiebung um [32..255] Bits ist größer als die Größe des Ausdruckstyps 'UInt32' '(uint) 1'“
Ein Code, für den diese Warnung ausgegeben wird:
byte bitPos = (byte)(numBits & 0x1F); uint mask = (uint)1 << bitPos;
Es ist ersichtlich, dass bitPos einen Wert aus dem Bereich [0..31] haben kann, aber der Analysator glaubt, dass er einen Wert aus dem Bereich [0..31] haben kann, was falsch ist.
Ich werde keine ähnlichen Fälle nennen, da sie gleichwertig sind.
Zusätzliche Gedanken zu einigen Schecks
Es schien mir unerwünscht zu warnen, dass 'x! = Null' immer dann wahr ist, wenn x das Ergebnis des Aufrufs einer Methode ist. Ein Beispiel:
private X GetX(int y) { if (y > 0) return new X(...); if (y == 0) return new X(...); throw new ArgumentOutOfRangeException(nameof(x)); } private Method() {
Ja, formal hat der Analysator Recht: x ist immer nicht null, da GetX entweder eine vollwertige Instanz zurückgibt oder eine Ausnahme auslöst. Aber wird der Code das Entfernen der Prüfung um null verbessern? Was ist, wenn sich GetX später ändert? Muss Method die GetX-Implementierung kennen?
Innerhalb des Teams waren die Meinungen geteilt. Es wurde vorgeschlagen, dass die aktuelle Methode einen Vertrag hat, nach dem sie nicht null zurückgeben sollte. Und es macht keinen Sinn, bei jedem Aufruf redundanten Code „für die Zukunft“ zu schreiben. Und wenn sich der Vertrag ändert, muss der Anrufcode aktualisiert werden.
Zur Unterstützung dieser Meinung wurde das folgende Urteil gefällt: Das Überprüfen auf Null ist wie das Umschließen jedes Aufrufs in try / catch, falls die Methode in Zukunft Ausnahmen auslöst.
Infolgedessen beschlossen sie nach dem
YAGNI- Prinzip, die Schecks nicht festzuhalten, und löschten sie. Alle Warnungen wurden von theoretisch / formal auf gerechtfertigt übertragen.
Gerne lese ich Ihre Meinung in den Kommentaren.
Schlussfolgerungen
Statische Analyse ist eine gute Sache. Mit PVS-Studio können Sie echte Fehler finden.
Ja, es gibt unangemessene / falsche Warnungen. Trotzdem hat PVS-Studio echte Fehler in einem Projekt gefunden, das bereits Code Analysis verwendet. Unser Produkt wird durch Tests ziemlich gut abgedeckt, es wird auf die eine oder andere Weise von unseren Kunden getestet, aber
Roboter machen es besser. Eine statische Analyse ist immer noch von Vorteil.
Zum Schluss noch einige Statistiken.
Top 3 unangemessene Warnungen
V3081 „Der '
X' -Zähler wird nicht in einer verschachtelten Schleife verwendet. Überprüfen Sie die Verwendung des 'Y'-Zählers. “
1 von 1 für unvernünftig befunden
V3125 „Das Objekt wurde verwendet, nachdem es gegen null verifiziert wurde. Linien prüfen: N1, N2 “
9 von 10 werden für unbegründet erklärt
V3134 "Verschiebung um N Bits ist größer als die Größe des Typs"
4 von 5 erwiesen sich als unbegründet
Top 3 wichtige Warnungen
V3083 “Unsicherer Aufruf des Ereignisses, NullReferenceException ist möglich. Überlegen Sie, ob Sie einer lokalen Variablen ein Ereignis zuweisen möchten, bevor Sie es aufrufen. “
5 von 5 wurden als wichtig angesehen.
V3020 „Ein bedingungsloses 'Break /
Continue / Return / Goto' innerhalb einer Schleife“
V3080 "Mögliche Null-Dereferenzierung"
2 von 2 wurden als wichtig anerkannt.
V3019 „Es ist möglich, dass eine falsche Variable nach der Typkonvertierung mit dem Schlüsselwort 'as' mit null verglichen wird.“
V3127 „Es wurden zwei ähnliche Codefragmente gefunden. Vielleicht ist dies ein Tippfehler und 'X'-Variable sollte anstelle von' Y 'verwendet werden. “
1 von 1 wurde als wichtig angesehen.