Русский
Русский
English
Статистика
Реклама

Старый код

Как внедрить статический анализатор кода в legacy проект и не демотивировать команду

20.06.2020 18:10:45 | Автор: admin
PVS-Studio охраняет сон программиста

Попробовать статический анализатор кода легко. А вот, чтобы внедрить его, особенно в разработку большого старого проекта, потребуется умение. При неправильном подходе анализатор может добавить работы, замедлить разработку и демотивировать команду. Давайте кратко поговорим, как правильно подойти к интеграции статического анализа в процесс разработки и начать его использовать как часть CI/CD.


Введение


Недавно моё внимание привлекла публикация "Getting Started With Static Analysis Without Overwhelming the Team". С одной стороны, это хорошая статья, с которой стоит познакомиться. С другой стороны, как мне кажется, в ней так и не прозвучало полного ответа, как безболезненно внедрить статический анализ в проект с большим количеством legacy кода. В статье говорится, что можно смириться с техническим долгом и работать только с новым кодом, но нет ответа на то, что делать с этим техническим долгом потом.

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

Проблематика


Запустить и посмотреть, как работает статический анализатор, как правило, несложно [1]. Можно увидеть в коде интересные ошибки или даже страшные потенциальные уязвимости. Можно даже что-то поправить, но вот дальше у многих программистов опускаются руки.

Все статические анализаторы выдают ложные срабатывания. Такова особенность методологии статического анализа кода, и с ней ничего нельзя сделать. В общем случае это нерешаемая задача, что подтверждается теоремой Райса [2]. Не помогут и алгоритмы машинного обучения [3]. Если даже человек не всегда может сказать, ошибочен тот или иной код, то не стоит ждать этого от программы :).

Ложные срабатывания не являются проблемой, если статический анализатор уже настроен:
  • Отключены неактуальные наборы правил;
  • Отключены отдельные неактуальные диагностики;
  • Если мы говорим о C или C++, то размечены макросы, содержащие специфические конструкции, из-за которых появляются бесполезные предупреждению в каждом месте, где такие макросы используются;
  • Размечены собственные функции, выполняющие действия аналогичные системным функциям (свой аналог memcpy или printf) [4];
  • Точечно с помощью комментариев отключены ложные срабатывания;
  • И так далее.

В таком случае, можно ожидать низкий уровень ложных срабатываний на уровне порядка 10-15% [5]. Другим словами, 9 из 10 предупреждений анализатора будут указывать на реальную проблему в коде или, по крайней мере, на код с сильным запахом. Согласитесь, такой сценарий крайне приятен, и анализатор является настоящим другом программиста.
Предупреждения

В реальности в большом проекте первоначальная картина будет совсем иная. Анализатор выдаёт сотни или тысячи предупреждений на legacy код. Что из этих предупреждений по делу, а что нет, быстро понять невозможно. Сесть и начать разбираться со всеми этими предупреждениями нерационально, так как основная работа в этом случае остановится на дни или недели. Как правило, команда не может позволить себе такой сценарий. Также возникнет огромное количество diff-ов, которые портят историю изменений. А быстрая массовая правка такого количества фрагментов в коде неизбежно выльется в новые опечатки и ошибки.

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

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

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

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

Здесь та же аналогия, что и с предупреждениями компилятора. Неспроста рекомендуют держать количество предупреждений компилятора на уровне 0. Если предупреждений 1000, то, когда их станет 1001, на это никто не обратит внимание, да и не понятно, где искать это самое новое предупреждение.
Неправильное внедрение статического анализа

Самое худшее в этой истории, если кто-то сверху в этот момент заставит использовать статический анализ кода. Это только демотивирует команду, так как с их точки зрения появится дополнительная бюрократическая сложность, которая только мешает. Отчёты анализатора никто не будет смотреть, и всё использование будет только на бумаге. Т.е. формально анализ встроен в DevOps процесс, но на практике от этого никому никакой пользы нет. Мы слышали подробные истории при общении на стендах от посетителей конференций. Подобный опыт может надолго, если не на всегда, отбить желание у программистов связываться с инструментами статического анализа.

Внедрение и устранение технического долга


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

CI/CD


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

Фиксация существующего технического долга и работа с новыми предупреждениями


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

Чтобы быстро начать использовать статический анализ, мы предлагаем пользователям PVS-Studio воспользоваться механизмом массового подавления предупреждений [6]. Общая идея в следующем. Пользователь запустил анализатор и получил множество предупреждений. Раз проект, разрабатываемый много лет, жив, развивается и приносит деньги, то, скорее всего, в отчёте не будет много предупреждений, указывающих на критические дефекты. Другими словами, критические баги так или иначе уже поправлены более дорогими способами или благодаря фидбеку от клиентов. Соответственно, всё, что сейчас находит анализатор, можно считать техническим долгом, который непрактично стараться устранить сразу.

Можно указать PVS-Studio считать эти предупреждения пока неактуальными (отложить технический долг на потом), и он больше не будет их показывать. Анализатор создаёт специальный файл, где сохраняет информацию о пока неинтересных ошибках. И теперь PVS-Studio будет выдавать предупреждения только на новый или измененный код. Причем, всё это реализовано умно. Если, например, в начало файла с исходным кодом будет добавлена пустая строка, то анализатор понимает, что, по сути, ничего не изменилось, и по-прежнему будет молчать. Этот файл разметки можно заложить в систему контроля версий. Файл большой, но это не страшно, так как часто его закладывать смысла нет.

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

Итак, первая проблема, с внедрением анализатора в большой старый проект решена. Теперь давайте разберёмся, что делать с техническим долгом.

Исправление ошибок и рефакторинг


Самое простое и естественное это выделить некоторое время на разбор массово подавленных предупреждений анализатора и постепенно разбираться с ними. Где-то следует исправить ошибки в коде, где-то провести рефакторинг, чтобы подсказать анализатору, что код не является проблемным. Простой пример:
if (a = b)

На подобный код ругается большинство С++ компиляторов и анализаторов, так как высока вероятность, что на самом деле хотели написать (a == b). Но существует негласное соглашение, и это обычно отмечено в документации, что если стоят дополнительные скобки, то считается, что программист сознательно написал такой код, и ругаться не надо. Например, в документации PVS-Studio к диагностике V559 (CWE-481) явно написано, что следующая строчка будет считаться корректной и безопасной:
if ((a = b))

Другой пример. Забыт ли в этом C++ коде break или нет?
case A:  foo();case B:  bar();  break;

Анализатор PVS-Studio выдаст здесь предупреждение V796 (CWE-484). Возможно, это не ошибка, и тогда следует дать анализатору подсказку, добавив атрибут [[fallthrough]] или, например, __attribute__((fallthrough)):
case A:  foo();  [[fallthrough]];case B:  bar();  break;

Можно сказать, что подобные изменения кода не исправляют ошибки. Да, это так, но выполняется два полезных действия. Во-первых, отчёт анализатора избавляется от ложных срабатываний. Во-вторых, код становится более понятным для людей, занимающихся его сопровождением. И это очень важно! Ради одного этого уже стоит проводить мелкий рефакторинг, чтобы код стал понятней и легче в сопровождении. Раз анализатору непонятно, нужен break или нет, это будет непонятно и коллегам-программистам.

Помимо исправления ошибок и рефакторинга, можно точечно подавлять явно ложные предупреждения анализатора. Какие-то неактуальные диагностики можно отключить. Например, кто-то считает бессмысленными предупреждения V550 о сравнении значений типа float/double. А кто-то относит их к важным и заслуживающим изучения [7]. Какие предупреждения считать актуальными, а каким нет, решать только команде разработчиков.

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

Также, мы всегда помогаем нашим клиентам настроить PVS-Studio, если возникают какие-то сложности. Более того, были случае, когда мы сами устраняли ложные предупреждения и исправляли ошибки [8]. На всякий случай решил упомянуть, что возможен и такой вариант расширенного сотрудничества :).

Метод храповика


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

Храповик


Фиксируется количество предупреждений, которое выдаёт статический анализатор. Quality gate настраивается таким образом, что теперь можно заложить только такой код, который не увеличивает количество срабатываний. В результате запускается процесс постепенного уменьшения количества срабатываний за счёт настройки анализатора и правки ошибок.

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

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

У автора статьи есть и доклад на эту тему: "Непрерывный статический анализ".

Заключение


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

Существуют и другие типовые сомнения, сможет ли статический анализатора действительно быть удобен и полезен. Я постарался развеять большинство таких сомнений в публикации Причины внедрить в процесс разработки статический анализатор кода PVS-Studio [9].

Спасибо за внимание, и приходите скачать и попробовать анализатор PVS-Studio.

Дополнительные ссылки


  1. Андрей Карпов. Как быстро посмотреть интересные предупреждения, которые выдает анализатор PVS-Studio для C и C++ кода?
  2. Wikipedia. Теорема Райса.
  3. Андрей Карпов, Виктория Ханиева. Использование машинного обучения в статическом анализе исходного кода программ.
  4. PVS-Studio. Документация. Дополнительная настройка диагностик.
  5. Андрей Карпов. Характеристики анализатора PVS-Studio на примере EFL Core Libraries, 10-15% ложных срабатываний.
  6. PVS-Studio. Документация. Массовое подавление сообщений анализатора.
  7. Иван Андряшин. О том, как мы опробовали статический анализ на своем проекте учебного симулятора рентгенэндоваскулярной хирургии.
  8. Павел Еремеев, Святослав Размыслов. Как команда PVS-Studio улучшила код Unreal Engine.
  9. Андрей Карпов. Причины внедрить в процесс разработки статический анализатор кода PVS-Studio.



Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. How to introduce a static code analyzer in a legacy project and not to discourage the team.
Подробнее..

Статический анализ baseline файлы vs diff

25.06.2020 12:16:27 | Автор: admin

В статических анализаторах рано или поздно приходится решать задачу упрощения интеграции в существующие проекты, где поправить все предупреждения на legacy коде невозможно.


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



baseline или так называемый suppress profile


Этот метод имеет несколько названий: baseline файл в Psalm и Android Lint, suppress база (или профиль) в PVS-Studio, code smell baseline в detekt.


Данный файл генерируется линтером при запуске на проекте:


superlinter --create-baseline baseline.xml ./project

Внутри себя он содержит все предупреждения, которые были выданы на этапе создания.


При запуске статического анализатора с baseline.xml, все предупреждения, которые содержатся в этом файле, будут проигнорированы:


superlinter --baseline baseline.xml ./project

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


Обычно, мы хотим достичь следующего:


  • На новый код выдаются все предупреждения
  • На старый код предупреждения выдаются только если его редактировали
  • (опционально) Переносы файлы не должны выдавать предупреждения на весь файл

То, что мы можем конфигурировать в этом подходе это какие поля предупреждения формируют хеш (или "сигнатуру") срабатывания. Чтобы уйти от проблем смещения строк кода, лучше не добавлять номер строки в эти поля.


Вот примерный список того, что вообще может являться полем сигнатуры:


  • Название или код диагностики
  • Текст предупреждения
  • Название файла
  • Строка исходного кода, на которую сработало предупреждение

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


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


Ещё один менее очевидный признак имя функции или метода, в которой выдавалось предупреждение. Это позволяет снизить количество коллизий, но провоцирует шквал предупреждений на всю функцию, если вы её переименуете.


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


Хорошо подобранный набор признаков увеличивает эффективность baseline подхода.


Коллизии в методе baseline


Допустим, диагностика W104 находит вызовы die в коде.


В проверяемом проекте есть файл foo.php:


function legacy() {  die('test');}

Предположим, используются признаки {имя файла, код диагностики, строка исходного кода}.


Наш анализатор при создании baseline добавляет вызов die('test') в свою базу исключений:


{  "filename": "foo.php",  "diag": "W104",  "line": "die('test');"}

Теперь добавим немного нового кода:


+ function newfunc() {+   die('test');+ }  function legacy() {    die('test');  }

По всем используемым признакам новый вызов die('test') будет распознаваться как игнорируемый код. Совпадение сигнатур для потенциально разных кусков кода мы и называем коллизией.


Одним из решений является добавление дополнительного признака, который позволит различать эти вызовы. К примеру, это может быть поле "имя содержащей функции".


Что делать, если вызов die('test') был добавлен в ту же функцию? Этот вызов может иметь одинаковые соседние строки в обоих случаях, поэтому добавление предыдущей и следующей строки в сигнатуре не поможет.


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


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


Метод, основанный на diff возможностях VCS


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


Утилита revgrep принимает на stdin поток предупреждений, анализирует git diff и выдаёт на выход только те предупреждения, которые исходят от новых строк.


golangci-lint использует форк revgrep как библиотеку, так что в основе его вычисления diff'а лежат те же алгоритмы.

Если выбран этот путь, придётся искать ответ на следующие вопросы:


  • Как выбрать окно коммитов для вычисления diff?
  • Как будем обрабатывать коммиты, пришедшие из основной ветки (merge/rebase)?

Вдобавок нужно понимать, что иногда мы всё же хотим выдавать предупреждения, выходящие за рамки diff. Пример: вы удалили класс директора по мемам, MemeDirector. Если этот класс упоминался в каких-либо doc-комментариях, желательно, чтобы линтер сообщил об этом.


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


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


diff режим в NoVerify


NoVerify имеет два режима работы: diff и full diff.


Обычный diff может найти предупреждения по файлам, которые были затронуты изменениями в рамках рассматриваемого диапазона коммитов. Это работает быстро, но хорошего анализа зависимостей изменений не происходит, поэтому новые предупреждения из незатронутых файлов мы найти не можем.


Full diff запускает анализатор дважды: один раз на старом коде, затем на новом коде, а потом фильтрует результаты. Это можно сравнить с генерацией baseline файла на лету с помощью того, что мы можем получить предыдущую версию кода через git. Ожидаемо, этот режим увеличивает время выполнения почти вдвое.


Изначальная схема работы предполагалась такая: на pre-push хуках запускается более быстрый анализ, в режиме обычного diff'а, чтобы люди получали обратную связь как можно быстрее. На CI агентах полный diff. В результате время от времени люди спрашивают, почему на агентах проблемы были найдены, а локально всё чисто. Удобнее иметь одинаковые процессы проверки, чтобы при прохождении pre-push хука была гарантия прохождения на CI фазы линтера.


full diff за один проход


Мы можем делать приближенный к full diff аналог, который не требует двойного анализа кода.


Допустим, в diff попала такая строка:


- class Foo {

Если мы попробуем классифицировать эту строку, то определим её как "Foo class deletion".


Каждая диагностика, которая может как-то зависеть от наличия класса, должна выдавать предупреждение, если имеется факт удаления этого класса.


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


Переименования не требуют дополнительной обработки. Мы считаем, что символ со старым именем был удалён, а с новым добавлен:


- class MemeManager {+ class SeniorMemeManager {

Основной сложностью будет правильно классифицировать строки изменений и выявить все их зависимости, не замедлив алгоритм до уровня full diff с двойным обходом кодовой базы.


Выводы


baseline: простой подход, который используется во многих анализаторах. Очевидным недостатком данного подхода является то, что этот baseline файл нужно будет где-то разместить и, время от времени, обновлять его. Точность подхода зависит от удачного подбора признаков, определяющих сигнатуру предупреждения.


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


baseline diff
+ легко сделать эффективным + не требует хранить файлов
+ простота реализации и конфигурации + проще отличать новый код от старого
- нужно решать коллизии - сложно правильно приготовить

Могут существовать гибридные подходы: сначала берёшь baseline файл, а потом разрешаешь коллизии и вычисляешь смещения строк кода через git.


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

Подробнее..

Категории

Последние комментарии

  • Имя: Макс
    24.08.2022 | 11:28
    Я разраб в IT компании, работаю на арбитражную команду. Мы работаем с приламы и сайтами, при работе замечаются постоянные баны и лаги. Пацаны посоветовали сервис по анализу исходного кода,https://app Подробнее..
  • Имя: 9055410337
    20.08.2022 | 17:41
    поможем пишите в телеграм Подробнее..
  • Имя: sabbat
    17.08.2022 | 20:42
    Охренеть.. это просто шикарная статья, феноменально круто. Большое спасибо за разбор! Надеюсь как-нибудь с тобой связаться для обсуждений чего-либо) Подробнее..
  • Имя: Мария
    09.08.2022 | 14:44
    Добрый день. Если обладаете такой информацией, то подскажите, пожалуйста, где можно найти много-много материала по Yggdrasil и его уязвимостях для написания диплома? Благодарю. Подробнее..
© 2006-2024, personeltest.ru