Code Fix - changing accessibility

Question

I am trying to get the hang of Roslyn at the moment, and to implement a couple of code fixes for specific fields/properties that should usually be readonly (in the case of a property, to have no setter).

I've started from the "Analyzer with Code Fix (NUGET + VSIX)" template and now have two diagnostics implemented in the DiagnosticAnalyzer class and two corresponding code fixes in the CodeFixProvider class. But in this second class, it all feels a bit "messy" - so I'm looking for feedback on what I can do to clean it up:

using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

namespace LiveVariables.Analyzers
{
    [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(LiveVariablesAnalyzersCodeFixProvider)), Shared]
    public class LiveVariablesAnalyzersCodeFixProvider : CodeFixProvider
    {
        private const string readonlyAction = "Make readonly";

        public sealed override ImmutableArray<string> FixableDiagnosticIds
        {
            get { return ImmutableArray.Create(LiveVariablesAnalyzersAnalyzer.LV1001DiagnosticId, LiveVariablesAnalyzersAnalyzer.LV1002DiagnosticId); }
        }

        public sealed override FixAllProvider GetFixAllProvider()
        {
            return WellKnownFixAllProviders.BatchFixer;
        }

        public sealed 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;

            if (diagnostic.Id == LiveVariablesAnalyzersAnalyzer.LV1001DiagnosticId)
            {
                // Find the field declaration identified by the diagnostic.
                var declaration = root.FindToken(diagnosticSpan.Start).Parent.AncestorsAndSelf().OfType<FieldDeclarationSyntax>().First();

                context.RegisterCodeFix(
                        CodeAction.Create(
                                title: readonlyAction,
                                createChangedDocument: c => MakeFieldReadOnly(context.Document, declaration, c),
                                equivalenceKey: readonlyAction),
                        diagnostic);
            }
            else
            {
                // Find the property declaration identified by the diagnostic.
                var declaration = root.FindToken(diagnosticSpan.Start).Parent.AncestorsAndSelf().OfType<PropertyDeclarationSyntax>().First();

                context.RegisterCodeFix(
                        CodeAction.Create(
                                title: readonlyAction,
                                createChangedDocument: c => MakePropertyReadOnly(context.Document, declaration, c),
                                equivalenceKey: readonlyAction),
                        diagnostic);
            }
        }

        private async Task<Document> MakeFieldReadOnly(Document document, FieldDeclarationSyntax fieldDecl, CancellationToken cancellationToken)
        {
            var genny = SyntaxGenerator.GetGenerator(document);
            var nFieldDecl = genny.WithModifiers(fieldDecl, genny.GetModifiers(fieldDecl) | DeclarationModifiers.ReadOnly);
            var newSyntax = (await document.GetSyntaxRootAsync()).ReplaceNode(fieldDecl, nFieldDecl);
            return document.WithSyntaxRoot(newSyntax);
        }

        private async Task<Document> MakePropertyReadOnly(Document document, PropertyDeclarationSyntax propDecl, CancellationToken cancellationToken)
        {
            var genny = SyntaxGenerator.GetGenerator(document);
            var setter = genny.GetAccessor(propDecl, DeclarationKind.SetAccessor);
            var newSyntax = (await document.GetSyntaxRootAsync()).RemoveNode(setter, SyntaxRemoveOptions.KeepNoTrivia);
            return document.WithSyntaxRoot(newSyntax);
        }
    }
}

Specific things that feel messy to me:

• Should I even be implementing multiple code fixes in a single CodeFixProvider? - the API definitely supports it but it feels it could easily get out of hand
• I'm working out what diagnostic I'm responding to, inside RegisterCodeFixesAsync by doing a string comparison - is there a stronger-typed way of working this out?
• Creating the SyntaxGenerator within the MakeFieldReadOnly and MakePropertyReadOnly feels a bit wrong somehow - is there a more natural way for me to make my changes?

And of course, anything else people would care to suggest will be interesting.

(I'm generally happy with the code within DiagnosticAnalyzer but can add it here also if people think that it's important to see all of the code within the project)

Answer 1

Addressing your comments first:

Should I even be implementing multiple code fixes in a single CodeFixProvider? - the API definitely supports it but it feels it could easily get out of hand

It's okay to do so. I typically use the same diagnostic ID for both these things and simply change the message. For example the diagnostic with ID "MemberCanBeReadOnly" can have a message "Field A can be made readonly" and "Property B can be made readonly".

I'm working out what diagnostic I'm responding to, inside RegisterCodeFixesAsync by doing a string comparison - is there a stronger-typed way of working this out?

Not that I'm aware of but it's okay -- IDs are supposed to be unique anyway. You might want to add a prefix of your own though to indicate it's a diagnostic part of your own library (and thus avoid collisions with other libraries). An example is the "CS" prefix used by Microsoft themselves.

Creating the SyntaxGenerator within the MakeFieldReadOnly and MakePropertyReadOnly feels a bit wrong somehow - is there a more natural way for me to make my changes?

You're just getting a document twice. If this bothers you, make it once and pass it along to those two methods.

---

Now, actual code remarks:

• You're using SyntaxGenerator but you're not necessarily generating new code -- you're editing existing code. I would advise to use the DocumentEditor class to make changes to code because it prevents problems when you have to edit more than one area in the same document. You will notice that its syntax also feels a little more fluent.

• Asynchronous methods are supposed to end with the Async prefix

• Time for C# 6 baby:

    public sealed override ImmutableArray<string> FixableDiagnosticIds
    {
        get { return ImmutableArray.Create(LiveVariablesAnalyzersAnalyzer.LV1001DiagnosticId, LiveVariablesAnalyzersAnalyzer.LV1002DiagnosticId); }
    }

    public sealed override FixAllProvider GetFixAllProvider()
    {
        return WellKnownFixAllProviders.BatchFixer;
    }

becomes

public sealed override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(LiveVariablesAnalyzersAnalyzer.LV1001DiagnosticId, LiveVariablesAnalyzersAnalyzer.LV1002DiagnosticId);

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

---

Code fixes are typically boring though. Show us the analyzer! Analyzers are where the real work is done and where all the edge cases popup. I can see many edge cases:

• Are the members set from another class?
• Have you thought about public members being accessed from another solution?
• Did you make the distinction between read-only properties that can be set from the constructor?

There's more but these are just off the top of my head.