From 61b8a1386760a6d983c13dfeb9ac2f2e8e500b43 Mon Sep 17 00:00:00 2001 From: Jay Malhotra Date: Mon, 22 Jan 2024 22:11:43 +0000 Subject: [PATCH 1/4] Make SA1600 trigger on undocumented record primary ctor parameters Add analysis of XML tags to SA1600 so that it is raised on record primary constructor parameters. --- .../SA1600CSharp9UnitTests.cs | 194 ++++++++++++++++++ .../SA1600ElementsMustBeDocumented.cs | 83 +++++++- .../Lightup/RecordDeclarationSyntaxWrapper.cs | 14 ++ 3 files changed, 288 insertions(+), 3 deletions(-) create mode 100644 StyleCop.Analyzers/StyleCop.Analyzers/Lightup/RecordDeclarationSyntaxWrapper.cs diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs index 42ae8fef9..094fd9e1b 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs @@ -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 = $@" +/// +/// Record. +/// +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 = $@" +/// +/// Record. +/// +/// Parameter one. +/// Parameter two. +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 = $@" +/// +/// Record. +/// +/// Parameter one. +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 = $@" +/// +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 = $@" +/// +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 = $@" +/// +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 = $@" +/// +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 = @" + + + Foo + + + "; + string typeWithPartialParameterDocumentation = @" + + + Foo + + Param 2. + Param 3. + + "; + string typeWithParameterDocumentation = @" + + + Foo + + Param 1. + Param 2. + Param 3. + + "; + + 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|}() { }") diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs index f64633941..1c98dd15f 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs @@ -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; @@ -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 parameters = recordDeclaration.ParameterList?.Parameters ?? Enumerable.Empty(); + + 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())); + } } } } @@ -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.Instance; + } + + if (documentation.Content.GetFirstXmlElement(XmlCommentHelper.InheritdocXmlTag) != null) + { + hasInheritdoc = true; + return EmptyArray.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 ?? "", LoadOptions.None); + + if (includedDocumentation.Nodes().OfType().Any(element => element.Name == XmlCommentHelper.InheritdocXmlTag)) + { + hasInheritdoc = true; + return EmptyArray.Instance; + } + + IEnumerable paramElements = includedDocumentation.Nodes() + .OfType() + .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 xmlNodes = documentation.Content.GetXmlElements(XmlCommentHelper.ParamXmlTag); + return xmlNodes.Select(XmlCommentHelper.GetFirstAttributeOrDefault) + .Where(x => x != null) + .Select(x => x.Identifier.Identifier.ValueText) + .ToArray(); + } + } + + private static class EmptyArray + { + public static T[] Instance { get; } = new T[0]; + } } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/RecordDeclarationSyntaxWrapper.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/RecordDeclarationSyntaxWrapper.cs new file mode 100644 index 000000000..ec74730be --- /dev/null +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/RecordDeclarationSyntaxWrapper.cs @@ -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 +{ + public static implicit operator RecordDeclarationSyntaxWrapper(TypeDeclarationSyntax node) + { + return new RecordDeclarationSyntaxWrapper(node); + } +} From 703b1dd24adabd23d3762fadf74a7473cc726ebb Mon Sep 17 00:00:00 2001 From: Jay Malhotra Date: Mon, 29 Jan 2024 23:15:09 +0000 Subject: [PATCH 2/4] Respond to review comments --- .../SA1600CSharp11UnitTests.cs | 6 ++ .../SA1600CSharp9UnitTests.cs | 46 ++++++++----- .../SA1600ElementsMustBeDocumented.cs | 69 +++---------------- .../StyleCop.Analyzers/Helpers/XmlComment.cs | 29 ++++++++ .../Helpers/XmlCommentHelper.cs | 59 ++++++++++++++++ .../Lightup/RecordDeclarationSyntaxWrapper.cs | 14 ---- 6 files changed, 134 insertions(+), 89 deletions(-) create mode 100644 StyleCop.Analyzers/StyleCop.Analyzers/Helpers/XmlComment.cs delete mode 100644 StyleCop.Analyzers/StyleCop.Analyzers/Lightup/RecordDeclarationSyntaxWrapper.cs diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs index 1d1777cda..ef0381b4b 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs @@ -3,9 +3,15 @@ namespace StyleCop.Analyzers.Test.CSharp11.DocumentationRules { + using Microsoft.CodeAnalysis.Testing; using StyleCop.Analyzers.Test.CSharp10.DocumentationRules; public partial class SA1600CSharp11UnitTests : SA1600CSharp10UnitTests { + protected override DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructor(DiagnosticResult[] results) + { + // Roslyn bug fix: Diagnostics are reported twice in the C# 9 and C# 10 tests, but this is fixed in C# 11. + return results; + } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs index 094fd9e1b..9852f8e6c 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs @@ -5,13 +5,12 @@ namespace StyleCop.Analyzers.Test.CSharp9.DocumentationRules { + using System.Linq; 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>; @@ -32,12 +31,10 @@ public async Task TestRecordPrimaryConstructorNoParameterDocumentationAsync(stri DiagnosticResult[] expectedResults = new[] { Diagnostic().WithLocation(0), - Diagnostic().WithLocation(0), - Diagnostic().WithLocation(1), Diagnostic().WithLocation(1), }; - await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode, this.GetExpectedResultTestRecordPrimaryConstructor(expectedResults), CancellationToken.None).ConfigureAwait(false); } [Theory] @@ -51,12 +48,10 @@ public async Task TestRecordPrimaryConstructorNoDocumentationAsync(string keywor DiagnosticResult[] expectedResults = new[] { Diagnostic().WithLocation(0), - Diagnostic().WithLocation(0), - Diagnostic().WithLocation(1), Diagnostic().WithLocation(1), }; - await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode, this.GetExpectedResultTestRecordPrimaryConstructor(expectedResults), CancellationToken.None).ConfigureAwait(false); } [Theory] @@ -90,10 +85,9 @@ public async Task TestRecordPrimaryConstructorPartialParameterDocumentationAsync DiagnosticResult[] expectedResults = new[] { Diagnostic().WithLocation(0), - Diagnostic().WithLocation(0), }; - await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode, this.GetExpectedResultTestRecordPrimaryConstructor(expectedResults), CancellationToken.None).ConfigureAwait(false); } [Theory] @@ -120,6 +114,18 @@ public async Task TestRecordPrimaryConstructorIncludeParameterDocumentationAsync 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 TestRecordPrimaryConstructorIncludeInheritdocAsync(string keyword) + { + string testCode = $@" +/// +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")] @@ -132,10 +138,9 @@ public async Task TestRecordPrimaryConstructorIncludePartialParameterDocumentati DiagnosticResult[] expectedResults = new[] { Diagnostic().WithLocation(0), - Diagnostic().WithLocation(0), }; - await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode, this.GetExpectedResultTestRecordPrimaryConstructor(expectedResults), CancellationToken.None).ConfigureAwait(false); } [Theory] @@ -150,14 +155,11 @@ public async Task TestRecordPrimaryConstructorIncludeMissingParameterDocumentati 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); + await VerifyCSharpDiagnosticAsync(testCode, this.GetExpectedResultTestRecordPrimaryConstructor(expectedResults), CancellationToken.None).ConfigureAwait(false); } protected static Task VerifyCSharpDiagnosticAsync(string source, DiagnosticResult[] expected, CancellationToken cancellationToken) @@ -187,6 +189,11 @@ protected static Task VerifyCSharpDiagnosticAsync(string source, DiagnosticResul Param 2. Param 3. + "; + string typeWithIncludedInheritdoc = @" + + + "; var test = new CSharpTest @@ -197,6 +204,7 @@ protected static Task VerifyCSharpDiagnosticAsync(string source, DiagnosticResul { "MissingParameterDocumentation.xml", typeWithoutParameterDocumentation }, { "WithParameterDocumentation.xml", typeWithParameterDocumentation }, { "WithPartialParameterDocumentation.xml", typeWithPartialParameterDocumentation }, + { "WithInheritdoc.xml", typeWithIncludedInheritdoc }, }, }; @@ -220,5 +228,11 @@ protected override DiagnosticResult[] GetExpectedResultTestRegressionMethodGloba return base.GetExpectedResultTestRegressionMethodGlobalNamespace(code); } + + protected virtual DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructor(DiagnosticResult[] results) + { + // Roslyn reports diagnostics twice for C# 9 and C# 10. Fixed in C# 11. + return results.Concat(results).ToArray(); + } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs index 1c98dd15f..e7b3f95e9 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/SA1600ElementsMustBeDocumented.cs @@ -152,21 +152,27 @@ public static void HandleBaseTypeDeclaration(SyntaxNodeAnalysisContext context, return; } - if (!XmlCommentHelper.HasDocumentation(declaration)) + XmlComment xmlComment = context.GetParsedXmlComment(context.Node); + + if (xmlComment.IsMissing) { context.ReportDiagnostic(Diagnostic.Create(Descriptor, declaration.Identifier.GetLocation())); } + if (xmlComment.HasInheritdoc) + { + return; + } + if (context.Node is TypeDeclarationSyntax typeDeclaration && RecordDeclarationSyntaxWrapper.IsInstance(typeDeclaration)) { - RecordDeclarationSyntaxWrapper recordDeclaration = typeDeclaration; + RecordDeclarationSyntaxWrapper recordDeclaration = (RecordDeclarationSyntaxWrapper)typeDeclaration; - string[] documentedParameterNames = GetDocumentedParameters(context, typeDeclaration, out bool hasInheritdoc); IEnumerable parameters = recordDeclaration.ParameterList?.Parameters ?? Enumerable.Empty(); foreach (var parameter in parameters) { - if (!hasInheritdoc && !documentedParameterNames.Contains(parameter.Identifier.ValueText)) + if (!xmlComment.DocumentedParameterNames.Contains(parameter.Identifier.ValueText)) { context.ReportDiagnostic(Diagnostic.Create(Descriptor, parameter.Identifier.GetLocation())); } @@ -363,61 +369,6 @@ 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.Instance; - } - - if (documentation.Content.GetFirstXmlElement(XmlCommentHelper.InheritdocXmlTag) != null) - { - hasInheritdoc = true; - return EmptyArray.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 ?? "", LoadOptions.None); - - if (includedDocumentation.Nodes().OfType().Any(element => element.Name == XmlCommentHelper.InheritdocXmlTag)) - { - hasInheritdoc = true; - return EmptyArray.Instance; - } - - IEnumerable paramElements = includedDocumentation.Nodes() - .OfType() - .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 xmlNodes = documentation.Content.GetXmlElements(XmlCommentHelper.ParamXmlTag); - return xmlNodes.Select(XmlCommentHelper.GetFirstAttributeOrDefault) - .Where(x => x != null) - .Select(x => x.Identifier.Identifier.ValueText) - .ToArray(); - } - } - - private static class EmptyArray - { - public static T[] Instance { get; } = new T[0]; - } } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/XmlComment.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/XmlComment.cs new file mode 100644 index 000000000..50ade7432 --- /dev/null +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/XmlComment.cs @@ -0,0 +1,29 @@ +// 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.Helpers +{ + using System.Collections.Generic; + using System.Linq; + + internal class XmlComment + { + internal XmlComment(string[] documentedParameterNames, bool hasInheritdoc) + { + this.DocumentedParameterNames = documentedParameterNames; + this.HasInheritdoc = hasInheritdoc; + } + + internal XmlComment() + { + } + + internal static XmlComment MissingSummary => new() { IsMissing = true }; + + internal bool IsMissing { get; private set; } + + internal bool HasInheritdoc { get; private set; } + + internal IEnumerable DocumentedParameterNames { get; private set; } = Enumerable.Empty(); + } +} diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/XmlCommentHelper.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/XmlCommentHelper.cs index 24d2728d1..29ff07497 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/XmlCommentHelper.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/Helpers/XmlCommentHelper.cs @@ -5,11 +5,13 @@ namespace StyleCop.Analyzers.Helpers { + using System.Collections.Generic; using System.Linq; using System.Text; using System.Xml.Linq; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp.Syntax; + using Microsoft.CodeAnalysis.Diagnostics; using StyleCop.Analyzers.Helpers.ObjectPools; /// @@ -436,6 +438,63 @@ internal static bool IsBlockElement(this XmlNodeSyntax nodeSyntax) return false; } + /// + /// Parses a object from a . + /// + /// The analysis context that will be checked. + /// The node to parse the documentation for. + /// The parsed . + internal static XmlComment GetParsedXmlComment(this SyntaxNodeAnalysisContext context, SyntaxNode node) + { + var documentation = node.GetDocumentationCommentTriviaSyntax(); + + if (documentation == null || IsMissingOrEmpty(documentation.ParentTrivia)) + { + return XmlComment.MissingSummary; + } + + bool hasInheritdoc = false; + string[] documentedParameterNames; + + if (documentation.Content.GetFirstXmlElement(InheritdocXmlTag) != null) + { + hasInheritdoc = true; + } + + bool hasIncludedDocumentation = documentation.Content.GetFirstXmlElement(IncludeXmlTag) != null; + + if (hasIncludedDocumentation) + { + var declaredSymbol = context.SemanticModel.GetDeclaredSymbol(node, context.CancellationToken); + var rawDocumentation = declaredSymbol?.GetDocumentationCommentXml(expandIncludes: true, cancellationToken: context.CancellationToken); + var includedDocumentation = XElement.Parse(rawDocumentation ?? "", LoadOptions.None); + + if (includedDocumentation.Nodes().OfType().Any(element => element.Name == InheritdocXmlTag)) + { + hasInheritdoc = true; + } + + IEnumerable paramElements = includedDocumentation.Nodes() + .OfType() + .Where(x => x.Name == ParamXmlTag); + + documentedParameterNames = paramElements + .SelectMany(x => x.Attributes().Where(y => y.Name == NameArgumentName)) + .Select(x => x.Value) + .ToArray(); + } + else + { + IEnumerable xmlNodes = documentation.Content.GetXmlElements(ParamXmlTag); + documentedParameterNames = xmlNodes.Select(GetFirstAttributeOrDefault) + .Where(x => x != null) + .Select(x => x.Identifier.Identifier.ValueText) + .ToArray(); + } + + return new XmlComment(documentedParameterNames, hasInheritdoc); + } + private static bool IsInlineElement(string localName) { switch (localName) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/RecordDeclarationSyntaxWrapper.cs b/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/RecordDeclarationSyntaxWrapper.cs deleted file mode 100644 index ec74730be..000000000 --- a/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/RecordDeclarationSyntaxWrapper.cs +++ /dev/null @@ -1,14 +0,0 @@ -// 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 -{ - public static implicit operator RecordDeclarationSyntaxWrapper(TypeDeclarationSyntax node) - { - return new RecordDeclarationSyntaxWrapper(node); - } -} From ad82a37a54f6e26f4eb02e4ef099e27100a35cd2 Mon Sep 17 00:00:00 2001 From: Jay Malhotra Date: Mon, 29 Jan 2024 23:53:36 +0000 Subject: [PATCH 3/4] Clean up tests --- .../SA1600CSharp11UnitTests.cs | 63 +++++++++- .../SA1600CSharp9UnitTests.cs | 108 ++++++++++++------ 2 files changed, 136 insertions(+), 35 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs index ef0381b4b..a5c57669b 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs @@ -5,13 +5,70 @@ namespace StyleCop.Analyzers.Test.CSharp11.DocumentationRules { using Microsoft.CodeAnalysis.Testing; using StyleCop.Analyzers.Test.CSharp10.DocumentationRules; + using static StyleCop.Analyzers.Test.Verifiers.StyleCopDiagnosticVerifier< + StyleCop.Analyzers.DocumentationRules.SA1600ElementsMustBeDocumented>; public partial class SA1600CSharp11UnitTests : SA1600CSharp10UnitTests { - protected override DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructor(DiagnosticResult[] results) + + /* These methods reflect a fix for a Roslyn issue from C# 9 where diagnostics were reported twice. */ + + protected override DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorNoParameterDocumentation() + { + return new[] + { + // /0/Test0.cs(5,28): warning SA1600: Elements should be documented + Diagnostic().WithLocation(0), + + // /0/Test0.cs(5,43): warning SA1600: Elements should be documented + Diagnostic().WithLocation(1), + }; + } + + protected override DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorNoDocumentation() + { + return new[] + { + // /0/Test0.cs(2,15): warning SA1600: Elements should be documented + Diagnostic().WithLocation(0), + + // /0/Test0.cs(2,28): warning SA1600: Elements should be documented + Diagnostic().WithLocation(1), + }; + } + + protected override DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorPartialParameterDocumentation() { - // Roslyn bug fix: Diagnostics are reported twice in the C# 9 and C# 10 tests, but this is fixed in C# 11. - return results; + return new[] + { + // /0/Test0.cs(6,43): warning SA1600: Elements should be documented + Diagnostic().WithLocation(0), + }; + } + + protected override DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorIncludePartialParameterDocumentation() + { + + return new[] + { + // /0/Test0.cs(3,28): warning SA1600: Elements should be documented + Diagnostic().WithLocation(0), + }; + } + + protected override DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorIncludeMissingParameterDocumentation() + { + return new[] + { + // /0/Test0.cs(3,28): warning SA1600: Elements should be documented + Diagnostic().WithLocation(0), + + // /0/Test0.cs(3,43): warning SA1600: Elements should be documented + Diagnostic().WithLocation(1), + + // /0/Test0.cs(3,56): warning SA1600: Elements should be documented + Diagnostic().WithLocation(2), + }; } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs index 9852f8e6c..e117e8283 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs @@ -28,13 +28,9 @@ public async Task TestRecordPrimaryConstructorNoParameterDocumentationAsync(stri /// public {keyword} MyRecord(int {{|#0:Param1|}}, string {{|#1:Param2|}});"; - DiagnosticResult[] expectedResults = new[] - { - Diagnostic().WithLocation(0), - Diagnostic().WithLocation(1), - }; + DiagnosticResult[] expectedResults = this.GetExpectedResultTestRecordPrimaryConstructorNoParameterDocumentation(); - await VerifyCSharpDiagnosticAsync(testCode, this.GetExpectedResultTestRecordPrimaryConstructor(expectedResults), CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false); } [Theory] @@ -45,13 +41,9 @@ public async Task TestRecordPrimaryConstructorNoDocumentationAsync(string keywor string testCode = $@" public {keyword} {{|#0:MyRecord|}}(int {{|#1:Param1|}});"; - DiagnosticResult[] expectedResults = new[] - { - Diagnostic().WithLocation(0), - Diagnostic().WithLocation(1), - }; + DiagnosticResult[] expectedResults = this.GetExpectedResultTestRecordPrimaryConstructorNoDocumentation(); - await VerifyCSharpDiagnosticAsync(testCode, this.GetExpectedResultTestRecordPrimaryConstructor(expectedResults), CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false); } [Theory] @@ -82,12 +74,9 @@ public async Task TestRecordPrimaryConstructorPartialParameterDocumentationAsync /// Parameter one. public {keyword} MyRecord(int Param1, string {{|#0:Param2|}});"; - DiagnosticResult[] expectedResults = new[] - { - Diagnostic().WithLocation(0), - }; + DiagnosticResult[] expectedResults = this.GetExpectedResultTestRecordPrimaryConstructorPartialParameterDocumentation(); - await VerifyCSharpDiagnosticAsync(testCode, this.GetExpectedResultTestRecordPrimaryConstructor(expectedResults), CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false); } [Theory] @@ -135,12 +124,9 @@ public async Task TestRecordPrimaryConstructorIncludePartialParameterDocumentati /// public {keyword} MyRecord(int {{|#0:Param1|}}, string Param2, bool Param3);"; - DiagnosticResult[] expectedResults = new[] - { - Diagnostic().WithLocation(0), - }; + DiagnosticResult[] expectedResults = this.GetExpectedResultTestRecordPrimaryConstructorIncludePartialParameterDocumentation(); - await VerifyCSharpDiagnosticAsync(testCode, this.GetExpectedResultTestRecordPrimaryConstructor(expectedResults), CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false); } [Theory] @@ -152,14 +138,9 @@ public async Task TestRecordPrimaryConstructorIncludeMissingParameterDocumentati /// public {keyword} MyRecord(int {{|#0:Param1|}}, string {{|#1:Param2|}}, bool {{|#2:Param3|}});"; - DiagnosticResult[] expectedResults = new[] - { - Diagnostic().WithLocation(0), - Diagnostic().WithLocation(1), - Diagnostic().WithLocation(2), - }; + DiagnosticResult[] expectedResults = this.GetExpectedResultTestRecordPrimaryConstructorIncludeMissingParameterDocumentation(); - await VerifyCSharpDiagnosticAsync(testCode, this.GetExpectedResultTestRecordPrimaryConstructor(expectedResults), CancellationToken.None).ConfigureAwait(false); + await VerifyCSharpDiagnosticAsync(testCode, expectedResults, CancellationToken.None).ConfigureAwait(false); } protected static Task VerifyCSharpDiagnosticAsync(string source, DiagnosticResult[] expected, CancellationToken cancellationToken) @@ -229,10 +210,73 @@ protected override DiagnosticResult[] GetExpectedResultTestRegressionMethodGloba return base.GetExpectedResultTestRegressionMethodGlobalNamespace(code); } - protected virtual DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructor(DiagnosticResult[] results) + /* The below result overrides are due to a Roslyn bug causing diagnostics to be reported twice, which was fixed in a later version. */ + + protected virtual DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorNoParameterDocumentation() + { + return new[] + { + // /0/Test0.cs(5,28): warning SA1600: Elements should be documented + Diagnostic().WithLocation(0), + Diagnostic().WithLocation(0), + + // /0/Test0.cs(5,43): warning SA1600: Elements should be documented + Diagnostic().WithLocation(1), + Diagnostic().WithLocation(1), + }; + } + + protected virtual DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorNoDocumentation() { - // Roslyn reports diagnostics twice for C# 9 and C# 10. Fixed in C# 11. - return results.Concat(results).ToArray(); + return new[] + { + // /0/Test0.cs(2,15): warning SA1600: Elements should be documented + Diagnostic().WithLocation(0), + Diagnostic().WithLocation(0), + + // /0/Test0.cs(2,28): warning SA1600: Elements should be documented + Diagnostic().WithLocation(1), + Diagnostic().WithLocation(1), + }; + } + + protected virtual DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorPartialParameterDocumentation() + { + return new[] + { + // /0/Test0.cs(6,43): warning SA1600: Elements should be documented + Diagnostic().WithLocation(0), + Diagnostic().WithLocation(0), + }; + } + + protected virtual DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorIncludePartialParameterDocumentation() + { + + return new[] + { + // /0/Test0.cs(3,28): warning SA1600: Elements should be documented + Diagnostic().WithLocation(0), + Diagnostic().WithLocation(0), + }; + } + + protected virtual DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorIncludeMissingParameterDocumentation() + { + return new[] + { + // /0/Test0.cs(3,28): warning SA1600: Elements should be documented + Diagnostic().WithLocation(0), + Diagnostic().WithLocation(0), + + // /0/Test0.cs(3,43): warning SA1600: Elements should be documented + Diagnostic().WithLocation(1), + Diagnostic().WithLocation(1), + + // /0/Test0.cs(3,56): warning SA1600: Elements should be documented + Diagnostic().WithLocation(2), + Diagnostic().WithLocation(2), + }; } } } From bcdc55bc24f6cf2c75cfaeb7b96ee3115bcd6907 Mon Sep 17 00:00:00 2001 From: Jay Malhotra Date: Tue, 30 Jan 2024 00:00:19 +0000 Subject: [PATCH 4/4] Fix formatting --- .../DocumentationRules/SA1600CSharp11UnitTests.cs | 2 -- .../DocumentationRules/SA1600CSharp9UnitTests.cs | 1 - 2 files changed, 3 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs index a5c57669b..6f414baf8 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp11/DocumentationRules/SA1600CSharp11UnitTests.cs @@ -10,7 +10,6 @@ namespace StyleCop.Analyzers.Test.CSharp11.DocumentationRules public partial class SA1600CSharp11UnitTests : SA1600CSharp10UnitTests { - /* These methods reflect a fix for a Roslyn issue from C# 9 where diagnostics were reported twice. */ protected override DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorNoParameterDocumentation() @@ -48,7 +47,6 @@ protected override DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstruc protected override DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorIncludePartialParameterDocumentation() { - return new[] { // /0/Test0.cs(3,28): warning SA1600: Elements should be documented diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs index e117e8283..47daac12c 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test.CSharp9/DocumentationRules/SA1600CSharp9UnitTests.cs @@ -252,7 +252,6 @@ protected virtual DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstruct protected virtual DiagnosticResult[] GetExpectedResultTestRecordPrimaryConstructorIncludePartialParameterDocumentation() { - return new[] { // /0/Test0.cs(3,28): warning SA1600: Elements should be documented