Field Can Be Made Readonly

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:

• It will only fire on private fields to make things simpler (fields in other scopes should be properties, anyway).

• If the field is only assigned inline or in the ctor, it should be readonly

• 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 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();
foreach (var item in classSymbol.GetMembers())
{
var symbol = item as IFieldSymbol;

var nonReadonlyFieldMembers = new List();
...
unassignedSymbols.Contains(symbol)
...
unassignedSymbols.Remove(symbol);

If this needs to be fast, then using a HashSet 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 is passed around by reference so there's no need to return it.

private static void WalkTree(
    SemanticModel model,
    SyntaxNode node,
    HashSet results,
    Action> 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(
    SemanticModel model,
    SyntaxNode node,
    HashSet 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(context.SemanticModel, classNode, membersCanBeReadonly, FilterSymbol);

(I hope I have the generics right)

StackExchange Code Review Q#151193, answer score: 4