# Топ 10 ошибок в проектах Java за 2020 год Новый год неумолимо приближается \- а, значит, настало время подводить итоги\. Продолжая традицию, мы прошлись по нашим статьям о проверках Java\-проектов из мира open\-source за этот год и составили рейтинг 10 самых интересных ошибок\. ![0788_Top_10_Java_bugs_2020_ru/image1.png](https://import.viva64.com/docx/blog/0788_Top_10_Java_bugs_2020_ru/image1.png) За уходящий год мы \(Java\-команда PVS\-Studio\) разобрали в наших статьях ошибки из пяти open\-source проектов и совсем немного рассказали про нашу внутреннюю кухню: * [АНБ, Ghidra и единороги](https://pvs-studio.ru/ru/blog/posts/java/0738/) * [Под капотом PVS\-Studio для Java: разработка диагностик](https://pvs-studio.ru/ru/blog/posts/java/0752/) * [Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze](https://pvs-studio.ru/ru/blog/posts/java/0758/) * [Проверка WildFly \- сервера JavaEE приложений](https://pvs-studio.ru/ru/blog/posts/java/0763/) * [Единороги на страже вашей безопасности: исследуем код Bouncy Castle](https://pvs-studio.ru/ru/blog/posts/java/0775/) * [Big/Bug Data: анализируем исходный код Apache Flink](https://pvs-studio.ru/ru/blog/posts/java/0781/) Предлагаем читателю сначала ознакомиться с этими статьями и составить свой личный рейтинг, чтобы потом сравнить его с нашим и сказать, что мы неправы :\)\. ## Десятое место: "Обманчивое равенство" Источник: [Big/Bug Data: анализируем исходный код Apache Flink](https://pvs-studio.ru/ru/blog/posts/java/0781/) [V6001](https://pvs-studio.ru/ru/docs/warnings/v6001/) There are identical sub\-expressions 'processedData' to the left and to the right of the '\=\=' operator\. CheckpointStatistics\.java\(229\) ```cpp @Override public boolean equals(Object o) { .... CheckpointStatistics that = (CheckpointStatistics) o; return id == that.id && savepoint == that.savepoint && triggerTimestamp == that.triggerTimestamp && latestAckTimestamp == that.latestAckTimestamp && stateSize == that.stateSize && duration == that.duration && alignmentBuffered == that.alignmentBuffered && processedData == processedData && // <= persistedData == that.persistedData && numSubtasks == that.numSubtasks && numAckSubtasks == that.numAckSubtasks && status == that.status && Objects.equals(checkpointType, that.checkpointType) && Objects.equals( checkpointStatisticsPerTask, that.checkpointStatisticsPerTask); } ``` Простая и очень обидная ошибка из\-за невнимательности: поле _processedData_ сравнивается с самим собой\. Из\-за этой ошибки сравнение объектов типа _CheckpointStatistics_ иногда будет выдавать ложноположительный результат\. Но основная опасность этой опечатки состоит в том, что _equals_ крайне активно используется в коллекциях, и некорректная реализация этого метода может привести к очень странному поведению, на отладку которого уйдёт огромное количество времени\. Хочу заметить, что ошибаться в функциях сравнения для разработчиков привычное дело\. Мой коллега даже написал большую статью "[Зло живет в функциях сравнения](https://pvs-studio.ru/ru/blog/posts/cpp/0509/)" с множеством примеров и объяснений\. ## Девятое место: "Недостижимый код" Источник: [Единороги на страже вашей безопасности: исследуем код Bouncy Castle](https://pvs-studio.ru/ru/blog/posts/java/0775/)\. [V6019](https://pvs-studio.ru/ru/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. XMSSTest\.java\(170\) ```cpp public void testSignSHA256CompleteEvenHeight2() { .... int height = 10; .... for (int i = 0; i < (1 << height); i++) { byte[] signature = xmss.sign(new byte[1024]); switch (i) { case 0x005b: assertEquals(signatures[0], Hex.toHexString(signature)); break; case 0x0822: assertEquals(signatures[1], Hex.toHexString(signature)); break; .... } } } ``` Ветвь _switch_ для значения _i_ _\=\=_ _0x0822\(2082\)_ оказалась недостижимой\. Как же так получилось? Если обратить внимание на условие цикла _1 << height_ , где _height_ всегда равен 10_,_ то всё сразу встанет на свои места\. Согласно условию цикла, счётчик _i_ в цикле _for_ не может быть больше, чем 1024 \(1 << 10\)\. Естественно, выполнение рассматриваемой ветви _switch_ никогда не произойдет\. ## Восьмое место: "Проаннотированный метод" Источник: [Под капотом PVS\-Studio для Java: разработка диагностик](https://pvs-studio.ru/ru/blog/posts/java/0752/)\. [V6009](https://pvs-studio.ru/ru/docs/warnings/v6009/) Collection is empty\. The call of the 'clear' function is senseless\. MetricRepositoryRule\.java\(90\) ```cpp protected void after() { this.metricsById.clear(); this.metricsById.clear(); } ``` Часть наших диагностик сильно полагается на механизм аннотирования методов\. Аннотации предоставляют дополнительную информацию анализатору об используемых методах, например: * Чистый ли это метод, * Какие накладываются ограничения на аргументы, * Возвращаемый результат, * \.\.\. и всякое прочее\. Некоторые аннотации анализатор выводит сам из исходного кода, некоторые мы проставляем вручную \(например, для методов стандартной библиотеки\)\. История этой ошибки началась с того, что мы не в полной мере проаннотировали метод Map\#clear\. После того, как мы это заметили и исправили, на наших тестовых проектах повылезали новые срабатывания, среди которых был и наш интересный случай\. На первый взгляд, повторная очистка словаря – не ошибка\. И мы бы даже решили, что это случайно продублированная строка, если бы не обратили внимание на поля класса: ```cpp private final Map metricsByKey = new HashMap<>(); private final Map metricsById = new HashMap<>(); ``` У класса есть два поля с похожими именами _metricsById_ и _metricsByKey_\. Это и наталкивает на мысль, что автор кода хотел очистить оба словаря, но \.\.\. этого не произошло\. Таким образом, два словаря, которые хранят связанные данные, будут рассинхронизированы после вызова _after_\. ## Седьмое место: "Ожидание / реальность" Источник: [Проверка WildFly \- сервера JavaEE приложений](https://pvs-studio.ru/ru/blog/posts/java/0763/)\. [V6058](https://pvs-studio.ru/ru/docs/warnings/v6058/) The 'equals' function compares objects of incompatible types: String, ModelNode\. JaxrsIntegrationProcessor\.java\(563\) ```cpp // Send value to RESTEasy only if it's not null, empty string, or the // default value. private boolean isTransmittable(AttributeDefinition attribute, ModelNode modelNode) { if (modelNode == null || ModelType .UNDEFINED.equals(modelNode.getType())) { return false; } String value = modelNode.asString(); if ("".equals(value.trim())) { return false; } return !value.equals(attribute.getDefaultValue()); // <= } ``` Обратив внимание на предшествующий методу комментарий, можно ожидать, что метод вернет _true,_ если: 1. _modelNode _не _null,_ 1. строковое представление _modelNode_ не пустое, 1. _modelNode_ – не значение по умолчанию\. Несмотря на авторский комментарий и корректную, на первый взгляд, логику, поведение метода будет отличаться\. Причиной этому служит проверка _modelNode_ на равенство со значением по умолчанию в последней строчке метода\. Строковое представление _modelNode_ сравнивается с объектом типа _ModelNode_, и, как можно догадаться, такое сравнение всегда будет возвращать отрицательный результат из\-за несовместимости типов\. Последствия ошибки: непредвиденное разрешение к отправке значения _modelNode,_ когда оно равно значению по умолчанию \(_attribute\.getDefaultValue\(\)_\)\. ## Шестое место: "Копипаст\-ориентированное программирование" Источник: [Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze](https://pvs-studio.ru/ru/blog/posts/java/0758/)\. [V6072](https://pvs-studio.ru/ru/docs/warnings/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\) ```cpp @Test public void testArcaneAdaptationGiveType() { addCard(Zone.HAND, playerA, "Arcane Adaptation", 1); 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)); } } } ``` В этом году, как и в прошлом \([Топ 10 ошибок за 2019](https://pvs-studio.ru/ru/blog/posts/java/0699/)\), классная copy\-paste ошибка от диагностического правила V6072 заслуживает место в десятке\. Характер ошибки заключается в том, что когда разработчику нужно сделать схожие действия для различных переменных, то он копирует добросовестно написанный ранее код и изменяет название переменной\. Но делает это уже не совсем добросовестно и забывает доизменить переменные\. В данном фрагменте кода так и произошло\. Автор теста имитировал игру между игроками, раскидывая между ними одинаковые карты по игровым зонам, но из\-за copy\-paste игроку _playerA_ дважды досталась одна и та же карта\. Из\-за этого игровая зона _Zone\.GRAVEYARD_ игрока _playerB_ осталась без тестирования\. Подробное описание ошибки можно почитать в самой статье\. ## Пятое место: "Ненормальное распределение" Источник: [Big/Bug Data: анализируем исходный код Apache Flink](https://pvs-studio.ru/ru/blog/posts/java/0781/) [V6048](https://pvs-studio.ru/ru/docs/warnings/v6048/) This expression can be simplified\. Operand 'index' in the operation equals 0\. CollectionUtil\.java\(76\) ```cpp public static Collection> partition(Collection elements, int numBuckets) { Map> buckets = new HashMap<>(numBuckets); int initialCapacity = elements.size() / numBuckets; int index = 0; for (T element : elements) { int bucket = index % numBuckets; // <= buckets.computeIfAbsent(bucket, key -> new ArrayList<>(initialCapacity)) .add(element); } return buckets.values(); } ``` Ошибка была обнаружена в утилитном методе _partition_, который разбивает переданную коллекцию _elements _на _numBuckets_ коллекций\. Суть ошибки в том, что индекс коллекции _bucket_, в которую хотят поместить каждый рассматриваемый элемент, имеет константное значение \(0\)\. Причиной этому служит то, что разработчик забыл инкрементировать переменную _index_ на каждой итерации цикла\. Вследствие чего метод _partition_ будет всегда возвращать коллекцию _elements_, обернутую в другую коллекцию\. А это – вряд ли задуманное поведение\. ## Четвертое место: "Бомба замедленного действия" Источник: [АНБ, Ghidra и единороги](https://pvs-studio.ru/ru/blog/posts/java/0738/)\. [V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Null dereference of 'selectedNode' in function 'setViewPanel'\. OptionsPanel\.java\(266\) ```cpp private void processSelection(OptionsTreeNode selectedNode) { if (selectedNode == null) { setViewPanel(defaultPanel, selectedNode); // <= return; } .... } private void setViewPanel(JComponent component, OptionsTreeNode selectedNode) { .... setHelpLocation(component, selectedNode); .... } private void setHelpLocation(JComponent component, OptionsTreeNode node) { Options options = node.getOptions(); .... } ``` В приведенном фрагменте кода явно напортачили\. Если вы проследите за _selectedNode_ из _processSelection\(\)_, когда_ selectedNode \=\= null_, то сразу же обнаружите, что при таком исходе нас ждет неминуемый _NullPointerException_\. О чем и предупреждает нас анализатор\. Но, изучив немного код, автор статьи пришел к выводу, что выполнение программы никогда не встретится с _NullPointerException,_ так как _processSelection_\(\) вызывается всего в двух местах, перед вызовом которых _selectedNode_ явно проверяется на _null\._ Несмотря на это, такой код – бомба замедленного действия, поскольку другой разработчик может увидеть, что метод явно обрабатывает случай _selectedNode \=\= null_, и решить, что это валидное значение, что потом выльется в падение приложения\. ## Третье место: "Всегда false" Источник: [Проверка кода XMage и почему недоступны специальные редкие карточки для коллекции Dragon's Maze](https://pvs-studio.ru/ru/blog/posts/java/0758/)\. [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'filter\.getMessage\(\)\.toLowerCase\(Locale\.ENGLISH\)\.startsWith\("Each "\)' is always false\. SetPowerToughnessAllEffect\.java\(107\) ```cpp @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(); } ``` Кто же сравнивает строку, приведенную в нижний регистр, со строкой, которая начинается с заглавной буквы? Отсюда и всегда ложный результат проверки сообщения\. Результат дефекта не критический, но тоже неприятный: где\-то будет фигурировать неграмотно составленный текст\. ## Второе место: "2\-в\-1" Источник: [АНБ, Ghidra и единороги](https://pvs-studio.ru/ru/blog/posts/java/0738/)\. [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'index \>\= 0' is always true\. ExternalNamesTableModel\.java\(105\) [V6019](https://pvs-studio.ru/ru/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. ExternalNamesTableModel\.java\(109\) ```cpp public void setValueAt(Object aValue, int row, int column) { .... int index = indexOf(newName); if (index >= 0) { // <= Window window = tool.getActiveWindow(); Msg.showInfo(getClass(), window, "Duplicate Name", "Name already exists: " + newName); return; } ExternalPath path = paths.get(row); // <= .... } private int indexOf(String name) { for (int i = 0; i < paths.size(); i++) { ExternalPath path = paths.get(i); if (path.getName().equals(name)) { return i; } } return 0; } ``` Метод _indexOf_ всегда возвращает неотрицательное число\. А всё из\-за того, что автор метода в случае отсутствия искомого _newName_ по ошибке возвращает 0, а не \-1\. Такая ошибка приводит к тому, что поток выполнения программы всегда будет заходить в then\-ветку условного оператора _if \(index \>\= 0\)_, в котором будет выдавать сообщение о существующем _newName_ и успешно выходить из метода, даже тогда, когда в реальности _newName_ не был найден\. Но и это ещё не всё\. Так как then\-ветка условного оператора прекращает выполнение метода, то до кода после условного оператора дело так и не дойдет\. Об этом и предупреждает нас анализатор\. ## Первое место: "А то ли мы проверили?" Источник: [Под капотом PVS\-Studio для Java: разработка диагностик](https://pvs-studio.ru/ru/blog/posts/java/0752/)\. [V6080](https://pvs-studio.ru/ru/docs/warnings/v6080/) Consider checking for misprints\. It's possible that an assigned variable should be checked in the next condition\. Menu\.java\(40\) ```cpp public class Menu { private Map> menus = new HashMap>(); public void putMenuItem(String menu, String item) { List items = menus.get(menu); if (item == null) // <= { items = new ArrayList(); menus.put(menu, items); } items.add(item); } .... } ``` По задумке автора предполагалось создать коллекцию по ключу _menu_, если таковой ещё не было\. Но проверка не той переменной разрушила всю задумку, прорубив лазеечку для _NullPointerException\._ Метод выбросит исключение, когда в словаре ключ _menu_ будет отсутствовать, и значение _item_, которое хотели добавить, не будет _null_\. ## Заключение Проверки open\-source проектов с помощью PVS\-Studio из года в год доказывают, что такой рубеж защиты, как статический анализ кода, должен обязательно присутствовать в разработке\. Каким бы вы мастером своего дела ни были, ошибки обязательно найдут лазеечку в ваш проект, и причин этому множество: устали, завал на работе или вовсе отвлеклись на котиков\. А если вы работаете в команде, то количество возможностей попасть ошибкам в код вырастает пропорционально количеству коллег\. Если вам понравился наш обзор, то не ждите следующего конца года\. Статьи о проверках начнутся сразу же с первого месяца 2021, а если вам не терпится – смелее [скачивайте](https://pvs-studio.ru/ru/pvs-studio/download/) анализатор и самостоятельно проверяйте open\-source проекты\.