12
\$\begingroup\$

Here is my Field Can Be Made Readonly analyzer and quick fix. I would appreciate tips on how to improve my code, and on missed test cases.

First, the rules it works with:

  1. It will only fire on private fields to make things simpler (fields in other scopes should be properties, anyway).
  2. If the field is only assigned inline or in the ctor, it should be readonly
  3. Fields cannot be readonly if they are:
    A. Assigned in any scope other than a ctor or inline.
    B. Passed as a ref or out argument.
    C. Incremented or decremented with the prefix or postfix ++ or -- operators.

Here is the analyzer code:

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public class FieldCanBeReadonlyAnalyzer : DiagnosticAnalyzer
{
    private const DiagnosticSeverity Severity = DiagnosticSeverity.Warning;

    private static readonly string Category = VSDiagnosticsResources.GeneralCategory;
    private static readonly string Message = VSDiagnosticsResources.FieldCanBeReadonlyAnalyzerMessage;
    private static readonly string Title = VSDiagnosticsResources.FieldCanBeReadonlyAnalyzerTitle;

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

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

    public override void Initialize(AnalysisContext context) => context.RegisterSyntaxNodeAction(AnalyzeSymbol, SyntaxKind.ClassDeclaration, SyntaxKind.StructDeclaration);

    private static void AnalyzeSymbol(SyntaxNodeAnalysisContext context)
    {
        var classSymbol = (ITypeSymbol)context.SemanticModel.GetDeclaredSymbol(context.Node);
        if (classSymbol.TypeKind != TypeKind.Class &&
            classSymbol.TypeKind != TypeKind.Struct)
        {
            return;
        }

        var nonReadonlyFieldMembers = new List<IFieldSymbol>();
        foreach (var item in classSymbol.GetMembers())
        {
            var symbol = item as IFieldSymbol;
            if (symbol != null && symbol.DeclaredAccessibility == Accessibility.Private && !symbol.IsReadOnly)
            {
                nonReadonlyFieldMembers.Add(symbol);
            }
        }

        var membersCanBeReadonly = nonReadonlyFieldMembers;
        foreach (var syntaxReference in classSymbol.DeclaringSyntaxReferences)
        {
            var classNode = syntaxReference.SyntaxTree.GetRoot().FindNode(syntaxReference.Span);
            membersCanBeReadonly = WalkTree(context.SemanticModel, classNode, membersCanBeReadonly);
        }

        foreach (var symbol in membersCanBeReadonly)
        {
            context.ReportDiagnostic(Diagnostic.Create(Rule, symbol.Locations[0], symbol.Name));
        }
    }

    private static List<IFieldSymbol> WalkTree(SemanticModel model, SyntaxNode node, List<IFieldSymbol> unassignedSymbols)
    {
        // todo check whether node is reference to a field
        // check whether reference is assignment or ref/out param
        foreach (var child in node.ChildNodes())
        {
            if (child is ConstructorDeclarationSyntax)
            {
                continue;
            }

            var symbol = model.GetSymbolInfo(child).Symbol as IFieldSymbol;
            if (symbol != null && unassignedSymbols.Contains(symbol))
            {
                var assignmentNode = child.Parent as AssignmentExpressionSyntax;
                if (assignmentNode?.Left == child)
                {
                    unassignedSymbols.Remove(symbol);
                }

                var argumentNode = child.Parent as ArgumentSyntax;
                if (argumentNode?.RefOrOutKeyword != null)
                {
                    unassignedSymbols.Remove(symbol);
                }

                var postFixExpressionNode = child.Parent as PostfixUnaryExpressionSyntax;
                if (postFixExpressionNode != null)
                {
                    unassignedSymbols.Remove(symbol);
                }

                var preFixExpressionNode = child.Parent as PrefixUnaryExpressionSyntax;
                if (preFixExpressionNode != null)
                {
                    unassignedSymbols.Remove(symbol);
                }
            }

            unassignedSymbols = WalkTree(model, child, unassignedSymbols);
        }

        return unassignedSymbols;
    }
}

The code fix adds the readonly keyword to the node:

[ExportCodeFixProvider(DiagnosticId.FieldCanBeReadonly + "CF", LanguageNames.CSharp), Shared]
public class FieldCanBeReadonlyCodeFix : CodeFixProvider
{
    public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(FieldCanBeReadonlyAnalyzer.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 fieldDeclaration = (FieldDeclarationSyntax)root.FindNode(diagnosticSpan).Parent.Parent;

        context.RegisterCodeFix(
            CodeAction.Create(VSDiagnosticsResources.FieldCanBeReadonlyCodeFixTitle,
                x => MakeReadonlyAsync(context.Document, root, fieldDeclaration), FieldCanBeReadonlyAnalyzer.Rule.Id), diagnostic);
    }

    private Task<Document> MakeReadonlyAsync(Document document, SyntaxNode root, FieldDeclarationSyntax declaration)
    {
        var newDeclaration = declaration.AddModifiers(SyntaxFactory.Token(SyntaxKind.ReadOnlyKeyword));

        var newRoot = root.ReplaceNode(declaration, newDeclaration);
        var newDocument = document.WithSyntaxRoot(newRoot);
        return Task.FromResult(newDocument);
    }
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$
var nonReadonlyFieldMembers = new List<IFieldSymbol>();
...
unassignedSymbols.Contains(symbol)
...
unassignedSymbols.Remove(symbol);

If this needs to be fast, then using a HashSet<IFieldSymbol> might be a good idea.

The Contains and Remove methods you're currently using perform a linear search, which means those operations are O(n). With a HashSet, both would be O(1) (if you don't need to maintain the order, of course).


I think the WalkTree method should be split into two—that is, extract the filtering logic into a new method and let WalkTree do only tree scanning.

It now takes a delegate that can do something with a child. The result is generic so it should even be possible to reuse it. The HashSet<T> is passed around by reference so there's no need to return it.

private static void WalkTree<TResult>(
    SemanticModel model,
    SyntaxNode node,
    HashSet<TResult> results,
    Action<SemanticModel, SyntaxNode, HashSet<TResult>> processNode)
{
    // TODO: check whether node is reference to a field
    // TODO: check whether reference is assignment or ref/out param
    foreach (var child in node.ChildNodes())
    {
        if (child is ConstructorDeclarationSyntax)
        {
            continue;
        }

        processNode(model, child, results);

        WalkTree(model, child, results, processNode);
    }   
}

The filtering logic now belongs in the FilterSymbol method. It expects the same parameters as its prececessor WalkTree but without the delegate.

private static void FilterSymbol<TResult>(
    SemanticModel model,
    SyntaxNode node,
    HashSet<TResult> results)
{
    var symbol = model.GetSymbolInfo(node).Symbol as IFieldSymbol;
    if (symbol != null && results.Contains(symbol))
    {
        results.Remove(symbol);

        var assignmentNode = node.Parent as AssignmentExpressionSyntax;
        if (assignmentNode?.Left == node)
        {
            results.Remove(symbol);
        }

        var argumentNode = node.Parent as ArgumentSyntax;
        if (argumentNode?.RefOrOutKeyword != null)
        {
            results.Remove(symbol);
        }

        var postFixExpressionNode = node.Parent as PostfixUnaryExpressionSyntax;
        if (postFixExpressionNode != null)
        {
            results.Remove(symbol);
        }

        var preFixExpressionNode = node.Parent as PrefixUnaryExpressionSyntax;
        if (preFixExpressionNode != null)
        {
            results.Remove(symbol);
        }
    }
}

The use would be now without the return value as this is no longer necessary. Additionally, it may required to specify the result type:

WalkTree<IFieldSymbol>(context.SemanticModel, classNode, membersCanBeReadonly, FilterSymbol);

(I hope I have the generics right)

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.