New rule: Do not catch NullReferenceException type

[x3ntrix]

NullReferenceException should be fixed and not caught. In most situations it should be possible to convert/replace the try-catch-block into a null check. Any code from the catch block could be moved to the "is null" branch of an if-statement.

[mavasani]

We have CA1031 that flags use of generic exceptions in catch clauses. So, this should avoid conservatively swallowing NRE exceptions with catch all handlers.

This means the proposed rule here will only be helpful for code with an explicit catch clause with NullReferenceException as the exception type catch(NullReferenceException ...) - honestly, I have never seen production code with such a catch clause, and I don't expect one unless the code review standard/bar is very low. So, I am not sure if this rule will ever fire on real code bases and is worth the implementation/code review cost. Tagging @dotnet/roslyn-analysis for more views.

[x3ntrix]

I agree that a review should reveal if someone used catch(NullReferenceException ...), but reviews are done by humans and humans made errors. Also the review argument could be applied to many analyzer rules.
Isn't the purpose of analyzers to check automatically what can be checked automatically to allow the reviewers to focus on other things?

[sharwell]

I've never seen a developer type this except in cases where a call into external code may result in this exception due to a known bug in the external code that the developers needed to work around. In such a case, the analyzer would report a diagnostic but the diagnostic would always be suppressed.

I could see the rule being useful for certain edge cases where code is getting ported from Java to C#, but again that's an edge case that wouldn't apply to most users.

Analyzers can do just about anything someone thinks of, but running the analyzers isn't free. We have to balance the number of users likely to encounter the scenario (a small number) with the value the analyzer provides when we do (minimal because there would not be an automatic code fix). For me, this particular analyzer is better suited to a separate analyzer package rather than roslyn-analyzers. It's quite easy to create custom analyzers using the SDK and incorporate them into a specific project, so that would be my recommendation.

[joachimmarder]

honestly, I have never seen production code with such a catch clause

Well, I have. The developer thought: Why do a dozen of null checks when I can simply catch a NullReferenceException instead at one place. Not entirely absurd.

I don't expect one unless the code review standard/bar is very low.

Why leave such a simple violation of good coding standards to manual code review if it can be detected automatically? And yes, sometimes review standards are lower than desired due to a lack of developers.

Analyzers can do just about anything someone thinks of, but running the analyzers isn't free. We have to balance the number of users likely to encounter the scenario (a small number) with the value the analyzer provides

Maybe we should widen the discussion a bit. I think there are more exception that typically should not be caught. For example: ArgumentNullException, IndexOutOfRangeException, InvalidOperationException, NotImplementedException, NotSupportedException. That should further increase the value of the analyzer.

minimal because there would not be an automatic code fix

I think it's wrong to measure the value of an analyzer based on whether an automatic code fix is available, the value is determined by the likelyhood that the detected code violates good coding standards or may lead to a bug.

[mavasani]

Well, I have. The developer thought: Why do a dozen of null checks when I can simply catch a NullReferenceException instead at one place. Not entirely absurd.

@joachimmarder Can you please point me to an actual open source repo with such code? I completely agree with @sharwell that we need to balance addition of new analyzers with the value add. There is investment in implementing, reviewing, maintaining analyzers from the implementation side. There is additional performance impact on customers, especially if the number of such analyzers that fire on extreme edge cases keep increasing.

think there are more exception that typically should not be caught. For example: ArgumentNullException, IndexOutOfRangeException, InvalidOperationException, NotImplementedException, NotSupportedException. That should further increase the value of the analyzer.

I do not agree with this - I think this will make the analyzer extremely noisy if these are enabled by default.

One option might be to add an .editorconfig option to CA1031, such as this one to allow end users to configure exception types that should be flagged if they are caught. The default behavior will still flag just catch clauses catching System.Exception but end users can customize this.

Outside of a configurable option based approach, I am not sure we would be able to add a new analyzer to our package, and it would be more suited to a separate custom analyzer package.

[sharwell]

💡 Keep in mind it's fairly easy to create a custom analyzer package. The first time setup is the only tricky part but after that it's not difficult to maintain and even spread the work across the team. There are many teams and companies using this approach today.

[joachimmarder]

Can you please point me to an actual open source repo with such code?

This code was written in a closed source project a few years ago, before we could use the Null-conditional operator ?. The code was accessing an object tree deserialzed from an XML file, and mandatory elements were accessed unconditionally and so caused a NullReferenceException.

I think this will make the analyzer extremely noisy

I agree this analyzer should not be noisy. I checked some of our source code and in a few cases it was indeed necessary to catch NotSupportedExceptions and InvalidOperationExceptions. But I cannot imagine a case in which it could make sense to catch ArgumentNullException, IndexOutOfRangeException, NotImplementedException or NullReferenceException. If needed, typically something went wrong in architecture or error handling.

I cannot comment much on the performance but when to choose I would always choose clean code over analyzer performance.

[x3ntrix]

I like the idea of @mavasani to add an .editorconfig option to CA1031. It would be more flexible than an extra rule for only a specific exception. Any user could configure the rule according to their own project requirements.