Code Style: Code fixes for xml doc comments

[Pilchie]

We already have the "Insert Comment" command (and /// experience), but we should hook it up as a Code Fix to the compiler warnings about missing XML doc comments.

CSharp

Below are the diagnostics that we could potentially create code fixes for:

• CS1571: XML comment has a duplicate param tag for '{0}'
• CS1572: XML comment has a param tag for '{0}', but there is no parameter by that name
• CS1573: Parameter '{0}' has no matching param tag in XML comment for '{1}' (but other parameters do)
• CS1591: Missing XML comment for publicly visible type or member '{0}'

Here's code that will exhibit all four diagnostics when the /doc argument is passed to the compiler.

public class C
{
    /// <param name="x"></param>
    /// <param name="x"></param>
    /// <param name="z"></param>
    public void Foo(int x, int y)
    {

    }
}

Visual Basic

Below are the diagnostics that we could potentially create code fixes for:

• BC42301: Only one XML comment block is allowed per language element.
• BC42303: XML comment cannat appear with a method or a property. XML comment will be ignored.
• BC42305: XML comment tag '{0}' appears with identical attributes more than once in the same XML comment block.
• BC42306: XML comment tag '{0}' is not permitted on a 'sub' language element.
• BC42307: XML comment parameter '{0}' does not match a parameter on the corresponding 'sub' statement.
• BC42308: XML comment parameter must have 'name' attribute.
• BC42312: XML documentation comments must precede member or type declarations.
• BC42313: XML comment tag 'returns' is not permitted on a 'WriteOnly' Property.
• BC42314: XML comment cannot be applied more than once on a partial class. XML comments for this class will be ignored.
• BC42315: XML comment tag 'returns' is not permitted on a 'declare sub' language element.
• BC42317: XML comment type parameter '{0}' does not match a type parameter on the corresponding '{1}' statement.
• BC42318: XML comment type parameter must have a 'name' attribute.
• BC42319: XML comment exception must have a 'cref' attribute.

Here's code that will surface each of the above diagnostics.

''' <summary>
''' 
''' </summary>
Partial Class C

End Class

''' <summary>
''' 
''' </summary>
Partial Class C

    '''

    ''' <summary>
    ''' 
    ''' </summary>
    ''' <param name="i"></param>
    ''' <param name="i"></param>
    ''' <param></param>
    ''' <param name="j"></param>
    ''' <typeparam name="T"></typeparam>
    ''' <typeparam></typeparam>
    ''' <returns></returns>
    ''' <exception></exception>
    Public Sub M(i As Integer)

        ''' <summary></summary>

    End Sub

    ''' <returns></returns>
    WriteOnly Property P As Integer
        Set(value As Integer)
        End Set
    End Property

    ''' <returns></returns>
    Declare Sub Foo Lib "User" ()

End Class

''' <summary>
''' </summary>

[sharwell]

💭 Well this could be interesting, especially if you find a way to support custom implementations of the code fix.

[aluanhaddad]

👍

[Hosch250]

In VSDiagnostics (provides analyzers/fixes for C#), we have issues for redundant/missing returns, missing exception blocks, missing summaries, and generally malformed docs (like a missing closing tag), in addition to the parameter ones.

[Pilchie]

It doesn't look like we're going to get to this before 2.0 😦

Moving back to unknown until we decide to pick it up, but marking as Up for Grabs, because we'd likely take contributions that added the fixes.

[Hosch250]

I'd be interested in tackling this, but I didn't say anything because I saw there were two open PR's for it, so I thought it was already spoken for.

[Pilchie]

@Hosch250 - thanks for pointing that out. I see #10520, but I don't see another PR?

[Hosch250]

#5611 and #10520 are both referenced in the issue just above your comment from an hour ago.

[Pilchie]

Right, but #5611 is another (related) issue, not a PR :)

[Hosch250]

Oh, my mistake.

[Pilchie]

Anyway, I pinged @3jr about #10520 to see if it's abandoned.

[daviburg]

Also known as StyleCop's rule SA1600 : CSharp.Documentation : The {class|...} must have a documentation header.

[kendrahavens]

Closed developer feedback as duplicate.

Additional notes:

• Make configurable public, protected, internal or private members/classes/interfaces/etc...

[BDisp]

Is there any compiler warning when a parameterized method without any XML comments parameters, but already with the summary XML comment?

[CyrusNajmabadi]

@BDisp can you give an example of what you mean?

[BDisp]

@BDisp can you give an example of what you mean?

Of course, thanks.

/// <summary>
/// Function foo
/// </summary>
public void Foo(string a, string b)
{
}

This should give a compiler warning, such as missing parameters in the XML comment. But it doesn't.

[CyrusNajmabadi]

@BDisp Thanks for the example!

[BDisp]

@CyrusNajmabadi in my opinion there also should have compiler warnings for the following situations, whose alerts can be disabled or enabled by the user:

Missing returns tag in the XML Comment

/// <summary>
/// Foo
/// </summary>
/// <param name="x"></param>
/// <param name="a"></param>
public string Foo (int x, string a)
{
	return $"{a}{x}";
}

Missing typeparam in the XML Comment

/// <summary>
/// Foo
/// </summary>
/// <param name="a"></param>
/// <param name="x"></param>
public void Foo<S, N> (string a, int x) where S : Bar, new() where N : FooBar, new()
{
}

It does not make much sense the compiler alerts that a parameter is missing when there is more than one and not alert when there is none.

[CyrusNajmabadi]

Note: you'd be able to get this today (which is much more likely) just by writing your own analzyers. It's possible stylecop may already have this.

[BDisp]

Thanks. The compilation it's very slow. I prefer to wait for the CS fix :-)

[BDisp]

It would be great to warn if any tag is empty.

"Documentation text should not be empty, for 'x' and 'a'"

/// <summary>
/// Foo
/// </summary>
/// <param name="x"></param>
/// <param name="a"></param>
public void Foo (int x, string a)
{
}

[CyrusNajmabadi]

@BDisp imo, it's very unlikely we would do this. Adding new diagnostics for existing legal code is almost entirely preferred to be done through analyzers.

[sharwell]

Note: you'd be able to get this today (which is much more likely) just by writing your own analzyers. It's possible stylecop may already have this.

StyleCop Analyzers does already have a few rules for this.
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/DocumentationRules.md

Some of the rules also have code fixes; a table showing them is here:
https://dotnetanalyzers.github.io/StyleCopAnalyzers/

[CyrusNajmabadi]

Thanks @sharwell ! Given that, i believe we'd be much more inclined to keep things as the status quo (As far as roslyn or csharplang are concerned). This would be a space we'd say is ripe for external analyzers to step in for.

In terms of this issue @sharwell, what do you think we should do? I personally don't think we need code-style/fixes in the IDE. However, i would be ok with code refactorings, which users could bring up in existing doc-comments to "add the missing elements" as it were. So, if you had:

/// <summary>
/// Function foo
/// </summary>
public int Foo(string a, string b)
{
}

And you brought up the lightbulb in the doc comment, there would be items like:

1. Add missing <param> tags
2. Add missing <return> tag
3. Add all missing tags

Would that be something you think would be good for us to have?

[BDisp]

@CyrusNajmabadi, it's not code fixes I asking for, but only warnings at compile time. Some analyzers tools read this warnings and presents suggestion for some code fixes.

[CyrusNajmabadi]

@BDisp

but only warnings at compile time.

I understand. This is a perfect place for analyzers. Their purpose is exactly to do things like just give warnings at compile time. The compiler/lang is highly unlikely to add these as it's already possible to do today, fairly simply, with analyzers. And there is already plenty of precedent for tools (like style-cop) to do this.