La qualité est importante pour nous. Et nous avons entendu parler de PVS-Studio. Tout cela a conduit à vouloir vérifier Docotic.Pdf et à découvrir quoi d'autre pourrait être amélioré.
Docotic.Pdf est une bibliothèque polyvalente pour travailler avec des fichiers PDF. Il est écrit en C #, il n'y a pas de code dangereux, pas de dépendances externes autres que le runtime .NET. Il fonctionne à la fois sous .NET 4+ et sous .NET Standard 2+.
La bibliothèque est en développement depuis un peu plus de 10 ans et compte 110 000 lignes de code sans prendre en compte les tests, les exemples et autres. Pour l'analyse statique, nous utilisons constamment l'analyse de code et StyleCop. Plusieurs milliers de tests automatisés nous protègent des régressions. Nos clients de différents pays et de différentes industries font confiance à la qualité de la bibliothèque.
Quels problèmes détectera PVS-Studio?
Installation et première impression
J'ai téléchargé la version d'essai sur le site Web de PVS-Studio. Agréablement surpris par la petite taille de l'installateur. Installé avec les paramètres par défaut: moteurs d'analyse, un environnement PVS-Studio séparé, intégration dans Visual Studio 2017.
Après l'installation, rien n'a commencé et deux raccourcis avec les mêmes icônes ont été ajoutés au menu Démarrer: autonome et PVS-Studio. Pendant un moment, j'ai pensé par où commencer. Lancement autonome et a été désagréablement surpris par l'interface. L'échelle de 200% définie pour mon Windows est prise en charge de manière tordue. Une partie du texte est trop petite, une partie du texte ne tient pas dans l'espace prévu à cet effet. Le nom, la licorne et la liste des actions sont rognés pour n'importe quelle taille de fenêtre. Même en plein écran.
Bon, d'accord, j'ai décidé d'ouvrir mon dossier de projet. Du coup, le menu Fichier n'a pas trouvé une telle opportunité. Là, on m'a seulement proposé d'ouvrir des fichiers individuels. Merci, j'ai pensé, je préfère essayer une autre option. Lancement de PVS-Studio - ils m'ont montré une fenêtre avec du texte flou. L'échelle de 200% s'est de nouveau fait sentir. Le texte a rapporté:
recherchez-moi dans Three Crowns, recherchez le menu PVS-Studio dans Visual Studio. Ok, a ouvert le Studio.
Solution ouverte. En effet, il existe un menu PVS-Studio, et il a la possibilité de vérifier le «Projet en cours». Il a réalisé le projet dont j'avais besoin et a lancé un chèque. Une fenêtre est apparue dans le Studio avec les résultats de l'analyse. Une fenêtre est apparue en arrière-plan avec la progression de l'analyse, mais je ne l'ai pas immédiatement trouvée. Au début, on avait le sentiment que le contrôle n'avait pas commencé ou s'était terminé immédiatement.
Résultat du premier contrôle
L'analyseur a vérifié tous les 1253 fichiers de projet en environ 9 minutes et 30 secondes. À la fin de la vérification, le compteur de fichiers n'a pas changé aussi rapidement qu'au début. Il existe peut-être une dépendance non linéaire de la durée d'analyse sur le nombre de fichiers analysés.
Des informations sur 81 avertissements élevés, 109 moyens et 175 faibles sont apparues dans la fenêtre des résultats. Si vous calculez la fréquence, vous obtenez 0,06 avertissements élevés / fichier, 0,09 avertissements moyens / fichier et 0,14 avertissements bas / fichier. Ou
0,74 Avertissements élevés pour mille lignes de code, 0,99 Avertissements moyens pour mille lignes de code et 1,59 Avertissements faibles pour mille lignes de code.
Ici,
dans cet article , il est
indiqué que dans CruiseControl.NET, avec ses 256 mille lignes de code, l'analyseur a trouvé 15 avertissements High, 151 Medium et 32 Low.
Il s'avère qu'en termes de pourcentage pour Docotic.Pdf, plus d'avertissements ont été émis dans chacun des groupes.
Que trouve-t-on?
J'ai décidé d'ignorer les avertissements Low à ce stade.
J'ai trié les avertissements par la colonne Code et il s'est avéré que le détenteur absolu d'enregistrement pour la fréquence était
V3022 "L'expression est toujours vraie / fausse" et
V3063 "Une partie de l'expression conditionnelle est toujours vraie / fausse si elle est évaluée". À mon avis, il s'agit d'une chose. Au total, ces deux avertissements donnent 92 cas sur 190. Fréquence relative = 48%.
La logique de la division en Élevé et Moyen n'est pas entièrement claire. Je m'attendais à
V3072 "La classe" A "contenant les membres IDisposable
n'implémente pas elle-même IDisposable" et
V3073 "Tous les membres IDisposable ne sont pas correctement éliminés. Appelez «Dispose» lorsque vous disposez de la classe «A» »dans le groupe High, par exemple. Mais c'est du goût, bien sûr.
Surpris que
V3095 «L'objet a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: N1, N2 ”est marqué deux fois comme élevé et une fois comme moyen. Bug?
Faire confiance mais vérifier
Il est temps de vérifier le caractère raisonnable des avertissements. Y a-t-il de vraies erreurs trouvées? Y a-t-il des avertissements incorrects?
J'ai divisé les avertissements trouvés dans les groupes ci-dessous.
Avertissements importants
Leur correction a augmenté la stabilité, résolu les problèmes de fuites de mémoire, etc. Vraies erreurs / imperfections.
16 d'entre eux ont été émis, ce qui représente environ 8% de tous les avertissements.
Je vais donner quelques exemples.
V3019 "Il est possible qu'une variable incorrecte soit comparée à null après la conversion de type à l'aide du mot clé" as ". Vérifiez les variables 'couleur', 'indexées' »
public override bool IsCompatible(ColorImpl color) { IndexedColorImpl indexed = color as IndexedColorImpl; if (color == null) return false; return indexed.ColorSpace.Equals(this); }
Comme vous pouvez le voir, au lieu d'être indexée, la couleur variable est comparée à null. Ceci est incorrect et peut conduire à NRE.
V3080 «
Déréférence nulle possible. Pensez à inspecter 'cstr_index.tile_index' »
Un petit fragment pour illustrer:
if (cstr_index.tile_index == null) { if (cstr_index.tile_index[0].tp_index == null) {
Évidemment, la première condition impliquait! = Null. Dans le formulaire actuel, le code lancera NRE à chaque appel.
V3083 “L'appel non sécurisé de l'événement 'OnProgress', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. »
public void Updated() { if (OnProgress != null) OnProgress(this, new EventArgs()); }
Un avertissement a aidé à corriger une exception potentielle. Pourquoi cela peut-il survenir? Stackoverflow a une
bonne explication .
V3106 «L'index est peut-être hors limites. L'index «0» pointe au-delà de la limite «v» »
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]); }
L'erreur est que le résultat de createPairs est ignoré et à la place une liste vide est accessible. Apparemment, createPairs a initialement accepté la liste comme paramètre, mais une erreur a été commise lors du changement de méthode.
V3117 'Le paramètre constructeur' validateType 'n'est pas utilisé
Un avertissement a été émis pour un code similaire à celui-ci
public SomeClass(IDocument document, bool validateType = true) : base(document, true) { m_provider = document; }
L'avertissement lui-même ne semble pas important. Mais le problème est plus grave qu'il n'y paraît à première vue. Lors de l'ajout du paramètre optionnel validateType, ils ont oublié de corriger l'appel au constructeur de la classe de base.
V3127 „Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable "plage" doit être utilisée à la place de "domaine" "
private void fillTransferFunction(PdfStreamImpl function) {
Un avertissement ne sera peut-être pas émis si les parties du code sont légèrement différentes. Mais dans ce cas, une erreur a été détectée lors de l'utilisation du copier-coller.
Avertissements théoriques / formels
Ils sont corrects, mais leur correction ne corrige aucune erreur spécifique et n'affecte pas la lisibilité du code. Ou ils indiquent des endroits où il pourrait y avoir une erreur, mais cela n'existe pas. Par exemple, l'ordre des paramètres est intentionnellement modifié. Pour de tels avertissements, vous n'avez rien à changer dans le programme.
De ce nombre, 57 ont été émis, ce qui représente environ 30% de tous les avertissements. Je vais donner des exemples de cas qui méritent attention.
V3013 "Il est étrange que le corps de la fonction 'BeginText' soit entièrement équivalent au corps de la fonction 'EndText' (166, ligne 171)"
public override void BeginText() { m_state.ResetTextParameters(); } public override void EndText() { m_state.ResetTextParameters(); }
Les deux fonctions corporelles sont en fait les mêmes. Mais c'est vrai. Et est-ce vraiment si étrange que les corps de fonctions d'une même ligne coïncident?
V3106 „Valeur d'index négative possible. La valeur de l'indice «c1» pourrait atteindre -1 »
freq[256] = 1;
Je suis d'accord, j'ai donné un morceau de l'algorithme pas si clair. Mais, à mon avis, dans ce cas, l'analyseur s'inquiète en vain.
V3107 «Expression identique de« neighsum »à gauche et à droite de l'affectation composée»
L'avertissement est provoqué par un code assez courant:
neighsum += neighsum;
Oui, il peut être réécrit par multiplication. Mais il n'y a pas d'erreur.
V3109 „La sous-expression 'l_cblk.data_current_size' est présente des deux côtés de l'opérateur. L'expression est incorrecte ou elle peut être simplifiée. "
if ((l_cblk.data_current_size + l_seg.newlen) < l_cblk.data_current_size) {
Le commentaire du code clarifie l'intention. Encore une fausse alarme.
Avertissements justifiés
Leur correction a eu un effet positif sur la lisibilité du code. Autrement dit, il a réduit les conditions inutiles, les contrôles, etc. L'effet sur le fonctionnement du code n'est pas évident.
De ce nombre, 103 ont été émis, ce qui représente environ 54% de tous les avertissements.
V3008 „La variable 'l_mct_deco_data' reçoit deux fois de suite des valeurs. C'est peut-être une erreur »
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;
Analyseur de droits: affectation à l'intérieur s'il n'est pas nécessaire.
V3009 "Il est étrange que cette méthode renvoie toujours une seule et même valeur"
private static bool opj_dwt_decode_tile(opj_tcd_tilecomp_t tilec, OPJ_UINT32 numres) { if (numres == 1U) return true;
Sur les conseils de l'analyseur, la méthode a été modifiée et ne renvoie plus rien.
V3022 "Expression '! Ajouter' est toujours vrai"
private void addToFields(PdfDictionaryImpl controlDict, bool add) {
En effet, cela ne sert à rien dans le second si. La condition sera toujours vraie.
V3029 "L'expression conditionnelle des instructions 'if' situées côte à côte est identique"
if (stroke) extGState.OpacityStroke = opacity; if (stroke) state.AddReal(Field.CA, opacity);
On ne sait pas d'où vient ce code. Mais maintenant, nous l'avons corrigé.
V3031 „Un contrôle excessif peut être simplifié. Le '||' l'opérateur est entouré d'expressions opposées "
Ceci est une condition de cauchemar:
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)))) {
Après les changements, c'est devenu beaucoup mieux
if (!(cp.m_enc.m_tp_on != 0 && (opj_codec_t.OPJ_IS_CINEMA(cp.rsiz) || t2_mode == J2K_T2_MODE.FINAL_PASS))) {
V3063 «Une partie de l'expression conditionnelle est toujours vraie si elle est évaluée: x! = Null»
V3022 "L'expression 'x! = Null' est toujours vraie"
Ici, j'ai inclus des avertissements selon lesquels la vérification de null n'a pas de sens. Le bien-fondé de le faire est une question controversée. Ci-dessous, j'ai décrit plus en détail l'essence du problème.
Avertissements sans fondement
Faux positifs En raison d'erreurs dans la mise en œuvre d'un test particulier ou d'une sorte de défaut de l'analyseur.
Parmi ceux-ci, 14 ont été émis, ce qui représente environ 7% de tous les avertissements.
V3081 «Le compteur« i »n'est pas utilisé dans une boucle imbriquée. Envisagez d'inspecter l'utilisation du compteur "j" "
Une version légèrement simplifiée du code pour lequel cet avertissement a été émis:
for (uint i = 0; i < initialGlyphsCount - 1; ++i) { for (int j = 0; j < initialGlyphsCount - i - 1; ++j) {
Évidemment, i est utilisé dans une boucle imbriquée.
V3125 "L'objet a été utilisé après avoir été vérifié par rapport à null"
Code pour lequel un avertissement est émis:
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 ne peut pas être nul lorsque CompareTo () est appelé.
Un autre code pour lequel cet avertissement est émis:
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; }
la table ne peut pas être nulle dans une boucle.
V3134 „Décalage de [32..255] bits est supérieur à la taille du type d'expression 'UInt32' '(uint) 1'“
Un morceau de code pour lequel cet avertissement est émis:
byte bitPos = (byte)(numBits & 0x1F); uint mask = (uint)1 << bitPos;
On peut voir que bitPos peut avoir une valeur de la plage [0..31], mais l'analyseur pense qu'il peut avoir une valeur de la plage [0..31], ce qui est incorrect.
Je ne donnerai pas d'autres cas similaires, car ils sont équivalents.
Réflexions supplémentaires sur certains chèques
Il ne m'a pas semblé souhaitable d'avertir que 'x! = Null' est toujours vrai dans les cas où x est le résultat de l'appel d'une méthode. Un exemple:
private X GetX(int y) { if (y > 0) return new X(...); if (y == 0) return new X(...); throw new ArgumentOutOfRangeException(nameof(x)); } private Method() {
Oui, formellement, l'analyseur a raison: x ne sera toujours pas nul, car GetX retournera une instance à part entière ou lèvera une exception. Mais le code améliorera-t-il la suppression du chèque par null? Et si GetX change plus tard? La méthode doit-elle connaître l'implémentation de GetX?
Au sein de l'équipe, les opinions étaient partagées. Il a été suggéré que la méthode actuelle a un contrat par lequel elle ne devrait pas retourner null. Et cela n'a aucun sens d'écrire du code redondant «pour l'avenir» à chaque appel. Et si le contrat change, le code d'appel doit être mis à jour.
À l'appui de cette opinion, le jugement suivant a été rendu: la vérification de null revient à encapsuler chaque appel dans try / catch au cas où la méthode commencerait à lever des exceptions à l'avenir.
En conséquence, selon le principe
YAGNI , ils ont décidé de ne pas conserver les chèques et les ont supprimés. Tous les avertissements ont été transférés de théorique / formel à justifié.
Je me ferai un plaisir de lire votre avis dans les commentaires.
Conclusions
L'analyse statique est une bonne chose. PVS-Studio vous permet de trouver de vraies erreurs.
Oui, il y a des avertissements déraisonnables / incorrects. Mais PVS-Studio a toujours trouvé de vraies erreurs dans un projet qui utilise déjà l'analyse de code. Notre produit est assez bien couvert par les tests, il est d'une manière ou d'une autre testé par nos clients, mais les
robots le font mieux. L' analyse statique est toujours bénéfique.
Enfin, quelques statistiques.
3 principaux avertissements déraisonnables
V3081 „Le compteur 'X' n'est pas utilisé dans une boucle imbriquée. Envisagez d'inspecter l'utilisation du compteur "Y" "
1 sur 1 trouvé déraisonnable
V3125 „L'objet a été utilisé après avoir été vérifié par rapport à null. Vérifier les lignes: N1, N2 “
9 sur 10 sont déclarés non fondés
V3134 "Le décalage de N bits est supérieur à la taille du type"
4 sur 5 jugés non fondés
3 principaux avertissements importants
V3083 «L'appel d'événement à risque, NullReferenceException est possible. Envisagez d'affecter un événement à une variable locale avant de l'invoquer "
5 sur 5 ont été jugés importants.
V3020 „Une rupture / poursuite / retour / goto inconditionnel dans une boucle“
V3080 "
Déréférence nulle possible"
2 sur 2 ont été reconnus comme importants.
V3019 „Il est possible qu'une variable incorrecte soit comparée à null après la conversion de type en utilisant le mot-clé 'as'“
V3127 „Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable "X" devrait être utilisée à la place de "Y" "
1 sur 1 a été jugé important.