HiveBrain v1.2.0
Get Started
← Back to all entries
debugcsharpMinor

DiagnosticAnalyzer for Roslyn that guards against catch-all exception clauses

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
roslyndiagnosticanalyzerexceptionallcatchagainstthatforguardsclauses

Problem

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

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

public void AnalyzeNode(SyntaxNode node, SemanticModel semanticModel, Action addDiagnostic, CancellationToken cancellationToken)
{
var clause = node as CatchClauseSyntax;
var exceptionType = clause.Declaration.Type;
var identifier = semanticModel.GetSymbolInfo(exceptionType);
var isGeneralException = identifier.Symbol.Name == typeof

Solution

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().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()
                                          .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;

Code Snippets

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";
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;
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;
var clause = node as CatchClauseSyntax;
    if (clause == null) return;

Context

StackExchange Code Review Q#47400, answer score: 3

Revisions (0)

No revisions yet.