14
\$\begingroup\$

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)
        {
            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;
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ The tests that Microsoft have developed for some of their own diagnostic analyzers can be found here. \$\endgroup\$ – Nicole Calinoiu Apr 17 '14 at 12:57
3
+50
\$\begingroup\$

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;
\$\endgroup\$
  • 3
    \$\begingroup\$ Since CatchClause is defined as SyntaxKindsOfInterest, that conversion should always succeed. Because of that, I would prefer a cast instead of as. \$\endgroup\$ – svick Apr 30 '14 at 19:16
1
\$\begingroup\$

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.