debugcsharpMinor
DiagnosticAnalyzer for Roslyn that guards against catch-all exception clauses
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
I realize the working code (
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
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:
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:
I think the relationships between the variables would be more obvious with some vertical whitespace:
I believe there's a possible execution path where the
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.