В прошлый раз мы остановились на проблемах с форматированием, а сегодня пришло время заняться исключениями.
ErrorProne.NET унаследовал многие возможности из моего другого проекта – ExceptionAnalyzer, но в несколько измененном виде. Вот какие правила он содержит:
- ERP021 – Incorrect exception propagation: «неправильный» проброс исключения с помощью throw ex; вместо throw;
- ERP022 – Unobserved exception in generic exception handler: когда блок catch, который перехватывает все исключения возвращает управления без попытки «наблюдения» перехваченного исключения.
- ERP023 – Suspicious exception handling: only Message property was observed: когда в обобщенном блоке catch идет обращение лишь к свойству ex.Message исключения.
Теперь обо всех этих исключениях более подробно.
Incorrect exception propagation
На этой проблеме нет смысла останавливаться слишком долго. Если вы проходили хоть одно собеседование в своей жизни или работали в команде из более чем пары человек, то наверняка знаете, в чем разница между throw ex; и throw;:
try { throw new Exception(); }
catch (Exception ex)
{
throw ex;
// ~~
// Incorrect exception propagation. Use throw; instead
}
Тут никаких сюрпризов. throw ex; «сломает» стек вызовов оригинального исключения в результате чего последующий анализ логов и понимание того, что же пошло не так, будет затруднительным. Поскольку эта известная проблема, то подобные ошибки встречаются на практике довольно редко. Но несмотря на относительную это, я нашел один пример сомнительного проброса исключений в коде Розлина: CompilationWithAnalyzer.cs:602.
Unobserved exception in generic exception handler
Поскольку идея этой тулы в поиске максимально подозрительных мест, большинство правил, связанных с обработкой исключений являются простыми, конкретными и не слишком назойливыми. Так, например, как бы ни хотелось пуристам запретить все блоки catch(Exception) в их коде, на практике сделать это будет довольно сложно ввиду их распространенности. Но, ИМХО, можно и нужно предупреждать, если такие блоки возвращают управления даже не удосужившись проверить, что же за исключение они поймали.
Поэтому, следующий код приводит к выдаче предупреждения:
try { throw new Exception(); }
catch (Exception e)
{
if (e is AggregateException) return;
// ~~~~~~~
// Exit point 'return;' swallows an exception.
throw;
}
Любое обращение к переменной e в блоке catch (типа e.Message, ProcessExcpetion(e), etc) «деактивирует» это правило. Я почему-то считал, что срабатывать это правило должно относительно нечасто, и был крайне удивлен, что на коде Розлина, оно срабатывает более 60 (!) раз. Вот лишь несколько примеров, для затравки.
- AssemblyIdentityUtils.cs:40
- PdbWriter.cs:796
- AssemblyIdentity.DisplayName.cs:842
- AnalyzerFileReference.cs:158
- AbstractAnalyzerAssemblyLoader.cs:102
Да, это же правило будет срабатывать только на блоки catch {}, catch(System.Exception) и catch(System.AggregateException). Для более специфических исключений, вполне нормально возвращать управление без обращения к объекту исключения.
Suspicious exception handling: only Message property was observed
Что может быть хуже проглатывания исключений? Записывание не всей информации об исключении! Каждый раз, когда вы пишите лишь ex.Message в лог файл, всякие непонятные высокие силы делают так, чтобы ваш код грохнулся в продакшне с TypeLoadException и вас подняли в 2 часа ночи для того, чтобы выяснить, что же пошло не так с чудесно проверенной на локальном компе билдом. Чтобы вы с просоня смотрели в лог и долго думали над волшебными записями в логе типа: «Exception has been thrown by the target of an invocation.», «The type initializer for 'MyAwesomeType' threw an exception.» или «One or more errors occurred.» и гадали, “А что же, блин, произошло!».
Можно, конечно, сказать, что код мы ревьюим внимательно, что наши статические конструкторы исключений не бросают, TPL мы не признаем, да и вообще мы пишем код с первого раза и без ошибок. На это я обычно привожу пример, который показывает, что исключения заворачиваются даже там, где большинство людей этого не ожидают:
class WillThrow
{
public WillThrow()
{
throw new Exception("Oops!");
}
}
public static T Create<T>() where T : new()
{
return new T();
}
public void Sample()
{
try { Create<WillThrow>(); }
// Warn for ex.Message: Only ex.Message was observed in the exception block!
catch (Exception ex)
{
// ex.Message:
// Exception has been thrown by the target of an invocation.
Console.WriteLine(ex.Message);
// ~~~~~~~~~~
// Only ex.Message property was observed in exception block!
}
}
Поскольку ограничение new() приводит к использованию Activator.CreateInstance, то любое исключение в конструкторе объекта, создаваемого с помощью обобщенных методов, типа Create<T>, приведет к обарачиванию исходного исключения в TargetInvocationExcpetion. Такое легко пропустить во время ревью, но гораздо проще заворачивать любой код, который пишет лишь сообщение исключения, и не выводит стек-трейса + внутренних искючений.
И снова я был удивлен, количеством нарушений этого правила в коде Roslyn-а. Более десятка нарушений. Многие из которых, явные опечатки, типа вот этого, когда при генерации нового исключения, явно забыли передать старое исключение в качестве вложенного.
Комментариев нет:
Отправить комментарий