# Нашёл ошибки в моде Create и проверил их в игре

В этой статье мы разберём ошибки в одном из самых популярных модов для Minecraft — Create\. Посмотрим, как они проявляются прямо в игре, и отправим пулл\-реквесты с исправлениями\.

![1417_Create_ru/image1.png](https://import.viva64.com/docx/blog/1417_Create_ru/image1.png)

## Введение

[Create](https://modrinth.com/mod/create) — это модификация для Minecraft, которая добавляет валы, шестерни и прочую механику\. Из них собираются машины, работающие на вращении, от простой мельницы до полностью автоматизированного [завода по производству тортиков](https://www.youtube.com/watch?v=rR8W-f9YhYA)\.

Сам по себе этот мод очень популярен: только на CurseForge [у него больше 206 миллионов](https://www.curseforge.com/minecraft/mc-mods/create) скачиваний\. И это без учёта [Modrinth](https://modrinth.com/mod/create) и того, что мод до сих пор портируется на новые версии игры\. Я уверен, что почти каждый зашедший в эту статью читать тоже наверняка играл в этот мод\. 

![1417_Create_ru/image2.png](https://import.viva64.com/docx/blog/1417_Create_ru/image2.png)

## Проверка проекта

<details>
   <summary>Важная информация</summary>

* Для проверки проекта мы будем использовать статический анализатор PVS\-Studio, разработчиком которого автор статьи и является\.
* Во время чтения вы встретите примеры кода\. Большинство из них сокращено, чтобы не перегружать читателя\. Пометкой сокращённого кода является многоточие "\.\.\.\."\.
* На момент проверки проекта последней ревизией был коммит [87b3c6a](https://github.com/Creators-of-Create/Create/tree/87b3c6a65fd00c023a07b37b0353144bc7e6a5bf), её мы и будем проверять статическим анализатором\.
* Все исходники, которые проверялись, и все суждения, основанные на других исходных файлах, снабжены постоянной ссылкой, по которой их можно найти\.
* В статье приведены только те ошибки, что показались интересными именно автору \(да, вкусовщина\)\. Если кто\-то хочет посмотреть и на остальные, то всегда можно скачать анализатор и проверить проект самостоятельно\.


</details>


### Ctrl \+ V не работает

Файл [SchematicEditScreen\.java\(130\)](https://github.com/Creators-of-Create/Create/blob/0924e93639ad5f61cfc39a221d909e16f2893df1/src/main/java/com/simibubi/create/content/schematics/client/SchematicEditScreen.java#L130)

Заглянем в меню, отвечающее за вставку схематика\.

<details>
   <summary>Что такое схематик?</summary>

Схематик — это фрагмент игрового мира, сохранённый в отдельный файл \(так можно, например, перенести свой дом из одного мира в другой\)\.


</details>


```cpp
public boolean keyPressed(int code, ....) {
  if (isPaste(code)) {
    String coords = minecraft.keyboardHandler.getClipboard();
    if (coords != null && !coords.isEmpty()) {
      coords.replaceAll(" ", "");             // <=
      String[] split = coords.split(",");
      if (split.length == 3) {
        boolean valid = true;
        for (String s : split) {
          try {
            Integer.parseInt(s);
          } catch (NumberFormatException e) {
            valid = false;
          }
        }
        if (valid) {
          xInput.setValue(split[0]);
          yInput.setValue(split[1]);
          zInput.setValue(split[2]);
          return true;
        }
      }
    }
  }
  ....
}
```

Предупреждение PVS\-Studio:

[V6010](https://pvs-studio.ru/ru/docs/warnings/v6010/) The return value of function 'replaceAll' is required to be utilized\. SchematicEditScreen\.java 130

Задумка следующая: игрок скопировал координаты, нажал Ctrl \+ V в специальном окне, и поля в нём заполнились сами\.

Но вызов `replaceAll` в строке, помеченной `// <=`, бесполезен\. Строки в Java неизменяемы, поэтому метод не правит исходную строку, а возвращает новую\. Она тут же теряется, и пробелы остаются на месте\.

Дальше строка режется по запятой, и во втором и третьем элементах остаётся пробел\. И `Integer.parseInt(" 67")` бросает [`NumberFormatException`](https://docs.oracle.com/javase/8/docs/api/java/lang/NumberFormatException.html)\. В итоге поля остаются пустыми\.

![1417_Create_ru/image3.png](https://import.viva64.com/docx/blog/1417_Create_ru/image3.png)

Открыв меню, я увидел ожидаемое поведение:

* `-156, 67, 204` — не работает;
* `-156,67,204` — работает\.

Исправление умещается в одну строку: нужно добавить присваивание перед операцией `replaceAll`:

```cpp
coords = coords.replaceAll(" ", "");
```

### Ненужный максимум

Файл [ScheduleScreen\.java\(557\)](https://github.com/Creators-of-Create/Create/blob/0924e93639ad5f61cfc39a221d909e16f2893df1/src/main/java/com/simibubi/create/content/trains/schedule/ScheduleScreen.java#L557)

```cpp
for (List<ScheduleWaitCondition> list : entry.conditions) {
  int maxWidth = getConditionColumnWidth(list);
  for (int i = 0; i < list.size(); i++) {
    ScheduleWaitCondition scheduleWaitCondition = list.get(i);
    Math.max(maxWidth, renderInput(....));   // <=
    scheduleWaitCondition.renderSpecialIcon(....);
  }
  AllGuiTextures.SCHEDULE_CONDITION_APPEND.render(
    graphics, 
    xOffset + (maxWidth - 10) / 2,
    29 + list.size() * 18
  );
  xOffset += maxWidth + 10;
}
```

Предупреждение PVS\-Studio:

[V6010](https://pvs-studio.ru/ru/docs/warnings/v6010/) The return value of function 'max' is required to be utilized\. ScheduleScreen\.java 557

Суть ошибки хорошо объясняет само сообщение анализатора — вызов `Math.max(....)` никуда не записывается\. 

Я полез в историю, ожидая увидеть след рефакторинга: подсчёт ширины вынесли в отдельный метод, присваивание результата в `maxWidth` убрали, а вызов `Math#max` оставили\. Но ничего подобного\. Файл появился 1 февраля 2022 года [одним коммитом на 941 строку](https://github.com/Creators-of-Create/Create/commit/576d00d3a0e502d418488ef463ad7eb4237560ab#diff-59b447844d14ec067c0449bccbecf6179277ee5603f60cc4cb682911967f0bc8), и `Math#max` без присваивания был в нём с первой же версии\. Изначальная инициализация `maxWidth` через вызов `getConditionColumnWidth()` там уже тоже была\.

А что изменилось бы, допиши автор `maxWidth = Math.max(....)`? Ничего\. Ширина колонки, в которой будет располагаться текст \(на фото ниже\), считается заранее и с запасом: `getConditionColumnWidth()` проходит по всей колонке и берёт максимум, так что ни одна строка за неё не вылезет\. Я проверил с помощью отладки: во всех случаях, которые мне удалось протестировать, возвращалась одна и та же ширина\. Получается, в коде ищется максимум из двух одинаковых чисел\.

![1417_Create_ru/image4.png](https://import.viva64.com/docx/blog/1417_Create_ru/image4.png)

Строка не сломалась со временем и не была затронута переделкой\. Она не делала ничего с самого первого дня — на протяжении четырёх лет и нескольких портов на новые версии Minecraft подряд\.

Предлагаю всё же убрать лишнюю обёртку и оставить `renderInput` в одиночестве\.

### Не обрабатываем ошибку

Файл [MechanicalCrafterBlockEntity\.java\(535\)](https://github.com/Creators-of-Create/Create/blob/0924e93639ad5f61cfc39a221d909e16f2893df1/src/main/java/com/simibubi/create/content/kinetics/crafter/MechanicalCrafterBlockEntity.java#L535)

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

![1417_Create_ru/image5.png](https://import.viva64.com/docx/blog/1417_Create_ru/image5.png)

<details>
   <summary>Пояснение к картинке</summary>

В обычном Minecraft вещи собирают вручную на верстаке, раскладывая ингредиенты по клеткам\. Механический крафтер делает то же самое, но сам\. Из нескольких блоков собирают сетку, каждый отвечает за одну клетку рецепта, и в него кладут свой ингредиент\. На скриншоте выложен рецепт железной кирки: три слитка в верхнем ряду и две палки по центру\.

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

Работает всё это от вращения, так что сбоку обязательно нужен привод\. На скриншоте сетку крутит творческий двигатель\.


</details>


Давайте посмотрим один метод из его логики:

```cpp
protected void continueIfAllPrecedingFinished() {
  List<MechanicalCrafterBlockEntity> preceding = 
    RecipeGridHandler.getPrecedingCrafters(this);

  if (preceding == null) { // <=
    ejectWholeGrid();
    return;
  }

  for (MechanicalCrafterBlockEntity blockEntity : preceding)
    if (blockEntity.phase != Phase.WAITING)
      return;
  
  phase = Phase.ASSEMBLING;
  countDown = 1;
}
```

Предупреждение PVS\-Studio:

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'preceding \=\= null' is always false\. New returns not\-null reference\. MechanicalCrafterBlockEntity\.java 535

Метод `getPrecedingCrafters` возвращает крафтеры, стоящие выше по цепочке, то есть те, что отдают предметы в текущий\. Вернуть `null` он не может\. Список создаётся в самом начале, и оба выхода из метода возвращают именно его\. Даже если блок не оказался крафтером, наружу уйдёт просто пустой список:

```cpp
public static List<MechanicalCrafterBlockEntity> getPrecedingCrafters(
  MechanicalCrafterBlockEntity crafter
) {
  BlockPos pos = crafter.getBlockPos();
  Level world = crafter.getLevel();
  List<MechanicalCrafterBlockEntity> crafters = new ArrayList<>();
  BlockState blockState = crafter.getBlockState();

  if (!isCrafter(blockState))
    return crafters;

  ....
  return crafters;
}
```

Получается, ветка с `ejectWholeGrid()` недостижима\.  Список `preceding` никогда не может быть `null`, но пустым бывает регулярно\. А что если заменить проверку на `isEmpty()` и проверить в игре? Давайте посмотрим\.

До:

![1417_Create_ru/image6.gif](https://import.viva64.com/docx/blog/1417_Create_ru/image6.gif)

После правки с `isEmpty`:

![1417_Create_ru/image7.gif](https://import.viva64.com/docx/blog/1417_Create_ru/image7.gif)

На видео видно, что до правки предметы при попадании в цикл перебрасывались из одного крафтера в другой бесконечно\. Потенциально это могло кому\-то насолить и сломать чей\-то завод :\)

А вот после замены на `isEmpty()` зацикливанию даже не дали образоваться\. Метод увидел, что ожидать предметов больше неоткуда, и выбросил предметы\.

Но стоило добавить к ним ещё два крафтера, и правка перестала работать:

![1417_Create_ru/image8.gif](https://import.viva64.com/docx/blog/1417_Create_ru/image8.gif)

Здесь возможны два варианта\. Либо поиск зацикливаний в механическом крафтере должен работать как\-то иначе — тогда простая правка с заменой на `isEmpty()` недостаточна\. Либо это и ситуация с зацикливанием багом не является\. В таком случае проверку на `null` можно просто удалить\.

### Падающее логирование

Файл [TrainRelocationPacket\.java\(52\)](https://github.com/Creators-of-Create/Create/blob/0924e93639ad5f61cfc39a221d909e16f2893df1/src/main/java/com/simibubi/create/content/trains/entity/TrainRelocationPacket.java#L52)

Ниже представлен обработчик клиент\-серверного пакета перемещения поезда:

```cpp
public void handle(ServerPlayer sender) {
  Train train = Create.RAILWAYS.trains.get(trainId);
  Entity entity = sender.level().getEntity(entityId);

  String messagePrefix = sender.getName()
      .getString() + " could not relocate Train ";

  if (
    train == null || // <=
    !(entity instanceof CarriageContraptionEntity cce)
  ) {
    Create.LOGGER.warn(messagePrefix + train.id.toString()
        .substring(0, 5) + ": not present on server");
    return;
  }

  if (!train.id.equals(cce.trainId))
    return;

  ....
}
```

Предупреждение PVS\-Studio:

[V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Potential null dereference of 'train'\. TrainRelocationPacket\.java 52

Условие срабатывает в двух случаях: поезд не найден, либо сущность оказалась не того класса\. Со вторым всё в порядке, в лог уйдёт аккуратное предупреждение\. А вот в первом код обращается к `train.id` ровно тогда, когда `train` равен `null`\.

Самое занятное, что сообщение, которое мы пытаемся записать, заканчивается словами `not present on server`\. Ветка написана специально для отсутствующего поезда и об него же и спотыкается\.

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

Сервер не упадёт: Create регистрирует пакеты так, что они выполняются безопасно, и даже если появится исключение, оно будет проглочено в лог, и пакет просто не обработается\.

Чинится тем, что идентификатор и так лежит в самом пакете:

```cpp
messagePrefix + trainId.toString().substring(0, 5)
```

### Теория о двух состояниях

Файл [MechanicalMixerBlockEntity\.java\(238\)](https://github.com/Creators-of-Create/Create/blob/0924e93639ad5f61cfc39a221d909e16f2893df1/src/main/java/com/simibubi/create/content/kinetics/mixer/MechanicalMixerBlockEntity.java#L238)

```cpp
protected List<Recipe<?>> getMatchingRecipes() {
  List<Recipe<?>> matchingRecipes = super.getMatchingRecipes();  
  if (!AllConfigs.server().recipes.allowBrewingInMixer.get())
    return matchingRecipes;

  Optional<BasinBlockEntity> basin = getBasin();
  if (!basin.isPresent())
    return matchingRecipes;

  BasinBlockEntity basinBlockEntity = basin.get();
  if (basin.isEmpty()) // <= 
    return matchingRecipes;

  IItemHandler availableItems = level.getCapability(
  Capabilities.ItemHandler.BLOCK, 
  basinBlockEntity.getBlockPos(), null
  );

  if (availableItems == null)
    return matchingRecipes;

  for (int i = 0; i < availableItems.getSlots(); i++) {
    ....
  }
  
  return matchingRecipes;
}
```

Предупреждение PVS\-Studio:

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'basin\.isEmpty\(\)' is always true\. MechanicalMixerBlockEntity\.java 238

В этом месте `basin.isEmpty()` никогда не вернёт `true`, `return` в этой ветке недостижим\. Скорее всего, разработчик хотел проверить `basinBlockEntity`, а не сам `Optional`\.

На геймплей игрока это никак не повлияет\. Ниже по коду есть проверки, работающие с этим же блоком, и если "чаша" окажется пустой, то либо не запустится цикл, либо `availableItems` будет `null`\.

Но мёртвый `if` опаснее, чем кажется\. Это подстраховка, которая на самом деле не страхует ни от чего\. Стоит кому\-то переписать проверки ниже, и здесь останется дыра, которую никто не станет искать, ведь проверка вроде бы на месте\.

### Важно соблюдать контракты методов

Файл [Contraption\.java\(230\)](https://github.com/Creators-of-Create/Create/blob/0924e93639ad5f61cfc39a221d909e16f2893df1/src/main/java/com/simibubi/create/content/contraptions/Contraption.java#L230)

```cpp
public static Contraption fromNBT(Level world, CompoundTag nbt, 
                  boolean spawnData) {
  String type = nbt.getString("Type");
  Contraption contraption = ContraptionType.fromType(type);
  contraption.readNBT(world, nbt, spawnData);
  ....
}
```

Увидели ошибку? А если заглянем внутрь `ContraptionType#fromType`?

```cpp
/**
 * Lookup the ContraptionType with the given ID, 
 * and create a new Contraption from it if present.
 * If it doesn't exist, returns null.
 */
@Nullable
public static Contraption fromType(String typeId) {
  ContraptionType legacy = AllContraptionTypes.BY_LEGACY_NAME
                        .get(typeId);
  if (legacy != null) {
    return legacy.factory.get();
  }
  ResourceLocation id = ResourceLocation.tryParse(typeId);
  ContraptionType type = CreateBuiltInRegistries.CONTRAPTION_TYPE.get(id);
  return type == null ? null : type.factory.get();
}
```

Теперь понятно\. Метод честно предупреждает о `null` дважды: аннотацией и комментарием\. А вызывающий код не проверяет ничего\.

Предупреждение PVS\-Studio:

[V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Potential null dereference of 'contraption'\. Contraption\.java 230

Причём сломать это проще, чем кажется\. Первая же строка берёт `nbt.getString("Type")`, а если ключа нет, вернётся пустая строка\. Такой тип в реестре тоже не найдётся, и наружу снова уйдёт `null`\.

Убедиться можно так\. Выдаём себе вагонетку с несуществующим типом штуковины:

```cpp
/give @p
create:minecart_contraption[create:minecart_contraption_data=
{Type:"create:does_not_exist",InitialOrientation:"north"}]
```

<details>
   <summary>Что такое штуковина?</summary>

Штуковина \(contraption\) — это постройка, которую мод "отрывает" от мира и превращает в один подвижный объект: платформа на вагонетке, поршень с грузом, вращающийся мост\. Блоки внутри перестают быть блоками мира и хранятся в данных одной сущности\.

И да, в русской версии мода эта конструкция [действительно называется "Штуковиной"](https://github.com/Creators-of-Create/Create/blob/0924e93639ad5f61cfc39a221d909e16f2893df1/src/main/resources/assets/create/lang/ru_ru.json#L3149)\.


</details>


Сама команда ничего не сломает, предмет спокойно ляжет в инвентарь\. А вот когда мы поставим вагонетку на рельсы, `MinecartContraptionItem` полезет собирать штуковину из этих данных, и там нас встретит [stacktrace с `NullPointerException`](https://gist.github.com/TheLivan/ad251e6a4357230f1181eb59ae06d7c3)\. В итоге вместо штуковины игрока, поставится обычная вагонетка из Minecraft\.

![1417_Create_ru/image9.png](https://import.viva64.com/docx/blog/1417_Create_ru/image9.png)

Проверку добавить несложно:

```cpp
Contraption contraption = ContraptionType.fromType(type);
if (contraption == null) {
  Create.LOGGER.warn("Unknown contraption type: {}", type);
  return null;
}
```

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

### Прерывание

Файл [ServerLagger\.java\(14\)](https://github.com/Creators-of-Create/Create/blob/0924e93639ad5f61cfc39a221d909e16f2893df1/src/main/java/com/simibubi/create/infrastructure/command/ServerLagger.java#L14)

```cpp
public void tick() {
  if (!isLagging || tickTime <= 0)
    return;

  try {
    Thread.sleep(tickTime);
  } catch (InterruptedException e) {
    e.printStackTrace();
  }
}
```

Предупреждение PVS\-Studio:

[V6103](https://pvs-studio.ru/ru/docs/warnings/v6103/) Ignored InterruptedException could lead to delayed thread shutdown\. ServerLagger\.java 14

`ServerLagger` — это вспомогательный класс для отладочной команды `killtps start <tickTime>`\. Эта команда заставляет сервер спать указанное число миллисекунд на каждом тике, чтобы посмотреть, как мод себя ведёт на просевшем TPS\. Доступна она только в debug\-сборке и только оператору \(так называется тип администратора сервера\), так что паниковать не о чем\. Но код внутри всё равно неправильный, и объяснить причину будет полезно\.

Когда поток прерывают, `sleep` немедленно бросает `InterruptedException` и заодно сбрасывает флаг прерывания\. То есть после `catch` внешне ничего не осталось: флага нет, исключения нет, есть только строчка в `System.err`\. Метод спокойно возвращает управление, и сервер продолжает работать, как будто его никто не звал\.

[Прерывание](https://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html) — это просьба остановиться, адресованная всему потоку, а не только тому месту, где оно поймано\. Перехватив его и ничего не сделав, вы забираете эту просьбу себе и выбрасываете\. Об этом как раз указано в [Javadoc](https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/lang/InterruptedException.html)\. Достаточно одной строки, чтобы передать прерывание дальше:

```cpp
} catch (InterruptedException e) {
  Thread.currentThread().interrupt();
}
```

## Заключение

На этом на сегодня всё\. Все ошибки я собрал как в статье, так и в пулл\-реквестах для разработчиков этого проекта \([тык](https://github.com/Creators-of-Create/Create/pull/10696), [тык](https://github.com/Creators-of-Create/Create/pull/10721)\)\. 

А вы хотите попробовать статический анализ? Вы всегда можете внедрить его в свой проект, используя инструмент PVS\-Studio\. Скачать пробную версию можно [здесь](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. Для открытых проектов лицензия [полностью бесплатная](https://pvs-studio.ru/ru/order/open-source-license/)\.