SA1611 is not raised for class primary constructor parameters that are missing documentation about stylecopanalyzers HOT 9 CLOSED

I'd be interested in taking a look at this issue.

I've had a quick look through with a debugger and it seems that SA1611ElementParametersMustBeDocumented.HandleXmlElement is returning early as parameterList is null here: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1611ElementParametersMustBeDocumented.cs#L64

Naively, I think this could be fixed by adding an additional check on the ParameterList of ClassDeclarationSyntax in GetParameters:

 private static IEnumerable<ParameterSyntax> GetParameters(SyntaxNode node)
 {
     return (node as BaseMethodDeclarationSyntax)?.ParameterList?.Parameters
         ?? (node as IndexerDeclarationSyntax)?.ParameterList?.Parameters
         ?? (node as DelegateDeclarationSyntax)?.ParameterList?.Parameters
+        ?? (node as ClassDeclarationSyntax)?.ParameterList?.Parameters;
 }

However, if I'm reading the documentation right, this property of ClassDeclarationSyntax is only available from v4.6.0 of Microsoft.CodeAnalysis.CSharp, and the StyleCop.Analyzers project itself is on v1.2.1.

This is my first time working with this project but I assume there is some specific reason why this package is left out of date. Is it still possible to pick up the parameters of a primary constructor declaration in some other way, or is this blocked on whatever it is that is blocking the aforementioned package upgrade?

from stylecopanalyzers.

@SapiensAnatis I defined a helper method for this in #3774

from stylecopanalyzers.

Note that records should have their parameters treated like properties, while non-records should have primary constructor parameters treated like other constructor parameters.

from stylecopanalyzers.

This is my first time working with this project but I assume there is some specific reason why this package is left out of date. Is it still possible to pick up the parameters of a primary constructor declaration in some other way, or is this blocked on whatever it is that is blocking the aforementioned package upgrade?

These analyzers would fail to load in older Roslyn versions otherwise, since the code would in that case reference symbols that doesn't exist. So instead, we detect these additions at runtime with reflection, using the code in the Lightup folder on the StyleCop.Analyzers project. By doing it this way, one nuget package can be created regardless of Roslyn/Visual Studio version used by the consumers.

from stylecopanalyzers.

Ah, that makes sense -- that's a clever solution to multi-targeting. Thanks @sharwell for implementing that helper, I'll take another look soon.

I'll make sure also that my changes don't have any adverse affect on records. From what I can remember, the documentation rules for those work correctly at the minute.

from stylecopanalyzers.

I've opened a PR for the issue with class primary constructors specifically at #3777.

Regarding records, having taken a closer look it appears they suffer from the same issue, in that the following code does not produce any diagnostics:

/// <summary>
/// Record.
/// </summary>
public record Record(int Param1);

My understanding is that a diagnostic should be raised unless a <param> tag is added, which will add documentation to both the constructor parameter and the implicitly declared property.

/// <summary>
/// Record.
/// </summary>
/// <param name="Param1">Parameter 1.</param>
public record Record(int Param1);

I can also look to address this, would it be best done as part of #3777 or separately? Additionally, @sharwell, in regards to your earlier statement:

Note that records should have their parameters treated like properties

Does this mean you'd prefer to see SA1600 raised on missing record <param> tags instead of SA1611, to match the behaviour of a class property whose summary documentation is missing?

from stylecopanalyzers.

Does this mean you'd prefer to see SA1600 raised on missing record <param> tags instead of SA1611

Yes, that's correct. I don't think we should include records in SA1611. You can either fix/verify SA1600 at the same time, or leave it for a future change.

from stylecopanalyzers.

@sharwell would you like me to open a new issue to cover adding SA1600 to record primary constructors?

from stylecopanalyzers.

Sure 👍

from stylecopanalyzers.