vlade1k 16 hours ago

Карты, Java, 2 null'а. XMage edition

Medium

13 min

581

PVS-Studio corporate blogJava*Programming*Open source*Perfect code*

Компьютерная игра на Java — вещь довольно редкая, но всегда интересная. Поэтому мы не упустили возможность проверить статическим анализатором проект XMage и поделиться результатами. Посмотрим, что нашёл PVS-Studio в исходном коде проекта.

О проекте

XMage — проект, который является бесплатной open source версией игры Magic: the Gathering. Игра позволяет драться в карточных сражениях как локально с ботами, так и онлайн с друзьями или случайными игроками.

Проект полностью написан на Java и уже больше 10 лет активно развивается: сейчас в репозитории больше 47 тысяч коммитов и его размер — 1.8 миллионов строк. Пять лет назад мы уже проверяли XMage и написали статью. Тогда проект был меньше на ~800 тысяч строк, и мы решили посмотреть, как ситуация обстоит сейчас.

Важно упомянуть, что в проекте используются различные средства контроля качества кода. И статический анализ в том числе. Так что нам было вдвойне интересно посмотреть, что найдёт наш анализатор. Приступим.

Unit-тесты всему голова

Извечный вопрос: если тесты контролируют качество написанного кода, то что контролирует качество написанных тестов?

В этой части статьи мы рассмотрим ошибки, найденные анализатором в unit-тестах.

Проверяется не то

@Test
public void test_NamesEquals() {
  ....
  // same names (empty names can't be same)
  Assert.assertFalse(CardUtil.haveSameNames(
        "", 
        ""));
  Assert.assertFalse(CardUtil.haveSameNames(
        EmptyNames.FACE_DOWN_CREATURE.getObjectName(), 
        ""));
  Assert.assertFalse(CardUtil.haveSameNames(
        EmptyNames.FACE_DOWN_CREATURE.getObjectName(), 
        EmptyNames.FACE_DOWN_CREATURE.getObjectName()));
  Assert.assertFalse(CardUtil.haveSameNames(
        EmptyNames.FACE_DOWN_TOKEN.getObjectNam(), 
        ""));
  Assert.assertFalse(CardUtil.haveSameNames(
        EmptyNames.FACE_DOWN_TOKEN.getObjectNam(), 
        EmptyNames.FACE_DOWN_CREATURE.getObjectName())); // <=
  ....
}

Предупреждение PVS-Studio: V6072 Two similar code fragments were found. Perhaps, this is a typo and 'FACE_DOWN_TOKEN' variable should be used instead of 'FACE_DOWN_CREATURE'. AliasesApiTest.java 30

Анализатор указывает, что, возможно, в отмеченной строке стоит использовать FACE_DOWN_TOKEN вместо FACE_DOWN_CREATURE.

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

Ситуация выглядит так, что перед нами результат неудачной копипасты. FACE_DOWN_CREATURE проверяется на идентичность с собой же, а FACE_DOWN_TOKEN — нет.

Я заменил константу, и тест прошёл удачно.

Неравный бой

@Test
public void test_Simple_LongGame() {
    ....
    addCard(Zone.LIBRARY, playerA, "Mountain", 10);
    addCard(Zone.LIBRARY, playerA, "Forest", 10);
    addCard(Zone.LIBRARY, playerA, "Lightning Bolt", 20);
    addCard(Zone.LIBRARY, playerA, "Balduvian Bears", 10);
    //
    addCard(Zone.LIBRARY, playerB, "Mountain", 10);
    addCard(Zone.LIBRARY, playerA, "Forest", 10);        // <=
    addCard(Zone.LIBRARY, playerB, "Lightning Bolt", 20);
    addCard(Zone.LIBRARY, playerB, "Balduvian Bears", 10);
    ....
}

Предупреждение PVS-Studio: V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. SimulationPerformanceAITest.java 60

Думаю, увидев стрелку в приведённом выше фрагменте кода, вы поняли, почему он здесь :)

В этом тесте проверяется, что драка AI vs AI с точки зрения перфоманса программы происходит адекватно. Однако нечестно. Господину A досталось больше лесных карт, нежели господину B. Опять же, выглядит как результат неудачной копипасты. Также, как и в предыдущем случае, при исправлении этого фрагмента ничего не поломалось — тест зелёный.

Очепятки и странные моменты

Логический переполох

Предупреждение PVS-Studio: V6008 Null dereference of 'permanent'. ThreeTreeScribe.java 78

@Override
public boolean checkTrigger(GameEvent event, Game game) {
    ....
    Permanent permanent = zEvent.getTarget();
    return (    permanent != null 
            || !permanent.isControlledBy(getControllerId()))  // <=
            && (permanent.getId().equals(getSourceId()) 
                || permanent.isCreature(game));
}

В этом фрагменте программы явно есть ошибочное условие, которое заставляет немного улыбнуться. Внутри return мы разыменовываем permanent в случае, если он равен null.

Следующая ситуация тоже про диагностику V6008 и тоже про условия. Но на один фрагмент кода сразу два срабатывания:

V6008 Potential null dereference of 'player'. DelifsCone.java 104

V6008 Potential null dereference of 'permanent'. DelifsCone.java 104

@Override
public boolean apply(Game game, Ability source) {
    Player player = game.getPlayer(source.getControllerId());
    Permanent 
        permanent = game.getPermanent(....);
    if (player != null || permanent != null) {
        player.gainLife(permanent.getPower().getValue(), game, source);
        return true;
    }
    return false;
}

Проверять на null лишь один объект, а разыменовывать сразу оба из них, неправильно. Явно в условии подразумевалось && вместо ||. Такие опечатки, кстати, в разных проектах мы встречаем достаточно часто.

Ну, почти...

static {
    for (int count = 1; count <= 6; count++) {
        FilterCard filter = new FilterCreatureCard(
                "creature card that's exactly " +
                        CardUtil.numberToText(count) +
                        " color" + (count > 0 ? "s" : "")
        );
        filter.add(new EvolvingDoorPredicate(count));
        filterMap.put(count, filter);
    }
}

Предупреждение PVS-Studio: V6007 Expression 'count > 0' is always true. EvolvingDoor.java 76

Действительно, странное условие. Если посмотреть на формируемую строку, видно, что вне зависимости от значения цикла будет добавляться окончание "s" к слову "color". С заменой 0 на 1 в английской грамматике всё будет хорошо и цикл отработает как надо.

Всё, что мы сделали – мы уже могли

Предупреждение PVS-Studio: V6060 The 'table' reference was utilized before it was verified against null. TableController.java 725

private void startGame(UUID choosingPlayerId) throws GameException {
    try {
        ....
    } catch (Exception ex) {
        logger.fatal("Error starting game table: " + table.getId(), ex);
        if (table != null) {
            managerFactory.tableManager().removeTable(table.getId());
        }
        .... 
    }
}

Проверять на null дело хорошее. Но всему своё время. Вот и здесь сначала table используют, а уже потом проверяют, не null ли он.

В другом файле практически аналогичное срабатывание:

V6060 The 'enchantment' reference was utilized before it was verified against null. PublicEnemy.java 79

@Override
public boolean applies(Permanent permanent, Ability source, Game game) {
    Permanent 
        enchantment = game.getPermanent(source.getSourceId());
    Permanent 
        enchantedCreature = game.getPermanent(enchantment.getAttachedTo());
    if (enchantment == null || enchantedCreature == null) {
        return false;
    }
    if (permanent.isControlledBy(enchantedCreature.getControllerId())) {
        return false;
    }
    return true;
}

Объект enchantment используют, чтобы создать объект enchantedCreature, а уже только потом проверяют не являются ли они null.

Недомапили

Предупреждение PVS-Studio: V6012 The '?:' operator, regardless of its conditional expression, always returns one and the same value 'card'. SkyclaveApparition.java 108

@Override
public boolean apply(Game game, Ability source) {
    .... 
    Set<UUID> owners = new HashSet<>();
    int totalCMC = exile
            .getCards(game)
            .stream()
            .filter(Objects::nonNull)
            .map(card -> owners.add(card.getOwnerId()) ? card : card) // <=
            .mapToInt(MageObject::getManaValue)
            .sum();
    ....
    return true;
}

Достаточно странный код, способный запутать того, кто не в контексте этого класса. То ли с объектом card хотели что-то сделать в одной из веток тернарного оператора, но забыли, то ли просто все объекты stream'а хотят добавить в Set owners. Если речь про второй случай, то достаточно воспользоваться методом peek вместо map.

Напоминание про методы peek и map

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

map — возвращает поток данных, состоящий из результатов применения заданной функции к поступившему ему потоку данных.

И снова null

В рамках следующего срабатывания придётся прибегнуть к небольшому межпроцедурному анализу своими глазками.

У нас есть вот такой участок кода:

@Override
public boolean apply(Game game, Ability source) {
    ....
    Card card = getCard(player, toReveal, game);
    ....
    player.moveCards(card, Zone.BATTLEFIELD, source, game);
    ....
    Permanent creature = game.getPermanent(card.getId());  // <=
    ....
}

Предупреждение PVS-Studio: V6008 Potential null dereference of 'card'. AspiringChampion.java 84

Заглянем в метод getCard, из которого мы получаем наш объект card. В нём есть вариант исполнения, при котором может вернуться null:

private static Card getCard(Player player, Cards toReveal, Game game) {
    for (Card card : player.getLibrary().getCards(game)) {
        toReveal.add(card);
        if (card.isCreature(game)) {
            return card;
        }
    }
    return null;
}

В реализациях метода moveCards, который использует объект card, это учитывается:

@Override
public boolean moveCards(Card card, ....) {
    Set<Card> cardList = new HashSet<>();
    if (card != null) {
        cardList.add(card);
    }
    return moveCards(cardList, toZone, source, game, 
                     tapped, faceDown, byOwner, appliedEffects);
}

А если мы вернёмся к изначальному фрагменту кода, то увидим, что в отмеченной строчке кода к нему обращаются, не учитывая, что он может быть null.

Вспоминаем основы Java через срабатывания анализатора

Аккуратнее со сравнением String объектов

Предупреждение PVS-Studio: V6013 Strings 'referenceName' and '""' are compared by reference. Possibly an equality comparison was intended. UrzasHotTub.java 112

private boolean sharesWordWithName(String str) {
    if (referenceName == null || referenceName == "") {
        return false;
    }
    . . . .
}

Ошибка, с которой мы часто встречаемся в различных проектах (особенно если участок кода очень возрастной). Строки не являются примитивами, поэтому, используя ==, сравниваются ссылки на объекты, а не само содержимое объектов. И несмотря на то, что в Java есть String Pool, и две разные строковые переменные могут ссылаться на один объект, не факт, что это действительно произойдёт. Это поведение является недетерминированным, и, как следствие, результат проверки тоже. Нужно использовать метод equals (а конкретно в этой ситуации — метод isEmpty).

Аккуратнее с десериализацией

Предупреждение PVS-Studio: V6087 InvalidClassException may occur during deserialization. Non-serializable ancestor 'RoomImpl' is missing an accessible no-arg constructor. GamesRoomImpl.java 32

public class GamesRoomImpl extends RoomImpl implements GamesRoom, Serializable {
    ....
}

Класс является сериализуемым, но при этом у его первого несериализуемого предка нет public no-arg конструктора:

public abstract class RoomImpl implements Room {
    ....
    public RoomImpl(ChatManager chatManager) {
        roomId = UUID.randomUUID();
        chatId = chatManager.createChatSession("Room " + roomId);
    }
    ....
}

Почему это неправильно?

У нас есть статья, в которой мы чуть-чуть посмотрели на внутренности нативной сериализации и десериализации в Java (тык). Сочту приемлемым сослаться на один её фрагмент:

Десериализуемый объект создаётся не через свой конструктор, а через конструктор своего первого несериализуемого предка. И это довольно логично, ведь десериализуемый объект и его предки, поддерживающие сериализацию, восстанавливаются из потока байтов. Для них выполнять код из конструкторов и инициализаторов бессмысленно. При этом у нас остаются несериализуемые предки. Чтобы их инициализировать, мы вызываем конструктор первого несериализуемого родителя восстанавливаемого объекта.

В случае, если при десериализации нужный конструктор не будет найден, мы увидим исключение InvalidClassException c сообщением no valid constructor.

Так что, как говорится: "Назвался Serializable — имей родителя с public no-arg конструктором".

Аккуратнее с синхронизацией

Предупреждение PVS-Studio: V6102 Inconsistent synchronization of the 'continuousRuleModifyingEffects' field. Consider synchronizing the field on all usages. ContinuousEffects.java 43

public class ContinuousEffects {
    ....
    private ContinuousEffectsList<....> 
      continuousRuleModifyingEffects = new ContinuousEffectsList<>();
    ....
    public synchronized void removeInactiveEffects(Game game) {
        ....
        continuousRuleModifyingEffects.removeInactiveEffects(game);
        ....
    }
    ....
    public synchronized void removeEndOfCombatEffects() {
        ....
        continuousRuleModifyingEffects.removeEndOfCombatEffects();
        ....
    }
    ....
    public boolean preventedByRuleModification(....) {
        for (.... effect : continuousRuleModifyingEffects) {     // <=
            if (!effect.checksEventType(event, game)) {
                continue;
            }
            ....
        }
        ....
    }
    ....
    public synchronized void addEffect(....) {
        ....
        switch (effect.getEffectType()) {
            ....
            case CONTINUOUS_RULE_MODIFICATION:
                continuousRuleModifyingEffects.addEffect(....);
                break;
            ....
        }
    }
    ....
}

В пример я привёл лишь пару мест, однако в коде в восьми методах поле continuousRuleModifyingEffects используется в синхронизированном контексте, а в девятом нет. Каждый из фрагментов я приводить не стал — в таком случае код бы занимал слишком много места.

Раз методы, через которые с нашим полем взаимодействуют, помечены как synchronized, то предполагается, что взаимодействие с объектом происходит в многопоточной среде. Однако взаимодействие с полем continuousRuleModifyingEffects в методе preventedByRuleModification никак не синхронизируется.

Изменение коллекции происходит в одном потоке (в одном из синхронизированных методов), а чтение в другом (в несинхронизированном методе). Это может привести к следующим проблемам:

выброс исключения ConcurrentModificationException, поскольку эта коллекция наследуется от ArrayList, а в несинхронизированном методе итерирование по ней происходит через for-each;
поток будет читать объект, который из-за изменения другим потоком находится в невалидном состоянии;
проблема видимости: поток, читающий коллекцию, может не увидеть изменения, сделанные другим потоком.

Чтобы таких проблема не возникало, следует синхронизировать обращения к коллекции. Это можно сделать как через синхронизированные обращения извне, так и через использование коллекций, которые уже являются потокобезопасными.

Аккуратнее с equals и hashCode

Метод equals переопределён, а hashCode нет.

public class CounterView implements Serializable {
    . . . .

    @Override
    public boolean equals(Object other) {
        if (other == this) {
            return true;
        }
        if (other == null) {
            return false;
        }
        if (!(other instanceof CounterView)) {
            return false;
        }
        CounterView oth = (CounterView) other;
        return
                (count == oth.count) &&
                        (name.equals(oth.name));
    }
}

Предупреждение PVS-Studio: V6049 Classes that define 'equals' method must also define 'hashCode' method. Class: 'CounterView'. CounterView.java 10

Лично у меня сложилось впечатление, что правило переопределения equals и hashCode у каждого джависта отложилось на подкорке. Но срабатываний, которые сообщали о нарушении контракта их переопределения, в проекте было несколько. Поэтому считаю нужным эту информацию продублировать. К тому же возможно такое, что статьи наши читают не только опытные ребята, но и те, кто только начал изучать Java. Так что предлагаю повторить основу.

Существует свод правил по переопределению методовequals и hashCode. Все их я здесь приводить не буду. Нас интересует только одно, описанное в документации к методу hashCode:

If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.

И чтобы этому правилу соответствовать, нужно переопределять методы equals и hashCode в паре. Об этом же и говорится в документации к методу equals:

It is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.

А для чего такое правило существует? Для того, чтобы получать корректные результаты при взаимодействии со структурами данных, использующими под капотом хэш-таблицу. HashMap, к примеру, использует методы hashCode и equals, чтобы проверить наличие объекта в структуре и получить его. Также при отсутствии переопределённого метода hashCode у класса в нашем HashSet могут появиться дубликаты объектов, чего мы точно не будет ожидать.

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

Аккуратнее с закрытием ресурсов

Предупреждение PVS-Studio: V6114 The 'ComputerPlayerMCTS' class contains the 'threadPoolSimulations' Closeable field, but the resources that the field is holding are not released inside the class. ComputerPlayerMCTS.java 39

public class ComputerPlayerMCTS extends ComputerPlayer {
  ....
  private ExecutorService threadPoolSimulations = null;
  ....
  protected void applyMCTS(....) {
    if (this.threadPoolSimulations == null) {
      ....
      this.threadPoolSimulations = new ThreadPoolExecutor(....);
    }
    ....
    try {
      List<Future<Boolean>> 
        runningTasks = threadPoolSimulations.invokeAll(tasks, 
                                                       thinkTime, 
                                                       TimeUnit.SECONDS);
      ....
    }
    ....
  }
  ....
}

Срабатывание указывает, что Closable-объект не закрывается. А с каких пор ExecutorService Closable? С 19 Java ExecutorSerivce реализует интерфейс AutoClosable, что позволяет удобным образом использовать ExecutorService в try-with-resource.

При этом важно уточнить, что у проекта минимально поддерживаемая версия — Java 8. А собирал и проверял я его на 21 версии. Отсюда и Closable в срабатывании :) То есть в самом проекте не подразумевается, что у них ExecutorService реализует интерфейс Closable. Тем не менее, его ресурсы закрываться должны, а здесь этого не происходит. На это указывает документация этого класса:

An unused ExecutorService should be shut down to allow reclamation of its resources.

В этой же документации есть примеры того, как можно корректно закрыть экземпляр ExecutorService.

Забавности, заставившие улыбнуться

После просмотра большого количества кода немного устаёшь, хочется отдохнуть и чем-то себя порадовать. Следующие пару фрагментов показались мне забавными, и я решил ими поделиться.

Супер-дупер технологии

public class Session {
  ....
  // TODO: enableafter full research
  private static final boolean 
    SUPER_DUPER_BUGGY_AND_FASTEST_ASYNC_CONNECTION = false; 
  ....
  public void fireCallback(final ClientCallback call) {
    ....
    try {
      if (valid && callBackLock.tryLock(50, TimeUnit.MILLISECONDS)) {
        ....
        boolean sendAsync = SUPER_DUPER_BUGGY_AND_FASTEST_ASYNC_CONNECTION
                && call.getMethod().getType().canComeInAnyOrder();        // <=
        ....
      }
      ....
    }
    ....
  }
  ....
}

Предупреждение PVS-Studio: V6019 Unreachable code detected. It is possible that an error is present. Session.java 438

Срабатывание указывает на то, что участок кода недостижимый. Взглянув на него, я расплылся в улыбке. Не каждый день встречаешься с SUPER_DUPER_BUGGY_AND_FASTEST_ASYNC_CONNECTION. К слову, в этом же классе у них есть большое обсуждение внедрения асинхронной передачи данных от сервера к клиенту во время игры. На большой стене из комментариев расписываются плюсы и минусы внедрения этого в игровой процесс. Комментарий "enable after full research" у объявления поля как раз про это :)

Реально просто передаём и не используем?

// TODO: investigate why we just discard sourceId and provide source directly?
public ZoneChangeGroupEvent(...., UUID sourceId, Ability source, ....) {
        super(....);
        this.fromZone = fromZone;
        this.toZone = toZone;
        this.cards = cards;
        this.tokens = tokens;
        this.source = source;
}

Предупреждение PVS-Studio: V6022 Parameter 'sourceId' is not used inside constructor body. ZoneChangeGroupEvent.java 23

Когда над большим проектом работает группа людей, спокойно может произойти такое, что один разработчик ни разу не сталкивался с кодом, который пишут его коллеги. Ситуация выше будто как раз об этом.

Анализатор заметил, что sourceId в конструкторе не используется. Об этом же в TODO комментарии написал один из разработчиков, что заглянул в этот класс. Лично меня это сообщение, оставленное в коде, заставило немного улыбнуться.

Конец

Надеюсь, вам, как и мне, было интересно посмотреть на код XMage, пусть хоть и совсем чуть-чуть. Меня радует, что этот проект-долгожитель до сих пор активно развивается. А ещё я рад, что на Java есть такие интересные проекты, которые являются полноценными играми.

К слову, у нас есть статья, где мы воспроизводили баги, обнаруженные анализатором в Minecraft моде, и где мы анализировали игровой движок, написанный на Java.

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

Настала пора прощаться. До скорых встреч!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Vladislav Bogdanov. Lock, Java, and two nulls: XMage edition.

Hubs: