Pull to refresh

Comments 47

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


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

UFO just landed and posted this here
Специально отключают эту или все инспекции?

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


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


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

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

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

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


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

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

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

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

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

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

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

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


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)) {

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

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

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

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

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


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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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


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

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

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

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

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

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

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