ErrorProne.NET. Часть 2

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

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-а. Более десятка нарушений. Многие из которых, явные опечатки, типа вот этого, когда при генерации нового исключения, явно забыли передать старое исключение в качестве вложенного.