# DiagnosticAnalyzer for Roslyn that guards against catch-all exception clauses

Dabbling around with Roslyn and made a small analyzer just now. This one will show a warning in Visual Studio when you have a try-catch statement that only has a catch(Exception e).

I realize the working code (AnalyzeNode) is rather small, but I'm looking for feedback on best-practices (insofar there are already best practices established) and general remarks on scenarios that I might have overlooked.

I have also been looking for a way to unit test this, but haven't come up with a good solution yet. Is there an elegant way to test these analyzers instead of looking over them by hand? Or perhaps an API that exposes some crude methods which I could provide a wrapper for?

# Analyzer

[DiagnosticAnalyzer]
[ExportDiagnosticAnalyzer(DiagnosticId, LanguageNames.CSharp)]
class SingleGeneralExceptionAnalyzer : ISyntaxNodeAnalyzer<SyntaxKind>
{
private const string DiagnosticId = "SingleGeneralException";
private const string Description = "Verifies whether a try-catch block does not contain just a single Exception clause.";
private const string MessageFormat = "A catch-all clause has been used.";
private const string Category = "Exceptions";
private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor(DiagnosticId, Description, MessageFormat, Category, DiagnosticSeverity.Warning);

public ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics
{
get
{
return ImmutableArray.Create(Rule);
}
}

public ImmutableArray<SyntaxKind> SyntaxKindsOfInterest
{
get
{
return ImmutableArray.Create(SyntaxKind.CatchClause);
}
}

public void AnalyzeNode(SyntaxNode node, SemanticModel semanticModel, Action<Diagnostic> addDiagnostic, CancellationToken cancellationToken)
{
var clause = node as CatchClauseSyntax;
var exceptionType = clause.Declaration.Type;
var identifier = semanticModel.GetSymbolInfo(exceptionType);
var isGeneralException = identifier.Symbol.Name == typeof(Exception).Name;
var hasMultipleClauses = clause.Parent.ChildNodes().OfType<CatchClauseSyntax>().ToList().Count > 1;

// Less-specific exceptions can't be caught after a more general exception, or a compile error occurs
// Therefore we don't need to look at the order of the clauses

if (isGeneralException && !hasMultipleClauses)
{
}
}
}


# Testclass

class SingleExceptionClauseAnalyzer
{
void SingleException_ShouldCause_Warning()
{
try
{
int x = Int32.Parse("5");
}
catch (Exception e)
{
int x = 8;
}
}

void MultipleExceptions_ShouldNotCause_Warning()
{
try
{
int x = Int32.Parse("5");
}
catch (FormatException e)
{
int x = 7;
}
catch (Exception e)
{
int x = 8;
}
}

void SingleException_WithFullyQualifiedName_ShouldCause_Warning()
{
try
{
int x = Int32.Parse("5");
}
catch (System.Exception e)
{
int x = 8;
}
}

void MultipleExceptions_WithFullyQualifiedName_ShouldNotCause_Warning()
{
try
{
int x = Int32.Parse("5");
}
catch (System.FormatException e)
{
int x = 7;
}
catch (System.Exception e)
{
int x = 8;
}
}
}

• The tests that Microsoft have developed for some of their own diagnostic analyzers can be found here. – Nicole Calinoiu Apr 17 '14 at 12:57

I've never coded a diagnostics analyzer, so I don't know if that's a possibility, but I think these:

private const string DiagnosticId = "SingleGeneralException";
private const string Description = "Verifies whether a try-catch block does not contain just a single Exception clause.";
private const string MessageFormat = "A catch-all clause has been used.";
private const string Category = "Exceptions";


Would be better off defined in a .resx file, so you can localize it - not everyone runs an English IDE, I'd try to have the the messages be shown in the same language as the stack traces.

In these declarations:

    var clause = node as CatchClauseSyntax;
var exceptionType = clause.Declaration.Type;
var identifier = semanticModel.GetSymbolInfo(exceptionType);
var isGeneralException = identifier.Symbol.Name == typeof(Exception).Name;
var hasMultipleClauses = clause.Parent.ChildNodes().OfType<CatchClauseSyntax>().ToList().Count > 1;


I think the relationships between the variables would be more obvious with some vertical whitespace:

    var clause = node as CatchClauseSyntax;

var exceptionType = clause.Declaration.Type;
var hasMultipleClauses = clause.Parent.ChildNodes()
.OfType<CatchClauseSyntax>()
.ToList().Count > 1;

var identifier = semanticModel.GetSymbolInfo(exceptionType);
var isGeneralException = identifier.Symbol.Name == typeof(Exception).Name;


I believe there's a possible execution path where the clause would be null (because of the as cast), in which case the next line would throw an easily avoidable NullReferenceException:

    var clause = node as CatchClauseSyntax;
if (clause == null) return;

• Since CatchClause is defined as SyntaxKindsOfInterest, that conversion should always succeed. Because of that, I would prefer a cast instead of as. – svick Apr 30 '14 at 19:16

The above answer mentions it all but missed one thing about the LINQ query mentioned above.

 var hasMultipleClauses = clause.Parent.ChildNodes()
.OfType<CatchClauseSyntax>()
.ToList().Count > 1;


This is trying to access the Count property after enumeration to List, so it is okay to get the result directly from a single Count() enumeration as mentioned below

 var hasMultipleClauses = clause.Parent.ChildNodes()
.OfType<CatchClauseSyntax>()
.Count() > 1;

• Better yet, use .Any() instead of checking the Count() > 1 – Bryan Aug 15 '14 at 19:28
• @Bryan .Any() is equivalent to > 0, not > 1. – David Pfeffer Jan 13 '15 at 20:44
• Oops...you are right. Good catch. – Bryan Jan 14 '15 at 20:52
• .Skip(1).Any() would be equivalent to > 1 but would not require iterating over the list. – HaroldHues Apr 23 '16 at 22:41