Detect and Fix Switching over an Enum Without Handling all Members

Question

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<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

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

    private void AnalyzeSymbol(SyntaxNodeAnalysisContext context)
    {
        var switchBlock = context.Node as SwitchStatementSyntax;
        if (switchBlock == null) { return; }

        var enumType = context.SemanticModel.GetTypeInfo(switchBlock.Expression).Type as INamedTypeSymbol;
        if (enumType == null || enumType.TypeKind != TypeKind.Enum) { return; }

        var caseLabels = switchBlock.Sections.SelectMany(l => l.Labels)
                .OfType<CaseSwitchLabelSyntax>()
                .Select(l => l.Value)
                .ToList();

        // these are the labels like `MyEnum.EnumMember`
        var labelNames = caseLabels
                .OfType<MemberAccessExpressionSyntax>()
                .Select(l => l.Name.Identifier.ValueText)
                .ToList();

        // these are the labels like `EnumMember` (such as when using `using static Namespace.MyEnum;`)
        labelNames.AddRange(caseLabels.OfType<IdentifierNameSyntax>().Select(l => l.Identifier.ValueText).ToList());

        if (enumType.MemberNames.Where(m => !m.StartsWith(".")).Any(member => !labelNames.Contains(member)))
        {
            context.ReportDiagnostic(Diagnostic.Create(Rule, switchBlock.GetLocation()));
        }
    }
}

[ExportCodeFixProvider(nameof(SwitchDoesNotHandleAllEnumOptionsCodeFix), LanguageNames.CSharp), Shared]
internal class SwitchDoesNotHandleAllEnumOptionsCodeFix : CodeFixProvider
{
    public override ImmutableArray<string> FixableDiagnosticIds
        => ImmutableArray.Create(SwitchDoesNotHandleAllEnumOptionsAnalyzer.Rule.Id);

    public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

    public override async Task RegisterCodeFixesAsync(CodeFixContext context)
    {
        var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
        var diagnostic = context.Diagnostics.First();
        var diagnosticSpan = diagnostic.Location.SourceSpan;

        var statement = root.FindNode(diagnosticSpan);
        context.RegisterCodeFix(
            CodeAction.Create(VSDiagnosticsResources.SwitchDoesNotHandleAllEnumOptionsCodeFixTitle,
                x => AddMissingCaseAsync(context.Document, (CompilationUnitSyntax)root, statement),
                SwitchDoesNotHandleAllEnumOptionsAnalyzer.Rule.Id), diagnostic);
    }

    private async Task<Solution> AddMissingCaseAsync(Document document, CompilationUnitSyntax root, SyntaxNode statement)
    {
        var semanticModel = await document.GetSemanticModelAsync();

        var switchBlock = (SwitchStatementSyntax)statement;

        var enumType = (INamedTypeSymbol) semanticModel.GetTypeInfo(switchBlock.Expression).Type;
        var caseLabels = switchBlock.Sections.SelectMany(l => l.Labels)
                .OfType<CaseSwitchLabelSyntax>()
                .Select(l => l.Value)
                .ToList();

        var missingLabels = GetMissingLabels(caseLabels, enumType);

        // use simplified form if there are any in simplified form or if there are not any labels at all
        var hasSimplifiedLabel = caseLabels.OfType<IdentifierNameSyntax>().Any();
        var useSimplifiedForm = hasSimplifiedLabel || !caseLabels.OfType<MemberAccessExpressionSyntax>().Any();

        var qualifier = GetQualifierForException(root);

        var notImplementedException =
            SyntaxFactory.ThrowStatement(SyntaxFactory.ParseExpression($" new {qualifier}NotImplementedException()"))
                .WithAdditionalAnnotations(Simplifier.Annotation);
        var statements = SyntaxFactory.List(new List<StatementSyntax> { notImplementedException });

        var newSections = SyntaxFactory.List(switchBlock.Sections);

        foreach (var label in missingLabels)
        {
            // If an existing simplified label exists, it means we can assume that works already and do it ourselves as well (ergo: there is a static using)
            var caseLabel =
                SyntaxFactory.CaseSwitchLabel(
                    SyntaxFactory.ParseExpression(hasSimplifiedLabel ? $"{label}" : $"{enumType.Name}.{label}")
                        .WithTrailingTrivia(SyntaxFactory.ParseTrailingTrivia(Environment.NewLine)));

            var section =
                SyntaxFactory.SwitchSection(SyntaxFactory.List(new List<SwitchLabelSyntax> {caseLabel}), statements)
                    .WithAdditionalAnnotations(Formatter.Annotation);

            // ensure that the new cases are above the default case
            newSections = newSections.Insert(0, section);
        }

        var newNode = useSimplifiedForm
            ? switchBlock.WithSections(newSections).WithAdditionalAnnotations(Formatter.Annotation, Simplifier.Annotation)
            : switchBlock.WithSections(newSections).WithAdditionalAnnotations(Formatter.Annotation);

        var newRoot = root.ReplaceNode(switchBlock, newNode);
        var newDocument = await Simplifier.ReduceAsync(document.WithSyntaxRoot(newRoot));
        return newDocument.Project.Solution;
    }

    private IEnumerable<string> GetMissingLabels(List<ExpressionSyntax> caseLabels, INamedTypeSymbol enumType)
    {
        // these are the labels like `MyEnum.EnumMember`
        var labels = caseLabels
            .OfType<MemberAccessExpressionSyntax>()
            .Select(l => l.Name.Identifier.ValueText)
            .ToList();

        // these are the labels like `EnumMember` (such as when using `using static Namespace.MyEnum;`)
        labels.AddRange(caseLabels.OfType<IdentifierNameSyntax>().Select(l => l.Identifier.ValueText));

        // don't create members like ".ctor"
        return enumType.MemberNames.Except(labels).Where(m => !m.StartsWith("."));
    }

    private string GetQualifierForException(CompilationUnitSyntax root)
    {
        var qualifier = "System.";
        var usingSystemDirective =
            root.Usings.Where(u => u.Name is IdentifierNameSyntax)
                .FirstOrDefault(u => ((IdentifierNameSyntax) u.Name).Identifier.ValueText == nameof(System));

        if (usingSystemDirective != null)
        {
            qualifier = usingSystemDirective.Alias == null
                ? string.Empty
                : usingSystemDirective.Alias.Name.Identifier.ValueText + ".";
        }
        return qualifier;
    }
}

Answer 1

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:

  1. MemberAccessExpression is too broad a check. You'll check many things that are not enums but are SimpleMemberAccess, like property accesses.
  2. 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.