Skip to content

Create a reusable FixAllProvider based on IntroduceFieldFromConstructorCodeFixProviderAll #910

@MaStr11

Description

@MaStr11

Related to #352 (Test fix all providers)

The Roslyn BatchFixAllProvider often fails to fix several diagnostics in a document because it merges code changes. The Roslyn team discussed the pro and cons of several approaches for FixAllProvider and I think there is a sweet-spot between the Sequential and the Batch approaches that can be used for a variety of fixes.

I propose to create a custom FixAllProvider with the following characteristics:

  • Runs for all documents in the scope in parallel.
  • Runs the fixes for a single document in sequence like that:
    • Get all fixable diagnostics for the document
    • Track the nodes associated with the diagnostic with SyntaxAnnotations
    • Apply the fixes within the document in sequence
    • Remove the SyntaxAnnotations from the tree.

The IntroduceFieldFromConstructorCodeFixProviderAll already works like that.

This new custom FixAllProvider would have the limitation, that the fix for a diagnostic is only allowed to change the current document (otherwise there would be merge conflicts).

To make the FixAllProvider reusable I propose the following architecture:

The IntroduceFieldFromConstructorCodeFixProviderAll reuses the CodeFixProvider logic by calling the static IntroduceFieldFromConstructorCodeFixProvider.IntroduceFieldFromConstructor method.
This would be abstracted by introducing an interface that the CodeFixProvider needs to implement.
This interface might look like that:

/// <summary>
/// This interface must be implemented by the associated CodeFixProvider. The CodeFixProvider must operate on a single document and
/// should only change the document. This limits the possible operations of the CodeFixProvider to change only document internals without
/// effecting other parts of the solution.
/// </summary>
public interface IFixDocumentInternalsOnly
{
    Task<Document> FixDocumentAsync(SyntaxNode nodeWithDiagnostic, Document document, CancellationToken cancellationToken);
}

The custom FixAllProvider would operate on all the Documents in scope in parallel:

private async static Task<Solution> GetFixedDocumentsAsync(FixAllContext fixAllContext, IEnumerable<Document> documents)
{
    var solution = fixAllContext.Solution;
    var newDocuments = documents.ToDictionary(d => d.Id, d => GetFixedDocumentAsync(fixAllContext, d));
    await Task.WhenAll(newDocuments.Values).ConfigureAwait(false);
    var changedDocuments = from kvp in newDocuments
                           where kvp.Value.Result != null
                           select new { DocumentId = kvp.Key, Document = kvp.Value.Result };
    foreach (var newDocument in changedDocuments)
        solution = solution.WithDocumentSyntaxRoot(newDocument.DocumentId, await newDocument.Document.GetSyntaxRootAsync().ConfigureAwait(false));
    return solution;
}

A single document is fixed like that:

private async static Task<Document> GetFixedDocumentAsync(FixAllContext fixAllContext, Document document)
{
    var codeFixer = fixAllContext.CodeFixProvider as IFixDocumentInternalsOnly;
    if (codeFixer == null) throw new ArgumentException("This CodeFixAllProvider requires that your CodeFixProvider implements the IFixDocumentInternalsOnly.");
    var diagnostics = await fixAllContext.GetDocumentDiagnosticsAsync(document).ConfigureAwait(false);
    if (diagnostics.Length == 0) return null;
    var root = await document.GetSyntaxRootAsync(fixAllContext.CancellationToken).ConfigureAwait(false);
    var nodes = diagnostics.Select(d => root.FindNode(d.Location.SourceSpan)).Where(n => !n.IsMissing);
    var annotations = new List<SyntaxAnnotation>();
    var newRoot = root.ReplaceNodes(nodes, (original, rewritten) =>
        {
            var annotation = new SyntaxAnnotation(SyntaxAnnotationKey);
            annotations.Add(annotation);
            var newNode = original.WithAdditionalAnnotations(annotation);
            return newNode;
        });
    var newDocument = document.WithSyntaxRoot(newRoot);
    newDocument = await FixCodeForAnnotatedNodesAsync(newDocument, codeFixer, annotations, fixAllContext.CancellationToken).ConfigureAwait(false);
    newDocument = await RemoveAnnontationsAsync(newDocument, annotations).ConfigureAwait(false);
    return newDocument;
}

private static async Task<Document> FixCodeForAnnotatedNodesAsync(Document document, IFixDocumentInternalsOnly codeFixer, IEnumerable<SyntaxAnnotation> annotations, CancellationToken cancellationToken)
{
    foreach (var annotation in annotations)
    {
        var newRoot = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
        var annotatedNodes = newRoot.GetAnnotatedNodes(annotation);
        var node = annotatedNodes.FirstOrDefault();
        if (node == null) continue;
        document = await codeFixer.FixDocumentAsync(node, document, cancellationToken).ConfigureAwait(false);
    }
    return document;
}

private static async Task<Document> RemoveAnnontationsAsync(Document document, IEnumerable<SyntaxAnnotation> annotations)
{
    var root = await document.GetSyntaxRootAsync().ConfigureAwait(false);
    var nodes = annotations.SelectMany(annotation => root.GetAnnotatedNodes(annotation));
    root = root.ReplaceNodes(nodes, (original, rewritten) => original.WithoutAnnotations(annotations));
    var newDocument = document.WithSyntaxRoot(root);
    return newDocument;
}

I started a local branch for such a FixAllProvider for the new analyzer #904 that is already working on my machine.

If there is interest in such a FixAllProvider we need to discuss

  • Where it should be located (CodeCracker.Common.dll or CodeCracker.CSharp.dll)
  • The proposed architecture (especially the interface approach)
  • How to proceed with the consumers (I think it is okay to first merge Pull Request New Analyzer CC0125 - getter only auto property #904 than create another Pull Request with the new FixAllProvider and related test and then change IntroduceFieldFromConstructorCodeFixProvider and ReplaceWithGetterOnlyAutoPropertyCodeFixProvider to use the new FixAllProvider)

As I worked on the implementation I came across some issues I would like to document:

  • The fixer might need the SemanticModel of the SyntaxTree. That's why I used the Document instead of the Root SyntaxTree for the parameters in the interface (The SemanticModel is only available via Document.GetSemanticModelAsync).
  • This approach will fail on partial classes that are defined in two ore more documents (the Fixer will not be able to operate on nodes in the other documents).

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions