-
Notifications
You must be signed in to change notification settings - Fork 506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SA1611 is not raised for class primary constructor parameters that are missing documentation #3770
Comments
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 Naively, I think this could be fixed by adding an additional check on the 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 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? |
@SapiensAnatis I defined a helper method for this in #3774 |
Note that records should have their parameters treated like properties, while non-records should have primary constructor parameters treated like other constructor parameters. |
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. |
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. |
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 /// <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:
Does this mean you'd prefer to see SA1600 raised on missing record |
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. |
@sharwell would you like me to open a new issue to cover adding SA1600 to record primary constructors? |
Sure 👍 |
For a traditional constructor in the below example, SA1611 is raised on the constructor parameter
myString
to indicate that it does not have a entry in the constructor documentation:However, when making a similar mistake using a primary constructor, SA1611 is not reported:
I would expect SA1611 to be reported unless the parameter is appropriately documented beneath the class summary:
Edit: I am on version
1.2.0-beta.556
The text was updated successfully, but these errors were encountered: