PVS-Studio для Java

    PVS-Studio для Java

    В седьмой версии статического анализатора PVS-Studio мы добавили поддержку языка Java. Пришло время немного рассказать, как мы начинали делать поддержку языка Java, что у нас получилось и какие дальнейшие планы. И, конечно, в статье будут приведены первые испытания анализатора на открытых проектах.

    PVS-Studio


    Для Java разработчиков, которые ранее не слышали об инструменте PVS-Studio, приведу совсем краткое его описание.

    PVS-Studio — это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java. Работает в среде Windows, Linux и macOS.

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

    Начало


    Я бы мог придумать умную историю, как мы два года размышляли о том, какой следующий язык поддержать в PVS-Studio. О том, что Java — это разумный выбор, основанный на высокой популярности этого языка и так далее.

    Однако как это бывает в жизни, всё решил не глубокий анализ, а эксперимент :). Да, мы размышляли, в какую сторону дальше развивать анализатор PVS-Studio. Рассматривались такие языки программирования, как: Java, PHP, Python, JavaScript, IBM RPG. Причём мы склонялись именно к языку Java, но окончательный выбор так ещё и не был сделан. Тех, у кого взгляд застрял на незнакомом IBM RPG, отсылаю к вот этой заметке, из которой всё станет ясно.

    В конце 2017 года коллега Егор Бредихин посмотрел, какие есть готовые библиотеки разбора кода (проще говоря — парсеры) под интересные нам новые направления. И наткнулся на несколько проектов для разбора Java кода. На основе Spoon, ему довольно быстро удалось сделать прототип анализатора с парой диагностик. Более того, стало понятно, что мы сможем использовать в Java анализаторе некоторые механизмы C++ анализатора с помощью SWIG. Мы посмотрели на то, что получилось, и поняли, что наш следующий анализатор будет для Java.

    Спасибо Егору за его начинание и активную работу, проделанную им над Java анализатором. Как именно шла разработка он описал в статье "Разработка нового статического анализатора: PVS-Studio Java".

    Конкуренты?


    В мире существует множество бесплатных и коммерческих статических анализаторов кода для Java. Перечислять их все в статье не имеет смысла, и я просто оставлю ссылку на "List of tools for static code analysis" (смотрите раздел Java и Multi-language).

    Однако я знаю, что в первую очередь нас спросят про IntelliJ IDEA, FindBugs и SonarQube (SonarJava).

    IntelliJ IDEA

    В IntelliJ IDEA встроен очень мощный статический анализатор кода. Причем анализатор развивается, а его авторы внимательно следят за нашей деятельностью. С IntelliJ IDEA нам будет сложнее всего. Превзойти IntelliJ IDEA в диагностических возможностях мы не сможем, по крайней мере, сейчас. Поэтому мы постараемся сконцентрироваться на других наших преимуществах.

    Статический анализ в IntelliJ IDEA – это, в первую очередь, одна из фишек среды разработки, что накладывает на него определённые ограничения. Мы же свободны в том, что можем делать с нашим анализатором. Например, мы можем быстро адаптировать анализатор под конкретные нужды заказчика. Быстрая и глубокая поддержка — наше конкурентное преимущество. Наши клиенты напрямую общаются непосредственно с программистами, разрабатывающими ту или иную часть PVS-Studio.

    В PVS-Studio много возможностей по интеграции его в цикл разработки больших старых проектов. Это интеграция с SonarQube. Это массовое подавление сообщений анализатора, что позволяет сразу начать использовать анализатор в большом проекте для отслеживания ошибок только в новом или изменённом коде. PVS-Studio встраивается в процесс непрерывной интеграции. Думаю, именно эти и другие возможности помогут нашему анализатору найти место под солнцем в Java мире.

    FindBugs

    Проект FindBugs заброшен. Но его следует вспомнить по той причине, что это, пожалуй, наиболее известный бесплатный статический анализатор Java кода.

    Преемником FindBugs можно назвать проект SpotBugs. Однако он менее популярен, и что с ним будет, тоже пока не совсем понятно.

    В общем, мы считаем, что, хотя FindBugs был и остаётся крайне популярным, да к тому же ещё и бесплатным анализатором, думать про него нам не следует. Этот проект просто тихо и спокойно уйдёт в прошлое.

    P.S. Кстати, теперь PVS-Studio также можно использовать бесплатно при работе с открытыми проектами.

    SonarQube (SonarJava)

    Мы считаем, что не конкурируем с SonarQube, а дополняем его. PVS-Studio интегрируется с SonarQube, что позволяет разработчикам находить большее количество ошибок и потенциальных уязвимостей в своих проектах. Как интегрировать в SonarQube инструмент PVS-Studio и другие анализаторы, мы регулярно рассказываем на мастер-классах, которые проводим в рамках различных конференций (пример).

    Как запустить PVS-Studio для Java


    Мы сделали доступными для пользователей самые популярные способы интеграции анализатора в сборочную систему:

    • Плагин для Maven;
    • Плагин для Gradle;
    • Плагин для IntelliJ IDEA

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

    Подробную информацию о всех способах запуска анализатора вы можете найти на странице документации "Как запустить PVS-Studio Java".

    Мы не могли обойти стороной платформу контроля качества кода SonarQube, так популярную среди Java разработчиков, поэтому добавили поддержку языка Java в наш плагин для SonarQube.

    Дальнейшие планы


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

    • Создание новых диагностик и доработка существующих;
    • Развитие Dataflow-анализа;
    • Повышение надёжности и удобства использования.

    Возможно, мы найдём время адаптировать плагин IntelliJ IDEA для CLion. Привет C++ разработчикам, читающим про анализатор Java :-)

    Примеры ошибок, найденных в открытых проектах


    Я буду не я, если не покажу в статье какие-то ошибки, найденные с помощью нового анализатора. Можно было бы взять какой-то большой открытый Java-проект и написать классическую статью с разбором ошибок, как мы обычно и делаем.

    Однако я сразу предвижу вопросы, а сможем ли мы найти что-то в таких проектах как IntelliJ IDEA, FindBugs и так далее. Поэтому у меня просто нет выхода, и я начну именно с этих проектов. Итак, я решил бегло проверить и выписать несколько интересных примеров ошибок из следующих проектов:

    • IntelliJ IDEA Community Edition. Думаю, не нужно объяснять, почему был выбран этот проект :).
    • SpotBugs. Как я уже писал ранее, проект FindBugs не развивается. Поэтому заглянем в проект SpotBugs, который является преемником FindBugs. SpotBugs — это классический статический анализатор Java-кода.
    • Что-то из проектов компании SonarSource, которая разрабатывает программное обеспечение для непрерывного контроля качества кода. Заглянем в проект SonarQube и SonarJava.

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

    Несмотря на всё это, мне придётся начать именно с этих проектов. Второго шанса написать что-то про них у меня не будет. Я уверен, что после выхода релиза PVS-Studio для Java, разработчики перечисленных проектов возьмут PVS-Studio на вооружение и начнут его использовать для регулярных или, по крайней мере, для периодических проверок своего кода. Например, я знаю, что Тагир Валеев (lany), один из разработчиков JetBrains, занимающийся статическим анализатором кода IntelliJ IDEA, в тот момент, пока я пишу статью, уже во всю играется с Beta-версией PVS-Studio. Он написал нам уже около 15 писем с баг-репортами и рекомендациями. Спасибо, Тагир!

    К счастью, мне не требуется найти как можно больше ошибок в одном конкретном проекте. Сейчас моя задача показать, что анализатор PVS-Studio для Java появился не зря и сможет пополнить линейку других инструментов, предназначенных для улучшения качества кода. Я просто бегло просмотрел отчёты анализатора и выписал несколько ошибок, которые показались мне интересными. По возможности я старался выписывать ошибки разного типа. Давайте посмотрим, что получилось.

    IntelliJ IDEA, целочисленное деление


    private static boolean checkSentenceCapitalization(@NotNull String value) {
      List<String> words = StringUtil.split(value, " ");
      ....
      int capitalized = 1;
      ....
      return capitalized / words.size() < 0.2; // allow reasonable amount of
                                               // capitalized words
    }

    Предупреждение PVS-Studio: V6011 [CWE-682] The '0.2' literal of the 'double' type is compared to a value of the 'int' type. TitleCapitalizationInspection.java 169

    По задумке, функция должна возвращать истину, если менее 20% слов начинаются с заглавной буквы. На самом деле, проверка не работает, так как происходит целочисленное деление. В результате деления можно получить только два значения: 0 или 1.

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

    IntelliJ IDEA, подозрительный цикл


    public int findPreviousIndex(int current) {
      int count = myPainter.getErrorStripeCount();
      int foundIndex = -1;
      int foundLayer = 0;
      if (0 <= current && current < count) {
        current--;
        for (int index = count - 1; index >= 0; index++) {        // <=
          int layer = getLayer(index);
          if (layer > foundLayer) {
            foundIndex = index;
            foundLayer = layer;
          }
        }
      ....
    }

    Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'index >= 0' is always true. Updater.java 184

    Вначале посмотрите на условие (0 <= current && current < count). Оно выполняется только в том случае, если значение переменной count больше 0.

    Теперь посмотрим на цикл:

    for (int index = count - 1; index >= 0; index++)

    Переменная index инициализируется выражением count — 1. Так как переменная count больше 0, то начальное значение переменной index всегда больше или равно 0. Получается, что цикл будет выполняться до тех пор, пока не произойдёт переполнение переменной index.

    Скорее всего, это просто опечатка и должен выполняться не инкремент, а декремент переменной:

    for (int index = count - 1; index >= 0; index--)

    IntelliJ IDEA, Copy-Paste


    @NonNls public static final String BEFORE_STR_OLD = "before:";
    @NonNls public static final String AFTER_STR_OLD = "after:"; 
    
    private static boolean isBeforeOrAfterKeyword(String str, boolean trimKeyword) {
      return (trimKeyword ? LoadingOrder.BEFORE_STR.trim() :
               LoadingOrder.BEFORE_STR).equalsIgnoreCase(str) ||
             (trimKeyword ? LoadingOrder.AFTER_STR.trim() :
               LoadingOrder.AFTER_STR).equalsIgnoreCase(str) ||
             LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str) ||         // <=
             LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str);           // <=
    }

    Предупреждение PVS-Studio: V6001 [CWE-570] There are identical sub-expressions 'LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str)' to the left and to the right of the '||' operator. Check lines: 127, 128. ExtensionOrderConverter.java 127

    Старый добрый эффект последней строки. Программист поторопился и, размножив строчку кода, забыл её исправить. В результате, дважды строка str сравнивается с BEFORE_STR_OLD. Скорее всего, одно из сравнений должно быть с AFTER_STR_OLD.

    IntelliJ IDEA, опечатка


    public synchronized boolean isIdentifier(@NotNull String name,
                                             final Project project) {
      if (!StringUtil.startsWithChar(name,'\'') &&
          !StringUtil.startsWithChar(name,'\"')) {
        name = "\"" + name;
      }
      if (!StringUtil.endsWithChar(name,'"') &&
          !StringUtil.endsWithChar(name,'\"')) {
        name += "\"";
      }
     ....
    }

    Предупреждение PVS-Studio: V6001 [CWE-571] There are identical sub-expressions '!StringUtil.endsWithChar(name,'"')' to the left and to the right of the '&&' operator. JsonNamesValidator.java 27

    Данный фрагмент кода проверяет, что имя взято в одинарные или двойные кавычки. Если это не так, то двойные кавычки добавляются автоматически.

    Из-за опечатки, окончание имени проверяется только на наличие двойных кавычек. В результате, имя, взятое в одинарные кавычки, будет обработано некорректно.

    Имя

    'Abcd'

    из-за добавления лишних двойных кавычек превратится в:

    'Abcd'"

    IntelliJ IDEA, неправильная защита от выхода за границу массива


    static Context parse(....) {
      ....
      for (int i = offset; i < endOffset; i++) {
        char c = text.charAt(i);
        if (c == '<' && i < endOffset && text.charAt(i + 1) == '/'
            && startTag != null
            && CharArrayUtil.regionMatches(text, i + 2, endOffset, startTag)) 
        {
          endTagStartOffset = i;
          break;
        }
      }
      ....
    }

    Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'i < endOffset' is always true. EnterAfterJavadocTagHandler.java 183

    Подвыражение i < endOffset в условии оператора if не имеет смысла. Переменная i и так всегда меньше endOffset, что следует из условия выполнения цикла.

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

    • text.charAt(i + 1)
    • CharArrayUtil.regionMatches(text, i + 2, endOffset, startTag)

    В этом случае подвыражение для проверки индекса должно быть таким: i < endOffset — 2.

    IntelliJ IDEA, повторяющаяся проверка


    public static String generateWarningMessage(....)
    {
      ....
      if (buffer.length() > 0) {
        if (buffer.length() > 0) {
          buffer.append(" ").append(
            IdeBundle.message("prompt.delete.and")).append(" ");
        }
      }
      ....
    }

    Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'buffer.length() > 0' is always true. DeleteUtil.java 62

    Это может быть как безобидным избыточным кодом, так и серьезной ошибкой.

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

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

    Примечание. Кстати, разных избыточных проверок находится очень много. Причём часто видно, что это не ошибка. Однако и назвать сообщения анализатора ложными срабатываниями тоже нельзя. Для пояснения приведу вот такой пример, также взятый из IntelliJ IDEA:

    private static boolean isMultiline(PsiElement element) {
      String text = element.getText();
      return text.contains("\n") || text.contains("\r") || text.contains("\r\n");
    }

    Анализатор говорит, что функция text.contains("\r\n") всегда возвращает ложь. И действительно, если не найден символ "\n" и "\r", то и нет смысла искать "\r\n". Это не ошибка, и код плох только тем, что работает чуть медленнее, выполняя бессмысленный поиск подстроки.

    Как быть с подобным кодом, в каждом конкретном случае уже решать программистам. Я при написании статей, как правило, просто не обращаю внимания на подобный код.

    IntelliJ IDEA, что-то не так


    public boolean satisfiedBy(@NotNull PsiElement element) {
      ....
      @NonNls final String text = expression.getText().replaceAll("_", "");
      if (text == null || text.length() < 2) {
        return false;
      }
      if ("0".equals(text) || "0L".equals(text) || "0l".equals(text)) {
        return false;
      }
      return text.charAt(0) == '0';
    }

    Предупреждение PVS-Studio: V6007 [CWE-570] Expression '«0».equals(text)' is always false. ConvertIntegerToDecimalPredicate.java 46

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

    В начале проверятся, что строка должна содержать не менее двух символов. Если это не так, то функция возвращает false.

    Далее следует проверка «0».equals(text). Она бессмысленна, так как строка не может содержать только один символ.

    В общем, что-то здесь не так и код следует поправить.

    SpotBugs (преемник FindBugs), ошибка ограничения по количеству итераций


    public static String getXMLType(@WillNotClose InputStream in) throws IOException
    {
      ....
      String s;
      int count = 0;
      while (count < 4) {
        s = r.readLine();
        if (s == null) {
          break;
        }
        Matcher m = tag.matcher(s);
        if (m.find()) {
          return m.group(1);
        }
      }
      throw new IOException("Didn't find xml tag");
      ....
    }

    Предупреждение PVS-Studio: V6007 [CWE-571] Expression 'count < 4' is always true. Util.java 394

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

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

    SpotBugs (преемник FindBugs), затирание значения


    private void reportBug() {
      int priority = LOW_PRIORITY;
      String pattern = "NS_NON_SHORT_CIRCUIT";
    
      if (sawDangerOld) {
        if (sawNullTestVeryOld) {
          priority = HIGH_PRIORITY;                                           // <=
        }  
        if (sawMethodCallOld || sawNumericTestVeryOld && sawArrayDangerOld) {
          priority = HIGH_PRIORITY;                                           // <=
          pattern = "NS_DANGEROUS_NON_SHORT_CIRCUIT";
        } else {
          priority = NORMAL_PRIORITY;                                         // <=
        }
      }
    
      bugAccumulator.accumulateBug(
        new BugInstance(this, pattern, priority).addClassAndMethod(this), this);
    }

    Предупреждение PVS-Studio: V6021 [CWE-563] The value is assigned to the 'priority' variable but is not used. FindNonShortCircuit.java 197

    Значение переменной priority выставляется в зависимости от значения переменной sawNullTestVeryOld. Однако это не играет никакой роли. Далее переменной priority в любом случае будет присвоено другое значение. Явная ошибка в логике работы функции.

    SonarQube, Copy-Paste


    public class RuleDto {
      ....
      private final RuleDefinitionDto definition;
      private final RuleMetadataDto metadata;
      ....
      private void setUpdatedAtFromDefinition(@Nullable Long updatedAt) {
        if (updatedAt != null && updatedAt > definition.getUpdatedAt()) {
          setUpdatedAt(updatedAt);
        }
      }
    
      private void setUpdatedAtFromMetadata(@Nullable Long updatedAt) {
        if (updatedAt != null && updatedAt > definition.getUpdatedAt()) {
          setUpdatedAt(updatedAt);
        }
      }
      ....
    }

    PVS-Studio: V6032 It is odd that the body of method 'setUpdatedAtFromDefinition' is fully equivalent to the body of another method 'setUpdatedAtFromMetadata'. Check lines: 396, 405. RuleDto.java 396

    В методе setUpdatedAtFromMetadata используется поле definition. Скорее всего, должно использоваться поле metadata. Это очень похоже на последствия неудачного Copy-Paste.

    SonarJava, дубликаты при инициализации Map


    private final Map<JavaPunctuator, Tree.Kind> assignmentOperators =
      Maps.newEnumMap(JavaPunctuator.class);
    
    public KindMaps() {
      ....
      assignmentOperators.put(JavaPunctuator.PLUSEQU, Tree.Kind.PLUS_ASSIGNMENT);
      ....
      assignmentOperators.put(JavaPunctuator.PLUSEQU, Tree.Kind.PLUS_ASSIGNMENT);
      ....
    }

    Предупреждение PVS-Studio: V6033 [CWE-462] An item with the same key 'JavaPunctuator.PLUSEQU' has already been added. Check lines: 104, 100. KindMaps.java 104

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

    Заключение


    Да какое тут может быть заключение?! Приглашаю всех, не откладывая, скачать PVS-Studio и попробовать проверить свои рабочие проекты на языке Java! Скачать PVS-Studio.

    Спасибо всем за внимание. Надеюсь, скоро мы порадуем читателей циклом статей, посвящённых проверке различных открытых Java-проектов.



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. PVS-Studio for Java.
    PVS-Studio
    792.84
    Static Code Analysis for C, C++, C# and Java
    Share post

    Similar posts

    Comments 47

      0

      Конкуренция — это хорошо, больше инструментов — лучше инструменты.


      Я думаю что все что вы нашли, могла найти и Idea. Дело в том что множество инспекций отключено по умолчанию, из-за компромиса между скоростью работы/найденными ошибками.

        +3

        Да, по умолчанию в IDEA многие инспекции выключены. Но, например, в кейсе с целочисленным делением IDEA может только сказать, что сравниваются double и int, тогда как PVS вычисляет области значений и более явно предупреждает о косяке. Случай с циклом IDEA вообще не опознаёт. Ошибку копипаста, да, определить может, но не с настройками по умолчанию. Интересно, что проверки вида "expression is always true/false" производятся уже с настройками по умолчанию, но авторы IDEA их не заметили. Специально отключают эту или все инспекции?

          +7
          Специально отключают эту или все инспекции?

          Нет, не отключаем. Думаю, дело в том, что мы сильно толще вас. У нас только в мастер community 500 коммитов в неделю. А если ещё закрытая часть, которая не меньше. Представьте, что я улучшаю инспекцию, и она находит не одно или два, а сто новых подозрительных мест в коде. Это, кстати, не предел, мой личный рекорд — 800 новых предупреждений (речь о инспекциях категории Probable bugs, а не о каких-нибудь стилистических). Причём исправлять их надо вдумчиво, автоматом не получится. И что, бросать всё и начинать править? Часто проблемы находятся в коде пятнадцатилетней давности, который относится к поддержке какого-нибудь устаревшего фреймворка, у которого полтора пользователя теперь, и никто особо не в курсе даже что надо сделать в Идейке, чтобы метод с новым предупреждением выполнился. Да, можно тратить силы на это, но кажется, что можно потратить их более рационально.


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


          Ну и, к сожалению, местами просто не хватает культуры. Я не коммичу код с новыми варнингами и стараюсь исправлять варнинги в любом коде, который трогаю. Но не все так делают, и это не форсируется у нас. Последнее время мы стали исправлять ситуацию. Некоторые инспекции у нас включены в так называемый список Zero tolerance inspections. Предупреждения от этих инспекций рисуются у нас кроваво-красным цветом, их сложно игнорировать. А если код с таким предупреждением закоммитить, то падает один из билдов, мне приходит письмо, и я с этим разбираюсь. Постепенно мы увеличиваем список таких инспекций, но, конечно, до идеала далеко. В частности, например, сообщений вида condition is always true пока слишком много, чтобы вот так легко их все исправить.

            +2

            Было бы интересно взглянуть на профайл инспекций, которые вы используете для проверки своего кода. Понятно, что, в общем случае, там всё зависит от проекта. Но кому как не создателям инспекций лучше знать, какие из них важнее других? Даже если глобально, для всех включить их по-умолчанию не очень оправдано.
            Это ведь не какая-то секретная информация?
            Или, быть может, это в вашем Community-проекте на GitHub можно посмотреть?

              0

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

              0

              Бывает до смешного доходит. Пользователь присылает отчёт об ошибке, что в Идейке произошёл NullPointerException, мы смотрим стектрейс, а в этом месте сама Идейка подсвечивает код, что NullPointerException здесь возможен. Мол, «я же вас предупреждала, а вы!»


                +4
                А теперь серьезно.

                Когда меня спрашивают о том, не сошли ли мы с ума делая статический анализатор кода для Java, я как никогда спокоен и уверен в себе. Да, IDEA – the best (ни капли сарказма), а разработчики из JetBrains – элита индустрии. В этом нет сомнений и мы не претендуем на это.

                Но статический анализ кода – это не только собственно технологии анализа кода. Как построить дерево разбора и как по нему пробежаться знают многие. В конце концов это описано в книгах!

                Статический анализ кода – это еще и инфраструктура, то есть куча вспомогательных инструментов. Это и методология использования, и, главное, методология внедрения. Философия статического анализа кода – это как потратить деньги на статический анализ с пользой.

                Многие думают, что статический анализ – это вот статьи моих коллег про «смотрите какую ошибку мы нашли в Chrome». Это все прикольно, вызывает интерес аудитории, но это вишенка на торте. Сам же торт намного больше и вкуснее.

                Этой темой мы занимаемся более 10 лет. И мы в ней сильны. Поэтому нет, я не боюсь конкуренции ни с JetBrains, ни с SonarQube, ни с кем-либо еще. В этой области у нас есть крутейшая экспертиза. Это знаю я, знает команда и знают те клиенты, которые из года в год продлевают лицензии на PVS-Studio.

                Про это я кстати подробно буду рассказывать на конференциях по Java в 2019 году. Приходите на наши доклады!
          +21

          Да, тут напрашивается ответ, конечно :-) Во-первых, поздравляю вас с релизом, вы молодцы и делаете нужное дело. Пройдёмся по предупреждениям


          return capitalized / words.size() < 0.2;

          Это наглядный пример, что конкуренция — двигатель прогресса! Я заметил во время бета-тестирования, что вы тут умнее нас. Но очевидно, это не ваше достоинство, а наша недоработка :-) Я сообщил ответственному человеку, и тот уже исправил, в следующей версии будем это репортить.


          for (int index = count - 1; index >= 0; index++) {

          Мы знаем, что count в этом месте положительный и count - 1 неотрицательный. Кстати, в этом можно убедиться, нажав в IDE два раза Ctrl+Shift+P:



          Однако наш dataflow аккуратно вычисляет все переполнения и не считает, что условие цикла всегда истинно, потому что это не так.


          Зато недавно мы сделали другую инспекцию, которая здесь срабатывает. В принципе никакой разницы нет, с какого значения мы начинаем этот цикл, главное — это связка операторов в условии и инкременте. Связки >/++ и </-- — это почти всегда баги, о которых и сообщает новая инспекция. Будет в следующей версии. Так что эту проблему мы находим, хоть и по-другому.


          LoadingOrder.BEFORE_STR_OLD.equalsIgnoreCase(str) || и следующая

          Эта инспекция почему-то выключена по умолчанию. Надо будет разобраться с этим. Если работает нормально, то включим.


          for (int i = offset; i < endOffset; i++)

          Это мы видим, да. Но исправить всем лень. И, кстати, вы правы, я смог сломать Идейку, вызвав исключение в этом месте! Если кому интересно, надо в конце файла набрать


          /**
           * <p><

          Встать курсором между >< и нажать Enter! Если вы это повторите, у вас замигает в правом нижнем углу восклицательный знак. Report to JetBrains нажимать необязательно, мы уже в курсе :-) Вот, кстати, вам хороший пример пользы от статического анализа. Теперь-то исправим.


          if (buffer.length() > 0) {

          Это видим тоже. Не очень давно, кстати, может с прошлой версии. Думаю, в данном случае это действительно просто лишняя проверка.


          text.contains("\n") || text.contains("\r") || text.contains("\r\n");

          Вот это прям очень круто было. Если у вас dataflow это выводит, а не паттерн, то поднимаю лапки и признаюсь, что нам такое слабо пока.


          if ("0".equals(text) || "0L".equals(text) || "0l".equals(text)) {

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

            +7
            Лайкнул коммент.

            И, Тагир, огромное спасибо за участие в бета-тестировании! Ребята до сих пор разбирают кучу комментариев и рекомендаций, до релиза не все успели проработать разумеется.
              0

              Тагир, не знаю, по адресу я обращаюсь или нет. Я года полтора назад репортил баг в валидаторе xslt шаблонов у idea, который реально мешает жить. Там речь про использовании внутри шаблона конструкций из javasript, валидатор ругался на семантику xsl внутри js. Но похоже xslt не в приоритете и его так и не поправили.

                +4

                Слушайте, ну если вы ожидаете от другого человека помощи, почему бы не сделать максимум работы самому? Если вы репортили баг, дайте хотя бы ссылку на него. У нас десятки тысяч открытых багов, мне сейчас идти и искать по вашему расплывчатому описанию?


                Кстати, если уж хочется привлечь внимание к тикету, есть способы лучше, чем писать разработчику совсем другой подсистемы. Например, твиттер. Особо хитрые жалуются на реддит. Ещё можно подговорить всех коллег проголосовать за баг. Только не сразу, а где-нибудь раз в неделю. Или можно время от времени писать дополнительные комментарии типа "проблема всё ещё актуальна в свежей версии, пожалуйста почините". Бывает, что работает. Только я вам этого не говорил :-)

                0
                Зато недавно мы сделали другую инспекцию, которая здесь срабатывает

                Перепутал ссылку на тикет, вот правильная.

                –2
                Далее следует проверка «0».equals(text). Она бессмысленна, так как строка не может содержать только один символ.

                По-моему может спокойно…
                  +2
                  Там перед этим выход из функции для строк короче двух.
                   if (text == null || text.length() < 2) {
                      return false;
                    }
                    +3
                    Извините, я почему-то решил что разговор про ВООБЩЕ :)
                  +2
                  А я только подготовил скрипты-аналоги того же что для плюсов делал, но даже не успел запустить тестовый прогон :(
                  У меня сейчас лапки, но я буду рад еще раз поклянчить код когда буду готов посвятить этому время продуктивно, а не по часу вечером :(
                  У вас отличный отлов копипасты, и я уверен что это очень мощная штукень.
                    0
                    Очень приятная новость, особенно с тем учётом, что в последнее время переехал на Джава и назад не очень хочу. Спасибо.
                    По моим наблюдениям упомянутые здесь анализаторы, в отличие от PVS, занимаются поверхностными проверками: лишнее поле, неверное именование, не рекомендуемое употребление конструкции и пр., тогда как PVS держит «в уме» не столько формальные проверки, сколько смысловые.
                    И вот вопрос: не приходила ли в голову идея сделать качественный идиоматичный кодогенератор для какого-либо языка? Либо настолько же «вдумчивый» конвертер кода.
                      +5
                      Позвольте! Идеевский анализатор вполне себе вдумчивый!
                        0
                        Вам виднее. :) может быть, мои наблюдения не достаточно полные. :)
                        +1
                        Пока мы не думаем в какую-либо сторону кроме анализа кода. Т.е. может быть мы будем думать, дальше в php или javascript, но не про кодогенерацию.
                          +2
                          Имхо golang еще интересный вариант.
                        +2

                        Есть планы на счёт поддержки Kotlin?

                          0
                          Сейчас таких планов нет.
                          –1
                          Это не ошибка, и код плох только тем, что работает чуть медленнее, выполняя бессмысленный поиск подстроки

                          Это неправда потому что в джаве || — short-circuit

                            +2

                            Если в тексте нет ни \n, ни \r, то будет выполнен третий поиск, который гарантированно ничего не найдет.

                              +2
                              Как это часто бывает, анализатор вновь оказался внимательнее человека. :) Вот поэтому анализаторы и нужны. :)
                                0
                                Справедливости ради замечу, что анализатор не сказал «код работает чуть медленнее, чем мог бы», это уже человек, анализируя сообщение, пришёл к такому выводу. И asm0dey оспорил вывод человека, а не программы.

                                В целом лишнюю проверку вида condition is always true компилятор нередко удалит так же легко, как анализатор заметит, поэтому не всегда имеется ухудшение производительности. В данном случае это, конечно, вряд ли сработает, но классифицировать такие случаи на вредные или нейтральные для производительности ваш анализатор вроде бы пока не научился :-)
                                  0
                                  Комментарий для сторонних читаталей, Тагир сам это и так знает конечно.

                                  Вообще все сообщения анализатора следует воспринимать как «Здесь что-то подозрительное, кажется вот это, но не уверен. Проверьте.» Очень нередки ситуации, когда анализатора об ошибке говорит не теми словами, которыми хотелось бы. Но он говорит все же об ошибке.
                                    0
                                    Всё правильно, но к нашей ситуации не относится. Ещё раз: asm0dey оспорил не предупреждение анализатора, а ухудшение производительности. Предупреждение анализатора он не оспаривал, поэтому утверждать, что анализатор внимательнее человека, в данном случае неверно. Я ровно это хотел сказать.
                            0
                            Скажите, раз вы смогли успешно интегрироваться в IntelliJ платформу, стоит ли ожидать появление плагина PVS-Studio С++ для CLion?
                              +3
                              Внимательно прочитав статью, можно найти ответ на этот вопрос :)
                                +2
                                Теперь увидел, спасибо.
                              0

                              Я так понял, PVS-Studio анализирует исходники. А почему не байт-код, как это делает findbugs? Это позволило бы одним инструментом покрыть много разных JVM-языков. Ведь наверняка PVS-Studio парсит исходники и строит какой-нибудь control flow graph, ищет def-use chains (а то и в SSA всё перегоняет). Всё то же самое можно прекрасно делать с байт-кодом. Или в подобных рассуждениях есть какой-то подвох?

                                +1
                                Подвох есть. В байт-коде потеряна часть информации, которая помогает понять, аномален код или нет. Мы решили пусть мы будем анализировать только Java, но зато сделаем это более глубоко.
                                  0

                                  А можно пример кода, который при компиляции даёт байт-код, в котором потеряна информация, необходимая для статического анализа?

                                    +3
                                    Конкретные примеры сейчас не покажу, но вот некоторые мысли.

                                    • Может исчезнуть недостижимый код, а, следовательно, он не будет найден. Как узнать, что он там был? Все равно надо анализировать изначальный исходный код.
                                    • Могут исчезать бессмысленные (всегда истинные/ложные) условия. Они могут «оптимизироваться в ничто» и их не будет в исходном коде. Например, можно удалить в условии (x.a == y.a && x.b == x.b) правую часть. Это классическая опечатка и имелось в виду x.b == y.b. А как узнать из байта кода, что что-то было удалено? Все равно надо анализировать изначальный исходный код.
                                    • Упрощение. Выражение (A < 1/2) может превратится в байт коде (A < 0). Как узнать из бйткода, что это подозрительное сравнение? Все равно надо анализировать изначальный исходный код.
                                    • И т.д.

                                    В общем всё сводится к анализу именно исходного кода. Иначе многие виды диагностик недоступны.
                                      0
                                      Какие-то сложные примеры с допущениями.
                                      У вас же есть инспекции, учитывающие отступы в исходном тексте?
                                      Отступы в скомпилированном коде исчезают всегда, без всяких «если».

                                      Чтобы два раза не вставать, вопрос: После просмотра этого доклада создалось впечатление, что у вас под капотом не SSA. Если это так, то почему?
                                        0
                                        Механизм, реализованный в PVS-Studio похож на SSA, но является он таковым с формальной точки зрения или нет, я не знаю. Да и не интересно, главное, что решаются нужные нам задачи. По поводу пробелов: да пробелы учитываются, но в очень малом количестве диагностик.
                                          0

                                          В скомпилированном виде может сохраняться отладочная информация. Именно отступы оттуда вытащить легко, вот только вопрос сколько будет ложных срабатываний из‐за особенностей компиляции в байткод. Тем более, что у каждого языка будут свои особенности.

                                            0

                                            Если мы говорим о JVM и *.class-файлах, то каким образом?
                                            Аттрибут LineNumberTable содержит информацию только о номерах строк, аттрибут SourceDebugExtension, насколько я помню, тоже.

                                              –1

                                              Ну так можно взять и посмотреть в файле. Для получения величины отступа ничего кроме номера строки и файла с исходным кодом и не нужно, даже знать, что там за язык. Анализ ведь запускается на своих исходниках.

                                          +7

                                          Ваши примеры по большей части неактуальны. Джава-компилятор довольно глупенький и не делает таких умностей при компиляции в байткод, оставляя это джиту. Но как человек, посвятивший немало времени и сил анализу именно байткода, могу подтвердить, что идея от konsoletyper не пролезет. Даже для чистой джавы часть информации никаким образом оттуда не вытащишь, а часть наоборот добавляется зазря. С другими языками вроде Котлина всё ещё хуже. Компилятор может спокойно добавлять заведомо недостижимые ветки, которых в исходном коде не было и я могу это выяснить статически. В результате выдаётся предупреждение о проблеме, которой нет в исходниках. Вдобавок усложняется репортинг ошибок: у меня из отладочной информации есть только номер строки. А если строка длинная, и в ней три ошибки, попробуй-ка спозиционируйся.


                                          Реальные примеры проблем такие. Ошибки с возможной инверсией приоритетов операций дают ложные сработки, если в исходнике были явные скобки, которые говорили "да, я хочу именно этого". Безусловный выход из цикла не поймаешь, потому что в байткоде такой цикл не отличишь от if. Саппрессить варнинги очень сложно. Стандартная аннотация @SuppressWarnings не попадает в байткод вообще. Дженерик-параметры локальных переменных исчезают напрочь, их приходится восстанавливать эвристически. В исходниках могло быть написано что-то другое, и это иногда влияет на предупреждения. Про типы-пересечения вообще молчу. JVM про них ничего не знает.


                                          Кроме того, трудно найти вменяемый движок для работы с байт-кодом на достаточно высоком уровне, который бы поддерживался актуальным. Самому писать ещё труднее. Знаете, сколько всего надо поменять в движке вроде Spoon, чтобы поддержать конкатенацию строк из Java9+? Ничего не надо менять, в исходниках она не изменилась. А в байткоде там ад и погибель. Тот же дешугаринг лямбд наверняка ещё будет меняться. И при обсуждении реализации новых фич в джаве постоянно ребята из Оракла говорят "давайте через indy зафигачим". Если каждый инди-бутстрэп не распознавать, ваш байткод-анализатор будет очень уныло работать. А indy-бутстрэпы языко-специфичны. Наверняка если вы напишете анализатор байткода, тестируя его только на джава-коде, а потом возьмётесь за условный JRuby, вы поймёте, что ваш анализатор бесполезен. Там будет куча маленьких синтетических методов и через слово indy. Наконец, такой анализатор тупо надо тестировать на всех языках. Вам может казаться, что вы всё хорошо поддержали, но по факту вы поддержали только байткод, который генерирует javac. Встретите байткод от scalac, а там последовательность инструкций, которая сносит крышу движку — и всё пошло-поехало.

                                            0
                                            Дженерик-параметры локальных переменных исчезают напрочь, их приходится восстанавливать эвристически.

                                            Если скомпилировать с -g, то остаются в LocalVariableTypeTable. Только так обычно не делают. Ну и checkcast — друг наш.

                                              +1
                                              С чеккастами дофига проблем из-за этого, кстати. В данном случае он больше враг, чем друг. Инспекции вида «cast may fail» практически всегда получаются очень мусорными на байткоде, репортя места, где каста не было вообще в сорцах.
                                    0
                                    Попробовал запустить PVS-Studio через плагин к идее. Проект модульный. Почти все модули были пропущены из-за:

                                    «V061 Unable to start the analysis on 'core' module.»

                                    В сапорт отписал вчера. Но пока никакой реакции.
                                      0
                                      В письме не было вопроса, поэтому мы на него не ответили.
                                        0
                                        :) ну ок. Я думал поддержу проект, зарепорчу багу. Можно было ответить что-то вроде «о проблема знаем, работаем». Банальная вежливость. Понимаю, что много кто пишет, трудно отвечать всем и тд.

                                        По факту — я потратил время, Вы потратили время. Инструмент проверить не удалось.

                                    Only users with full accounts can post comments. Log in, please.