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

Detect and Fix Switching over an Enum Without Handling all Members

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

Problem

VSDiagnostic's latest refactoring and code fix detects when a switch does not contain case statements for each of the enum members and adds any missing members. So, for example, with the following enum and switch, VSDiagnostics will detect that some members are missing and add them as shown in the second switch:

enum MyEnum
{
    Fizz,
    Buzz,
    FizzBuzz
}


var e = MyEnum.Fizz;
switch (e)
{
    case MyEnum.Buzz:
        break;
    default:
        break;
}


var e = MyEnum.Fizz;
switch (e)
{
    case MyEnum.FizzBuzz:
        throw new System.NotImplementedException();
    case MyEnum.Fizz:
        throw new System.NotImplementedException();
    case MyEnum.Buzz:
        break;
    default:
        break;
}


The full test suite can be found on GitHub here: Tests for SwitchDoesNotHandleAllEnumOptions. All comments are welcome, but I am especially interested in utilizing the Roslyn framework better if I am misusing it or not using any helpful features.

```
[DiagnosticAnalyzer(LanguageNames.CSharp)]
internal class SwitchDoesNotHandleAllEnumOptionsAnalyzer : DiagnosticAnalyzer
{
private const DiagnosticSeverity Severity = DiagnosticSeverity.Warning;

private static readonly string Category = VSDiagnosticsResources.GeneralCategory;
private static readonly string Message = VSDiagnosticsResources.SwitchDoesNotHandleAllEnumOptionsAnalyzerMessage;
private static readonly string Title = VSDiagnosticsResources.SwitchDoesNotHandleAllEnumOptionsAnalyzerTitle;

internal static DiagnosticDescriptor Rule
=> new DiagnosticDescriptor(DiagnosticId.SwitchDoesNotHandleAllEnumOptions, Title, Message, Category, Severity, true);

public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule);

public override void Initialize(AnalysisContext context)
{
context.RegisterSyntaxNodeAction(AnalyzeSymbol, SyntaxKind.SwitchStatement);
}

private void AnalyzeSymbol(SyntaxNodeAnalysi

Solution

Some quick notes:

-
I think you can just cast context.Node to SwitchStatementSyntax. The analyzer driver should ensure you should only get called for things you signed up for. If you do want to defensively check, though, I think you should check the node's SyntaxKind. It's more specific and much faster/cheaper.

-
I would avoid LINQ in analyzers. You could potentially be called in many compiler hot loops and you want to allocate as little as possible. (Really, you want to do as little as possible).

-
Enumerating over the Labels and doing string checks seems brittle. There are two main things I see:

  • MemberAccessExpression is too broad a check. You'll check many things that are not enums but are SimpleMemberAccess, like property accesses.



  • I think you need to make sure that the labels actually reference the enum names. Right now you'll get a false negative if someone uses the name of the enum member in the a label but doesn't actually reference that member. The way I would expect this to work is that you grab the symbol from the semantic model for each of the labels and the symbol for each of the enum members. You would then compare the symbol lists to make sure they're equal.

Context

StackExchange Code Review Q#122947, answer score: 6

Revisions (0)

No revisions yet.