Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make SA1600 trigger on undocumented record primary ctor parameters #3783

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Make SA1600 trigger on undocumented record primary ctor parameters #3783

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61b8a13
Make SA1600 trigger on undocumented record primary ctor parameters
SapiensAnatis Jan 22, 2024
703b1dd
Respond to review comments
SapiensAnatis Jan 29, 2024
ad82a37
Clean up tests
SapiensAnatis Jan 29, 2024
bcdc55b
Fix formatting
SapiensAnatis Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 194 additions & 0 deletions ...op.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,205 @@

namespace StyleCop.Analyzers.Test.CSharp9.DocumentationRules
{
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Testing;
using StyleCop.Analyzers.DocumentationRules;
using StyleCop.Analyzers.Test.CSharp8.DocumentationRules;
using StyleCop.Analyzers.Test.Helpers;
using StyleCop.Analyzers.Test.Verifiers;
using Xunit;
using static StyleCop.Analyzers.Test.Verifiers.StyleCopDiagnosticVerifier<
StyleCop.Analyzers.DocumentationRules.SA1600ElementsMustBeDocumented>;

public partial class SA1600CSharp9UnitTests : SA1600CSharp8UnitTests
{
[Theory]
[MemberData(nameof(CommonMemberData.RecordTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))]
[WorkItem(3780, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3780")]
public async Task TestRecordPrimaryConstructorNoParameterDocumentationAsync(string keyword)
{
string testCode = $@"
/// <summary>
/// Record.
/// </summary>
public {keyword} MyRecord(int {{|#0:Param1|}}, string {{|#1:Param2|}});";

DiagnosticResult[] expectedResults = new[]
{
Diagnostic().WithLocation(0),
Diagnostic().WithLocation(0),
Diagnostic().WithLocation(1),
Diagnostic().WithLocation(1),
};

await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false);
}

[Theory]
[MemberData(nameof(CommonMemberData.RecordTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))]
[WorkItem(3780, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3780")]
public async Task TestRecordPrimaryConstructorNoDocumentationAsync(string keyword)
{
string testCode = $@"
public {keyword} {{|#0:MyRecord|}}(int {{|#1:Param1|}});";

DiagnosticResult[] expectedResults = new[]
{
Diagnostic().WithLocation(0),
Diagnostic().WithLocation(0),
Diagnostic().WithLocation(1),
Diagnostic().WithLocation(1),
};

await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false);
}

[Theory]
[MemberData(nameof(CommonMemberData.RecordTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))]
[WorkItem(3780, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3780")]
public async Task TestRecordPrimaryConstructorCompleteParameterDocumentationAsync(string keyword)
{
string testCode = $@"
/// <summary>
/// Record.
/// </summary>
/// <param name=""Param1"">Parameter one.</param>
/// <param name=""Param2"">Parameter two.</param>
public {keyword} MyRecord(int Param1, string Param2);";

await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
}

[Theory]
[MemberData(nameof(CommonMemberData.RecordTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))]
[WorkItem(3780, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3780")]
public async Task TestRecordPrimaryConstructorPartialParameterDocumentationAsync(string keyword)
{
string testCode = $@"
/// <summary>
/// Record.
/// </summary>
/// <param name=""Param1"">Parameter one.</param>
public {keyword} MyRecord(int Param1, string {{|#0:Param2|}});";

DiagnosticResult[] expectedResults = new[]
{
Diagnostic().WithLocation(0),
Diagnostic().WithLocation(0),
};

await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false);
}

[Theory]
[MemberData(nameof(CommonMemberData.RecordTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))]
[WorkItem(3780, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3780")]
public async Task TestRecordPrimaryConstructorInheritdocAsync(string keyword)
{
string testCode = $@"
/// <inheritdoc />
public {keyword} MyRecord(int Param1, string Param2);";

await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
}

[Theory]
[MemberData(nameof(CommonMemberData.RecordTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))]
[WorkItem(3780, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3780")]
public async Task TestRecordPrimaryConstructorIncludeParameterDocumentationAsync(string keyword)
{
string testCode = $@"
/// <include file='WithParameterDocumentation.xml' path='/TestType/*' />
public {keyword} MyRecord(int Param1, string Param2, bool Param3);";

await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
}

[Theory]
[MemberData(nameof(CommonMemberData.RecordTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))]
[WorkItem(3780, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3780")]
public async Task TestRecordPrimaryConstructorIncludePartialParameterDocumentationAsync(string keyword)
{
string testCode = $@"
/// <include file='WithPartialParameterDocumentation.xml' path='/TestType/*' />
public {keyword} MyRecord(int {{|#0:Param1|}}, string Param2, bool Param3);";

DiagnosticResult[] expectedResults = new[]
{
Diagnostic().WithLocation(0),
Diagnostic().WithLocation(0),
};

await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false);
}

[Theory]
[MemberData(nameof(CommonMemberData.RecordTypeDeclarationKeywords), MemberType = typeof(CommonMemberData))]
[WorkItem(3780, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3780")]
public async Task TestRecordPrimaryConstructorIncludeMissingParameterDocumentationAsync(string keyword)
{
string testCode = $@"
/// <include file='MissingParameterDocumentation.xml' path='/TestType/*' />
public {keyword} MyRecord(int {{|#0:Param1|}}, string {{|#1:Param2|}}, bool {{|#2:Param3|}});";

DiagnosticResult[] expectedResults = new[]
{
Diagnostic().WithLocation(0),
Diagnostic().WithLocation(0),
Diagnostic().WithLocation(1),
Diagnostic().WithLocation(1),
Diagnostic().WithLocation(2),
Diagnostic().WithLocation(2),
};

await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false);
}

protected static Task VerifyCSharpDiagnosticAsync(string source, DiagnosticResult[] expected, CancellationToken cancellationToken)
{
string typeWithoutParameterDocumentation = @"<?xml version=""1.0"" encoding=""utf-8"" ?>
<TestType>
<summary>
Foo
</summary>
</TestType>
";
string typeWithPartialParameterDocumentation = @"<?xml version=""1.0"" encoding=""utf-8"" ?>
<TestType>
<summary>
Foo
</summary>
<param name=""Param2"">Param 2.</param>
<param name=""Param3"">Param 3.</param>
</TestType>
";
string typeWithParameterDocumentation = @"<?xml version=""1.0"" encoding=""utf-8"" ?>
<TestType>
<summary>
Foo
</summary>
<param name=""Param1"">Param 1.</param>
<param name=""Param2"">Param 2.</param>
<param name=""Param3"">Param 3.</param>
</TestType>
";

var test = new CSharpTest
{
TestCode = source,
XmlReferences =
{
{ "MissingParameterDocumentation.xml", typeWithoutParameterDocumentation },
{ "WithParameterDocumentation.xml", typeWithParameterDocumentation },
{ "WithPartialParameterDocumentation.xml", typeWithPartialParameterDocumentation },
},
};

test.ExpectedDiagnostics.AddRange(expected);
return test.RunAsync(cancellationToken);
}

protected override DiagnosticResult[] GetExpectedResultTestRegressionMethodGlobalNamespace(string code)
{
if (code == "public void {|#0:TestMember|}() { }")
Expand Down
83 changes: 80 additions & 3 deletions StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@
namespace StyleCop.Analyzers.DocumentationRules
{
using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection.Metadata;
using System.Xml.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
Expand Down Expand Up @@ -143,11 +147,29 @@ public static void HandleBaseTypeDeclaration(SyntaxNodeAnalysisContext context,

Accessibility declaredAccessibility = declaration.GetDeclaredAccessibility(context.SemanticModel, context.CancellationToken);
Accessibility effectiveAccessibility = declaration.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
if (NeedsComment(settings.DocumentationRules, declaration.Kind(), declaration.Parent.Kind(), declaredAccessibility, effectiveAccessibility))
if (!NeedsComment(settings.DocumentationRules, declaration.Kind(), declaration.Parent.Kind(), declaredAccessibility, effectiveAccessibility))
{
if (!XmlCommentHelper.HasDocumentation(declaration))
return;
}

if (!XmlCommentHelper.HasDocumentation(declaration))
{
context.ReportDiagnostic(Diagnostic.Create(Descriptor, declaration.Identifier.GetLocation()));
}

if (context.Node is TypeDeclarationSyntax typeDeclaration && RecordDeclarationSyntaxWrapper.IsInstance(typeDeclaration))
{
RecordDeclarationSyntaxWrapper recordDeclaration = typeDeclaration;

string[] documentedParameterNames = GetDocumentedParameters(context, typeDeclaration, out bool hasInheritdoc);
IEnumerable<ParameterSyntax> parameters = recordDeclaration.ParameterList?.Parameters ?? Enumerable.Empty<ParameterSyntax>();

foreach (var parameter in parameters)
{
context.ReportDiagnostic(Diagnostic.Create(Descriptor, declaration.Identifier.GetLocation()));
if (!hasInheritdoc && !documentedParameterNames.Contains(parameter.Identifier.ValueText))
{
context.ReportDiagnostic(Diagnostic.Create(Descriptor, parameter.Identifier.GetLocation()));
}
}
}
}
Expand Down Expand Up @@ -341,6 +363,61 @@ public static void HandleEventFieldDeclaration(SyntaxNodeAnalysisContext context
}
}
}

private static string[] GetDocumentedParameters(SyntaxNodeAnalysisContext context, RecordDeclarationSyntaxWrapper typeDeclarationSyntax, out bool hasInheritdoc)
{
hasInheritdoc = false;

var documentation = context.Node.GetDocumentationCommentTriviaSyntax();
if (documentation == null)
{
return EmptyArray<string>.Instance;
}

if (documentation.Content.GetFirstXmlElement(XmlCommentHelper.InheritdocXmlTag) != null)
{
hasInheritdoc = true;
return EmptyArray<string>.Instance;
}

var hasIncludedDocumentation =
documentation.Content.GetFirstXmlElement(XmlCommentHelper.IncludeXmlTag) != null;

if (hasIncludedDocumentation)
{
var declaredSymbol = context.SemanticModel.GetDeclaredSymbol(typeDeclarationSyntax, context.CancellationToken);
var rawDocumentation = declaredSymbol?.GetDocumentationCommentXml(expandIncludes: true, cancellationToken: context.CancellationToken);
var includedDocumentation = XElement.Parse(rawDocumentation ?? "<doc></doc>", LoadOptions.None);

if (includedDocumentation.Nodes().OfType<XElement>().Any(element => element.Name == XmlCommentHelper.InheritdocXmlTag))
{
hasInheritdoc = true;
Copy link
Contributor

@bjornhellander bjornhellander Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is not executed in tests. I have no idea why it was needed where you copied it from :-), but maybe you can try to find out to see if more tests should be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It handles the case where your XML comment includes an XML file, which itself specifies an <inheritdoc />.

I've added a test that should cover this case.

return EmptyArray<string>.Instance;
}

IEnumerable<XElement> paramElements = includedDocumentation.Nodes()
.OfType<XElement>()
.Where(x => x.Name == XmlCommentHelper.ParamXmlTag);

return paramElements
.SelectMany(x => x.Attributes().Where(y => y.Name == XmlCommentHelper.NameArgumentName))
.Select(x => x.Value)
.ToArray();
}
else
{
IEnumerable<XmlNodeSyntax> xmlNodes = documentation.Content.GetXmlElements(XmlCommentHelper.ParamXmlTag);
return xmlNodes.Select(XmlCommentHelper.GetFirstAttributeOrDefault<XmlNameAttributeSyntax>)
.Where(x => x != null)
.Select(x => x.Identifier.Identifier.ValueText)
.ToArray();
}
}

private static class EmptyArray<T>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move it to the Helpers folder so it can be used in other places as well if needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a bit of refactoring and was no longer using this, so I've removed it

{
public static T[] Instance { get; } = new T[0];
}
}
}
}
14 changes: 14 additions & 0 deletions StyleCop.Analyzers/StyleCop.Analyzers/Lightup/RecordDeclarationSyntaxWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved.
// Licensed under the MIT License. See LICENSE in the project root for license information.

namespace StyleCop.Analyzers.Lightup;

using Microsoft.CodeAnalysis.CSharp.Syntax;

internal partial struct RecordDeclarationSyntaxWrapper : ISyntaxWrapper<TypeDeclarationSyntax>
{
public static implicit operator RecordDeclarationSyntaxWrapper(TypeDeclarationSyntax node)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This corresponds to a conversion from a base type to a sub type. Adding an implicit conversion operator for that is not a good thing in my book.

Copy link
Contributor Author

@SapiensAnatis SapiensAnatis Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, I am happy to make it an explicit operator. With the way it is being used I could also do something like:

bool RecordDeclarationSyntaxWrapper.TryWrap(TypeDeclarationSyntax node, out RecordDeclarationSyntaxWrapper result)

similar to int.TryParse and friends. I'm not sure if that's a bit out there though!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear: The explicit conversion operator already exists, so you can just remove this file and add a cast where you get an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, understood. Removed this file and used the explicit operator.

{
return new RecordDeclarationSyntaxWrapper(node);
}
}
Loading