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&lt;SyntaxKind&gt;
{
    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&lt;DiagnosticDescriptor&gt; SupportedDiagnostics
    {
        get
        {
            return ImmutableArray.Create(Rule);
        }
    }

    public ImmutableArray&lt;SyntaxKind&gt; SyntaxKindsOfInterest
    {
        get
        {
            return ImmutableArray.Create(SyntaxKind.CatchClause);
        }
    }

    public void AnalyzeNode(SyntaxNode node, SemanticModel semanticModel, Action&lt;Diagnostic&gt; 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&lt;CatchClauseSyntax&gt;().ToList().Count &gt; 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 &amp;&amp; !hasMultipleClauses)
        {
            addDiagnostic(Diagnostic.Create(Rule, clause.Declaration.GetLocation()));
        }
    }
}


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;
        }
    }
}
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&lt;CatchClauseSyntax&gt;().ToList().Count &gt; 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&lt;CatchClauseSyntax&gt;()
                                          .ToList().Count &gt; 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;