La publication par Microsoft du code source de ses projets est une bonne raison de les vérifier. Cette fois n'a pas fait exception, et aujourd'hui, nous examinons les endroits suspects trouvés dans le code Infer.NET. À bas avec l'annotation - allez droit au but!
Un peu sur le projet et l'analyseur
Infer.NET est un système d'apprentissage automatique développé par des spécialistes de Microsoft. Le code source du projet est récemment devenu disponible sur
GitHub , ce qui a été la raison de sa vérification. Vous trouverez plus de détails sur le projet, par exemple,
ici .
Le projet a été vérifié à l'aide de l'analyseur statique PVS-Studio version 6.26. Permettez-moi de vous rappeler que PVS-Studio recherche des erreurs de code en C \ C ++ \ C # (et bientôt en Java) sous Windows, Linux, macOS. Jusqu'à présent, nous analysons uniquement le code C # sous Windows. L'analyseur peut être
téléchargé et essayé sur votre projet.
Le contrôle en lui-même était extrêmement simple et sans problème. Auparavant, j'ai déchargé le projet de GitHub, restauré les packages requis (dépendances) et vérifié que le projet a été correctement construit. Ceci est nécessaire pour que l'analyseur ait accès à toutes les informations nécessaires pour une analyse complète. Après assemblage en quelques clics, j'ai lancé l'analyse de la solution via le plugin PVS-Studio pour Visual Studio.
À propos, ce n'est pas le premier projet de Microsoft testé avec PVS-Studio - il y en avait d'autres:
Roslyn ,
MSBuild ,
PowerShell ,
CoreFX et
autres .
Remarque Si vous ou vos connaissances êtes intéressés par l'analyse du code Java, vous pouvez nous écrire à l'
appui en sélectionnant «Je veux un analyseur pour Java». Il n'y a pas de version bêta publique de l'analyseur, mais il devrait être disponible bientôt. Quelque part dans un laboratoire secret (à travers le mur), les gars y travaillent activement.
Mais assez de discussions abstraites - regardons les problèmes dans le code.
Est-ce un bug ou une fonctionnalité?
Je suggère d'essayer de trouver l'erreur vous-même - une tâche complètement résoluble. Pas de blagues dans l'esprit de ce qui était dans l'article "
Top 10 des erreurs dans les projets C ++ pour 2017 ", honnêtement. Ne vous précipitez donc pas pour lire l'avertissement de l'analyseur fourni après l'extrait de code.
private void MergeParallelTransitions() { .... if ( transition1.DestinationStateIndex == transition2.DestinationStateIndex && transition1.Group == transition2.Group) { if (transition1.IsEpsilon && transition2.IsEpsilon) { .... } else if (!transition1.IsEpsilon && !transition2.IsEpsilon) { .... if (double.IsInfinity(transition1.Weight.Value) && double.IsInfinity(transition1.Weight.Value)) { newElementDistribution.SetToSum( 1.0, transition1.ElementDistribution, 1.0, transition2.ElementDistribution); } else { newElementDistribution.SetToSum( transition1.Weight.Value, transition1.ElementDistribution, transition2.Weight.Value, transition2.ElementDistribution); } .... }
Avertissement PVS-Studio :
V3001 Il existe des sous-expressions identiques «double.IsInfinity (transition1.Weight.Value)» à gauche et à droite de l'opérateur «&&». Runtime Automaton.Simplification.cs 479
Comme vous pouvez le voir dans l'extrait de code, la méthode fonctionne avec une paire de variables -
transition1 et
transition2 . L'utilisation de noms similaires est parfois tout à fait justifiée, mais il convient de rappeler que dans ce cas, la probabilité de faire accidentellement une erreur quelque part avec le nom augmente.
C'est
ce qui s'est produit lors de la vérification des nombres pour l'infini (
double.IsInfinity ). En raison d'une erreur, nous avons vérifié la valeur de la même variable 2 fois -
transition1.Weight.Value . La valeur vérifiée dans la deuxième sous-expression devait être la variable
transition2.Weight.Value .
Un autre code suspect similaire.
internal MethodBase ToMethodInternal(IMethodReference imr) { .... bf |= BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance; .... }
Avertissement PVS-Studio :
V3001 Il existe des sous-expressions identiques «BindingFlags.Public» à gauche et à droite de «|» opérateur. Compilateur CodeBuilder.cs 194
Lors de la formation de la valeur de la variable
bf , l'élément d'énumération
BindingFlags.Public est utilisé deux fois. Soit ce code contient une opération de signalisation supplémentaire, soit au lieu de la deuxième utilisation de
BindingFlags.Public, il doit
y avoir une valeur d'énumération différente.
Par ailleurs, dans le code source, ce code est écrit sur une seule ligne. Il me semble que s'il est formaté dans un style de tableau (comme ici), le problème est plus facile à détecter.
Continuons. J'apporte tout le corps de la méthode et je vous suggère à nouveau de trouver l'erreur (ou peut-être l'erreur) vous-même.
private void ForEachPrefix(IExpression expr, Action<IExpression> action) {
L'avez-vous trouvé? Nous vérifions!
Avertissements de PVS-Studio :
- V3003 L'utilisation du modèle 'if (A) {...} else if (A) {...}' a été détectée. Il y a une probabilité de présence d'erreur logique. Vérifiez les lignes: 1719, 1727. Compiler CodeRecognizer.cs 1719
- V3003 L'utilisation du modèle 'if (A) {...} else if (A) {...}' a été détectée. Il y a une probabilité de présence d'erreur logique. Vérifiez les lignes: 1721, 1729. Compiler CodeRecognizer.cs 1721
Simplifiez un peu le code pour rendre les problèmes plus évidents.
private void ForEachPrefix(IExpression expr, Action<IExpression> action) { if (....) .... else if (expr is IUnaryExpression) ForEachPrefix(((IUnaryExpression)expr).Expression, action); else if (expr is IAddressReferenceExpression) ForEachPrefix(((IAddressReferenceExpression)expr).Expression, action); .... else if (expr is IUnaryExpression) ForEachPrefix(((IUnaryExpression)expr).Expression, action); else if (expr is IAddressReferenceExpression) ForEachPrefix(((IAddressReferenceExpression)expr).Expression, action) .... }
Les expressions conditionnelles,
puis les branches de plusieurs
instructions if sont dupliquées. Peut-être que ce code a été écrit à l'aide de la méthode copier-coller, c'est pourquoi le problème est survenu. Maintenant, il s'avère que les branches des doublons ne seront jamais exécutées, car:
- si l'expression conditionnelle est vraie, le corps de la première instruction if de la paire correspondante est exécuté;
- si l'expression conditionnelle est fausse dans le premier cas, elle sera fausse dans le second.
Depuis, les branches contiennent les mêmes actions, maintenant cela ressemble à du code redondant qui prête à confusion. Il est possible qu'il s'agisse d'un type de problème différent - au lieu de doublons, d'autres vérifications auraient dû être effectuées.
Nous continuons.
public int Compare(Pair<int, int> x, Pair<int, int> y) { if (x.First < y.First) { if (x.Second >= y.Second) {
Avertissements de PVS-Studio :
- V3004 L' instruction 'then' est équivalente à l'instruction 'else'. Durée d'exécution RegexpTreeBuilder.cs 1080
- V3004 L' instruction 'then' est équivalente à l'instruction 'else'. Runtime RegexpTreeBuilder.cs 1093
Le code semble extrêmement suspect, car il contient deux instructions conditionnelles avec des corps identiques de branches
then et
else . Dans les deux cas, il vaut probablement la peine de renvoyer des valeurs différentes. Ou, s'il s'agit d'un comportement conçu, il sera utile de supprimer les instructions conditionnelles redondantes.
Il y a eu des cycles intéressants. Exemple ci-dessous:
private static Set<StochasticityPattern> IntersectPatterns(IEnumerable<StochasticityPattern> patterns) { Set<StochasticityPattern> result = new Set<StochasticityPattern>(); result.AddRange(patterns); bool changed; do { int count = result.Count; AddIntersections(result); changed = (result.Count != count); break; } while (changed); return result; }
PVS-Studio Warning :
V3020 Une «pause» inconditionnelle dans une boucle. Compilateur DefaultFactorManager.cs 474
En raison de l'instruction
break inconditionnelle, une seule itération de la boucle est effectuée et la variable de contrôle
modifiée n'est même pas utilisée. En général, le code semble étrange et suspect.
La même méthode (copie exacte) a été trouvée dans une autre classe. Avertissement correspondant de l'analyseur:
V3020 Une «pause» inconditionnelle dans une boucle. Visualizers.Windows FactorManagerView.cs 350
À propos, une méthode a rencontré une instruction
continue inconditionnelle dans une boucle (elle a été trouvée par l'analyseur avec les mêmes diagnostics), mais il y avait un commentaire au-dessus confirmant qu'il s'agissait d'une solution temporaire spéciale:
Je me souviens qu'il n'y avait pas de tels commentaires près de la déclaration de
pause inconditionnelle.
Continuons.
internal static DependencyInformation GetDependencyInfo(....) { .... IExpression resultIndex = null; .... if (resultIndex != null) { if (parameter.IsDefined( typeof(SkipIfMatchingIndexIsUniformAttribute), false)) { if (resultIndex == null) throw new InferCompilerException( parameter.Name + " has SkipIfMatchingIndexIsUniformAttribute but " + StringUtil.MethodNameToString(method) + " has no resultIndex parameter"); .... } .... } .... }
Avertissement PVS-Studio :
V3022 L' expression 'resultIndex == null' est toujours fausse. Compiler FactorManager.cs 382
Immédiatement, je note qu'entre la déclaration et la vérification ci-dessus, la valeur de la variable
resultIndex peut changer. Cependant, entre les vérifications
resultIndex! = Null et
resultIndex == null, la valeur ne peut pas déjà être modifiée. Par conséquent, le résultat de l'expression
resultIndex == null sera toujours
faux , ce qui signifie qu'une exception ne sera jamais levée.
J'espère que vous avez intérêt à trouver vous-même des erreurs, sans mes suggestions, pour trouver un problème, mais juste au cas où, je vous proposerais de le refaire. Le code de la méthode est petit, je vais le donner dans son intégralité.
public static Tuple<int, string> ComputeMovieGenre(int offset, string feature) { string[] genres = feature.Split('|'); if (genres.Length < 1 && genres.Length > 3) { throw new ArgumentException(string.Format( "Movies should have between 1 and 3 genres; given {0}.", genres.Length)); } double value = 1.0 / genres.Length; var result = new StringBuilder( string.Format( "{0}:{1}", offset + MovieGenreBuckets[genres[0]], value)); for (int i = 1; i < genres.Length; ++i) { result.Append( string.Format( "|{0}:{1}", offset + MovieGenreBuckets[genres[i].Trim()], value)); } return new Tuple<int, string>(MovieGenreBucketCount, result.ToString()); }
Voyons ce qui se passe ici. La chaîne d'entrée est analysée par le caractère «|». Si la longueur du tableau n'est pas celle attendue, une exception doit être levée.
Attendez une seconde ...
genres.Length <1 && genres.Length> 3 ? Puisqu'il n'y a pas de nombre qui tombe immédiatement dans la plage de valeurs requise par l'expression (
[int.MinValue..1) et
(3..int.MaxValue] ), le résultat de l'expression sera toujours
faux . Par conséquent, cette vérification ne protège contre rien et l'exception attendue ne sera pas levée.
C'est exactement ce que l'analyseur prévient:
V3022 Expression 'genres.Length <1 && genres.Length> 3' est toujours faux. Probablement le '||' L'opérateur doit être utilisé ici. Fonctions de l'évaluateur.cs 242
Opération de fission suspecte rencontrée.
public static void CreateTrueThetaAndPhi(....) { .... double expectedRepeatOfTopicInDoc = averageDocLength / numUniqueTopicsPerDoc; .... int cnt = Poisson.Sample(expectedRepeatOfTopicInDoc); .... }
PVS-Studio Warning :
V3041 L'expression a été implicitement
convertie du type 'int' en type 'double'. Pensez à utiliser un transtypage de type explicite pour éviter la perte d'une partie fractionnaire. Un exemple: double A = (double) (X) / Y;. LDA Utilities.cs 74
Ceci est suspect ici: une division entière est effectuée (les variables
averageDocLength et
numUniqueTopicsPerDoc sont de type
int ), et le résultat est écrit dans une variable de type
double . La question se pose: est-ce spécialement fait, ou la division des nombres réels était-elle toujours implicite? Si la variable attenduRepeatOfTopicInDoc était de type
int , cela clarifierait les questions possibles.
Dans d'autres endroits, la méthode
Poisson.Sample , dont l'argument est la variable suspecte
attenduRepeatOfTopicInDoc , est utilisée, par exemple, comme décrit ci-dessous.
int numUniqueWordsPerTopic = Poisson.Sample((double)averageWordsPerTopic);
averageWordsPerTopic est de type
int , qui est déjà converti en
double au lieu d'utilisation.
Et voici un autre lieu d'utilisation:
double expectedRepeatOfWordInTopic = ((double)numDocs) * averageDocLength / numUniqueWordsPerTopic; .... int cnt = Poisson.Sample(expectedRepeatOfWordInTopic);
Veuillez noter que les variables ont les mêmes noms que dans l'exemple d'origine, seule la division des nombres réels est utilisée pour initialiser le
attenduRepeatOfWordInTopic (en raison de la conversion explicite de
numDocs en
double ).
En général, il vaut la peine de regarder le point de départ où l'analyseur a émis un avertissement.
Mais des réflexions sur la question de savoir s'il vaut la peine d'être édité et comment, laissons les auteurs du code (ils le savent mieux), mais allons plus loin. À la prochaine division suspecte.
public static NonconjugateGaussian BAverageLogarithm(....) { .... double v_opt = 2 / 3 * (Math.Log(mx * mz / Ex2 / 2) - m); if (v_opt != v) { .... } .... }
PVS-Studio Warning :
V3041 L'expression a été implicitement
convertie du type 'int' en type 'double'. Pensez à utiliser un transtypage de type explicite pour éviter la perte d'une partie fractionnaire. Un exemple: double A = (double) (X) / Y;. Runtime ProductExp.cs 137
L'analyseur a de nouveau détecté l'opération suspecte de division entière, comme
2 et
3 sont des littéraux numériques entiers et le résultat de l'expression 2/3 sera
0 . En conséquence, l'expression entière prend la forme:
double v_opt = 0 * expr;
D'accord, un peu étrange. Plusieurs fois, je suis revenu sur cet avertissement, essayant de trouver une sorte de capture, et ne cherchant pas à l'ajouter à l'article. La méthode est remplie de mathématiques et de diverses formules (que, franchement, je ne voulais pas vraiment démonter), on ne sait jamais à quoi s’attendre. De plus, j'essaie d'être aussi sceptique que possible sur les avertissements que j'écris dans l'article, et je ne les décris qu'après les avoir mieux étudiés.
Mais cela m'est venu à l'esprit - pourquoi ai-je besoin d'un facteur de
0 , écrit
2/3 ? Donc, cet endroit vaut le détour de toute façon.
public static void WriteAttribute(TextWriter writer, string name, object defaultValue, object value, Func<object, string> converter = null) { if ( defaultValue == null && value == null || value.Equals(defaultValue)) { return; } string stringValue = converter == null ? value.ToString() : converter(value); writer.Write($"{name}=\"{stringValue}\" "); }
Avertissement PVS-Studio :
V3080 Déréférence nulle possible. Pensez à inspecter la «valeur». Compilateur WriteHelpers.cs 78
Affirmation assez juste de l'analyseur en fonction de la condition. Le déréférencement d'une référence null peut se produire dans l'expression
value.Equals (defaultValue) si
value == null . Puisque cette expression est l'opérande droit de l'opérateur ||, pour le calculer, l'opérande gauche doit être
faux , et pour cela il suffit qu'au moins une des variables
defaultValue \
value ne soit pas
nulle . Par conséquent, si
defaultValue! = Null et
value == null :
- defaultValue == null -> false ;
- defaultValue == null && value == null -> false ; (la vérification de la valeur n'a pas eu lieu)
- value.Equals (defaultValue) -> NullReferenceException , car la valeur est nulle .
Regardons un cas similaire:
public FeatureParameterDistribution( GaussianMatrix traitFeatureWeightDistribution, GaussianArray biasFeatureWeightDistribution) { Debug.Assert( (traitFeatureWeightDistribution == null && biasFeatureWeightDistribution == null) || traitFeatureWeightDistribution.All( w => w != null && w.Count == biasFeatureWeightDistribution.Count), "The provided distributions should be valid and consistent in the number of features."); .... }
Avertissement PVS-Studio :
V3080 Déréférence nulle possible. Pensez à inspecter «traitFeatureWeightDistribution». Recommender FeatureParameterDistribution.cs 65
Nous jetons l'excédent, ne laissant que la logique de calcul de la valeur booléenne, afin qu'il soit plus facile de comprendre:
(traitFeatureWeightDistribution == null && biasFeatureWeightDistribution == null) || traitFeatureWeightDistribution.All( w => w != null && w.Count == biasFeatureWeightDistribution.Count)
Encore une fois, l'opérande droit de la || Il est calculé uniquement si le résultat du calcul de gauche est
faux . L'opérande gauche peut être
faux , y compris lorsque
traitFeatureWeightDistribution == null et
biaisFeatureWeightDistribution! = Null . Ensuite, l'opérande droit de l'opérateur || sera calculé, et un appel à
traitFeatureWeightDistribution.All lèvera une
ArgumentNullException .
Un autre morceau de code intéressant:
public static double GetQuantile(double probability, double[] quantiles) { .... int n = quantiles.Length; if (quantiles == null) throw new ArgumentNullException(nameof(quantiles)); if (n == 0) throw new ArgumentException("quantiles array is empty", nameof(quantiles)); .... }
PVS-Studio Warning :
V3095 L'objet 'quantiles' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 91, 92. Runtime OuterQuantiles.cs 91
Notez que la propriété
quantiles.Length est d'
abord accessible , puis les
quantiles sont vérifiés pour
null . Par conséquent, si
quantiles == null , la méthode lèvera une exception, juste un peu fausse, et un peu pas là où elle était nécessaire. Apparemment, ils ont foiré les lignes par endroits.
Si vous avez réussi à gérer vous-même la détection des erreurs précédentes, je vous suggère de préparer une tasse de café et d'essayer de répéter l'exploit, en trouvant l'erreur dans la méthode ci-dessous. Pour le rendre un peu plus intéressant, je cite tout le code de la méthode.
(
Lien vers la taille réelle )
D'accord, d'accord, c'était une blague (ou avez-vous réussi?!). Simplifions un peu la tâche:
if (sample.Precision < 0) { precisionIsBetween = true; lowerBound = -1.0 / v; upperBound = -mean.Precision; } else if (sample.Precision < -mean.Precision) { precisionIsBetween = true; lowerBound = 0; upperBound = -mean.Precision; } else {
Cela s'est-il amélioré? L'analyseur a émis l'avertissement suivant pour ce code:
V3008 La variable 'lowerBound' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 324, 323. Runtime GaussianOp.cs 324
En effet, dans la dernière branche
else , la valeur de la variable
lowerBound est affectée deux fois de suite. Apparemment (et à en juger par le code ci-dessus), la variable
upperBound doit être impliquée dans l'une des affectations.
Nous suivons plus loin.
private void WriteAucMatrix(....) { .... for (int c = 0; c < classLabelCount; c++) { int labelWidth = labels[c].Length; columnWidths[c + 1] = labelWidth > MaxLabelWidth ? MaxLabelWidth : labelWidth; for (int r = 0; r < classLabelCount; r++) { int countWidth = MaxValueWidth; if (countWidth > columnWidths[c + 1]) { columnWidths[c + 1] = countWidth; } } .... }
PVS-Studio Warning :
V3081 Le
compteur 'r' n'est pas utilisé dans une boucle imbriquée. Envisagez d'inspecter l'utilisation du compteur «c». CommandLine ClassifierEvaluationModule.cs 459
Veuillez noter que le compteur de la boucle intérieure -
r - n'est pas utilisé dans le corps de cette boucle. Pour cette raison, il s'avère que pendant toutes les itérations de la boucle intérieure, les mêmes opérations sont effectuées sur les mêmes éléments - après tout, les indices utilisent également le compteur de la boucle extérieure (
c ), et non pas la boucle intérieure (
r ).
Voyons ce qui a été trouvé d'autre intéressant.
public RegexpFormattingSettings( bool putOptionalInSquareBrackets, bool showAnyElementAsQuestionMark, bool ignoreElementDistributionDetails, int truncationLength, bool escapeCharacters, bool useLazyQuantifier) { this.PutOptionalInSquareBrackets = putOptionalInSquareBrackets; this.ShowAnyElementAsQuestionMark = showAnyElementAsQuestionMark; this.IgnoreElementDistributionDetails = ignoreElementDistributionDetails; this.TruncationLength = truncationLength; this.EscapeCharacters = escapeCharacters; }
Avertissement PVS-Studio : le paramètre constructeur V3117 'useLazyQuantifier' n'est pas utilisé. Durée d'exécution RegexpFormattingSettings.cs 38
Le constructeur n'utilise pas un paramètre -
useLazyQuantifier . Cela semble particulièrement suspect dans le contexte du fait qu'une propriété avec le nom et le type correspondants est définie dans la classe -
UseLazyQuantifier . Apparemment, ils ont oublié de l'initialiser via le paramètre correspondant.
Rencontré plusieurs gestionnaires d'événements potentiellement dangereux. Un exemple de l'un d'eux est donné ci-dessous:
public class RecommenderRun { .... public event EventHandler Started; .... public void Execute() {
PVS-Studio Warning :
V3083 Invocation non
sûre de l'événement 'Started', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. Évaluateur RecommenderRun.cs 115
Le fait est qu'entre la vérification de
l' inégalité
nulle et l'appel du gestionnaire, un événement peut être désabonné, et si à l'heure actuelle entre la vérification de
null et l'appel des gestionnaires l'événement n'a pas d'abonnés, une
exception NullReferenceException sera
levée . Pour éviter de tels problèmes, par exemple, vous pouvez enregistrer le lien vers la chaîne déléguée dans une variable locale ou utiliser l'opérateur «?». pour appeler des gestionnaires.
En plus de l'extrait de code ci-dessus, il y avait 35 de ces endroits.
Soit dit en passant,
785 avertissements du
V3024 ont également été respectés. L'avertissement
V3024 est émis lors de la comparaison de nombres réels à l'aide des opérateurs '! =' Ou '=='. Je ne m'attarderai pas ici sur les raisons pour lesquelles ces comparaisons ne sont pas toujours correctes - plus à ce sujet est écrit dans la documentation, il y a aussi un lien vers
StackOverflow (c'est ça).
Étant donné que les formules et les calculs ont souvent été respectés, ces avertissements peuvent également être importants, bien qu'ils soient portés au niveau 3 (car ils sont loin d'être pertinents dans tous les projets).
Si vous êtes sûr que ces avertissements ne sont pas pertinents, vous pouvez les supprimer en
un seul clic , ce qui réduit le nombre total d'opérations de l'analyseur.
Conclusion
D'une certaine manière, il s'est avéré que pendant longtemps je n'avais pas écrit d'articles sur la vérification des projets, et toucher à nouveau ce processus était assez agréable. J'espère que vous avez également appris quelque chose de nouveau / utile de cet article, ou du moins que vous l'avez lu avec intérêt.
Je souhaite aux développeurs une correction précoce des zones problématiques et je vous rappelle que faire des erreurs est normal, mais nous sommes des gens. Pour cela, des outils supplémentaires comme des analyseurs statiques sont nécessaires pour trouver ce qu'une personne a manqué, non? Quoi qu'il en soit - bonne chance avec le projet et merci pour le travail!
Et rappelez-vous que l'avantage maximal d'un analyseur statique est obtenu grâce à son
utilisation régulière .
Meilleurs vœux!
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Sergey Vasiliev.
Quelles erreurs se cachent dans le code Infer.NET?