company_banner

Пылесосим код IDEA Ultimate с помощью анализа потоков данных

    IntelliJ IDEA содержит тысячи инспекций для Java-кода. Большинство из них работает как продвинутые регулярные выражения: по определённому шаблону они ищут фрагменты программы, которые выглядят как опечатки, избыточны, некрасивы или могут работать медленно. Но есть инспекция совсем другого рода. У неё несколько странное название: «Constant conditions & exceptions». В действительности она выполняет анализ потоков данных в Java-методах с помощью так называемого «символьного выполнения». В результате такого анализа могут обнаружиться некоторые подозрительные факты. Вот некоторые примеры таких фактов:

    • Разыменование ссылки может привести к NullPointerException
    • Условие всегда истинно или ложно
    • Индекс массива всегда за пределами допустимых границ
    • Приведение типа может привести к ClassCastException

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

    if (obj == null) {
      …
    } 
    else if (obj != null) { // Предупреждение: 'obj != null' всегда истинно
      …
    }

    Однако часто предупреждение от этой инспекции указывает на опечатку или говорит о том, что автор кода не совсем понял, что он написал. А главное, что эта инспекция способна обнаружить очень странные ошибки, которые не покрыты никакими шаблонами. Иногда даже мы, авторы IDE, удивляемся, глядя на код, в котором сработала инспекция «Constant conditions & exceptions».

    С годами инспекция постоянно улучшается. Раньше она в основном отвечала за возможные проблемы с null и несоответствием типов. Позже в ней появился анализ наличия значения в Optional, анализ целочисленных диапазонов, анализ границ массивов. В предыдущем мажорном выпуске 2017.3 мы улучшили анализ целочисленных диапазонов и добавили базовую поддержку цепочек Stream API и Optional.

    Каждое улучшение инспекции первым делом мы испытываем на коде IDEA Ultimate. Это крупный проект, содержащий десятки тысяч классов, которые пишутся второй десяток лет. К коду приложило руку не меньше сотни разработчиков с разным уровнем знаний и стилем. Поэтому всегда интересно прогнать «Constant conditions & exceptions» и обнаружить новый баг в древнем классе. Давайте посмотрим, что нового нашла инспекция после улучшений, сделанных для предстоящего релиза.

    Одно из самых интересных улучшений анализа в 2018.1 — это отслеживание отношений между переменными. Ранее инспекция отслеживала только равенство и неравенство переменных. Например, условие if(a == b && a != b) помечалось как всегда ложное. Когда мы стали отслеживать целочисленные диапазоны, появились новые предупреждения в условиях вроде if(a > 0 && a <= 0), потому что левая часть истинна, если значение a лежит в диапазоне [1..MAX_VALUE], а правая часть — если в диапазоне [MIN_VALUE..0], и эти диапазоны не пересекаются. Однако до сих пор не было предупреждения на условии if(a > b && a <= b), потому что мы здесь мы ничего не знаем о диапазонах. Теперь мы следим, какое значение больше, даже если они оба неизвестны, поэтому в 2018.1 это условие тоже будет подсвечено. Кажется, что такую тривиальную ошибку наверняка заметит сам программист, но не забывайте, что анализ потоков данных не просто ищет шаблоны.

    Во-первых, он также легко находит противоположный случай. И такая ошибка действительно обнаружилась в нашем коде обработки SQL-таблиц:

    idx &lt; myAncestorRefs.length || idx &gt;= myAncestorRefs.length

    Да, метод начинается с условия, которое всегда истинно, и поэтому всегда возвращает пустой список, потому что значение переменной idx либо меньше длины массива myAncestorRefs, либо больше, либо равно ей. Последующие десять строчек кода никогда не выполняются. Очевидно тут опечатка: имелось в виду idx < 0 вместо idx < myAncestorRefs.length.

    Кстати, о длине массива. Приятным сюрпризом оказалось, что эта фича сама подружилась с уже имевшимися проверками длин массивов. Мы совершенно не ожидали обнаружить ошибок, подобных вот этой, найденной в Groovy-плагине:

    if (initializers.length &lt; i) return initializers[i];

    Теперь при чтении элемента массива инспекция уже знает, что i в данном месте всегда больше, чем количество элементов в initializers. Поэтому исключение ArrayIndexOutOfBoundsException неизбежно, если условие когда-либо выполнится. Думаю, на самом деле оно никогда не выполнялось, иначе бы исключение заметили. Конечно же условие перевернули по ошибке.

    Ещё одна проблема, связанная с границами массива, обнаружилась в коде одной Java-инспекции, которая обрабатывает блоки catch:

    while (catchSections[index] != catchSection &amp;&amp; index &lt; catchSections.length)

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

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

    boolean[] result = new boolean[] {false}; &lt;...&gt; if (result[0])

    Давным давно одноэлементный массив result использовался внутри лямбды command, чтобы зафиксировать необходимость активации редактора. Со временем активация редактора стала не нужна, но ни одна существующая инспекция не замечала, что условие избыточно. Теперь мы это видим.

    Теперь мы считаем, что булевы методы без аргументов, имя которых начинается с “is”, возвращают один и тот же результат, если были дважды вызваны с одним и тем же квалификатором. Да, это эвристика, она не обязательно верна. Но если она стала причиной ложного срабатывания в вашем коде, задумайтесь. Возможно, это знак, что код труден для понимания не только для IDE, но и для ваших коллег. Зато такая эвристика позволяет найти настоящие баги. Например, такой код обнаружился при обработке CSS селекторов:

    localSelectorElement = (CssSimpleSelector)localElement;remoteSelectorElement = (CssSimpleSelector)localSelectorElement;if(localSelectorElement.isUniversalSelector() != remoteSelectorElement.isUniversalSelector())

    Хотя квалификаторы здесь различны, выше им присвоили одно и то же значение. Поэтому анализатор предполагает, что isUniversalSelector вернёт одинаковый результат. И это действительно так! Опечатка здесь не в условии, а строчкой выше: должно быть, конечно, remoteSelectorElement = (CssSimpleSelector)remoteSelectorElements[j].

    Также в 2018.1 инспекция выдаёт новое предупреждение. Если переменной присваивается значение, и то же самое значение она уже имела в этом месте всегда, мы это подсвечиваем. Опять же не всегда это именно ошибка, но присваивание заведомо избыточно. Настоящие ошибки с помощью этого предупреждения тоже нашлись. Например, это фрагмент юнит-теста, проверяющего форматирование HTML:

    int indentSize = htmlIndentOptions.INDENT_SIZE = 2;

    Проблема тремя строчками выше предупреждения: на конце не должно быть “= 2”. В результате размер отступа не восстанавливается после теста, как ожидалось.

    Также это предупреждение вскрыло дубликаты в цепочках инициализации полей или элементов массива:

    defaults[CONSTANT_Int] = ints;...defaults[CONSTANT_Int] = ints;

    Возможно, эта строчка просто избыточна, но может быть и имелось в виду что-то другое. Так или иначе это очевидно ошибка.

    Небольшое улучшение обработки this вскрыло ошибку в коде, обрабатывающем контрол с вкладками:

    while (component != this || component != null)

    Если левая часть истинна, то правая не проверяется. Если же левая ложна, то component равен this, а this точно не null, значит и component не null. Поэтому всё условие всегда истинно. Очень частая ошибка, когда используют || вместо && или наоборот.

    Наконец, мы улучшили обработку Stream API. В частности, она теперь работает и для незавершённых цепочек (то есть не заканчивающихся терминальной операцией). Ошибок в нашем коде это улучшение не выявило, но нашлись некоторые избыточные проверки. Например, такой метод имеется в обработке результатов code coverage:

    mapToObj(idx -&gt; Pair.create(...)).filter((x) -&gt; x != null &amp;&amp; ...); интересно, а слепые читают эту статью? Я тут альтернативный текст пишу ко всем картинкам, от этого польза есть или нету?

    Для метода Pair.create автоматически выводится аннотация @NotNull, поэтому условие x != null избыточно. Инспекция в курсе, что x имеет то же значение, которое вернула предыдущая лямбда.

    Инспекция “Constant conditions & exceptions” на удивление часто находит ошибки, которые прячутся в больших проектах годами. Ещё больше она помогает, когда вы пишете новый, непроверенный код. Поначалу некоторые предупреждения этой инспекции могут раздражать, но со временем понимаешь, как с ними жить дружно.

    Мы уже начали EAP-программу 2018.1, поэтому вы можете испытать новые возможности прямо сейчас. Эта инспекция постоянно улучшается, исчезают ложные сработки и добавляются полезные. У нас ещё много идей, что можно в ней улучшить, так что следите за новостями.
    JetBrains 128,90
    Делаем эффективные инструменты для разработчиков
    Поделиться публикацией
    Похожие публикации
    Комментарии 15
    • +10
      А вы сообщили разработчикам? А патч отправили? Какую версию этого проекта вы проверяли? Master? Так надо 173! А вы проверяли проект IDEA Community? А что так МАЛО? А есть версия IntelliJ IDEA под Linux? Вы проверяли IDEA с помощью IDEA? Где статья о результатах проверки?
      • +2

        Не распознают люди сарказм, вот и минусуют, прости их :-)

        • –1
          Я знаю, Тагир, я знаю :) И в профиль пользователя тоже не любят заглядывать.
          • +3
            Распознают, и в профиль заглядывают, и даже видят там уже поставленный плюс в карму. Но конкретно этот коммент, что называется, «слишком толстый». И парсится уже не как шутка, а как глупое отвлечение от темы.

            p.s. Сам пост отличный, спасибо!
            • 0
              Честно говоря, я больше опасался комментариев вроде «У… да ваша IDEA — глюкавище, как можно было такие баги допустить».
              • +2
                У… да ваша IDEA — глюкавище, как можно было такие баги допустить
                Все, страшное случилось, дальше боятся смысла нет :)
                Для фанатов Дюны
                image
        • 0
          Есть ли возможность пометить, что именно проверяют свои функции-проверки, как то

          boolean isNotEmpty(Object[] arr) { return arr != null && arr.length > 0; }
          

          • 0

            Есть контракты, но они довольно ограничены. Здесь можно написать @Contract("null -> false"), и это будет учитываться. А вот информация о длине массива во внешний метод не попадёт.

            • 0
              Спасибо. Попробую.
              • 0

                Если метод статический, скорее всего контракт сам выведется. Посмотрите по ctrl+q.

          • +1
            Спасибо за интересный рассказ!

            Один комментарий — очень плохо видно скриншоты кода. Понятно, что тут хочется показать реакцию IDEA, но факт — есть факт.
            • 0

              Да, со скриншотами не очень получилось. Шрифт явно не подходящий. В следующий раз учту :-)

            • +1
              IDE — хорошо, а как запустить инспекции в безлюдном режиме (в рамках CI) и получить отчет о найденных ошибках/предупреждениях? a la standalone static code analyzer

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

            Самое читаемое