Skip to content

Commit 134423e

Browse files
Copilotbcollamoreynsehoornenborg
authored
fix: PH2001 false positive when documentation has useful param tags (#835)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bcollamore <57269455+bcollamore@users.noreply.github.com> Co-authored-by: Collamore, Brian <brian.collamore@philips.com> Co-authored-by: Ynse Hoornenborg <ynse.hoornenborg@philips.com>
1 parent 667ffb3 commit 134423e

2 files changed

Lines changed: 176 additions & 17 deletions

File tree

‎Philips.CodeAnalysis.MaintainabilityAnalyzers/Documentation/XmlDocumentationShouldAddValueAnalyzer.cs‎

Lines changed: 51 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,12 @@ private void AnalyzeNamedNode(SyntaxNodeAnalysisContext context, Func<string> na
116116

117117
var lowercaseName = name.ToLowerInvariant();
118118

119+
// If any element contains useful information, skip all diagnostics
120+
if (HasUsefulElements(xmlElements, lowercaseName))
121+
{
122+
return;
123+
}
124+
119125
foreach (XmlElementSyntax xmlElement in xmlElements)
120126
{
121127
if (xmlElement.StartTag.Name.LocalName.Text != @"summary")
@@ -133,25 +139,53 @@ private void AnalyzeNamedNode(SyntaxNodeAnalysisContext context, Func<string> na
133139
continue;
134140
}
135141

136-
// Find the 'value' in the XML documentation content by:
137-
// 1. Splitting it into separate words.
138-
// 2. Filtering a predefined and a configurable list of words that add no value.
139-
// 3. Filter words that are part of the method name.
140-
// 4. Throw a Diagnostic if no words remain. This boils down to the content only containing 'low value' words.
141-
IEnumerable<string> words =
142-
SplitInWords(content)
143-
.Where(u => !additionalUselessWords.Contains(u) && !UselessWords.Contains(u))
144-
.Where(s => !lowercaseName.Contains(s));
145-
146-
// We assume here that every remaining word adds value to the documentation text.
147-
if (!words.Any())
148-
{
149-
var loc = Location.Create(context.Node.SyntaxTree, xmlElement.Content.FullSpan);
150-
var diagnostic = Diagnostic.Create(ValueRule, loc);
151-
context.ReportDiagnostic(diagnostic);
152-
}
142+
// Summary already evaluated by HasUsefulElements
143+
var loc = Location.Create(context.Node.SyntaxTree, xmlElement.Content.FullSpan);
144+
var valueDiagnostic = Diagnostic.Create(ValueRule, loc);
145+
context.ReportDiagnostic(valueDiagnostic);
146+
}
147+
}
148+
}
149+
150+
/// <summary>
151+
/// Checks if there are any XML documentation elements that have useful content.
152+
/// Elements like summary, param, returns, exception, etc. are considered if they have useful information.
153+
/// </summary>
154+
/// <param name="xmlElements">All XML documentation elements</param>
155+
/// <param name="lowercaseName">The lowercase name of the documented element</param>
156+
/// <returns>True if there are elements with useful content, false otherwise</returns>
157+
private bool HasUsefulElements(IEnumerable<XmlElementSyntax> xmlElements, string lowercaseName)
158+
{
159+
var xmlDocElements = new[] { "summary", "param", "returns", "exception", "remarks", "example", "value" };
160+
161+
foreach (XmlElementSyntax xmlElement in xmlElements)
162+
{
163+
var tagName = xmlElement.StartTag.Name.LocalName.Text;
164+
if (!xmlDocElements.Contains(tagName))
165+
{
166+
continue;
167+
}
168+
169+
var content = GetContent(xmlElement);
170+
171+
if (string.IsNullOrWhiteSpace(content))
172+
{
173+
continue;
174+
}
175+
176+
IEnumerable<string> words =
177+
SplitInWords(content)
178+
.Where(u => !additionalUselessWords.Contains(u) && !UselessWords.Contains(u))
179+
.Where(s => !lowercaseName.Contains(s));
180+
181+
// If there are remaining words, assume this element has useful content
182+
if (words.Any())
183+
{
184+
return true;
153185
}
154186
}
187+
188+
return false;
155189
}
156190

157191
private static string GetContent(XmlElementSyntax xmlElement)

‎Philips.CodeAnalysis.Test/Maintainability/Documentation/XmlDocumentationShouldAddValueAnalyzerTest.cs‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -618,5 +618,130 @@ public Foo()
618618
await VerifyFix(content, newContent);
619619
}
620620

621+
[TestMethod]
622+
[TestCategory(TestDefinitions.UnitTests)]
623+
public async Task EmptySummaryWithUsefulParamShouldNotTriggerAsync()
624+
{
625+
var content = $@"
626+
public class TestClass
627+
{{
628+
/// <summary></summary>
629+
/// <param name=""value"">The meaningful parameter description that provides useful information.</param>
630+
public void Foo(int value)
631+
{{
632+
}}
633+
}}
634+
";
635+
await VerifySuccessfulCompilation(content).ConfigureAwait(false);
636+
}
637+
638+
[TestMethod]
639+
[TestCategory(TestDefinitions.UnitTests)]
640+
public async Task UselessSummaryWithUsefulParamShouldNotTriggerAsync()
641+
{
642+
var content = $@"
643+
public class TestClass
644+
{{
645+
/// <summary>Foo</summary>
646+
/// <param name=""value"">The meaningful parameter description that provides useful information.</param>
647+
public void Foo(int value)
648+
{{
649+
}}
650+
}}
651+
";
652+
await VerifySuccessfulCompilation(content).ConfigureAwait(false);
653+
}
654+
655+
[TestMethod]
656+
[TestCategory(TestDefinitions.UnitTests)]
657+
public async Task EmptySummaryWithUsefulReturnsShouldNotTriggerAsync()
658+
{
659+
var content = $@"
660+
public class TestClass
661+
{{
662+
/// <summary></summary>
663+
/// <returns>A meaningful return value description that provides useful information.</returns>
664+
public int Foo()
665+
{{
666+
return 42;
667+
}}
668+
}}
669+
";
670+
await VerifySuccessfulCompilation(content).ConfigureAwait(false);
671+
}
672+
673+
[TestMethod]
674+
[TestCategory(TestDefinitions.UnitTests)]
675+
public async Task EmptySummaryWithUsefulExceptionShouldNotTriggerAsync()
676+
{
677+
var content = $@"
678+
public class TestClass
679+
{{
680+
/// <summary></summary>
681+
/// <exception cref=""System.ArgumentNullException"">Thrown when argument is null and operation cannot proceed.</exception>
682+
public void Foo(string arg)
683+
{{
684+
if (arg == null) throw new System.ArgumentNullException(nameof(arg));
685+
}}
686+
}}
687+
";
688+
await VerifySuccessfulCompilation(content).ConfigureAwait(false);
689+
}
690+
691+
[TestMethod]
692+
[TestCategory(TestDefinitions.UnitTests)]
693+
public async Task EmptySummaryWithMultipleUsefulElementsShouldNotTriggerAsync()
694+
{
695+
var content = $@"
696+
public class TestClass
697+
{{
698+
/// <summary></summary>
699+
/// <param name=""input"">The input value to process with specific business logic.</param>
700+
/// <returns>A processed result based on the input with additional computation.</returns>
701+
/// <exception cref=""System.ArgumentException"">Thrown when input is invalid for processing.</exception>
702+
public int ProcessValue(string input)
703+
{{
704+
if (string.IsNullOrEmpty(input)) throw new System.ArgumentException(""Invalid input"");
705+
return input.Length * 2;
706+
}}
707+
}}
708+
";
709+
await VerifySuccessfulCompilation(content).ConfigureAwait(false);
710+
}
711+
712+
[TestMethod]
713+
[TestCategory(TestDefinitions.UnitTests)]
714+
public async Task EmptySummaryWithUselessParamShouldStillTriggerAsync()
715+
{
716+
var content = $@"
717+
public class TestClass
718+
{{
719+
/// <summary></summary>
720+
/// <param name=""foo"">foo</param>
721+
public void Foo(int foo)
722+
{{
723+
}}
724+
}}
725+
";
726+
await VerifyDiagnostic(content, DiagnosticId.EmptyXmlComments).ConfigureAwait(false);
727+
}
728+
729+
[TestMethod]
730+
[TestCategory(TestDefinitions.UnitTests)]
731+
public async Task UselessSummaryWithUselessParamShouldTriggerAsync()
732+
{
733+
var content = $@"
734+
public class TestClass
735+
{{
736+
/// <summary>Foo</summary>
737+
/// <param name=""foo"">foo</param>
738+
public void Foo(int foo)
739+
{{
740+
}}
741+
}}
742+
";
743+
await VerifyDiagnostic(content, DiagnosticId.XmlDocumentationShouldAddValue).ConfigureAwait(false);
744+
}
745+
621746
}
622747
}

0 commit comments

Comments
 (0)