Clever Geek Handbook

Vérification du code XMage et pourquoi les cartes rares spéciales pour la collection Dragon's Maze ne sont pas disponibles

image1.png


XMage est une application client / serveur pour jouer à Magic: The Gathering (MTG). XMage a commencé à évoluer au début de 2010. Pendant ce temps, 182 communiqués ont été publiés, toute une armée de contributeurs s'est réunie et le projet se développe toujours activement. Une excellente occasion de participer également à son développement! Par conséquent, aujourd'hui, la licorne de PVS-Studio vérifiera la base de code XMage, et qui sait, elle pourrait entrer en conflit avec quelqu'un au combat.



En bref sur le projet



XMage se développe activement depuis 10 ans maintenant. Son objectif est de créer une version en ligne gratuite et open source du jeu de cartes Magic: the Gathering original .



Fonctionnalités de l'application:



  • accès à environ 19 000 cartes uniques émises au cours des 20 ans d'histoire de MTG;
  • contrôle automatique et application de toutes les règles du jeu existantes;
  • ;
  • (AI);
  • (Standard, Modern, Vintage, Commander );
  • , .




Je suis tombé sur le travail d' étudiants de l'Université de technologie de Delft 2018 (maîtrise en architecture logicielle ). Cela consistait dans le fait que les gars prenaient une part active dans des projets open source, qui devaient être assez complexes et se développer activement. Sur une période de huit semaines, les étudiants ont étudié le cours et les projets open source pour comprendre et décrire l'architecture du logiciel sélectionné.



Alors c'est tout. Dans ce travail, les gars ont analysé le projet XMage, et l'un des aspects de leur travail était d'obtenir diverses métriques à l'aide de SonarQube (nombre de lignes de code, complexité cyclomatique, duplication de code, odeurs de code, erreurs, vulnérabilités, etc.).



Mon attention a été attirée par le fait qu'au moment de 2018, l'analyse SonarQube montrait 700 défauts (bogues, vulnérabilités) pour 1000000 de lignes de code.



Après avoir fouillé dans l'histoire des contributeurs, j'ai découvert qu'à partir du rapport reçu avec des avertissements, ils ont fait une pull-request pour corriger environ 30 défauts de la catégorie "Blocker" ou "Critical". Qu'en est-il du reste des avertissements n'est pas connu, mais j'espère qu'ils n'ont pas été manqués.



Cela fait 2 ans depuis et la base de code a augmenté d'environ 250 000 lignes de code - une bonne raison pour voir comment les choses se passent.



À propos de l'analyse



Pour analyse, j'ai pris la version XMage - 1.4.44V0 .



J'ai eu beaucoup de chance avec le projet. Construire XMage avec Maven s'est avéré très simple (comme cela a été écrit dans la documentation): 



mvn clean install -DskipTests


Rien de plus n'était exigé de moi. Cool?



L'intégration du plugin PVS-Studio dans Maven n'a pas non plus posé de problème: tout est comme dans la documentation .



Après l'analyse, 911 avertissements ont été reçus, dont 674 pour des avertissements de 1 et 2 niveaux de confiance. Pour les besoins de cet article, je n'ai pas pris en compte les avertissements de niveau 3, car il existe généralement un pourcentage élevé de faux positifs. Je voudrais attirer votre attention sur le fait que lorsque vous utilisez un analyseur statique dans une vraie bataille, vous ne pouvez pas ignorer de tels avertissements, car ils peuvent également indiquer des défauts importants dans le code.



De plus, je n'ai pas tenu compte des avertissements de certaines règles au motif qu'elles sont mieux prises en compte par ceux qui connaissent le projet que moi:



  • V6022, /. 336 .
  • V6014, , . 73 .
  • V6021, , . 36 .
  • V6048, , . 17 .


De plus, plusieurs règles de diagnostic ont produit environ 20 faux positifs évidents du même type. Enregistré dans todo!



En conséquence, si nous soustrayons tout, environ 190 points positifs m'ont été soumis pour examen.



Lors de l'examen des déclencheurs, de nombreux défauts mineurs du même type ont été identifiés, qui étaient soit liés au débogage, soit à une vérification ou à une opération dénuée de sens. De plus, beaucoup de points positifs étaient associés à un morceau de code très étrange qui demandait d'être refactorisé.



En conséquence, pour cet article, j'ai identifié 11 règles de diagnostic et analysé l'un des déclencheurs les plus intéressants.



Jetons un coup d'œil à ce qui s'est passé.



Avertissement N1



V6003 L'utilisation du modèle 'if (card! = Null) {...} else if (card! = Null) {...}' a été détectée. Il existe une probabilité de présence d'erreur logique. TorrentialGearhulk.java (90), TorrentialGearhulk.java (102) 



@Override
public boolean apply(Game game, Ability source) {
  ....
  Card card = game.getCard(....);
  if (card != null) {
      ....
  } else if (card != null) {
      ....
  }
  ....
}


Tout est simple ici: le corps de la deuxième instruction conditionnelle if (card! = Null) dans la construction if-else-if ne sera jamais exécuté, puisque le programme n'atteindra pas ce point, ou card! = Null sera toujours faux .



Avertissement N2



V6004 L' instruction «then» équivaut à l'instruction «else». AsThoughEffectImpl.java (35), AsThoughEffectImpl.java (37) 



@Override
public boolean applies(....) {
  // affectedControllerId = player to check
  if (getAsThoughEffectType().equals(AsThoughEffectType.LOOK_AT_FACE_DOWN)) {
    return applies(objectId, source, playerId, game);
  } else {
    return applies(objectId, source, playerId, game);
  }
}


Une erreur courante qui s'est souvent produite dans ma pratique de vérification des projets open-source. Copier coller? Ou est-ce que je manque quelque chose? Je suppose que vous devez toujours renvoyer false dans la branche else . PS Si quoi que ce soit, il n'y a pas d'appel récursif s'applique (....) , car ce sont des méthodes différentes. Déclenchement similaire:











  • V6004 L'instruction «then» équivaut à l'instruction «else». GuiDisplayUtil.java (194), GuiDisplayUtil.java (198)


Avertissement N3



V6007 Expression 'filter.getMessage (). ToLowerCase (Locale.ENGLISH) .startsWith ("Each")' est toujours faux. SetPowerToughnessAllEffect.java (107) 



@Override
public String getText(Mode mode) {
  StringBuilder sb = new StringBuilder();
  ....
  if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) {
    sb.append(" has base power and toughness ");
  } else {
    sb.append(" have base power and toughness ");
  }
  ....
  return sb.toString();
}


Les déclencheurs de règles de diagnostic V6007 sont très populaires pour chaque projet en cours de vérification. XMage ne fait pas exception (79 pièces). Actionnement de la règle, en principe, tout est sur le cas, mais de nombreux cas tombent sur le débogage, puis sur la réassurance, puis sur autre chose. En général, il est préférable pour l'auteur du code de surveiller de tels points positifs que pour moi.



Ce déclenchement, cependant, est définitivement une erreur. Selon le début de la ligne filter.getMessage () to sble texte "a ..." ou "ont ..." est ajouté. Mais l'erreur est que les développeurs vérifient que la chaîne commence par une lettre majuscule, après avoir converti cette même chaîne en minuscules avant cela. Oups. En conséquence, la ligne ajoutée sera toujours "have ...". Le résultat du défaut n'est pas critique, mais aussi désagréable: un texte composé de manière illettrée apparaîtra quelque part.



Les points positifs que j'ai trouvés les plus intéressants:



  • V6007 L'expression 't.startsWith ("-")' est toujours fausse. BoostSourceEffect.java (103)
  • L'expression V6007 'setNames.isEmpty ()' est toujours fausse. TéléchargerPicturesService.java (300)
  • L'expression V6007 'existingBucketName == null' est toujours fausse. S3Uploader.java (23)
  • V6007 Expression '! LastRule.endsWith (".")' Est toujours vrai. Effets.java (76)
  • L'expression V6007 'subtypesToIgnore :: contains' est toujours fausse. VerifyCardDataTest.java (893)
  • L'expression V6007 'notStartedTables == 1' est toujours fausse. MageServerImpl.java (1330)


Avertissement N4



V6008 Déréférence nulle de «savedSpecialRares». DragonsMaze.java (230) 



public final class DragonsMaze extends ExpansionSet {
  ....
  private List<CardInfo> savedSpecialRares = new ArrayList<>();
  ....
  @Override
  public List<CardInfo> getSpecialRare() {
    if (savedSpecialRares == null) {                    // <=
      CardCriteria criteria = new CardCriteria();
      criteria.setCodes("GTC").name("Breeding Pool");
      savedSpecialRares.addAll(....);                   // <=
      criteria = new CardCriteria();
      criteria.setCodes("GTC").name("Godless Shrine");
      savedSpecialRares.addAll(....);
      ....
    }
    return new ArrayList<>(savedSpecialRares);
  }
}


L'analyseur se plaint du déréférencement de la référence null savedSpecialRares lorsque l'exécution atteint le premier remplissage de la collection.



La première chose qui me vient à l'esprit est simplement de confondre saveSpecialRares == null avec savedSpecialRares! = Null. Mais dans un tel cas, NPE peut se produire dans le constructeur de ArrayList lorsque la collection est renvoyée par la méthode, car saveSpecialRares == null est toujours possible. Corriger le code avec la première solution qui me vient à l'esprit n'est pas une bonne option. Après un peu de compréhension du code, j'ai découvert que s avedSpecialRares est immédiatement défini par une collection vide lorsqu'il est déclaré et n'est réaffecté nulle part ailleurs. Cela nous dit quesaveSpecialRares ne sera jamais nul, et le déréférencement d'une référence nulle, dont l'analyseur met en garde, ne se produira jamais, car il n'atteindra jamais la collection. En conséquence, la méthode retournera toujours une collection vide.



PS Pour résoudre ce problème, vous devez remplacer savedSpecialRares == null par savedSpecialRares.isEmpty () .



PPS Hélas, en jouant à XMage, vous ne pourrez pas obtenir de cartes rares spéciales pour la collection Dragon's Maze .



Un autre cas de déréférencement d'une référence nulle:



  • V6008 Déréférence nulle de «correspondance». TableController.java (973)


Avertissement N5



V6012 L' opérateur '?:', Quelle que soit son expression conditionnelle, renvoie toujours une seule et même valeur 'table.getCreateTime ()'. TableManager.java (418), TableManager.java (418) 



private void checkTableHealthState() {
  ....
  logger.debug(.... + formatter.format(table.getStartTime() == null
                                        ? table.getCreateTime()
                                        : table.getCreateTime()) + ....);
  ....
}


Ici l'opérateur ternaire ?: Renvoie la même valeur quelle que soit la condition table.getStartTime () == null . Je crois que l'achèvement du code a joué une blague cruelle au développeur. Option de correction: 



private void checkTableHealthState() {
  ....
  logger.debug(.... + formatter.format(table.getStartTime() == null
                                        ? table.getCreateTime()
                                        : table.getStartTime()) + ....);
  ....
}


Avertissement N6



V6026 Cette valeur est déjà affectée à la variable «this.loseOther». DevientCreatureTypeTargetEffect.java (54) 



public
BecomesCreatureTypeTargetEffect(final BecomesCreatureTypeTargetEffect effect) {
  super(effect);
  this.subtypes.addAll(effect.subtypes);
  this.loseOther = effect.loseOther;
  this.loseOther = effect.loseOther;
}


Chaîne d'affectation en double. Il semble que le développeur s'est un peu emporté avec les touches de raccourci et ne l'a pas remarqué. Mais étant donné que l' effet a un grand nombre de domaines, le fragment mérite d'être focalisé.



Avertissement N7



V6036 La valeur de l'option 'selectUser' non initialisée est utilisée. Session.java (227) 



public String connectUserHandling(String userName, String password)
{
  ....
  if (!selectUser.isPresent()) {  // user already exists
      selectUser = UserManager.instance.getUserByName(userName);
      if (selectUser.isPresent()) {
          User user = selectUser.get();
            ....
      }
  }
  User user = selectUser.get(); // <=
  ....
}


À partir de l'avertissement de l'analyseur, nous pouvons conclure que selectUser.get () peut lancer une NoSuchElementException.



Regardons de plus près ce qui se passe ici.



Si vous pensez que le commentaire que l' utilisateur existe déjà, aucune exception ne sera levée: 



....
if (!selectUser.isPresent()) {  // user already exists
  ....
}
User user = selectUser.get()
....


Dans ce cas, l'exécution du programme n'entrera pas dans le corps de l'instruction conditionnelle. Et tout ira bien. Mais alors la question se pose: pourquoi avons-nous besoin d'un opérateur conditionnel avec une sorte de logique complexe s'il n'est jamais exécuté?



Mais que se passe-t-il si le commentaire n'est rien? 



....
if (!selectUser.isPresent()) {  // user already exists
    selectUser = UserManager.instance.getUserByName(userName);
    if (selectUser.isPresent()) {
      ....
    }
}
User user = selectUser.get(); // <=
....


Ensuite, l'exécution entre dans le corps de l'instruction conditionnelle et récupère l'utilisateur via getUserByName (). La validité de l'utilisateur est à nouveau vérifiée, ce qui suggère que le selectUser n'est peut-être pas initialisé. Il n'y a pas d' autre branche pour ce cas, ce qui conduira en outre à une NoSuchElementException sur la ligne de code en question.



Avertissement N8



V6042 La compatibilité de l'expression avec le type «A» est vérifiée mais elle est convertie en type «B». CheckBoxList.java (586) 



/**
 * sets the model - must be an instance of CheckBoxListModel
 * 
 * @param model the model to use
 * @throws IllegalArgumentException if the model is not an instance of
 *           CheckBoxListModel
 * @see CheckBoxListModel
 */
@Override
public void setModel(ListModel model) {
  if (!(model instanceof CheckBoxListModel)) {
    if (model instanceof javax.swing.DefaultListModel) {
       super.setModel((CheckBoxListModel)model);         // <=
    }
    else {
      throw new IllegalArgumentException(
          "Model must be an instance of CheckBoxListModel!");
    }
  }
  else {
    super.setModel(model);
  }
}


L'auteur du code est confus à propos de quelque chose ici: d'abord il s'assure que le modèle n'est pas un CheckBoxListModel , puis par conséquent, il convertit explicitement l'objet dans ce type. À cause de cela, la setModel méthode va immédiatement lancer une ClassCastException quand il obtient là.



Le fichier CheckBoxList.java a été ajouté il y a 2 ans et ce bogue persiste dans le code depuis. Apparemment, il n'y a pas de tests pour des paramètres incorrects, il n'y a pas d'utilisation réelle de cette méthode avec des objets de types inappropriés, donc elle vit.



Si soudainement quelqu'un se connecte à cette méthode et lit le Javadoc, il s'attendra à une IllegalArgumentException , pas à une ClassCastException... Je ne pense pas que quiconque se heurtera délibérément à cette exception, mais qui sait.



Compte tenu de la documentation, le code devrait très probablement ressembler à ceci: 



public void setModel(ListModel model) {
  if (!(model instanceof CheckBoxListModel)) {
     throw new IllegalArgumentException(
        "Model must be an instance of CheckBoxListModel!");  
  }
  else {
    super.setModel(model);
  }
}


Avertissement N9



V6060 La référence 'player' a été utilisée avant d'être vérifiée contre null. VigeanIntuition.java (79), VigeanIntuition.java (78) 



@Override
public boolean apply(Game game, Ability source) {
    MageObject sourceObject = game.getObject(source.getSourceId());
    Player player = game.getPlayer(source.getControllerId());
    Library library = player.getLibrary();                           // <=
    if (player != null && sourceObject != null && library != null) { // <=
        ....
    }
}


V6060 avertit le développeur qu'un objet est en cours d'accès avant qu'il ne soit vérifié pour null . Les déclencheurs de cette règle se trouvent souvent dans les articles sur la vérification des projets open-source: généralement la raison en est l'échec de la refactorisation ou la modification des contrats pour les méthodes. Si vous faites attention à la déclaration de la méthode getPlayer () , alors tout se mettra immédiatement en place: 



// Result must be checked for null.
// Possible errors search pattern: (\S*) = game.getPlayer.+\n(?!.+\1 != null)
Player getPlayer(UUID playerId);


Avertissement N10



V6072 Deux fragments de code similaires ont été trouvés. C'est peut-être une faute de frappe et la variable «playerB» devrait être utilisée à la place de «playerA». SubTypeChangingEffectsTest.java (162), SubTypeChangingEffectsTest.java (158), SubTypeChangingEffectsTest.java (156), SubTypeChangingEffectsTest.java (160) 



@Test
public void testArcaneAdaptationGiveType() {
    addCard(Zone.HAND, playerA, "Arcane Adaptation", 1); // Enchantment {2}{U}
    addCard(Zone.BATTLEFIELD, playerA, "Island", 3);

    addCard(Zone.HAND, playerA, "Silvercoat Lion");
    addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=

    addCard(Zone.HAND, playerB, "Silvercoat Lion");
    addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=

    ....

    for (Card card : playerB.getGraveyard().getCards(currentGame)) {
        if (card.isCreature()) {
            Assert.assertEquals(card.getName() + " should not have ORC type",
                    false, card.getSubtype(currentGame).contains(SubType.ORC));
            Assert.assertEquals(card.getName() + " should have CAT type",
                    true, card.getSubtype(currentGame).contains(SubType.CAT));
        }
    }
}


Ayant vu que cette erreur se trouve dans les tests, vous pouvez immédiatement dévaloriser le défaut trouvé en pensant: "Eh bien, ce sont des tests." Si tel est le cas, je ne suis pas d’accord avec vous. Après tout, les tests jouent un rôle assez important dans le développement (mais pas aussi perceptible que la programmation), et lorsqu'un défaut apparaît dans une version, ils commencent immédiatement à pointer du doigt les tests / testeurs. Ainsi, les tests défectueux sont intenables. Pourquoi alors de tels tests sont-ils nécessaires? Pourquoi gaspiller des ressources sur eux?



La méthode testArcaneAdaptationGiveType () teste la carte "Arcane Adaptation". Chaque joueur reçoit des cartes dans une zone de jeu spécifique. Et grâce au copier-coller, playerA a obtenu 2 cartes "Silvercoat Lion" identiques dans l'aire de jeu "Cemetery" , et playerBdonc rien n'est arrivé. De plus, une sorte de magie et de se tester.



Lorsque le test arrive au "cimetière" du playerB dans le rallye en cours, alors l'exécution du test n'entre jamais dans la boucle, car il n'y avait rien dans le "cimetière". C'est ce que j'ai découvert avec le bon vieux System.out.println () lors du démarrage du test .



Copie-coller corrigé: 



....
addCard(Zone.HAND, playerA, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=

addCard(Zone.HAND, playerB, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerB, "Silvercoat Lion");   // <=
....


Après avoir modifié le code, lorsque j'ai exécuté le test, la recherche de créatures dans le cimetière de playerB a commencé à fonctionner. Ave, System.out.println () !



Le test est vert avant et après la correction, ce qui est beaucoup de chance. Mais en cas de modifications qui modifient la logique de l'exécution du programme, un tel test vous rendra un mauvais service, vous informant de la réussite même s'il y a des erreurs.



Même copier-coller ailleurs:



  • V6072 Deux fragments de code similaires ont été trouvés. C'est peut-être une faute de frappe et la variable «playerB» devrait être utilisée à la place de «playerA». PaintersServantTest.java (33), PaintersServantTest.java (29), PaintersServantTest.java (27), PaintersServantTest.java (31)
  • V6072 Deux fragments de code similaires ont été trouvés. C'est peut-être une faute de frappe et la variable «playerB» devrait être utilisée à la place de «playerA». SubTypeChangingEffectsTest.java (32), SubTypeChangingEffectsTest.java (28), SubTypeChangingEffectsTest.java (26), SubTypeChangingEffectsTest.java (30)


Avertissement N11



V6086 Formatage de code suspect. Le mot-clé «else» est probablement manquant. DeckImporter.java (23) 



public static DeckImporter getDeckImporter(String file) {
  if (file == null) {
    return null;
  } if (file.toLowerCase(Locale.ENGLISH).endsWith("dec")) {   // <=
    return new DecDeckImporter();
  } else if (file.toLowerCase(Locale.ENGLISH).endsWith("mwdeck")) {
    return new MWSDeckImporter();
  } else if (file.toLowerCase(Locale.ENGLISH).endsWith("txt")) {
    return new TxtDeckImporter(haveSideboardSection(file));
  }
  ....
  else {
    return null;
  }
}


La règle de diagnostic V6086 diagnostique un formatage if-else-if incorrect , impliquant l'omission de else .



Cet extrait de code illustre cela. Dans ce cas, en raison de l'expression de retour null , une inexactitude dans la mise en forme ne conduit à rien, mais c'est cool de trouver de tels cas, car ce n'est pas nécessaire.



Considérons un cas où l'omission de l' autre peut conduire à un comportement inattendu: 



public SomeType smtMethod(SomeType obj) {
  ....
  if (obj == null) {
    obj = getNewObject();
  } if (obj.isSomeObject()) {
    // some logic
  } else if (obj.isOtherSomething()) {
    obj = calulateNewObject(obj);
    // some logic
  } 
  ....
  else {
    // some logic
  }
  return obj;
}


Maintenant, dans le cas de obj == null , l'objet en question se verra attribuer une valeur, et le else manquant provoquera la vérification de l'objet nouvellement assigné le long de la chaîne if-else-if , alors que l'objet était censé revenir immédiatement de méthode.



Conclusion



Checking XMage est un autre article qui révèle les capacités des analyseurs statiques modernes. Dans le développement moderne, leur besoin ne fait qu'augmenter, à mesure que la complexité du logiciel augmente. Et peu importe le nombre de versions, de tests, de commentaires des utilisateurs: un bogue trouvera toujours une faille pour entrer dans votre base de code. Alors pourquoi ne pas ajouter une autre barrière à votre défense?



Comme vous le comprenez, les analyseurs sont sujets aux faux positifs (y compris PVS-Studio Java). Cela peut être le résultat à la fois d'une faille évidente et d'un code trop déroutant (hélas, l'analyseur ne l'a pas compris). Vous devez les traiter avec compréhension et vous désinscrire immédiatement sans hésitation , mais pendant que les faux positifs attendent leur correction, vous pouvez utiliser l'une des méthodessuppression des avertissements.



En conclusion, je vous suggère personnellement de "toucher" l'analyseur en le téléchargeant depuis notre site Internet.





Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Maxim Stefanov. Vérification du code de XMage et pourquoi vous ne pourrez pas obtenir les cartes rares spéciales de la collection Dragon's Maze .


More articles:


All Articles