Skip to content

Commit b638f62

Browse files
sharwellsharwell
committed
Merge remote-tracking branch 'DotNetAnalyzers/pr/2091'
Fix expression body property documentation checks
2 parents c219678 + e86f75f commit b638f62

2 files changed

Lines changed: 153 additions & 120 deletions

File tree

‎StyleCop.Analyzers/StyleCop.Analyzers.Test/DocumentationRules/SA1624UnitTests.cs‎

Lines changed: 55 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ public class SA1624UnitTests : CodeFixVerifier
2727
/// <param name="expectedArgument1">The first expected argument for the diagnostic.</param>
2828
/// <param name="expectedArgument2">The second expected argument for the diagnostic.</param>
2929
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
30-
[Theory]
30+
[Theory(DisplayName = "Property Findings")]
3131
[InlineData("public", "int", "get; internal set;", "Gets or sets", "get", "Gets")]
3232
[InlineData("public", "int", "get; private set;", "Gets or sets", "get", "Gets")]
3333
[InlineData("public", "int", "internal get; set;", "Gets or sets", "set", "Sets")]
@@ -85,13 +85,13 @@ public class TestClass
8585
/// <param name="type">The type to use for the property.</param>
8686
/// <param name="summaryPrefix">The prefix to use in the summary text.</param>
8787
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
88-
[Theory]
89-
[InlineData("public", "int", "Gets or sets")]
90-
[InlineData("public", "bool", "Gets or sets a value indicating whether")]
91-
[InlineData("protected", "int", "Gets or sets")]
92-
[InlineData("protected internal", "int", "Gets or sets")]
93-
[InlineData("internal", "int", "Gets or sets")]
94-
public async Task VerifyThatInvalidDocumentationWillReportDiagnosticForExpressionBodyAsync(string accessibility, string type, string summaryPrefix)
88+
[Theory(DisplayName = "ExpressionBody Gets")]
89+
[InlineData("public", "int", "Gets")]
90+
[InlineData("public", "bool", "Gets a value indicating whether")]
91+
[InlineData("protected", "int", "Gets")]
92+
[InlineData("protected internal", "int", "Gets")]
93+
[InlineData("internal", "int", "Gets")]
94+
public async Task VerifyThatValidDocumentationOnExpressionBodyIsAcceptedAsync(string accessibility, string type, string summaryPrefix)
9595
{
9696
var testCode = $@"
9797
public class TestClass
@@ -107,12 +107,58 @@ public class TestClass
107107
await this.VerifyCSharpDiagnosticAsync(testCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
108108
}
109109

110+
/// <summary>
111+
/// Verifies that documentation that starts with the proper text for multiple accessors will produce a diagnostic for expression body properties.
112+
/// </summary>
113+
/// <param name="accessibility">The accessibility of the property.</param>
114+
/// <param name="type">The type to use for the property.</param>
115+
/// <param name="summaryPrefix">The prefix to use in the summary text.</param>
116+
/// <param name="expectedArgument1">The first expected argument for the diagnostic.</param>
117+
/// <param name="expectedArgument2">The second expected argument for the diagnostic.</param>
118+
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
119+
[Theory(DisplayName = "ExpressionBody Gets&Sets")]
120+
[InlineData("public", "int", "Gets or sets", "get", "Gets")]
121+
[InlineData("public", "bool", "Gets or sets a value indicating whether", "get", "Gets a value indicating whether")]
122+
[InlineData("protected", "int", "Gets or sets", "get", "Gets")]
123+
[InlineData("protected internal", "int", "Gets or sets", "get", "Gets")]
124+
[InlineData("internal", "int", "Gets or sets", "get", "Gets")]
125+
public async Task VerifyThatInvalidDocumentationWillReportDiagnosticForExpressionBodyAsync(string accessibility, string type, string summaryPrefix, string expectedArgument1, string expectedArgument2)
126+
{
127+
var testCode = $@"
128+
public class TestClass
129+
{{
130+
/// <summary>
131+
/// {summaryPrefix} the test property.
132+
/// </summary>
133+
{accessibility} {type} TestProperty =>
134+
default({type});
135+
}}
136+
";
137+
138+
var fixedTestCode = $@"
139+
public class TestClass
140+
{{
141+
/// <summary>
142+
/// {expectedArgument2} the test property.
143+
/// </summary>
144+
{accessibility} {type} TestProperty =>
145+
default({type});
146+
}}
147+
";
148+
149+
var expected = this.CSharpDiagnostic(PropertySummaryDocumentationAnalyzer.SA1624Descriptor).WithLocation(7, 7 + accessibility.Length + type.Length).WithArguments(expectedArgument1, expectedArgument2);
150+
151+
await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
152+
await this.VerifyCSharpDiagnosticAsync(fixedTestCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
153+
await this.VerifyCSharpFixAsync(testCode, fixedTestCode).ConfigureAwait(false);
154+
}
155+
110156
/// <summary>
111157
/// Verify that no diagnostic will be reported when a public and a private accessor are present within a property that is defined in a contained class of a private class.
112158
/// </summary>
113159
/// <param name="typeKeyword">The type keyword to use.</param>
114160
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
115-
[Theory]
161+
[Theory(DisplayName = "PrivateContainer Findings")]
116162
[InlineData("class")]
117163
[InlineData("struct")]
118164
public async Task VerifyPrivateAccessorInPrivateContainerAsync(string typeKeyword)
@@ -139,12 +185,6 @@ public int TestProperty
139185
await this.VerifyCSharpDiagnosticAsync(testCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
140186
}
141187

142-
/// <inheritdoc/>
143-
protected override IEnumerable<string> GetDisabledDiagnostics()
144-
{
145-
yield return PropertySummaryDocumentationAnalyzer.SA1623Descriptor.Id;
146-
}
147-
148188
/// <inheritdoc/>
149189
protected override CodeFixProvider GetCSharpCodeFixProvider()
150190
{

‎StyleCop.Analyzers/StyleCop.Analyzers/DocumentationRules/PropertySummaryDocumentationAnalyzer.cs‎

Lines changed: 98 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -119,138 +119,131 @@ private static void AnalyzeSummaryElement(SyntaxNodeAnalysisContext context, Xml
119119
var textElement = summaryElement.Content.FirstOrDefault() as XmlTextSyntax;
120120
var text = textElement == null ? string.Empty : XmlCommentHelper.GetText(textElement, true).TrimStart();
121121

122-
if (getter != null || expressionBody != null)
122+
bool startsWithGetOrSet = text.StartsWith(startingTextGetsOrSets, StringComparison.Ordinal);
123+
bool getterVisible, setterVisible;
124+
if (getter != null && setter != null)
123125
{
124-
bool startsWithGetOrSet = text.StartsWith(startingTextGetsOrSets, StringComparison.Ordinal);
125-
126-
if (setter != null)
126+
if (!getter.Modifiers.Any() && !setter.Modifiers.Any())
127127
{
128-
// There is no way getter is null (can't have expression body and accessor list)
129-
bool getterVisible;
130-
bool setterVisible;
131-
132-
if (!getter.Modifiers.Any() && !setter.Modifiers.Any())
128+
// Case 1: The getter and setter have the same declared accessibility
129+
getterVisible = true;
130+
setterVisible = true;
131+
}
132+
else if (getter.Modifiers.Any(SyntaxKind.PrivateKeyword))
133+
{
134+
// Case 3
135+
getterVisible = false;
136+
setterVisible = true;
137+
}
138+
else if (setter.Modifiers.Any(SyntaxKind.PrivateKeyword))
139+
{
140+
// Case 3
141+
getterVisible = true;
142+
setterVisible = false;
143+
}
144+
else
145+
{
146+
var propertyAccessibility = propertyDeclaration.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
147+
bool propertyOnlyInternal = propertyAccessibility == Accessibility.Internal
148+
|| propertyAccessibility == Accessibility.ProtectedAndInternal
149+
|| propertyAccessibility == Accessibility.Private;
150+
if (propertyOnlyInternal)
133151
{
134-
// Case 1: The getter and setter have the same declared accessibility
152+
// Case 2: Property only internal and no accessor is explicitly private
135153
getterVisible = true;
136154
setterVisible = true;
137155
}
138-
else if (getter.Modifiers.Any(SyntaxKind.PrivateKeyword))
139-
{
140-
// Case 3
141-
getterVisible = false;
142-
setterVisible = true;
143-
}
144-
else if (setter.Modifiers.Any(SyntaxKind.PrivateKeyword))
145-
{
146-
// Case 3
147-
getterVisible = true;
148-
setterVisible = false;
149-
}
150156
else
151157
{
152-
var propertyAccessibility = propertyDeclaration.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
153-
bool propertyOnlyInternal = propertyAccessibility == Accessibility.Internal
154-
|| propertyAccessibility == Accessibility.ProtectedAndInternal
155-
|| propertyAccessibility == Accessibility.Private;
156-
if (propertyOnlyInternal)
158+
var getterAccessibility = getter.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
159+
var setterAccessibility = setter.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
160+
161+
switch (getterAccessibility)
157162
{
158-
// Case 2: Property only internal and no accessor is explicitly private
163+
case Accessibility.Public:
164+
case Accessibility.ProtectedOrInternal:
165+
case Accessibility.Protected:
166+
// Case 4
159167
getterVisible = true;
160-
setterVisible = true;
161-
}
162-
else
163-
{
164-
var getterAccessibility = getter.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
165-
var setterAccessibility = setter.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
166-
167-
switch (getterAccessibility)
168-
{
169-
case Accessibility.Public:
170-
case Accessibility.ProtectedOrInternal:
171-
case Accessibility.Protected:
172-
// Case 4
173-
getterVisible = true;
174-
break;
175-
176-
case Accessibility.Internal:
177-
case Accessibility.ProtectedAndInternal:
178-
case Accessibility.Private:
179-
default:
180-
// The property is externally accessible, so the setter must be more accessible.
181-
getterVisible = false;
182-
break;
183-
}
184-
185-
switch (setterAccessibility)
186-
{
187-
case Accessibility.Public:
188-
case Accessibility.ProtectedOrInternal:
189-
case Accessibility.Protected:
190-
// Case 4
191-
setterVisible = true;
192-
break;
193-
194-
case Accessibility.Internal:
195-
case Accessibility.ProtectedAndInternal:
196-
case Accessibility.Private:
197-
default:
198-
// The property is externally accessible, so the getter must be more accessible.
199-
setterVisible = false;
200-
break;
201-
}
168+
break;
169+
170+
case Accessibility.Internal:
171+
case Accessibility.ProtectedAndInternal:
172+
case Accessibility.Private:
173+
default:
174+
// The property is externally accessible, so the setter must be more accessible.
175+
getterVisible = false;
176+
break;
202177
}
203-
}
204178

205-
if (getterVisible && !setterVisible)
206-
{
207-
if (startsWithGetOrSet)
208-
{
209-
diagnosticProperties.Add(ExpectedTextKey, startingTextGets);
210-
diagnosticProperties.Add(TextToRemoveKey, startingTextGetsOrSets);
211-
context.ReportDiagnostic(Diagnostic.Create(SA1624Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), "get", startingTextGets));
212-
}
213-
else if (!text.StartsWith(startingTextGets, StringComparison.Ordinal))
214-
{
215-
diagnosticProperties.Add(ExpectedTextKey, startingTextGets);
216-
context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), startingTextGets));
217-
}
218-
}
219-
else if (!getterVisible && setterVisible)
220-
{
221-
if (startsWithGetOrSet)
179+
switch (setterAccessibility)
222180
{
223-
diagnosticProperties.Add(ExpectedTextKey, startingTextSets);
224-
diagnosticProperties.Add(TextToRemoveKey, startingTextGetsOrSets);
225-
context.ReportDiagnostic(Diagnostic.Create(SA1624Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), "set", startingTextSets));
226-
}
227-
else if (!text.StartsWith(startingTextSets, StringComparison.Ordinal))
228-
{
229-
diagnosticProperties.Add(ExpectedTextKey, startingTextSets);
230-
context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), startingTextSets));
181+
case Accessibility.Public:
182+
case Accessibility.ProtectedOrInternal:
183+
case Accessibility.Protected:
184+
// Case 4
185+
setterVisible = true;
186+
break;
187+
188+
case Accessibility.Internal:
189+
case Accessibility.ProtectedAndInternal:
190+
case Accessibility.Private:
191+
default:
192+
// The property is externally accessible, so the getter must be more accessible.
193+
setterVisible = false;
194+
break;
231195
}
232196
}
233-
else
197+
}
198+
}
199+
else
200+
{
201+
if (getter != null || expressionBody != null)
202+
{
203+
getterVisible = true;
204+
setterVisible = false;
205+
}
206+
else
207+
{
208+
getterVisible = false;
209+
setterVisible = setter != null;
210+
}
211+
}
212+
213+
if (getterVisible)
214+
{
215+
if (setterVisible)
216+
{
217+
if (!startsWithGetOrSet)
234218
{
235-
if (!startsWithGetOrSet)
236-
{
237-
diagnosticProperties.Add(ExpectedTextKey, startingTextGetsOrSets);
238-
context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), startingTextGetsOrSets));
239-
}
219+
diagnosticProperties.Add(ExpectedTextKey, startingTextGetsOrSets);
220+
context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), startingTextGetsOrSets));
240221
}
241222
}
242223
else
243224
{
244-
if (startsWithGetOrSet || !text.StartsWith(startingTextGets, StringComparison.Ordinal))
225+
if (startsWithGetOrSet)
226+
{
227+
diagnosticProperties.Add(ExpectedTextKey, startingTextGets);
228+
diagnosticProperties.Add(TextToRemoveKey, startingTextGetsOrSets);
229+
context.ReportDiagnostic(Diagnostic.Create(SA1624Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), "get", startingTextGets));
230+
}
231+
else if (!text.StartsWith(startingTextGets, StringComparison.Ordinal))
245232
{
246233
diagnosticProperties.Add(ExpectedTextKey, startingTextGets);
247234
context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), startingTextGets));
248235
}
249236
}
250237
}
251-
else if (setter != null)
238+
else if (setterVisible)
252239
{
253-
if (!text.StartsWith(startingTextSets, StringComparison.Ordinal))
240+
if (startsWithGetOrSet)
241+
{
242+
diagnosticProperties.Add(ExpectedTextKey, startingTextSets);
243+
diagnosticProperties.Add(TextToRemoveKey, startingTextGetsOrSets);
244+
context.ReportDiagnostic(Diagnostic.Create(SA1624Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), "set", startingTextSets));
245+
}
246+
else if (!text.StartsWith(startingTextSets, StringComparison.Ordinal))
254247
{
255248
diagnosticProperties.Add(ExpectedTextKey, startingTextSets);
256249
context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), startingTextSets));

0 commit comments

Comments
 (0)