Статический анализатор HuntBugs: проверяем IntelliJ IDEA25.05.2016 10:03

Как многие помнят, некоторое время я развивал статический анализатор 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: ошибки, куски кода, объяснения. Разумеется, в статью вошло только самое интересное.

Как правило ошибок типа 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, а затем почему-то опять сохранено в поле, хотя за это время оно поменяться не могло. Последнее приваивание странное, либо его надо убрать, либо имелось в виду что-то другое.

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

    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 следует убрать: эта часть кода бесполезна.

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

    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 гарантировано не выполнится. Возможно, когда в очередной раз меняли исходный файл лексера, константы были перегенерированы с другими значениями, а этот файл перегенерировать забыли. Так или иначе, код весьма подозрительный, о чём свидетельствует и комментарий.

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

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

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

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

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

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) { ... } }, чтобы подчеркнуть намерение сделать бесконечный цикл.

Дублирующиеся ветки оператора 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, чтобы было проще читать и поддерживать код.

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

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 переприсваивался, такая конструкция имела бы право на существование.

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

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

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

Обычно 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 of) {
  if (of == null) return null;
  JsonSchemaObject schema;
  for (JsonSchemaObject object : of) {
    schema = getChild(object, name); // <<<
    if (schema != null) return schema;
  }
  return null;
}

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

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

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

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

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

final long length = myIndexStream.length();
long totalCount = length / INDEX_ENTRY_SIZE; // INDEX_ENTRY_SIZE = 26
for(int i=0; i

private val XML_PROLOG = "".toByteArray()

String str = "";
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)");