Nous accueillons tous les amateurs d'exotisme et de peu d'erreurs dans le code. Aujourd'hui sur le banc d'essai PVS-Studio est un invité assez rare - un jeu en C #. À savoir, osu! Comme d'habitude: recherchez les erreurs, réfléchissez, jouez.
Le jeu
Osu! - Un jeu de rythme musical open source. À en juger par les informations du
site Web du jeu , il est assez populaire, car plus de 15 millions de joueurs inscrits sont déclarés. Le projet se caractérise par un gameplay gratuit, un design coloré avec la possibilité de personnaliser les cartes, des fonctionnalités avancées pour compiler un classement en ligne des joueurs, la présence du multijoueur, un large éventail de compositions musicales. Je ne décrirai pas le jeu en détail, les intéressés trouveront facilement toutes les informations sur le réseau. Par exemple,
ici .
Je suis plus intéressé par le code source du projet, qui peut être téléchargé depuis
GitHub . Un nombre important de validations (plus de 24 mille) dans le référentiel attire immédiatement l'attention, ce qui indique le développement actif du projet, qui se poursuit à ce jour (le jeu est sorti en 2007, mais le travail a probablement commencé plus tôt). Dans le même temps, il n'y a pas beaucoup de code source - 1813 fichiers .cs qui contiennent 135 000 lignes de code, à l'exclusion de celles vides. Ce code contient des tests que je ne prends généralement pas en compte dans les contrôles. Le code de test est contenu dans 306 fichiers .cs et, par conséquent, 25 000 lignes de code, à l'exclusion des lignes vides. Il s'agit d'un petit projet: à titre de comparaison, le cœur C # de l'analyseur PVS-Studio contient environ 300 000 lignes de code.
Au total, pour vérifier le jeu pour les erreurs, j'ai utilisé des projets non-test contenant 1507 fichiers de code source et 110 mille lignes. Cependant, le résultat m'a partiellement plu, car il y a eu plusieurs erreurs intéressantes dont je m'empresse de vous parler.
Erreurs
V3001 Il existe des sous-expressions identiques 'result == HitResult.Perfect' à gauche et à droite de '||' opérateur. DrawableHoldNote.cs 266
protected override void CheckForResult(....) { .... ApplyResult(r => { if (holdNote.hasBroken && (result == HitResult.Perfect || result == HitResult.Perfect)) result = HitResult.Good; .... }); }
Un bon exemple de programmation orientée copier-coller. Un terme comique que mon collègue Valery Komarov a récemment utilisé (introduit) dans son article "
Top 10 des erreurs dans les projets Java pour 2019 ".
Ainsi, deux contrôles identiques se succèdent. L'une des vérifications devrait très probablement contenir une autre
constante d' énumération
HitResult :
public enum HitResult { None, Miss, Meh, Ok, Good, Great, Perfect, }
Quelle constante particulière devait être utilisée, ou la deuxième vérification n'est-elle pas nécessaire du tout? Questions auxquelles seul le développeur peut répondre. Dans tous les cas, une erreur a été commise qui fausse la logique du programme.
V3001 Il existe des sous-expressions identiques «famille! = GetFamilyString (TournamentTypeface.Aquatico)» à gauche et à droite de l'opérateur «&&». TournamentFont.cs 64
public static string GetWeightString(string family, FontWeight weight) { .... if (weight == FontWeight.Regular && family != GetFamilyString(TournamentTypeface.Aquatico) && family != GetFamilyString(TournamentTypeface.Aquatico)) weightString = string.Empty; .... }
Et encore une fois copier-coller. J'ai formaté le code, donc l'erreur est facilement perceptible. Dans la version originale, la condition entière était écrite sur une seule ligne. Il est également difficile de dire comment le code peut être corrigé. L'énumération
TournamentTypeface contient une seule constante:
public enum TournamentTypeface { Aquatico }
La condition peut avoir utilisé la variable
familiale deux fois par erreur, mais ce n'est pas exact.
V3009 [CWE-393] Il est étrange que cette méthode renvoie toujours une seule et même valeur de 'false'. KeyCounterAction.cs 19
public bool OnPressed(T action, bool forwards) { if (!EqualityComparer<T>.Default.Equals(action, Action)) return false; IsLit = true; if (forwards) Increment(); return false; }
La méthode retournera toujours
false . Pour de telles erreurs, je vérifie généralement le code appelant, car ils n'utilisent souvent la valeur de retour nulle part, il n'y a pas d'erreur (sauf pour le style de programmation laid). Dans ce cas, je suis tombé sur le code suivant:
public bool OnPressed(T action) => Target.Children .OfType<KeyCounterAction<T>>() .Any(c => c.OnPressed(action, Clock.Rate >= 0));
Comme vous pouvez le voir, le résultat renvoyé par la méthode
OnPressed est utilisé. Et comme il est toujours
faux , le résultat de l'appel
OnPressed sera également toujours
faux . Je pense que vous devriez vérifier à nouveau ce code, car il y a une forte probabilité d'erreur.
Une autre erreur similaire:
- V3009 [CWE-393] Il est étrange que cette méthode renvoie toujours une seule et même valeur de 'false'. KeyCounterAction.cs 30
V3042 [CWE-476] Possible NullReferenceException. Le '?.' et '.' les opérateurs sont utilisés pour accéder aux membres de l'objet 'val.NewValue' TournamentTeam.cs 41
public TournamentTeam() { Acronym.ValueChanged += val => { if (....) FlagName.Value = val.NewValue.Length >= 2
Dans la condition de l'opérateur
?: , La variable
val.NewValue n'est pas sûre. L'analyseur a fait une telle conclusion, car dans la branche alors, une option sûre est utilisée pour accéder à la variable via l'
instruction d' accès conditionnel
val.NewValue? .Substring (....) .
Une autre erreur similaire:
- V3042 [CWE-476] Possible NullReferenceException. Le '?.' et '.' les opérateurs sont utilisés pour accéder aux membres de l'objet 'val.NewValue' TournamentTeam.cs 48
V3042 [CWE-476] Possible NullReferenceException. Le '?.' et '.' les opérateurs sont utilisés pour accéder aux membres de l'objet 'api' SetupScreen.cs 77
private void reload() { .... new ActionableInfo { Label = "Current User", ButtonText = "Change Login", Action = () => { api.Logout();
Ce code est moins clair, mais je pense que l'erreur est toujours là. Créez un objet de type
ActionableInfo . Le champ
Action est initialisé avec un lambda, dans le corps duquel il n'est pas sûr de travailler avec une
API de référence potentiellement nulle. L'analyseur a considéré ce modèle comme une erreur, car lors de l'initialisation du paramètre
Value , la variable
api fonctionne en toute sécurité. J'ai appelé l'erreur ambiguë, car le code lambda suppose une exécution retardée et, peut-être, le développeur garantit en quelque sorte une valeur non nulle du lien
api . Mais ce n'est qu'une hypothèse, car le corps lambda ne contient aucun signe de travail sûr avec le lien (contrôles préliminaires, par exemple).
V3066 [CWE-683] Ordre incorrect possible des arguments passés à la méthode 'Atan2': 'diff.X' et 'diff.Y'. SliderBall.cs 182
public void UpdateProgress(double completionProgress) { .... Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI); .... }
L'analyseur soupçonnait qu'en travaillant avec la méthode
Atan2 de la classe
Math , le développeur mélangeait l'ordre des arguments.
Annonce Atan2 :
Comme vous pouvez le voir, les valeurs ont été transférées dans l'ordre inverse. Je ne peux pas juger si c'est une erreur, car la méthode
UpdateProgress contient beaucoup de calculs non triviaux. Je note juste le fait d'un problème possible dans le code.
V3080 [CWE-476]
Déréférence nulle possible. Pensez à inspecter «Beatmap». WorkingBeatmap.cs 57
protected virtual Track GetVirtualTrack() { .... var lastObject = Beatmap.HitObjects.LastOrDefault(); .... }
L'analyseur a souligné le danger d'accès via le lien nul
Beatmap . Voici ce que c'est:
public IBeatmap Beatmap { get { try { return LoadBeatmapAsync().Result; } catch (TaskCanceledException) { return null; } } }
Eh bien, l'analyseur a raison.
Pour plus d'informations sur la façon dont PVS-Studio détecte ces erreurs, ainsi que sur les innovations C # 8.0 liées à des sujets similaires (travail avec des références potentiellement nulles), consultez l'article "
Types de référence nullables en C # 8.0 et analyse statique ".
V3083 [CWE-367] L'appel non sécurisé de l'événement 'ObjectConverted', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. BeatmapConverter.cs 82
private List<T> convertHitObjects(....) { .... if (ObjectConverted != null) { converted = converted.ToList(); ObjectConverted.Invoke(obj, converted); } .... }
Erreur non critique et assez courante. Entre la vérification de la
nullité de l'événement et son invocation, ils peuvent se désinscrire de l'événement, ce qui entraînera le plantage du programme.
L'un des correctifs:
private List<T> convertHitObjects(....) { .... converted = converted.ToList(); ObjectConverted?.Invoke(obj, converted); .... }
V3095 [CWE-476] L'objet 'colonnes' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 141, 142. SquareGraph.cs 141
private void redrawProgress() { for (int i = 0; i < ColumnCount; i++) columns[i].State = i <= progress ? ColumnState.Lit : ColumnState.Dimmed; columns?.ForceRedraw(); }
Il n'est pas sûr de contourner la collection de
colonnes dans une boucle. Dans le même temps, le développeur a supposé que le lien des
colonnes pouvait être nul, car plus tard dans le code, un opérateur d'accès conditionnel était utilisé pour accéder à la collection.
V3119 L' appel de l'événement ignoré 'OnNewResult' peut entraîner un comportement imprévisible. Envisagez d'implémenter explicitement les accesseurs d'événements ou utilisez un mot clé «scellé». DrawableRuleset.cs 256
private void addHitObject(TObject hitObject) { .... drawableObject.OnNewResult += (_, r) => OnNewResult?.Invoke(r); .... } public override event Action<JudgementResult> OnNewResult;
L'analyseur avertit des dangers liés à l'utilisation d'un événement ignoré ou virtuel. Quel est exactement le danger - je suggère de lire la
description du diagnostic. Aussi, à un moment donné, j'ai écrit un article sur ce sujet, «
Événements virtuels en C #: quelque chose s'est mal passé ».
Une autre construction dangereuse similaire dans le code:
- V3119 L'appel d'un événement ignoré peut entraîner un comportement imprévisible. Envisagez d'implémenter explicitement les accesseurs d'événements ou utilisez un mot clé «scellé». DrawableRuleset.cs 257
V3123 [CWE-783] Peut-être le '??' L'opérateur fonctionne d'une manière différente de celle attendue. Sa priorité est inférieure à la priorité des autres opérateurs dans sa partie gauche. OsuScreenStack.cs 45
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; }
Pour une meilleure compréhension du problème - je vais donner un exemple synthétique de la façon dont le code fonctionne maintenant:
x = (c * a) ?? b;
L'erreur a été commise du fait que l'opérateur * a une priorité plus élevée que l'opérateur ??. Version corrigée du code (crochets ajoutés):
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f); }
Une autre erreur similaire dans le code:
V3123 [CWE-783] Peut-être le '??' L'opérateur fonctionne d'une manière différente de celle attendue. Sa priorité est inférieure à la priorité des autres opérateurs dans sa partie gauche. FramedReplayInputHandler.cs 103
private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime - NextFrame?.Time ?? 0) <= AllowedImportantTimeSpan; } }
Ici, comme dans le fragment de code précédent, la priorité des opérateurs n'a pas été prise en compte. Maintenant, l'expression passée à la méthode
Math.Abs est évaluée comme suit:
(a – b) ?? 0
Code corrigé:
private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime – (NextFrame?.Time ?? 0)) <= AllowedImportantTimeSpan; } }
V3142 [CWE-561] Code inaccessible détecté. Il est possible qu'une erreur soit présente. DrawableHoldNote.cs 214
public override bool OnPressed(ManiaAction action) { if (!base.OnPressed(action)) return false; if (Result.Type == HitResult.Miss)
L'analyseur prétend que le
code du gestionnaire
OnPressed , en commençant par la deuxième
instruction if , est inaccessible. Cela découle de l'hypothèse que la première condition est toujours vraie, c'est-à-dire que la méthode
base.OnPressed retournera toujours
false .
Jetez un œil à la méthode
base.OnPressed :
public virtual bool OnPressed(ManiaAction action) { if (action != Action.Value) return false; return UpdateResult(true); }
Nous passons à la méthode
UpdateResult :
protected bool UpdateResult(bool userTriggered) { if (Time.Elapsed < 0) return false; if (Judged) return false; .... return Judged; }
Notez que l'implémentation de la propriété
Judged n'est pas importante ici, car la logique de la méthode
UpdateResult implique que la dernière
instruction de retour est équivalente à ceci:
return false;
Ainsi, la méthode
UpdateResult retournera toujours
false , ce qui entraînera une erreur avec du code inaccessible dans le code au-dessus de la pile.
V3146 [CWE-476]
Déréférence nulle possible de 'jeu de règles'. Le 'FirstOrDefault' peut renvoyer la valeur nulle par défaut. APILegacyScoreInfo.cs 24
public ScoreInfo CreateScoreInfo(RulesetStore rulesets) { var ruleset = rulesets.GetRuleset(OnlineRulesetID); var mods = Mods != null ? ruleset.CreateInstance()
L'analyseur considère que l'appel de
ruleset.CreateInstance () n'est pas sûr. La variable d'ensemble de
règles obtient précédemment la valeur suite à l'appel de
GetRuleset :
public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....);
Comme vous pouvez le voir, l'avertissement de l'analyseur est justifié, car la chaîne d'appel contient
FirstOrDefault , qui peut retourner
null .
Conclusion
En général, le projet de jeu "osu!" Satisfait d'un petit nombre d'erreurs. Néanmoins, je recommande aux développeurs de faire attention aux problèmes détectés. Et laissez le jeu continuer de plaire à ses fans.
Et pour ceux qui aiment approfondir le code, je vous rappelle que l'analyseur PVS-Studio, qui est facile à
télécharger sur le site officiel, sera d'une bonne aide. Je note également que les vérifications de projet ponctuelles, telles que celles décrites ci-dessus, n'ont rien à voir avec l'utilisation d'un analyseur statique dans le travail réel. Une efficacité maximale dans la lutte contre les erreurs ne peut être atteinte qu'avec une utilisation régulière de l'outil à la fois sur le serveur de build et directement sur l'ordinateur du développeur (analyse incrémentale). L'objectif maximum est d'empêcher les erreurs de pénétrer dans le système de contrôle de version, en corrigeant les défauts déjà au stade de l'écriture du code.
Bonne chance et succès!
Les références
Il s'agit de la première publication en 2020. Profitant de cette opportunité, je fournirai des liens vers des articles sur la vérification des projets C # l'année dernière:
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Sergey Khrenov.
Jouez "osu!", Mais attention aux bugs .