Pull to refresh

Comments 32

Хорошая статья. Прокомментирую только несколько несколько мест исходя из реального опыта.

Рискну утверждать: будучи впервые применён к вашему проекту, он найдёт в коде «занятные» места, но, скорее всего, не найдёт никаких дефектов, влияющих на качество работы вашей программы.
Обычно первый запуск даёт как раз самые сногсшибательные результаты на проекте, где ранее ничего не применялось.
Примеры дефектов, автоматически найденных анализаторами, впечатляют, но не следует забывать, что эти примеры найдены при помощи сканирования большого набора больших кодовых баз. По такому же принципу взломщики, имеющие возможность перебрать несколько простых паролей на большом количестве аккаунтов, в конце концов находят те аккаунты, на которых стоит простой пароль.
Думаю, совсем мимо. Примеры дефектов заранее известны, часто специально разрабатываются. Иногда собственный ляп в коде может стать примером для хорошей диагностики. Никакого абстрактного сканирования и переборов нет.
Спасибо за отзыв!
1. Ну это лотерея. Из моего опыта — где-то найдёт, где-то не найдёт. Я пытался донести в статье, что не стоит надеяться на одноразовое применение.
2. Я имел в виду примеры «реально найденных багов» из статей про анализаторы. Они конечно реально найденные, но искать их именно в том проекте никто не просил.
Иван, спасибо за статью и за то, что помогаете нам делать нашу работу – популяризовать технологию статического анализа кода.

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

Это моя личная боль, которую я не знаю как победить уже несколько лет. Дело в том, что статьи про проверку проектов:

  • Вызывают вау-эффект у людей. Людям нравится читать как косячат разработчики в Google, Epic Games, Microsoft и других компаниях. Людям приятно думать, что не только они сами ошибаются, но и лидеры индустрии делают ошибки. Люди любят читать такие статьи.
  • Можно писать на потоке, к тому же не сильно думая. Я не хочу оскорбить конечно же наших ребят, которые пишут эти статьи. Но придумывать каждый раз принципиально новую статью намного сложнее, чем написать статью о проверке проекта (десяток ошибок, пару шуток, разбавить картинками единорогов).

Вы написали очень хорошую статью. У меня тоже есть парочка статей на эту тему. И у других коллег. Более того, я езжу с докладом по компаниям с темой «Философия статического анализа кода», где рассказываю именно про процесс, а не конкретные найденные ошибки.
Но не возможно написать 10 статей про процесс. А нам для популяризации продукта надо писать регулярно и много.

Прокомментирую еще несколько моментов из статьи отдельным комментариев, чтобы удобнее дискуссию вести было.
Вот про «Философию статического анализа кода», о которой я рассказываю во время поездок по компаниям, в виде небольшой статьи.
Евгений, большое спасибо за содержательный отзыв на статью! Да, мою озабоченность насчёт «действия на неокрепшие умы» в моём посте Вы уловили совершенно правильно!!!

Причём обвинять-то здесь никого нельзя, т. к. авторы статей/докладов про анализаторы не имеют задачи делать статьи / доклады про анализ. Но после недавних постов Andrey2008 и lany я решил, что больше не могу молчать )))
Иван, как и писал выше, прокомментирую три момента из статьи. Те, что не комментирую – я с ними согласен значит.

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


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

Мне не довелось работать в PVS-studio, что касается моего опыта с Codacy, то основная их проблема заключается в том, что определение что есть «старая», а что «новая» ошибка — довольно сложный и не всегда правильно работающий алгоритм, особенно если файлы сильно изменяются или переименовываются.


В PVS-Studio это сделано офигенно удобно. Это одна из киллер-фич продукта, про которую, к сожалению, трудно писать статьи, поэтому ее мало знают. Мы сохраняем в базу информацию о имеющихся ошибках. Причем не просто «имя файла и строка», но и дополнительную информацию (хеши трех строк – текущая, предыдущая, следующая), чтобы в случае сдвига фрагмента кода мы его все-равно могли найти. Таким образом при незначительных модификациях мы все-равно понимаем, что это старая ошибка. И не ругаемся на нее. И теперь кто-то может сказать: «Ага, а если код сильно изменился, то значит это не сработает, и вы на него ругаетесь как на новый?» Да. Ругаемся. Но это и есть новый код. Если код сильно изменился, то это новый код, а не старый.

Благодаря этой фиче мы лично участвовали во внедрении в проект с 10 млн строк C++ кода, который каждый день «трогает» куча программистов. Все без проблем прошло.

Так что мы рекомендуем использовать эту фичу в PVS-Studio всем, кто внедряет статический анализ в свой процесс.

Вариант с фиксацией количества срабатываний относительно релиза – мне он намного менее симпатичен…

каким бы образом вы бы ни внедряли анализ в ваш конвейер поставки, версия анализатора должна быть фиксирована.


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

Не обновлять анализатор – это как не обновлять антивирусные базы («А вдруг начнут про вирусы писать»). Не будем правда здесь обсуждать целесообразность антивирусов в целом.

Если после обновления версии анализатора у вас много новых срабатываний – подавите их, как я писал выше через эту функцию. Но не обновлять версию… Как правило, такие клиенты (они конечно есть) не обновляют версию годами. Им все некогда. Они ПЛАТЯТ за продление лицензии, но не пользуются новыми версиями. Почему? Потому что когда-то они решили зафиксировать версию. Но продукт сегодня и продукт три года назад – это небо и земля. А так получается «куплю билет, но не поеду».
Речь всё-таки об автоматизированной CI больше, а в ней считается хорошей практикой иметь воспроизводимость билдов: два билда запущенных на одних и тех же исходниках (обычно на одном и том же коммите в билде) должны давать один и тот же результат даже если год между билдами прошёл (это и на дев-машинах хорошо, но в автоматических шагах CI просто маст хэв)
Да, совершенно верно, проблема здесь в воспроизводимости билдов.

EvgeniyRyzhkov, вот Вам реальный пример из опыта. Посмотрите вот этот коммит. Откуда он взялся? Мы поставили линтеры в TravisCI-скрипт. Они там работали как quality gates. Но внезапно, когда вышла новая версия Ansible-lint, которая стала находить больше ворнингов, у народа стали фейлится сборки пуллреквестов из-за ворнингов в коде, который они не меняли!!! В итоге был поломан процесс и срочные пуллреквесты стали мёржить без прохождения quality gates.

Никто не говорит, что не надо обновлять анализаторы. Конечно, надо! Как все другие компоненты сборки. Но это должен быть осознанный процесс, отражённый в исходном коде. И всякий раз действия будут по обстоятельствам (исправляем ли мы при этом вновь найденные предупреждения или просто ресетим «храповик»)
Как насчёт чтобы в условном build.gradle версия была указана конкретная, но при появлении новой версии робот на CI сам обновляет этот файл и коммитит обновлённый build-скрипт, возможно, разрешая на следующей сборке храповику прокрутиться назад? Тогда будет и обновление, и воспроизводимость.
Идея норм! По сути, это будет автоматизация ручной операции обновления версии анализатора, разрешать храповику прокручиваться назад при такой операции допустимо. Автоматизации нет предела )
Только если это серьёзно автоматизировать, надо учитывать вероятность того, что новая версия анализатора может оказаться неработоспособной, и откатывать изменение, если что-то пошло не так (иначе опять поломаем все билды). Но в целом да, наверное такое возможно.
Когда меня спрашивают: «А есть ли в PVS-Studio возможность проверять каждый коммит?», то я отвечаю, что есть. И тут же добавляю: «Только ради бога не фейлите билд, если PVS-Studio что-то найдет!» Потому что иначе рано или поздно PVS-Studio станут воспринимать как мешающую штуку. И бывают ситуации, когда НАДО закоммитить быстро, а не сражаться с инструментами, не пропускающими коммит.

Мое мнение — фейлить билд в этом случае очень плохо. Хорошо рассылать сообщения почтой автором проблемного кода.
Мое мнение — не бывает такого, что «надо быстро закоммитить». Бывает такое, что нет отлаженного процесса.

При хорошем процессе скорость возникает сама собой и не за счёт того, что мы взламываем процесс/кволити гейты, когда надо «по-быстрому».

Это не противоречит тому, что мы можем не фейлить билд на некоторых классах находок статического анализа. Это просто значит, что гейт настроен так, что определённые типы находок игнорируются, а для других находок у нас — Zero Tolerance.

Мой любимый commitstrip на тему «по-быстрому»: twitter.com/commitstrip/status/932673606840127489
1. Тут Вы правы. Готов согласиться с компилятором/парсером вначале, и даже пожалуй это стоит поменять в статье! Например, пресловутый spotbugs по-другому и не может, т. к. анализирует байт-код. Есть экзотические случаи, например, в пайплайне для Ansible playbooks статический анализ лучше ставить перед парсингом, т. к. там он легковеснее. Но это именно что экзотика )

2. Вариант с фиксацией количества срабатываний относительно релиза – мне он намного менее симпатичен… — ну да, он менее симпатичен, менее техничен, зато очень практичен :-) Главное — это общий метод, руководствуясь которым я, имея любую кодовую базу и любой анализатор (не обязательно ваш), используя палки и желуди groovy и shell скрипты на CI, могу эффективно внедрить статанализ — где угодно, в сколь угодно страшном проекте. Кстати, вот сейчас мы подсчитываем ворнинги в разбивке по модулям и тулам, но если разбить ещё более гранулированно (по файлам), то это будет ещё ближе к методу сравнения старых/новых. Но у нас прижилось так, и я как-то полюбил этот ratcheting за то, что он стимулирует разрабов следить за общим количеством ворнингов и сбивать это количество потихоньку. Если бы был метод старых/новых, стимулировало ли бы это разрабов следить за кривой количества ворнингов? — может быть, да, может, нет.

По п. 3 ответили ниже, сейчас продолжу ответ в той ветке.
Я категорический противник подхода использовать старую версию анализатора. Что делать, если пользователь нашел баг в этой версии? Он отпишет его разработчику инструмента, и разработчик инструмента его даже поправит. Но в новой версии. Никто не будет поддерживать старую версию специально для некоторых клиентов. Если это не контракты на миллионы долларов.
Евгений, так речь же не про то совсем. Никто не говорит, то их надо старыми держать. Речь идёт фиксации версий зависимостей компонент сборки для их контролируемого обновления — это общая дисциплина, она касается всего вообще, и библиотек, и инструментов.
Я понимаю «как надо теоретически». Но я вижу среди клиентов только два варианта. Или сидим на новой, или сидимо на старой. Такого что «у нас дисциплина и мы отсаем от текущей версии на два релиза» ПОЧТИ не бывает. Мне сейчас не важно сказать плохо это или хорошо. Я просто говорю о том, что вижу.
Я понял. Ещё ж всё сильно зависит о того, какие у ваших клиентов инструменты / процессы, и как их используют. Я, например, ничего не знаю про то, как бывает в мире C++)
В PVS-Studio это сделано офигенно удобно. Это одна из киллер-фич продукта, про которую, к сожалению, трудно писать статьи, поэтому ее мало знают. Мы сохраняем в базу информацию о имеющихся ошибках. Причем не просто «имя файла и строка», но и дополнительную информацию (хеши трех строк – текущая, предыдущая, следующая), чтобы в случае сдвига фрагмента кода мы его все-равно могли найти.
Да сделали вы замечательно, не спорю, да и в целом продукт замечательный,
Вот только, имхо эта фича меня немного раздражает, а именно, если при рефакторинге файл/кусок кода мигрирует из проекта в проект, то после регулярного анализа получаем по +1/+2 бестолковых комитта вида "update PVS-studio suppression db"(чтобы анализ на сервере пошел нормально). Потому что он не только находит но ещё и обновляет чтобы быстрее анализировалось, понятно что это для скорости. Но, если бы была настроечка включающая одну базу на sln/CmakeLists то было бы имхо попроще.
Потому что он не только находит но ещё и обновляет чтобы быстрее анализировалось, понятно что это для скорости. Но, если бы была настроечка включающая одну базу на sln/CmakeLists то было бы имхо попроще.

У нас можно добавлять suppress файл как на уровень sln (в случае Visual Studio проектов), так и при использовании прямой CMake интеграции (с нашим CMake модулем) Suppress файлы, однако, работают на уровне исходных файлов, поэтому если кусок кода переедет из одного файла в другой, то diff всё равно вылезет.

Также, если sln генерится CMake'ом, то добавлять в него suppress может быть неудобно — в таком случае можно в каждый ваш проект добавить общий suppress файл (т.е. все проекты будут ссылаться на один файл), а чтобы не модифицировать каждый проект по-отдельности, suppress файлы можно добавлять в проекты через общие MSBuild props'ы.
> Я не согласен, что первый шаг – это статический анализ, а только второй компиляция. Я считаю, что проверка компиляции в среднем быстрее и логичнее, чем сразу гонять «более тяжелый» статический анализ.

В моей практике эти два шага идут одновременно и неразрывно с редактированием кода. После каждой правки кода автоматически в фоне IDE находит и ошибки компиляции (не компилируя на самом деле, но это и не важно) и предупреждения статического анализа. Там есть расстановка приоритетов, хайлайтинг ошибок обычно вперёд успевает завершиться, поэтому формально «сперва компиляция». Но обычно разница во времени меньше секунды, для человека это непринципиально.
Я только за, чтобы разработчики использовали статический анализ во время написания кода. И конечно же в PVS-Studio есть такая фича.

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

Поэтому я и написал, что сначала компиляция.

А так — да, наибольшая эффективность от статического анализа как процесса достигается именно при использовании в IDE. Когда совсем нет траты времени человека на переключение контекста.
Согласен! И хорошо, что у нас, джавистов, есть горячо любимая IDEA ))
Тут речь идёт скорее о том, что происходит в Jenkins/Travis в момент, когда открывается Pull Request. В нашей практике мы не можем навязывать всем разработчикам одну IDE: в конце концов, PR можно сделать, хоть непосредственно отредактировав исходник через веб-интерфейс GitHub.

При этом quality gates не должны пропустить лажу вне зависимости от того, из какого текстового редактора она пришла. И вот при коммите запускается этот скрипт, который как можно быстрее должен сработать, пока разработчик «держит фокус». В моей практике, тесты на PR могут идти до получаса (больше — нет смысла), но фейлиться (если есть, за что зафейлить) оно должно чем раньше, тем лучше…

К вопросу об опечатках. У меня есть проект, в котором для нужд тестирования бэкапов, инсталляция поднимается в LXC-контейнерах (на jenkins). Там есть скрипт container-bootsrap.yaml. Да, я знаю, что в названии опечатка. Нет, я её не исправляю, потому что тогда придётся менять кучу скриптов (в т.ч. на машинах у разработчиков) и проще жить с "bootsrap'ом", чем с исправлением опечатки.


Так что, любите aspell. Но как его прикрутить к языку программирования, чтобы он не жаловался на тривиальное (def alloc_data — это не две опечатки, а нормальное в программировании)?

А я не проверяю aspell-ом названия файлов и методов. При проверке кода, Aspell выделяет и проверяет содержимое комментариев и строковых литералов. Основное применение Aspell — это спеллчекинг документации.
Когда мы внедрили SonarQube в легаси проект, мы не использовали его результаты для прекращения или продолжнения сборки при CI, НО! Все найденные ошибки просматривались в формате: Zero Tolerance к ошибкам типа Bug, Critical и Major, были заведены задачи для фикса подобных ошибок в течении двух спринтов, однако стоит отметить, что на 30 тысяч строк кода их было в районе 50 (багов было вообще штучно 2 или 3 не помню точно). В течении 2 спринтов мы избавились от этих трех и далее уже взялись за Minor, количество которых держали в пределах 100.
И кстати мы в итоге включили блокировку merge если наличествовали ошибки первых 3 типов и более 100 minor. Именно процесс CI был в итоге оставлен прежним.
Считаю такой подход тоже хорош, так как позволяет обойтись без дополнительных тикетов по результатам CI. Проблемы фиксились еще разработчиком до коммита в основную ветку.
Т. е. вы 1) взяли подмножество предупреждений, к которым решили применить Zero Tolerance 2) пофиксили их все и установили этот самый Zero Tolerance 3) остальные предупреждения ограничили простым гейтом по общему количеству. Да, вариант! Но не очень понимаю правда что такое «обойтись без дополнительных тикетов по результатам CI — проблемы фиксились еще разработчиком до коммита в основную ветку». Так ведь при любом описанном способе внедрения анализа дело даже даже не дойдёт до code review, пока quality gates не пропустят, не то что до merge в основную ветку.
На сколько я понял статью, Code Analysis внедрялся как один из степов в пайплайне, т.е. выполнялся на бранче уже после выкладывания. Мы же использовали связку TFS+SonarQube, для того, чтобы делать Code Analysis во время вливания, т.е. изменения не вливались если изменения не надлежащего качества по мнению Sonar. А дальше выкатывание происходило отдельным процессом.
Возможно я не так понял процесс описанный в статье.
Не совсем правильно поняли. Мы работаем по Github flow, и используем гейты при анализе Pull Request-ов, т. е. до их вливания в мастер. Т. е. кто-то делает PR, CI-сервер внутри себя выполняет «тестовое слияние» (без пуша в мастер) и прогоняет стат анализ и тесты. Если что-то из этого падает — PR «красный» и не может быть замёржен. Необходимое условие для вливания — «зелёный PR» + одобрение от ревьюеров.

Уже после вливания CI работает ещё раз, публикует сборку где надо и обновляет её метаданные.
На вход подаётся грязная вода
жестоко вы о чужом коде. Хотя эта фраза классно описывает ощущение от прочтения многих статей(не на хабре).

В целом статья хорошая, но зачастую стоит вопрос доступности таких решений для небольших проектов и стартапов, а также распространённости опыта интеграции данных решений среди разработчиков ввиду не самой большой доступности.
Не понимаю, что значит «вопрос доступности».

Если речь идёт о стоимости программных компонент, то Jenkins бесплатен, Checktyle / Spotbugs / Aspell и куча других инструментов бесплатны, Artifactory Pro платен, но даже для небольших проектов это подъёмная сумма, GitHub/GitLab можно использовать бесплатно или за небольшие деньги для маленьких проектов.

Если Вы имеете в виду доступность знаний… ну достаточно просто зайти и посмотреть как реализован пайплайн в хороших опен-сорс проектах. Попробуйте туда покоммитить или хотя бы поизучайте их CI пайплайны.
Sign up to leave a comment.

Articles

Change theme settings