Статический анализатор HuntBugs: проверяем IntelliJ IDEA

Как многие помнят, некоторое время я развивал статический анализатор Java-байткода FindBugs. Однако проблем в FindBugs накопилось столько, что я решил, что будет проще написать новый анализатор байткода. Я не очень творчески назвал его HuntBugs. Разработка ведётся на GitHub. Он пока в ранней стадии разработки, иногда глючит и покрывает примерно 35% диагностик из FindBugs, но при этом добавляет свои интересные штуки. Попробовать можно на вашем Maven-проекте с помощью команды mvn one.util:huntbugs-maven-plugin:huntbugs (отчёт пишется в target/huntbugs/report.html). Альтернативно можно собрать вручную из гита и запустить приложение командной строки one.util.huntbugs.HuntBugs, которому можно подавать на вход JAR-файлы или каталоги с .class-файлами.

Как-нибудь потом, когда проект несколько повзрослеет, я расскажу о нём более подробно. А в этой статье я покажу, чего интересного нашёл HuntBugs в IntelliJ IDEA Community Edition. Я скачал с официального сайта и поставил последнюю версию этой IDE, а затем натравил HuntBugs на файл lib/idea.jar, в котором почти всё и лежит. Я люблю тестировать статический анализ на IDEA, потому что это IDE, в которой самой есть очень неплохой статический анализатор и разработчики им явно пользуются. Интересно посмотреть, что остаётся после него.

Формат этой статьи не особо отличается от того, что делает PVS-Studio: ошибки, куски кода, объяснения. Разумеется, в статью вошло только самое интересное.

Field is assigned to itself

Как правило ошибок типа this.field = this.field уже никто не допускает, даже не самая новая IDE обычно о таких предупредит. Однако HuntBugs умеет смотреть немного глубже. Вот фрагмент кода:

  private int myLastOffsetInNewTree;
  ...

  private int getNewOffset(ASTNode node){
    int optimizedResult = haveNotCalculated;
    ...
    if (myLastNode == prev) {
        ...
        optimizedResult = myLastOffsetInNewTree;

        myLastNode = node;
        myLastOffsetInNewTree = optimizedResult;
        ...
    }
  }

Поле myLastOffsetInNewTree загружено в локальную переменную optimizedResult, а затем почему-то опять сохранено в поле, хотя за это время оно поменяться не могло. Последнее приваивание странное, либо его надо убрать, либо имелось в виду что-то другое.

An integer value is cast to float and passed to the rounding method

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

    final int width = icon.getIconWidth();
    final int height = icon.getIconHeight();
    final int x = (int)Math.ceil((actionButton.getWidth() - width) / 2);
    final int y = (int)Math.ceil((actionButton.getHeight() - height) / 2);

Здесь дважды используется округление вверх (Math.ceil), но аргументом в обоих случаях подаётся целое число, так как в Java деление целого на целое выдаёт целое (с округлением вниз). Вероятно, подразумевалось поделить на 2.0 или иным образом перейти к дробным числам перед делением.Если же текущее поведение устраивает, то (int)Math.ceil следует убрать: эта часть кода бесполезна.

Switch branch is unreachable due to expression range

Весьма любопытный фрагмент кода, который, видимо, кто-то когда-то автоматически сгенерировал, а теперь уже никто не понимает, правильно ли это и что там должно быть:

    int state = getState() & 0xF;

    tokenType = fixWrongTokenTypes(tokenType, state);
    if (...) {

      // TODO: do not know when this happens!
      switch (state) {
        case __XmlLexer.DOCTYPE:
          tokenType = XmlTokenType.XML_DECL_START;
          break;
      }
    }

Константа __XmlLexer.DOCTYPE имеет значение 24, но выше выполняется state = getState() & 0xF, поэтому значение state может быть только от 0 до 15 и ветка switch гарантировано не выполнится. Возможно, когда в очередной раз меняли исходный файл лексера, константы были перегенерированы с другими значениями, а этот файл перегенерировать забыли. Так или иначе, код весьма подозрительный, о чём свидетельствует и комментарий.

Synchronization on getClass() rather than class literal

Этот фрагмент класса MatcherImpl синхронизируется на getClass(). Причём это делается в публичном нефинальном классе, у которого реально есть подкласс Matcher. В результате при выполнении этого кода из подкласса синхронизация будет происходить не по MatcherImpl.class, а по Matcher.class. Проблему усугубляет то, что в этом же классе есть явная синхронизация по MatcherImpl.class и обе критические секции (которые могут оказаться не взаимоисключающими) обновляют одно и то же статическое поле lastMatchData. В результате весь смысл синхронизации теряется. Обычно synchronized(getClass()) — это неправильно, используйте явный литерал класса synchronized(MatcherImpl.class).

Exception created and dropped rather than thrown

Довольно частая в Java ошибка: объект исключения создан, но не бросается. Например, здесь:

public void remove() {
  new OperationNotSupportedException();
}

О таких ситуациях IDEA сама тоже предупреждает. Ещё аналогичное место.

Invariant loop condition

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

boolean r = ...;
while (r) {
  if (!value(b, l + 1)) break;
  if (!empty_element_parsed_guard_(b, "json", c)) break;
  c = current_position_(b);
}

Здесь цикл с условием по локальной переменной r, значение которой в цикле не меняется, поэтому либо в цикл вообще не зайдём, либо никогда не выйдем по условию (только по break). Если это действительно подразумевалось, то в таких случаях лучше писать if(r) { while(true) { ... } }, чтобы подчеркнуть намерение сделать бесконечный цикл.

The switch operator has identical branches

Дублирующиеся ветки оператора switch иногда выглядят разумно, но когда в них большой кусок кода, как здесь, стоит приглядеться:

switch ((((PsiWildcardType)x).isExtends() ? 0 : 1) + (((PsiWildcardType)y).isExtends() ? 0 : 2)) {
case 0: /* ? super T1, ? super T2 */
     if (constraints != null && xType != null && yType != null) {
       constraints.add(new Subtype(yType, xType));
     }
     return balance(xType, yType, balancer, constraints);

case 1: /* ? extends T1, ? super T2 */
     if (constraints != null && xType != null && yType != null) {
       constraints.add(new Subtype(xType, yType));
     }
     return balance(xType, yType, balancer, constraints);

case 2: /* ? super T1, ? extends T2*/
     return null;

case 3: /* ? extends T1, ? extends T2*/
     if (constraints != null && xType != null && yType != null) {
       constraints.add(new Subtype(xType, yType));
     }
     return balance(xType, yType, balancer, constraints);
}

Не сразу заметно, но case 1 и case 3 абсолютно одинаковы (и отличаются от case 0). Если это и имелось в виду, возможно, разумнее объединить case 1 и case 3, чтобы было проще читать и поддерживать код.

The same condition is repeatedly checked

В этом коде зачем-то одно и то же условие проверяется два раза:

if (offsetToScroll < 0) {
  if (offsetToScroll < 0) {
    ...
  }
}

Может, просто внутреннюю проверку надо убрать, а может и что-то другое хотели проверить. Вот ещё аналогичный случай. Или вот ещё интересный случай:

        return o instanceof PsiElement && ((PsiElement)o).isValid() && ((PsiElement)o).isPhysical() ||
               o instanceof ProjectRootModificationTracker ||
               o instanceof PsiModificationTracker ||
               o == PsiModificationTracker.MODIFICATION_COUNT ||
               o == PsiModificationTracker.OUT_OF_CODE_BLOCK_MODIFICATION_COUNT || // <<<
               o == PsiModificationTracker.OUT_OF_CODE_BLOCK_MODIFICATION_COUNT || // <<<
               o == PsiModificationTracker.JAVA_STRUCTURE_MODIFICATION_COUNT;

А вот тут повторные условия не совсем рядом и их заметить ещё сложнее:

return SUPPORTED_TYPES.contains(token) || StdArrangementTokens.Regexp.NAME.equals(token) 
       || StdArrangementTokens.Regexp.XML_NAMESPACE.equals(token) || KEEP.equals(token)
       || BY_NAME.equals(token) || SUPPORTED_TYPES.contains(token);

Условие SUPPORTED_TYPES.contains(token) проверяется дважды. Разумеется, HuntBugs внимательно следит, чтобы между этими условиями ничего не поменялось. Если бы в промежуточных условиях token переприсваивался, такая конструкция имела бы право на существование.

Numeric comparison is always true or false

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

int size = myPanels.length;
final Dimension preferredSize = super.getPreferredSize();
if (size >= 0 && size <= 20) {
  return preferredSize;
}

В переменную size записана длина массива, которая никогда не может быть отрицательной. Неясно, зачем проверять size >= 0. Даже если ошибки нет, я считаю, что такие проверки надо удалять, потому что они смущают читателя. Неизвестно, может автор подразумевал size > 0, тогда это ошибка.

The chain of private methods is never called

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

@Nullable
private static JsonSchemaObject getChild(JsonSchemaObject current, String name) {
  JsonSchemaObject schema = current.getProperties().get(name);
  if (schema != null) return schema;

  schema = getChildFromList(name, current.getAnyOf()); // <<<
  if (schema != null) return schema;
  ...
}

@Nullable
private static JsonSchemaObject getChildFromList(String name, List<JsonSchemaObject> of) {
  if (of == null) return null;
  JsonSchemaObject schema;
  for (JsonSchemaObject object : of) {
    schema = getChild(object, name); // <<<
    if (schema != null) return schema;
  }
  return null;
}

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

Useless String.substring(0)

Честно говоря, не ожидал увидеть такую диагностику в production-коде, слишком уж она тривиальная. Но нет, бывают и тривиальные ошибки:

String str = (String)value;
if (str.startsWith(""")) {
  str = str.substring(0);
  str = StringUtil.trimEnd(str, """);
}

Видимо, автор подразумевал удалить первый символ строки, но по какой-то причине написал не substring(1), а substring(0) (этот вызов просто возвращает исходную строку). Это второй случай (помимо dropped exception), когда сама IDEA тоже подсвечивает проблемное место.

Result of integer multiplication promoted to long

Это предупреждение не всегда ведёт к реальной опасности, но тем не менее хочется показать пример:

final long length = myIndexStream.length();
long totalCount = length / INDEX_ENTRY_SIZE; // INDEX_ENTRY_SIZE = 26
for(int i=0; i<totalCount; i++) {
  final long indexOffset = length - (i + 1) * INDEX_ENTRY_SIZE;

Во-первых, уже подозрительно, что переменная цикла имеет тип int, а не long (возможно, на такую ситуацию стоит сделать отдельную диагностику). Если totalCount превышает 2³¹, то цикл никогда не завершится. Но ладно, это возможно только при длине индекса length больше 52 гигабайт, что всё-таки немало. Однако проблемы в этом коде начнутся уже при длине больше 2 гигабайт. Так как i и INDEX_ENTRY_SIZE имеют тип int, то умножение будет выполняться над 32-битными знаковыми целыми и успешно переполнится. Уже после этого результат умножения будет приведён к long и после выполнения вычитания смещение может вполне оказаться больше длины. Вероятно, таких больших кэшей никогда тут не было, но будет неприятно, когда они появятся. Исправить просто — объявить переменную цикла long.

А что с Котлином?

Известно, что часть IntelliJ IDEA написана на Котлине, который также компилируется в Java-байткод. Статические анализаторы байткода формально могут анализировать любой язык, но фактически если анализатор заточен на Java, то для других языков будет много ложных срабатываний. Часто они берутся оттого, что компилятор языка генерирует какие-то специфические конструкции (например, неявные проверки). Иногда, впрочем, такое ложное срабатывание — повод приглядеться к кодогенератору компилятора. Вот, например, класс com.intellij.configurationStore.FileBasedStorageKt. В самом классе есть такая строка:

private val XML_PROLOG = "<?xml version="1.0" encoding="UTF-8"?>".toByteArray()

В классе java.lang.String метода toByteArray(), как известно, нет. Это extension-method Котлина, причём inline-метод (который компилятор встраивает прямо в место использования), по умолчанию он выполняет String.getBytes(Charsets.UTF_8). Давайте посмотрим, во что эта строка скомпилировалась в Котлине. Я не буду показывать прямо байткод, а преобразую его в более понятный код на Java:

String str = "<?xml version="1.0" encoding="UTF-8"?>";
Charset charset = null;
int mask = 1;
Object obj = null; // зачем нужна эта переменная и какого она типа — я не знаю
// здесь зачем-то nop - пустая операция
if(obj != null) {
    throw new UnsupportedOperationException("Super calls with default arguments not supported in this target, function: toByteArray");
}
if(mask & 1 == 0) {
    charset = kotlin.text.Charsets.UTF_8;
}
// здесь опять зачем-то nop
XML_PROLOG = kotlin.jvm.internal.Intrinsics.checkExpressionValueIsNotNull(((String)str).getBytes(charset), 
                                              "(this as java.lang.String).getBytes(charset)");

Видно, что строка разрослась неимоверно. Переменная mask связана с передачей дефолтного параметра (про это рассказывал Дмитрий Жемеров на JPoint — смотрите слайд 40 и ниже. Здесь, очевидно, много лишнего, и HuntBugs справедливо ругается и на obj != null (сравнение null с null), и на mask & 1. Хотя автор кода совершенно не виноват. Надо полагать, со временем компилятор Котлина будет умнее и будет генерировать меньше мусора.

Заключение

Здесь можно написать обычный текст о важности статического анализа, который пишет Andrey2008 с коллегами после своих статей, но вы и так всё знаете. Интересно, что даже в коде, который разрабатывается с использованием статического анализа, удалось найти немало подозрительных мест, просто проверив его новым инструментом. Разумеется, в статью попало не всё. Помимо ложных сработок есть немало сообщений важных, но скучных. Много сообщений про производительность. Например, конкатенация строк в цикле — 59 штук. Или обход значений Map через keySet()+get(), когда быстрее через values() — 18 штук. Большое количество потенциальных проблем с многопоточностью. Скажем, неатомарные обновления volatile-полей — 50 штук. Или подозрительные сценарии использования wait()/notify() — 8 штук.

Пользуйтесь статическим анализом и следите за новостями!

Автор: lany

Источник