Уже давно хотел поразбираться с анализаторами на основе Розлина. Тем более, что меня уже был опыт создания плагинов для Resharper-а (R# Contract Editor Extension), поэтому хотелось сравнить разные инфраструктуры и удобство использования. Есть идея переписать этот плагин с помощью анализаторов Roslyn-а, но я решил начать с чего-то попроще.
Цель недельного проекта была такая: сделать простой анализатор, который будет показывать типовые ошибки обработки исключений. Самые болезненные с моей точки зрения такие:
- Повторная генерация исключений с помощью throw ex;
- “Проглатывание” всех исключений с помощью пустых блоков catch {} или catch(Exception) {}.
- “Проглатывание” исключений в определенных ветках блока catch.
- Сохранение в логгах только сообщения ex.Message, теряя при этом потенциально важную информацию о месте возникновения исключения.
- Некорректное пробрасывание новых исключений из блока catch.
Начало работы
Для начала разработки анализатора необходимо установить VS2015 CTP (проще всего взять готовую вирталку), после этого нужно поставить VS 2015 SDK, .NET Compiler Platform SDK Templates и .NET Compiler Platform Syntax Visualizer. Визуализатор будет незаменим для понимания того, как выглядят синтаксические деревья, как их правильно анализировать и как генерировать новые деревья в фиксах. Самое главное, нужно установить правильные версии инструментов (для CTP6 все VSIX пакеты должны быть для CTP6). На главной странице проекта Roslyn всегда будут находиться актуальная инструкция по установке.
Команда разработчиков проделала огромную работу, чтобы создание анализаторов было максимально простым и удобным. Достаточно создать новый проект, выбрать шаблон Extensibility -> Diagnostics with Code Fix (Nuget + VSIX), вбить имя анализатора и все. В результате, будет создано три проекта: сам анализатор, проект с юнит-тестами и проект с установщиком (VSIX). По умолчанию, в проект добавлен пример анализатора, который показывает предупреждение на имена типов с lowercase буквами.
После чего, можно запускать тесты, или же выбрать VSIX-проект в качестве стартового, и нажать F5. Тогда будет запущен еще один экземпляр Visual Studio с установелнным анализатором, который будет выдавать предупреждение на все типы с буквами в нижнем регистре:
Способы установки аналазиторов
Существует три способа установки анализатора:
· С помощью VSIX пакета, который может быть скачан с Visual Studio Gallary напрямую, или через “Extensions and Updates”.
· Вручную установить анализатор для каждого проекта:
· Также анализаторы можно распространять через NuGet вместе с библиотекой, или просто установить через Managed NuGet Packages:
При этом ссылка на пакет будет закомичена в source control, что позволит всем участникам проекта использовать один набор анализаторов.
Тестируемость
При разработке плагина для ReSharper-а мне больше всего не хватало простых юнит-тестов. Команда JetBrains разработала серьезную инфраструктуру тестирования, но все тесты являются интеграционными. В R# не существует абстрактных тестов, все они относятся к одной из категорий: тестирование доступности контекстного действия, тестирование результата «фикса» и т.п. При этом тесту обязательно нужно подсунуть cs-файл с кодом, по которому будет прогоняться анализатор и еще один файл для сравнения результатов. Проверить свою бизнес-логику в изоляции невозможно!
В Розлине пошли по более простому пути. Анализаторы работают с синтаксическим и семантическим деревьями (Syntax Tree и Semantic Tree), которые легко создать в тестах из файла или из строки. В результате, в тесте можно проверять сам анализатор, а можно и проверять свою бизнес модель, передавая ей фрагменты синтаксического дерева:
[TestMethod]
public void SimpleTestWarningOnEmptyBlockThatCatchesException()
{
var test = @"
using System;
namespace ConsoleApplication1
{
class TypeName
{
public static void Foo()
{
try { Console.WriteLine(); }
{on}catch(System.Exception) {}
}
}
}";
var warningPosition = test.IndexOf("{on}");
var diagnostic = GetSortedDiagnostics(test.Replace("{on}", "")).Single();
Assert.AreEqual(EmptyCatchBlockAnalyzer.DiagnosticId, diagnostic.Id);
Assert.AreEqual("'catch(System.Exception)' block is empty.
Do you really know what the app state is?",
diagnostic.GetMessage());
Assert.AreEqual(warningPosition, diagnostic.Location.SourceSpan.Start);
}
Также очень радует то, что тесты запускаются быстро, поскольку несмотря на несколько миллионов строк кода в проекте Розлин, он хорошо структурирован и не приходится загружать десятки лишних сборок.
Полученные возможности
Итак, что может полученный анализатор? На данный момент он поддерживает пять основных правил:
Каждое из них проще всего продемонстрировать с помощью примера. Некоторые примеры показаны с помощью анимации, которая далека от идеала. VS2015 установлена на виртуалке и захват изображения слегка косячит при этом. Но суть будет ясна.
1. Empty catch block considered harmful!
Самая серьезный code smell при работе с исключениями – это полное подавление всех исключений с пустым блоком catch или catch(Exception):
2. Swallow exceptions considered harmful
Особое место на котле должно быть приготовлено тем, кто перехватывает все исключения с помощью блока catch{}. Фикс в этом случае очень простой: добавление throw;
3. Catch block swallows an exception
Еще один анализатор, который предупреждает о подавлении исключений. Catch-блок может быть не пустым, при этом он все равно может «проглатывать» исключения. В этом случае довольно тяжело придумать правильный фикс, особенно, когда подавление исключения происходит лишь в одной из веток блока catch:
Да, попытка сделать подобный анализатор без Розлина привела бы к месяцам работы и все равно, он был бы кривым в доску. В Розлине есть встроенная поддержка анализа потока исполнения (control flow analysis), на основе которого сделать этот анализатор было не сложно.
4. Rethrow exception properly
Это одна из самых распространенных ошибок, когда проброс исключения осуществляется с помощью throw ex;, а не с помощью throw. На всякий случай, напомню, что в первом случае – стек-трейс исходного исключения будет утерян и будет впечатление, что блок catch является источником.
5. Tracing ex.Message considered harmful
Еще одна типовая ошибка обработки исключений, когда в консоль или лог сохраняется только ex.Message и ex.StackTrace. Поскольку исключения очень часто формируют дерево исключений, то сообщение верхнего уровня может вообще не содержать ничего полезного!
Данный анализ выдает предупреждение, если catch-блок использует («наблюдает») свойство Message, но не интересуется подробностями внутренностей исключения.
Именно из-за такой прекрасной модели трассировки исключений мне пришлось выходить на работу первого января и заливать хот-фикс на прод, который бы дал понять, а что же творится с системой. Никогда не логируйте только лишь ex.Message! НИКОГДА!
6. Add catched exception as inner exception
Блок catch может бросать исключение, но даже в этом случае информация об исходном исключении может быть потеряна. Чтобы избежать этого, новое исключение должно содержать исходное исключение в качестве вложенного.
Данный анализатор проверяет код генерации нового исключение и если исходное исключение не используется, то предлагает добавить его в качестве вложенного (естественно, для этого у генерируемого исключения должен быть конструктор, который принимает пару параметров – string и Exception):
Вместо заключения
Я не хочу останавливаться подробно на разработке самого анализатора, по крайней мере, не в этом посте. В сети есть довольно неплохие примеры (ссылки в конце статьи), к тому же, я не считаю себя в экспертом в этом деле. Смысл этого поста в том, чтобы показать легкость создания анализатор своими руками, поскольку всю черную работу за вас будет делать Розлин. Так что если вдруг у вас в команде есть некоторая типовая проблема с кодом и вы хотите формализовать некий стандарт кодирования, то написание собственного анализатора будет неплохой идеей.
Дополнительные ссылки
-
Exception Analyzer on Nuget.org (доступен через Add NuGet Package, только в качестве фида нужно использовать nuget.org, а не api.nuget.org)
Пара вводных статей в MSDN Magazine о создании анализаторов:
З.Ы. Если у вас есть пожелания к анализатору исключений, то свистите, я с удовольствием их добавлю.
З.Ы.Ы. Насколько будет интересным цикл постов про разработку анализаторов?
Отличный анализатор получился! Цикл постов однозначно нужен. Было бы интересно почитать про сам процесс разработки: как проводится работа с синтаксическим деревом, какие у Roslyn-а самые полезные API и т. п.
ОтветитьУдалитьАндрей, спасибо!
УдалитьКстати, было бы интересно твое мнение по поводу дополнительный сценариев этого анализатора.
Лично мне не очень нравится, когда в блоке catch стоит обычный Exception, а не его наследник. Этим программист как бы говорит «тут может случится что-то плохое, поэтому я на всякий случай буду отлавливать все исключения, хотя конкретные исключительные ситуации назвать не могу». В ряде случаев такой подход оправдан, но я предпочитаю всегда явно указывать типы исключений и обрабатывать их соответствующим образом. Скажем, если я работаю с файлами, то я ожидаю именно IOException. Если в этом же блоке я произвожу арифметические операции над считанными данными, то я могу предположить, что возникнет ArithmeticException. При этом изначальная логика обработки ошибок может совпадать, но важно их разделить на начальном этапе: этим мы обозначаем возможные проблемы. Скажем, завтра я захочу выдавать пользователю сообщения об ошибках. На IOException я скажу, что возникли проблемы с доступом к файлу, а на ArithmeticException — что проблемы с данными в файле.
УдалитьУниверсальный рефакторинг тут сложно придумать, но я бы выдавал программисту совет: «А может тебе стоит проанализировать код, определить возможные исключения и выписать их явно?» Подход обсуждаемый, далеко не все со мной согласны, но я бы себе такую «предупреждалку» точно бы поставил. Если Roslyn позволяет делать какие-нибудь настройки, то здорово было бы сделать фичу опциональной. У некоторых проектов специфика такова, что там на каждый чих может случится что угодно. А вот в моих проектах такой подход позволил бы повысить качество кода. Пока что приходится время от времени устраивать Ctrl+F-рейды, чтобы найти подобные фрагменты кода.
Андрей, все верно говоришь, но попробую быть КЭПом. Если убрать catch(Exception ex) то можно словить Breaking Changes. Раньше - функция не выбрасывала исключение - теперь - что-то ловим, что-то нет. Поэтому лучше все же специфицировать вначале, а потом уже общий (Exception ex). Это если про рефакторинг. Если новая функция - ну тут надо посмотреть. А вообще общего рецепта похоже нет :(
ОтветитьУдалитьНу, я специально несколько раз оговорился, что тут много на что смотреть нужно. В идеальном мире мы должны чётко знать все типы возможных исключений, чтобы корректно каждый обработать. Увы, не всегда так получается, поэтому я и предложил сделать фичу опциональной. Лично я стараюсь catch(Exception ex) делать только на самых верхних уровнях абстракции, чтобы отлавливать неожиданные штуки. И в этом случае обязательно пишу сообщение в лог, мол, в проекте что-то сделано криво, надо срочно править. Тут проблема не в том, что возникло какое-то исключение само по себе, а в том, что разработчики скорее всего пропустили какой-то источник проблем. Нужно в ситуации разобраться и прописать типизированную обработку исключения в нужном месте.
УдалитьКстати, даже в текущем виде, мне кажется анализатор деалет все, что нужно.
УдалитьСейчас он не просто ругается на использование catch(Exception), но ругается на каждую ветку исполнения, которая проглатывает исключение!
Ведь нет ничего плохо в самом боке catch(Exception), который логирует исключение и пробрасывает его с помощью throw;, проблема возникает только тогда, когда исклюение подавляется.
Так что твои запросы, ИМХО, должны быть здесь учтены:)
А что плохого в том, если исключение ловится, логируется и не пробрасывается дальше?
УдалитьНапример, я запускаю асинхронную задачу, которая вполне может завершиться аварийно (пользователь подсунул на обработку невалидный файл; отпала сеть и передача файла на сервер завершилась с ошибкой).
Поймал исключение, залогировал. Больше ничего не надо.
Противоречие детектыд:) Если пользователь подсунул невалидный файл, то ему нужно об этом сообщить:)
УдалитьДа, есть ряд мест в приложении, где перехват всех исключений является валидным сценарием: код пользовательского интерфейса - один вариант, методы потока или пула потока - другой.
Но даже вэтом случае, гораздо полезнее различать ожидаемое исключение о невалидном формате файла, от произвольного исключения, пойманного в блоке catch(Exception), ведь кто сказал, что пойманное исключение связано с невалидным форматом файла, а не с багом в вашем приложении?
Ну да, так сообщать все равно надо где-то именно в catch (Exception ex). И зачем после этого throw ?
УдалитьПосле этого throw не нужен. Мы с Андреем имели ввиду то, что нужен подобный высокоуровневый блок catch(Exception) намного реже, чем он используется на самом деле. Им очень часто злоупотребляют. И даже в случае с UI-м, гораздо лучше сделать `catch(ImportException)`, а не `catch(Exception)`.
УдалитьОк. Набросаем на вентилятор :). Твой код не выбрасывает исключения (ты описал это в документации). Однако, он использует стратегию, которая использует адаптер, который использует абстрактную фабрику (в доме который построил Джек...). Все это инстанцируется с помощью DI. Каждый из объектов может выкинуть exception. Сереж, ты будешь писать Exception ex? И чем более слабосвязанный код - тем сложнее отследить широту проблемы. Если говорить про анализаторы - то интересный вариант - пройти в ГЛУБИНУ и посмотреть КАКИЕ виду exceptions может выкинуть код под try. В общем случае задача не решается IMHO, но с ограничением глубины - решабельно.
ОтветитьУдалитьЖень, я понимаю проблему. И во многих случаях картина именно такова, как ты описал. Но вот совсем недавно, я видел такой код (новый код):
Удалитьint result;
try { result = int.Parse(str);}
catch { result = 0;}
В случае запутанной логики ПРИДЕТСЯ перехватывать все исключения, но это все равно нужно гораздо реже, чем используется на практике.
З.Ы. Анализ исключений сверху вниз - невозможен, это нерешаемая сейчас задача computer science (называется halting problem, кажется). Но можно пойти снизу вверх и сделать такие-себе проверяемые исключения. Вот такой анализ вполне возможен. Если каждый метод будет описывать свои типы исключений в секции exception, то вызывающий код (и анализатор) смогут проверить, что делается в блоке catch при вызовае этого метода. Вот эту штуку я попробую добавить в анализатор.