Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze

• Вкратце о проекте
• Небольшое отступление
• Об анализе
• Заключение

XMage - клиент-серверное приложение для игры в Magic: The Gathering (MTG). XMage начал развиваться еще в начале 2010 года. За это время было выпущено 182 релиза, набралась целая армия контрибьюторов, и проект до сих пор активно развивается. Отличный повод поучаствовать и нам в его развитии! Поэтому сегодня единорог из PVS-Studio проверит кодовую базу XMage, и кто знает, может и схлестнется с кем-нибудь в бою.

Вкратце о проекте

XMage - активно развивающееся приложение на протяжении уже 10 лет. Его цель - сделать бесплатную, с открытым исходным кодом, онлайновую версию оригинальной карточной игры Magic: the Gathering.

Возможности приложения:

• доступ к ~19 000 уникальных карт, выпущенных за 20-летнюю историю MTG;
• автоматический контроль и применение всех существующих правил игры;
• многопользовательский режим с поиском игроков на общем сервере;
• одиночный режим с игрой против компьютера (AI);
• десятки форматов и режимов игры (Standard, Modern, Vintage, Commander и многое другое);
• возможность проведения как одиночных матчей, так и турниров.

Небольшое отступление

Случайно наткнулся на работу студентов из Делфтского технического университета 2018 года (магистерский курс Software Architecture). Она заключалась в том, что ребята принимали активное участие в open-source проектах, которые должны были быть достаточно сложными и активно развиваться. В течение восьминедельного периода студенты изучали курс и open-source проекты, чтобы понять и описать архитектуру выбранного программного обеспечения.

Так вот. В этой работе ребята анализировали проект XMage, и одним из аспектов их работы было получение различных метрик при помощи SonarQube (количество строк кода, цикломатическая сложность, дублирование кода, запахи кода, ошибки, уязвимости и т.д.).

Моё внимание привлекло то, что на момент 2018 года сканирование SonarQube'ом показало 700 дефектов (bugs, vulnerabilities) на 1 000 000 строк кода.

Покопавшись в истории ребят-контрибьюторов, я выявил, что из полученного отчета с предупреждениями они сделали pull-request на исправление примерно 30 дефектов из категории "Blocker" или "Critical". Что с остальными предупреждениями - неизвестно, но надеюсь на то, что их не пропустили мимо глаз.

С тех пор прошло уже 2 года и кодовая база подросла примерно на 250 000 строк кода - неплохой повод посмотреть, как там дела.

Об анализе

Для анализа я взял релиз XMage - 1.4.44V0.

С проектом очень повезло. Собрать XMage при помощи Maven получилось очень просто (как и было написано в документации):

mvn clean install -DskipTests

Больше от меня ничего не потребовалось. Круто же?

С интеграцией плагина PVS-Studio в Maven тоже не возникло проблем -все как в документации.

После анализа было получено 911 предупреждений, из которых 674 приходится на предупреждения 1 и 2 уровня достоверности. В рамках данной статьи я не рассматривал предупреждения 3 уровня достоверности, так как там обычно велик процент ложных срабатываний. Хочу обратить ваше внимание на то, что при использовании статического анализатора в реальном бою игнорировать такие предупреждения нельзя, так как они могут также указывать на значимые дефекты в коде.

Кроме того, я не рассматривал предупреждения некоторых правил из соображения, что их лучше рассматривать тем, кто хорошо знаком с проектом, нежели мне:

• V6022, которое ищет неиспользуемые параметры в методах/конструкторах. На их долю пришлось аж 336 срабатываний.
• V6014, которое предупреждает о том, что все ветки выхода из метода возвращают одно и то же значение. 73 срабатывания.
• V6021, которое сигнализирует о том, что в переменную записывается некий результат и про эту переменную забывают. 36 срабатываний.
• V6048, которое предупреждает о том, что выражение можно упростить. 17 срабатываний.

Плюс к этому несколько диагностических правил выдали примерно 20 явных однотипных ложных срабатываний. Записали в todo!

В итоге, если все вычесть, то к рассмотрению ко мне попало примерно 190 срабатываний.

При просмотре срабатываний было выявлено много однотипных незначительных дефектов, которые были связаны либо с отладкой, либо с бессмысленной проверкой или операцией. Также много срабатываний было связано с очень странным фрагментом кода, который так и напрашивался на рефакторинг.

В итоге, для данной статьи я выделил 11 диагностических правил и разобрал по одному самому интересному срабатыванию.

Давайте глянем, что вышло.

Предупреждение N1

V6003 The use of 'if (card != null) {...} else if (card != null) {...}' pattern was detected. There is a probability of logical error presence. 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) {
      ....
  }
  ....
}

Тут все просто: тело второго условного оператора if (card != null) в конструкции if-else-if никогда не выполнится, так как выполнение программы либо не дойдет до этого места, либо card != null будет всегда false.

Предупреждение N2

V6004 The 'then' statement is equivalent to the 'else' statement. 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);
  }
}

Банальная ошибка, которая частенько встречалась на моей практике проверок open-source проектов. Copy-paste? Или я что-то не понимаю? Предположу, что в ветке else всё-таки нужно возвращать false.

P.S. Если что, тут нет рекурсивного вызова applies(....), так как это разные методы.

Аналогичное срабатывание:

• V6004 The 'then' statement is equivalent to the 'else' statement. GuiDisplayUtil.java(194), GuiDisplayUtil.java(198)

Предупреждение N3

V6007 Expression 'filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")' is always false. 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();
}

Срабатывания диагностического правила V6007 достаточно популярны для каждого проверяемого проекта. XMage не исключение (79 штук). Срабатывания правила, в принципе, все по делу, но много случаев приходится то на debug, то на перестраховывание, то еще на что. В общем, такие срабатывания лучше смотреть автору кода, нежели мне.

Данное срабатывание, однако, точно является ошибкой. В зависимости от начала строки filter.getMessage() к sb добавляется текст " has ...", либо " have ...". Но ошибочка в том, что разработчики проверяют, чтобы строка начиналась с заглавной буквы, преобразовав перед этим эту самую строку в нижний регистр. Упс. В результате добавленной строкой всегда будет " have ...". Результат дефекта не критический, но тоже неприятный: где-то будет фигурировать неграмотно составленный текст.

Срабатывания, которые мне показались наиболее интересными:

• V6007 Expression 't.startsWith("-")' is always false. BoostSourceEffect.java(103)
• V6007 Expression 'setNames.isEmpty()' is always false. DownloadPicturesService.java(300)
• V6007 Expression 'existingBucketName == null' is always false. S3Uploader.java(23)
• V6007 Expression '!lastRule.endsWith(".")' is always true. Effects.java(76)
• V6007 Expression 'subtypesToIgnore::contains' is always false. VerifyCardDataTest.java(893)
• V6007 Expression 'notStartedTables == 1' is always false. MageServerImpl.java(1330)

Предупреждение N4

V6008 Null dereference of '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);
  }
}

Анализатор ругается на разыменование нулевой ссылки savedSpecialRares, когда выполнение дойдет до первого заполнения коллекции.

Первое, что приходит на ум: просто перепутали savedSpecialRares == null с savedSpecialRares != null. Но в таком случае NPE может произойти в конструкторе ArrayList при возвращении коллекции из метода, так как savedSpecialRares == null по-прежнему не исключено. Исправлять код первым пришедшим в голову решением не очень хороший вариант. Немного разобравшись с кодом, выяснил, что savedSpecialRares сразу определяется пустой коллекцией при объявлении и при этом больше нигде не переприсваивается. Это говорит нам о том, что savedSpecialRares никогда не будет null, и разыменование нулевой ссылки, о котором предупреждает анализатор, так и не произойдет, так как до заполнения коллекции дело так и не дойдет. Как итог, метод всегда будет возвращать пустую коллекцию.

P.S. Для исправления нужно заменить savedSpecialRares == null на savedSpecialRares.isEmpty().

P.P.S. Увы, но пока что, играя в XMage, не получится получить специальные редкие карточки для коллекции Dragon's Maze.

Еще один случай разыменования нулевой ссылки:

• V6008 Null dereference of 'match'. TableController.java(973)

Предупреждение N5

V6012 The '?:' operator, regardless of its conditional expression, always returns one and the same value 'table.getCreateTime()'. TableManager.java(418), TableManager.java(418)

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

Здесь тернарный оператор ?: возвращает одно и тоже значение вне зависимости от условия table.getStartTime() == null. Полагаю, что автодополнение кода сыграло злую шутку с разработчиком. Вариант исправления:

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

Предупреждение N6

V6026 This value is already assigned to the 'this.loseOther' variable. BecomesCreatureTypeTargetEffect.java(54)

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

Дублирующая строка присваивания. Выглядит так, словно разработчик немного увлёкся использованием горячих клавиш и не заметил этого. Но с учетом того, что effect имеет большое количество полей, фрагмент заслуживает того, чтобы на нем заострили внимание.

Предупреждение N7

V6036 The value from the uninitialized 'selectUser' optional is used. 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(); // <=
  ....
}

По предупреждению анализатора можно сделать вывод, что selectUser.get() может бросить исключение NoSuchElementException.

Давайте рассмотрим более детально что тут происходит.

Если верить комментарию, что user уже существует, то исключения не возникнет:

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

В таком случае выполнение программы не будет заходить в тело условного оператора. И все будет в порядке. Но тогда возникает вопрос: зачем нужен условный оператор с какой-то сложной логикой, если он так и не выполнится?

А что, если комментарий ничего из себя не представляет?

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

Тогда выполнение заходит в тело условного оператора и заново получает пользователя через getUserByName(). Пользователя снова проверяют на валидность, и это наталкивает на мысль, что selectUser может быть неинициализированным. Ветки else на этот случай нет, что далее и приведет к NoSuchElementException на рассматриваемой строке кода.

Предупреждение N8

V6042 The expression is checked for compatibility with type 'A' but is cast to 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);
  }
}

Автор кода что-то здесь запутался: сначала убеждается в том, что model не является CheckBoxListModel, а потом в итоге явно приводит объект к этому типу. Из-за этого метод setModel сразу же выбросит ClassCastException, добравшись до этого места.

Файл CheckBoxList.java был добавлен 2 года назад, и эта ошибка живет в коде до сих пор. Тестов на некорректные параметры, видимо, нет, реального использования этого метода c объектами неподходящих типов тоже нет, поэтому и живет.

Если вдруг кто-нибудь завяжется на этот метод и прочтет Javadoc, то будет ожидать IllegalArgumentException, а не ClassCastException. Не думаю, что кто-то намеренно будет нарываться на это исключение, но кто знает.

Учитывая документацию, код, скорее всего, должен выглядеть следующим образом:

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

Предупреждение N9

V6060 The 'player' reference was utilized before it was verified against 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 предупреждает разработчика о том, что происходит обращение к объекту до того, как производится его проверка на null. Срабатывания этого правила частенько встречаются в статьях о проверках open-source проектов: обычно причиной этого становится неудачный рефакторинг или смена контрактов у методов. Если обратить внимание на объявление метода getPlayer(), то всё сразу станет на свои места:

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

Предупреждение N10

V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of '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));
        }
    }
}

Посмотрев, что эта ошибка в тестах, вы можете сразу же обесценить найденный дефект, подумав: "Нууу, это же теесты". Если это так, то я с вами не согласен. Ведь тесты играют достаточно важную роль в разработке (хотя и не настолько заметную, как программирование), и при проявлении дефекта в релизе сразу же начинают тыкать пальцами на тесты/тестировщиков. Так вот, дефектные тесты несостоятельны. Зачем тогда такие тесты нужны? Зачем тратить ресурсы на них?

Метод testArcaneAdaptationGiveType() тестирует карточку "Arcane Adaptation". Каждому игроку раздаются карты в определенную игровую зону. И благодаря copy-paste в игровую зону "Кладбище" игроку playerА попали 2 одинаковые карточки "Silvercoat Lion", а игроку playerB так ничего и не досталось. Далее какая-то магия и само тестирование.

Когда доходит тестирование до "кладбища" игрока playerB в текущем розыгрыше, то в цикл выполнение теста так и не заходит, ведь в "кладбище" ничего и не было. Это я выяснил старым добрым System.out.println() при запуске теста.

Исправленный вариант copy-paste:

....
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");   // <=
....

После того как я скорректировал код, при запуске теста проверка существ в кладбище игрока playerB начала работать. Аве, System.out.println()!

Тест при этом зеленый и до, и после исправления, что является большим везением. Но в случае каких-либо правок, изменяющих логику выполнения программы, такой тест окажет вам медвежью услугу, уведомляя об успешном прохождении даже при наличии ошибок.

Такой же copy-paste в других местах:

• V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. PaintersServantTest.java(33), PaintersServantTest.java(29), PaintersServantTest.java(27), PaintersServantTest.java(31)
• V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. SubTypeChangingEffectsTest.java(32), SubTypeChangingEffectsTest.java(28), SubTypeChangingEffectsTest.java(26), SubTypeChangingEffectsTest.java(30)

Предупреждение N11

V6086 Suspicious code formatting. 'else' keyword is probably missing. 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;
  }
}

Диагностическое правило V6086 диагностирует некорректное форматирование if-else-if, подразумевающее пропуск else.

Данный фрагмент кода это и демонстрирует. В данном случае из-за выражения return null неаккуратность в форматировании ни к чему не приводит, но все же находить такие случаи круто, так как раз на раз не приходится.

Давайте рассмотрим случай, когда пропуск else может привести к неожиданному поведению:

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;
}

Теперь, в случае obj == null, рассматриваемому объекту присвоится какое-то значение, и отсутствующий else приведет к тому, что вновь присвоенный объект начнет проверяться по цепочке if-else-if, в то время как предполагалось, что объект сразу же вернется из метода.

Заключение

Проверка XMage – очередная статья, которая раскрывает возможности современных статических анализаторов. В современной разработке потребность в них только растет, так как сложность ПО увеличивается. И сколько бы у вас не было релизов, тестов, обратной связи от пользователей: баг всегда найдет лазеечку, чтобы пробраться в вашу кодовую базу. Так почему не обзавестись еще одним барьером для своей защиты?

Как вы поняли, анализаторам свойственны ложные срабатывания (в том числе и PVS-Studio Java). Это может быть результатом как явной недоработки, так и слишком запутанного кода (увы, анализатор не разобрался). Нужно к ним относиться с пониманием и без стеснения сразу же отписываться, а пока ложные срабатывания ждут своего исправления, можно воспользоваться одним из способов подавления предупреждений.

В заключение предлагаю лично "пощупать" анализатор, скачав его с нашего сайта.