Removing XML Doc Comment Nodes

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 : CodeFixProvider
where TXmlElementSyntax : SyntaxNode
{
public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

public abstract override ImmutableArray 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, b

public abstract override ImmutableArray 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?

///  

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




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.

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

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

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

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

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

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

Or:

public override ImmutableArray 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.

StackExchange Code Review Q#139086, answer score: 5