Программы статического анализа кода — это необычный класс программ-верификаторов, и в течение некоторого времени я не был убежден в необходимости их использования при разработке для фейсбука. Я не терплю стилистические правила на своей шее, и ложные предупреждения об ошибках могут испортить всю задачу. Впрочем, в них есть и хорошее: если проверяющий механически ищет проблемы, которые традиционно не контролируются компилятором, то это должно почти всегда улучшать качество кода, как только проблема будет исправлена.
Флинт, программа Фейсбука для статического анализа, выдает ошибки анализа, которые автоматически появляются в нашей системе ревью (phabricator) рядом с каждым предложенным изменением кода, уведомляя программиста, что что-то может пойти не так. Flint стал важной частью работы, которую мы делаем в Фейсбуке, и я очень рад открыть его исходники, чтобы каждый мог проверить, что же мы делаем, и попробовать это для себя.
Но почему бы не использовать существующие анализаторы кода?
Написание анализатора кода для С++ — задача не для слабонервных, ведь С++ весьма сложен в разборе. Но тем не менее, в настоящее время есть целая куча анализаторов со множеством фич, некоторые даже с открытым кодом. Так что вопрос, почему мы решили написать свой собственный, а не использовать существующие, вполне логичен.
Когда мы начинали это проект, все опробованные нами программы были слишком медленными и не поддерживали большинства нововведений С++11, которые уже использовались нами в разработке. Clang, который сегодня будет логичной отправной точкой для анализа кода на С++, предлагал слишком мало поддержки в то время. И даже сейчас он не может компилировать часть нашего кода на C++.
И самое главное, правила анализаторов кода очень сильно зависят от характера организаций, которые их используют. Мы представляли, что мы ищем, и выяснили, что какой бы анализатор мы не выбрали, нам придется долго дорабатывать его. Так что мы решил разработать собственный анализатор кода.
Токены, комментарии и язык D, ох!
Основываясь на принципе «простейшее решение, которое будет работать», флинт является токен-ориентированным, что противопоставляется построению дерева разбора кода. Анализатор загружает входной файл, конвертирует в массив токенов и по-разному анализирует это массив. Каждый токен сохраняет предшествующий комментарий (если он есть), так что комментирующая информация сохранятся. Некоторые из наших правил требуют использования комментариев в специальном стиле, вы увидите это ниже.
Целью такого дизайна было реализовать быстрый токенайзер, добавить пару простых правил и выпустить его на свободу от фейсбука с надеждой, что люди будут добавлять анализатору интересные правила. Решение добавить парсинг выкинули, но наши инженеры добавили около двух дюжин правил, которые мы проверим за минуту.
Флинт написан на D, и это первый опенсорсный проект на этом языке от Фейсбука. На самом деле, наша первая версия была написана на С++; перевод на D начинался как эксперимент. По результатам измерений и историй, в которых мы участвовали, перевод на D был победой по всем направлениям: версия на D оказалась значительно меньше, значительно быстрее собиралась, значительно быстрее запускалась и была более легкой для добавления изменений.
Перевод флинта с С++ на D
Для переезда с С++ на D я решил сделать полумеханический перевод, т.е использовать ближайший код на D c той же семантикой, что и С++ код.
При реализации на С++ я выяснил, что использование генератора лексем приносило больше проблем, чем пользы, так что я просто написал быстрый, выделенный лексер с использованием макросов. В D нет макросов, что делает перевод один в один невозможным. Но D имеет кое-что получше — полную интерпретацию в течение компиляции, которая сочетается с возможностью самоанализа, генерации и компиляции сгенерированного кода налету.
Я написал функцию на 58 строк, которая генерирует развернутое дерево совпадений. Для С++ код разворачивается в 965 строк, которые затем подаются обратно в компилятор с примесью выражений.
В таком матчинге есть как минимум один плюс — он независим от языка, который мы разбиваем на лексемы; такой подход делает возможным засунуть его в библиотеку и оставить только язык-специфичные части (такие как парсинг чисел или комментариев). При этом подходе становится легче создавать и использовать оптимальные лексеры для любого языка без необходимости использования стороннего генератора. Реализация такого решения заняла часть первого дня, после чего у меня был работающий лексер, последующие дни были потрачены на портирование анализатора и его проверку на самом себе.
После перевода цикл редактирования/сборки/тестирования flint'а стал гораздо приятнее. Сборка flint на D примерно в пять раз быстрее сборки на С++, что оказалось очень важным в итерационной разработке. Скорость запуска стала немного лучше, между 5 и 25% — в зависимости от файла, выигрыш был больше для больших файлов.
Быстрая сборка дала интересный побочный эффект для меня. Разница между обычным временем сборки в C++ и D не удивительна, но это был первый раз, когда я работал над похожими проектами параллельно, переключаясь между ними, что дало мне возможность почувствовать разницу.
С++ может производить быстрый код, но сборка С++ сопровождается целой кучей лишних телодвижений и множеством шума. Инициация сборки неизбежно сопровождается некоторой церемониальностью и пышностью («назад, парни, возможна отдача!») и в течение дня я бы аккуратно следил за сборкой, чтобы максимизировать пользу от потраченного времени. Цикл сборки проектов на D происходит обескураживающе быстро — даже как-то обидно иногда. Бойкость, с которой компилятор D проходит сквозь код, сначала удивила меня, я даже заподозрил ошибку — что мой код вообще не был скомпилирован. На самом-то деле, новый язык просто способен лучше пробегать через код на значительно больших скоростях.
Проверки, совершаемые флинтом
Каждая проверка соответствует настоящему названию функции в кодовой базе, так что вы можете просто найти её, чтобы увидеть, как эта проверка работает.
- Черный список последовательностей токенов (checkBlacklistedSequences). Некоторые последовательности могут быть просто запрещены в организации. В Фейсбуке мы считаем «volatile» запрещенным, но не всегда: мы разрешаем последовательность «asm volatile», которая означает немного другое.
- Черный список идентификаторов (checkBlacklistedIdentifiers). Некоторые идентификаторы могут быть запрещены в организации. В фейсбуке мы исключаем печально известную функцию языка C — strtok. Для нее есть безопасные альтернативы, так что нет ни одной причины для использования strtok.
- Резервированные идентификаторы (checkDefinedNames). И в С, и в С++ есть часто забываемое правило именования, согласно которому все идентификаторы, начинающиеся с подчеркивания и последующей заглавной буквы, а также все идентификаторы, содержащиеся два последовательных подчеркивания, зарезервированы для служебных нужд. (Конечно в нашем коде есть исключения от этого правила, такие как _GNU_SOURCE или _XOPEN_SOURCE, поэтому флинт использует и whitelist при проверке зарезервированных идентификаторов).
- Идиома включения гардов (checkIncludeGuard). Большинство заголовочных файлов должно быть защищено явным образом (путем использования директивы #pragma once или #ifndef макроса), так что мы добавили правило для проверки этой защиты.
- Порядок аргументов в memset (checkMemset). Многие из нас иногда писали memset(&foo, sizeof(foo), 0); и рассказывали об этом жуткие истории, пугая племянниц на Хэллоуин. Соответствующее диагностическое правило флинта позволит легко избежать этой пагубной ошибки.
- Сомнительные инклуды (#include) — checkQuestionableIncludes. Многие организации используют несколько библиотек, которые хранятся только для обеспечения обратной совместимости и никогда не используются в новом коде. Значит, такие хидеры не должны включаться — хорошая работенка для анализатора. Одной из проблем, которую мы нашли в Фейсбуке, было то, что некоторые хидеры после препроцессора становились слишком большими, чтобы их можно было включить в другие — это приводило к слишком большому времени компиляции. Нам пришлось научить флинт решать и эту задачу.
- Выделение "...-inl.h" файлов (checkInlHeaderInclusions). Популярным способом организации встраиваемого и тяжелого шаблонного кода является соответствующее разделение инлайновых и шаблонных артифактов — например, «Widget.h» превращается в «Widget-inl.h». Последний файл не должен быть включен где-то, кроме «Widget.h», и специальное правило следит за этим.
- Инициализация переменной самой себя (checkInitializeFromItself). Мы выяснили, что люди пишут конструкторы в стиле
class X { .. int a_; X(const X& rhs) : a_(a_) {} X(int a) : a_(a_) {} };В этом примере необходимо использовать a_(rhs.a_) в первом конструкторе и a_(a) во втором. В другом написании почти никогда не бывает смысла, а компилятор помалкивает на таком коде. Мы любим говорить: «Для этого есть правило флинта», когда решаем проблему.
- Предпочтительнее использовать
shared_ptr<T> p(make_shared<T>(args))вместоshared_ptr<T> p(new T)(checkSmartPtrUsage). Первая версия делает только одно выделение памяти вместо двух. - Включение опенсорсного кода (checkOSSIncludes). Facebook использует свои внутренние разработки в разных проектах, но есть один вид зависимостей, которые мы никогда не используем: зависимости от проектов с открытым исходным кодом. Folly, например, не может зависеть от какой-либо частной библиотеки. Правило анализатора сохраняет проект в порядке и делает процесс открытия исходников легким и безопасным. Я уже говорил, что мы любим опенсорс?
- Размещение статических данных уровня пространства имен в хидерах (checkNamespaceScopedStatics). Отметить данные уровня нэймспейса как статические внутри заголовочного файла — всегда плохая идея. Маркировка данных таким образом потенциально создает один экземпляр статических данных внутри каждой единицы компиляции, включая сам хидер. Исправление такой ошибки привело к снижению размера и увеличению скорости работы исполняемых файлов.
- «Widget.cpp» должен включать «Widget.h» раньше чего-либо еще (checkIncludeAssociatedHeader). Это хорошее правило кодирования, потому что предотвращает возможные ошибки в определении «Widget.h» — мы можем забыть, что «Widget.h» должен включать какие-то дополнительные файлы.
- Все исключения должны перехватываться по ссылке (checkCatchByReference). 'Nuff said.
- Ликвидация распространенных ошибок в определении конструктора. Люди часто забывают указать конструктор с одним аргументом как explicit, но флинт напомнит автору об этой проблеме. При этом автор может отказаться от таких напоминания, добавив стилизованный комментарий /* implicit */. Флинт также сигнализирует о копирующих конструкторах не по константной ссылке и бесполезных конструкторах перемещения, как в примере ниже:
class C { ... // Плохие паттерны, флинт выдаст диагностическое сообщение C(int a) C(const C&&); // Хорошие паттерны, флинт такие любит explicit C(int a); /* implicit */ C(char* a); C(int a, double b); }; - Throw спецификации (checkThrowSpecification). Throw спецификации устарели и должны удаляться из кода. Флинт позволяет сделать еще одно улучшение: классы, наследующиеся от std::exception, должны добавлять throw() в деструкторе и реализовывать метод what().
- Проверка против бросания исключения у указателей, инициализированных с помощью new (checkThrowsHeapException). Это предотвращает антипаттерн throw new T.
- Конфликтующие директивы using namespace (checkUsingNamespaceDirectives). Некоторые пространства имен используют одинаковые идентификаторы — например, boost и STL: оба они определяют shared_ptr. Мы испытали проблему несовместимости, так что мы добавили правило анализатора, которое предотвращает использование конфликтующих пространств имен одновременно.
- Пространства имен в заголовочных файлах (checkUsingDirectives). «using namespace» никогда не должно встречаться внутри хидера. Но это может происходить внутри inline-функции в заголовочном файле. Фейсбук считает, что пространство имен «facebook» может быть введено только на верхнем уровне. Вам надо заменить название нэймспейса на какое-нибудь свое.
- Неправильное использование библиотек (checkFollyDetail). Это распространенная практика разместить библиотекозависимый код в нэймспейс типа «detail». Иногда такой код не может быть инкапсулирован от клиентской части из-за прозрачности шаблонов в C++. Флинт выдает предупреждения против использования таких нэймспейсов, названных в стиле «folly::detail».
- Передача «дешевых» типов по ссылке (checkFollyStringPieceByValue). Некоторые типы, такие как итераторы или пара итераторов, являются маленькими и дешевыми в копировании. Поэтому их легче передавать по значению вместо передачи по константной ссылке. Пример — StringPiece в Folly, который занимает два слова и имеет семантику простого копирования.
- Нет protected-наследованию (checkProtectedInheritance). Защищенное наследование — странная вещь, и оставлена лишь для полноты картины, но никак не для практического применения.
- Нет неявному преобразования операторов (checkImplicitCast). Неявное преобразование операторов опасно так же, как неявное преобразование конструкторов:
class C { // Плохие паттерны, флинт выдаст диагностическое сообщение operator string(); operator bool(); // Хорошие паттерны, флинт такие любит /* неявно! */ operator string(); explicit operator bool(); }; - Плохой и устаревший NULL должен быть везде заменен на nullptr (checkUpcaseNull).
- Проверка, что std::exception всегда отнаследован публично (checkExceptionInheritance). Обратите внимание:
class MyException : std::exception { ... };Автор хотел определить собственный класс исключения, но забыл указать наследование публичным. В соответствии с правилами языка, наследование по умолчанию будет приватным. В конечном счете, интересный эффект проявится в том, что код, который должен будет перехватывать все стандартные и пользовательские исключения — catch (const std::exception&), не сможет отработать правильно, потому что приватное наследование исключает неявное преобразование типов. Это не тот тип ошибок, который легко обнаружить без весьма сложного тестирования. Для предотвращения таких ошибок флинт статически отключает непубличное наследование от std::exception25.
- Проверка на «мимолетные” rvalue-конструкции, которые иногда бесполезны (checkMutexHolderHasName). Рассмотрим: mutex m_lock;… lock_guard(m_lock); Вне зависимости от цели, этот код не делает ничего полезного: это просто вызов конструктора lock_guard. Это создаст rvalue, которое будет жить в промежутке между закрывающей скобкой и точкой с запятой. В подтверждение Закона Кармака (»Все, что синтаксически правильно и принимается компилятором, рано или поздно попадет в ваш код") вы получаете жизненную необходимость в правилах синтаксического анализа. В идеале, оно должно перехватить все неиспользуемые переменные. На текущий момент мы определяем только несколько распространенных подозрительных мест.
Раскрытие исходников flint
Мы очень рады открыть исходники флинта, потому что это хорошая иллюстрация «простейшего решение, способного работать» и интересный пример кросс-языкового перевода. Надеемся, что вы сочтете флинт полезным — мы уже сочли. Оставайтесь с нами для будущих улучшений, мы с нетерпением ждем ваших отзывов.
Благодарности
Большое спасибо Nicholas Ormrod и Robbert Haarman за просмотр раннего черновика этой статьи. Слишком много инженеров внесли свой вклад в исходники флинта, чтобы указать их всех здесь; их вклад отмечен в журнале git'а.
Примечание переводчикаВ хабе С++ много статей про статический анализ кода — и почти все они посвящены PVS Studio и CppCat. Когда facebook заявил про свое подобие анализатора, эта новость осталась незамеченной. Статья Александреску поначалу показалась мне тяжелой для понимания, поэтому я решил ее перевести. После перевода она все же остается непростой — поэтому буду рад адекватной критике и помощи в переводе. Там есть неточности и непонятные места — и я буду признателен тем, кто поможет с этими местами. Я не могу сказать, что понял 16-е правило — буду рад, если в комментариях или личной переписке кто-то сможет объяснить его — я добавлю это в статью.
Мое субъективное мнение — флинт больше проверяет стиль и правила написания кода, нежели ищет фактические ошибки — в этом он явно уступает и CppCheck, и продуктам от Viva64. Диагностические правила действительно специфичны — часть из них применима к любому коду (защита хидеров и порядок их включения), а часть — наоборот: если вы не планируете выпускать свой код наружу, то нет смысла отказываться от опенсорсных библиотек.
Я не знаком с языком D, так что если кто-то опробует flint в деле и напишет об этом статью, то будет очень здорово. Ну и сравнение продукта от фейсбука с уже имеющимися альтернативами, рассказ о добавлении правил — я считаю, что это будет очень интересно — ждем заинтересованного автора!
Автор: vovochkin
