ErrorProne.NET. Part 2

Last time we discussed formatting problems, and now we will deal with exceptions.

ErrorProne.NET inherited a lot of features from another project of mine — ExceptionAnalyzer, but in a slightly different form. It includes rules like this:

ERP021 – Incorrect exception propagation: "incorrect" exception forwarding with the help of throw ex; instead of throw
ERP022 – Unobserved exception in generic exception handler: when the block catching all the exceptions returns the handler with no attempt to "observe" the trapped exception
ERP023 – Suspicious exception handling: only Message property was observed: when the generalized catch block refers only to ex.Message exception attribute

Now let's look at these exceptions in more detail.

Incorrect exception propagation

I won't spend a lot of time on this problem. If you've been to at least one job interview or worked on a team of more than a couple of people, I'm sure you know the difference between throw ex; and throw;:

ErrorProne.NET Incorrect Exception Propagation

There's no surprises here. throw ex; will "break" the callstack of the initial exception, which will make it difficult to analyze the log and find the problem. As it is a well-known problem, you will come across very few errors like these. In spite of this, I found an example of questionable exception forwarding in the Roslyn code: CompilationWithAnalyzer.cs:602.

Unobserved exception in generic exception handler

As this tool is intended for searching for the most suspicious places, the majority of rules related to exception processing are simple, precise and not too troublesome. Thus, for example, as much as purists would like to prohibit all catch(Exception) blocks in their code, it is difficult to do in practice because of their prevalence. But, IMHO, they can and should be prevented if such blocks return the handler without even bothering to check the exceptions they catch.

This is why the following code provides a warning:

ErrorProne.NET Unobserved Exception in Generic Exception Handler

Any reference to the e variable in the catch block (such as e.Message, ProcessExcpetion(e), etc.) "deactivates" this rule. For some reason, I thought that this rule wouldn't come into play very often, and I was very surprised to see that in the Roslyn code it was triggered more than 60 (!) times. Here just some examples, to begin with:

Yes, the same rule will be triggered only for catch {}, catch(System.Exception) and catch(System.AggregateException) blocks. For more specific exceptions, it is quite normal to return the handler without referring to the exception object.

Suspicious exception handling: only Message property was observed

What could be worse than swallowing an exception? Recording incomplete exception details! Every time you write only ex.Message to the log file, the higher forces will make your code fail in production indicating TypeLoadException and wake you up at 2 AM to find out what went wrong with the build that checked out perfectly on the local PC. And you will be half asleep, looking over the log and pondering over miraculous log entries such as: "Exception has been thrown by the target of an invocation.", "The type initializer for 'MyAwesomeType' threw an exception." or "One or more errors occurred." and think "What the hell happened?"

Of course, we can say that we review the code carefully, that our static constructors do not let exceptions slip, that we do not acknowledge TPL, and in general we write error-free code right from the start. But in reply to this, I usually give the example showing that exceptions turn up even in places where the majority of people do not expect them to be.

ErrorProne.NET Message property was observed

As the new () limitation leads to the use of Activator .CreateInstance, any exception in object construction created with the help of generalized methods like Create <T> will turn the initial exception into TargetInvocationException. Things like this are easy to overlook during a review, but it is much easier to wrap in any code that writes an exception message only and doesn't show stacktrace + internal exceptions.

Once again, I was surprised with the number of times this rule was broken in the Roslyn code. There are more than a dozen violations. Many of them are obvious typos, like when a new exception was generated, and they clearly forgot to define the old exception as embedded.

Sergey Teplyakov
Expert in .Net, С++ and Application Architecture