7
\$\begingroup\$

I recently wrote a code fix to handle many of the C# and VB.NET compiler diagnostics that was merged into Roslyn. Because it handles both, I implemented it as an abstract class with only the sections that are specific to the different languages in the language-specific implementations to eliminate code duplication. If you are interested, you can find my full test suites here: C# and VB.NET.

The abstract class controls most of the implementation. Before you tell me to use CodeAction.Create for my code action implementation, Roslyn has an internal diagnostic telling me not to use that.

internal abstract class AbstractRemoveDocCommentNodeCodeFixProvider<TXmlElementSyntax> : CodeFixProvider
    where TXmlElementSyntax : SyntaxNode
{
    public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

    public abstract override ImmutableArray<string> FixableDiagnosticIds { get; }

    protected abstract string DocCommentSignifierToken { get; }

    protected abstract SyntaxTriviaList GetRevisedDocCommentTrivia(string docCommentText);

    public async sealed override Task RegisterCodeFixesAsync(CodeFixContext context)
    {
        var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);

        if (GetParamNode(root, context.Span) != null)
        {
            context.RegisterCodeFix(
                new MyCodeAction(
                    c => RemoveDuplicateParamTagAsync(context.Document, context.Span, c)),
                context.Diagnostics);
        }
    }

    private TXmlElementSyntax GetParamNode(SyntaxNode root, TextSpan span, CancellationToken cancellationToken = default(CancellationToken))
    {
        // First, we get the node the diagnostic fired on
        // Then, we climb the tree to the first parent that is of the type XMLElement
        // This is to correctly handle XML nodes that are nested in other XML nodes, so we only
        // remove the node the diagnostic fired on and its children, but no parent nodes
        var paramNode = root.FindNode(span, findInsideTrivia: true);
        return paramNode.FirstAncestorOrSelf<TXmlElementSyntax>();
    }

    private async Task<Document> RemoveDuplicateParamTagAsync(
        Document document, TextSpan span, CancellationToken cancellationToken)
    {
        var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
        var paramNode = GetParamNode(root, span, cancellationToken);

        var removedNodes = new List<SyntaxNode> { paramNode };
        var paramNodeSiblings = paramNode.Parent.ChildNodes().ToList();

        // This should not cause a crash because the diagnostics are only thrown in
        // doc comment XML nodes, which, by definition, start with `///` (C#) or `'''` (VB.NET)
        // If, perhaps, this specific node is not directly preceded by the comment marker node,
        // it will be preceded by another XML node
        var paramNodeIndex = paramNodeSiblings.IndexOf(paramNode);
        var previousNodeTextTrimmed = paramNodeSiblings[paramNodeIndex - 1].ToFullString().Trim();

        if (previousNodeTextTrimmed == string.Empty || previousNodeTextTrimmed == DocCommentSignifierToken)
        {
            removedNodes.Add(paramNodeSiblings[paramNodeIndex - 1]);
        }

        // Remove all trivia attached to the nodes I am removing.
        // Really, any option should work here because the leading/trailing text
        // around these nodes are not attached to them as trivia.
        var newRoot = root.RemoveNodes(removedNodes, SyntaxRemoveOptions.KeepNoTrivia);
        return document.WithSyntaxRoot(newRoot);
    }

    private class MyCodeAction : CodeAction.DocumentChangeAction
    {
        public MyCodeAction(Func<CancellationToken, Task<Document>> createChangedDocument)
            : base(FeaturesResources.Remove_tag, createChangedDocument)
        {
        }
    }
}

Here is the C# implementation of the above class:

[ExportCodeFixProvider(LanguageNames.CSharp, Name = PredefinedCodeFixProviderNames.RemoveDocCommentNode), Shared]
internal class CSharpRemoveDocCommentNodeCodeFixProvider : AbstractRemoveDocCommentNodeCodeFixProvider<XmlElementSyntax>
{
    /// <summary>
    /// Duplicate param tag
    /// </summary>
    private const string CS1571 = nameof(CS1571);

    /// <summary>
    /// Param tag with no matching parameter
    /// </summary>
    private const string CS1572 = nameof(CS1572);

    /// <summary>
    /// Duplicate typeparam tag
    /// </summary>
    private const string CS1710 = nameof(CS1710);

    public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(CS1571, CS1572, CS1710);

    protected override string DocCommentSignifierToken { get; } = "///";

    protected override SyntaxTriviaList GetRevisedDocCommentTrivia(string docCommentText)
        => SyntaxFactory.ParseLeadingTrivia(docCommentText);
}

The VB.NET version handles more diagnostics because the two compilers have a different set of diagnostics they report for different cases:

<ExportCodeFixProvider(LanguageNames.VisualBasic, Name:=PredefinedCodeFixProviderNames.RemoveDocCommentNode), [Shared]>
Friend Class VisualBasicRemoveDocCommentNodeCodeFixProvider
    Inherits AbstractRemoveDocCommentNodeCodeFixProvider(Of XmlElementSyntax)

    ''' <summary>
    ''' XML comment tag with identical attributes
    ''' </summary>
    Private Const BC42305 As String = NameOf(BC42305)

    ''' <summary>
    ''' XML comment tag is not permitted on a 'sub' language element
    ''' </summary>
    Private Const BC42306 As String = NameOf(BC42306)

    ''' <summary>
    ''' XML comment type parameter does not match a type parameter
    ''' </summary>
    Private Const BC42307 As String = NameOf(BC42307)

    ''' <summary>
    ''' XML comment tag 'returns' is not permitted on a 'WriteOnly' property
    ''' </summary>
    Private Const BC42313 As String = NameOf(BC42313)

    ''' <summary>
    ''' XML comment tag 'returns' is not permitted on a 'declare sub' language element
    ''' </summary>
    Private Const BC42315 As String = NameOf(BC42315)

    ''' <summary>
    ''' XML comment type parameter does not match a type parameter
    ''' </summary>
    Private Const BC42317 As String = NameOf(BC42317)

    Friend ReadOnly Id As ImmutableArray(Of String) = ImmutableArray.Create(BC42305, BC42306, BC42307, BC42313, BC42315, BC42317)

    Public Overrides ReadOnly Property FixableDiagnosticIds As ImmutableArray(Of String)
        Get
            Return Id
        End Get
    End Property

    Protected Overrides ReadOnly Property DocCommentSignifierToken As String
        Get
            Return "'''"
        End Get
    End Property

    Protected Overrides Function GetRevisedDocCommentTrivia(docCommentText As String) As SyntaxTriviaList
        Return SyntaxFactory.ParseLeadingTrivia(docCommentText)
    End Function
End Class
\$\endgroup\$
  • \$\begingroup\$ funny, I didn't know that, everything else in the .net is called readonly-something but the array is an immutable-array - so much for consistance ;-) \$\endgroup\$ – t3chb0t Aug 19 '16 at 6:07
  • 1
    \$\begingroup\$ @t3chb0t There is a difference. A ReadOnlyCollection<T> can be changed, just not by you. An ImmutableArray<T> cannot be changed at all. \$\endgroup\$ – svick Aug 21 '16 at 20:10
5
\$\begingroup\$
public abstract override ImmutableArray<string> FixableDiagnosticIds { get; }

What is the reason for this line? You override an abstract property with an abstract property. The end result should be the the same as if this line did not exist.


var paramNodeSiblings = paramNode.Parent.ChildNodes().ToList();

var paramNodeIndex = paramNodeSiblings.IndexOf(paramNode);
var previousNodeTextTrimmed = paramNodeSiblings[paramNodeIndex - 1].ToFullString().Trim();

I don't like how you're using ToList() and IndexOf() just to find the previous node. An alternative approach would be to use TakeWhile() and Last() instead:

var paramNodeSiblings = paramNode.Parent.ChildNodes();

var previousNode = paramNodeSiblings.TakeWhile(s => s != paramNode).Last();
var previousNodeTextTrimmed = previousNode.ToFullString().Trim();

What happens if there is another XML element on the same line as the element that you're removing?

/// <param name="shouldBeRemoved"></param> <param name="shouldStay"></param>

I think your code would remove the XML element along with ///, resulting in invalid code:

<param name="shouldStay"></param>

There is also a rarely used multiline syntax /** for documentation comments. I think your code will handle that correctly. Is that intentional? Have you tested it?


You're calling GetParamNode() in RegisterCodeFixesAsync() and then again in RemoveDuplicateParamTagAsync(). This feels inefficient to me (no idea if it actually affects performance) and I think getting rid of the duplication would simplify the code a bit.


/// <summary>
/// Duplicate param tag
/// </summary>
private const string CS1571 = nameof(CS1571);

/// <summary>
/// Param tag with no matching parameter
/// </summary>
private const string CS1572 = nameof(CS1572);

/// <summary>
/// Duplicate typeparam tag
/// </summary>
private const string CS1710 = nameof(CS1710);

public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(CS1571, CS1572, CS1710);

This is fairly verbose code, just to create a collection of strings. What about:

public override ImmutableArray<string> FixableDiagnosticIds { get; } =
    ImmutableArray.Create(
        // Duplicate param tag
        "CS1571",
        // Param tag with no matching parameter
        "CS1572",
        // Duplicate typeparam tag
        "CS1710");

Or:

public override ImmutableArray<string> FixableDiagnosticIds { get; } =
    ImmutableArray.Create(
        "CS1571", // Duplicate param tag
        "CS1572", // Param tag with no matching parameter
        "CS1710"  // Duplicate typeparam tag
    );

protected abstract SyntaxTriviaList GetRevisedDocCommentTrivia(string docCommentText);

This method is declared and overridden, but never called, so I think it can be removed.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Great answer. I didn't know about the multiline doc comment--that sounds problematic. Also, I think you are right about the other bug--I'll have to check that one. \$\endgroup\$ – user34073 Aug 27 '16 at 16:49
  • 1
    \$\begingroup\$ After testing, the multiline comment thing does remove the node correctly, but it does not remove the trivia like the other one does. The other bug about removing the trivia does remove the trivia, but it also removes the whitespace trivia, moving the new trivia to immediately after the previous node, if there is one. If there isn't a previous node, it does remove the trivia and breaks the code. \$\endgroup\$ – user34073 Aug 27 '16 at 17:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy