Skip to content

Commit d6c83c4

Browse files
sharwellsharwell
committed
Merge pull request #1845 from sharwell/fix-1844
Fix handling of property accessor visibility
2 parents 4d0f9a2 + 855192c commit d6c83c4

3 files changed

Lines changed: 137 additions & 29 deletions

File tree

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

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,77 @@ public class TestClass
8181
await this.VerifyCSharpFixAsync(testCode, fixedTestCode).ConfigureAwait(false);
8282
}
8383

84+
/// <summary>
85+
/// Verifies that property documentation that does not start with the appropriate text will result in a
86+
/// diagnostic. This is a regression test for DotNetAnalyzers/StyleCopAnalyzers#1844.
87+
/// </summary>
88+
/// <param name="accessibility">The accessibility of the property.</param>
89+
/// <param name="type">The type to use for the property.</param>
90+
/// <param name="accessors">The accessors for the property.</param>
91+
/// <param name="expectedArgument">The expected argument for the diagnostic message.</param>
92+
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
93+
[Theory]
94+
[InlineData("public", "int", "{ get; set; }", "Gets or sets")]
95+
[InlineData("public", "int", "{ get; protected set; }", "Gets or sets")]
96+
[InlineData("public", "int", "{ get; protected internal set; }", "Gets or sets")]
97+
[InlineData("public", "int", "{ get; internal set; }", "Gets or sets")]
98+
[InlineData("public", "int", "{ get; private set; }", "Gets")]
99+
[InlineData("public", "int", "{ get; }", "Gets")]
100+
[InlineData("public", "int", "{ get; } = 0;", "Gets")]
101+
[InlineData("public", "int", "=> 0;", "Gets")]
102+
[InlineData("public", "int", "{ set { } }", "Sets")]
103+
[InlineData("public", "int", "{ internal get { return 0; } set { } }", "Gets or sets")]
104+
[InlineData("public", "int", "{ private get { return 0; } set { } }", "Sets")]
105+
[InlineData("public", "bool", "{ get; set; }", "Gets or sets a value indicating whether")]
106+
[InlineData("public", "bool", "{ get; }", "Gets a value indicating whether")]
107+
[InlineData("public", "bool", "{ get; } = true;", "Gets a value indicating whether")]
108+
[InlineData("public", "bool", "=> true;", "Gets a value indicating whether")]
109+
[InlineData("public", "bool", "{ get; private set; }", "Gets a value indicating whether")]
110+
[InlineData("public", "bool", "{ set { } }", "Sets a value indicating whether")]
111+
[InlineData("public", "bool", "{ private get { return false; } set { } }", "Sets a value indicating whether")]
112+
[InlineData("protected", "int", "{ get; private set; }", "Gets")]
113+
[InlineData("protected", "int", "{ private get { return 0; } set { } }", "Sets")]
114+
[InlineData("protected internal", "int", "{ get; internal set; }", "Gets or sets")]
115+
[InlineData("protected internal", "int", "{ get; private set; }", "Gets")]
116+
[InlineData("protected internal", "int", "{ internal get { return 0; } set { } }", "Gets or sets")]
117+
[InlineData("protected internal", "int", "{ private get { return 0; } set { } }", "Sets")]
118+
[InlineData("internal", "int", "{ get; private set; }", "Gets")]
119+
[InlineData("internal", "int", "{ private get { return 0; } set { } }", "Sets")]
120+
public async Task VerifyDocumentationOfPublicMethodInPrivateClassWillProduceDiagnosticAsync(string accessibility, string type, string accessors, string expectedArgument)
121+
{
122+
var testCode = $@"
123+
public class TestClass
124+
{{
125+
private class PrivateTestClass
126+
{{
127+
/// <summary>
128+
/// The first test value.
129+
/// </summary>
130+
{accessibility} {type} TestProperty {accessors}
131+
}}
132+
}}
133+
";
134+
135+
var fixedTestCode = $@"
136+
public class TestClass
137+
{{
138+
private class PrivateTestClass
139+
{{
140+
/// <summary>
141+
/// {expectedArgument} the first test value.
142+
/// </summary>
143+
{accessibility} {type} TestProperty {accessors}
144+
}}
145+
}}
146+
";
147+
148+
var expected = this.CSharpDiagnostic(PropertySummaryDocumentationAnalyzer.SA1623Descriptor).WithLocation(9, 11 + accessibility.Length + type.Length).WithArguments(expectedArgument);
149+
150+
await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
151+
await this.VerifyCSharpDiagnosticAsync(fixedTestCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
152+
await this.VerifyCSharpFixAsync(testCode, fixedTestCode).ConfigureAwait(false);
153+
}
154+
84155
/// <inheritdoc/>
85156
protected override CodeFixProvider GetCSharpCodeFixProvider()
86157
{

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ public class ContainerTestClass
125125
public {typeKeyword} TestType
126126
{{
127127
/// <summary>
128-
/// Gets or sets the test property.
128+
/// Gets the test property.
129129
/// </summary>
130130
public int TestProperty
131131
{{

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

Lines changed: 65 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -118,43 +118,80 @@ private static void AnalyzeSummaryElement(SyntaxNodeAnalysisContext context, Xml
118118
if (setter != null)
119119
{
120120
// There is no way getter is null (can't have expression body and accessor list)
121-
var getterAccessibility = getter.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
122-
var setterAccessibility = setter.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
123121
bool getterVisible;
124122
bool setterVisible;
125123

126-
switch (getterAccessibility)
124+
if (!getter.Modifiers.Any() && !setter.Modifiers.Any())
127125
{
128-
case Accessibility.Public:
129-
case Accessibility.ProtectedOrInternal:
130-
case Accessibility.Protected:
126+
// Case 1: The getter and setter have the same declared accessibility
131127
getterVisible = true;
132-
break;
133-
134-
case Accessibility.Internal:
135-
getterVisible = setterAccessibility == Accessibility.Private;
136-
break;
137-
138-
default:
139-
getterVisible = false;
140-
break;
128+
setterVisible = true;
141129
}
142-
143-
switch (setterAccessibility)
130+
else if (getter.Modifiers.Any(SyntaxKind.PrivateKeyword))
144131
{
145-
case Accessibility.Public:
146-
case Accessibility.ProtectedOrInternal:
147-
case Accessibility.Protected:
132+
// Case 3
133+
getterVisible = false;
148134
setterVisible = true;
149-
break;
150-
151-
case Accessibility.Internal:
152-
setterVisible = getterAccessibility == Accessibility.Private;
153-
break;
154-
155-
default:
135+
}
136+
else if (setter.Modifiers.Any(SyntaxKind.PrivateKeyword))
137+
{
138+
// Case 3
139+
getterVisible = true;
156140
setterVisible = false;
157-
break;
141+
}
142+
else
143+
{
144+
var propertyAccessibility = propertyDeclaration.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
145+
bool propertyOnlyInternal = propertyAccessibility == Accessibility.Internal
146+
|| propertyAccessibility == Accessibility.ProtectedAndInternal
147+
|| propertyAccessibility == Accessibility.Private;
148+
if (propertyOnlyInternal)
149+
{
150+
// Case 2: Property only internal and no accessor is explicitly private
151+
getterVisible = true;
152+
setterVisible = true;
153+
}
154+
else
155+
{
156+
var getterAccessibility = getter.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
157+
var setterAccessibility = setter.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
158+
159+
switch (getterAccessibility)
160+
{
161+
case Accessibility.Public:
162+
case Accessibility.ProtectedOrInternal:
163+
case Accessibility.Protected:
164+
// Case 4
165+
getterVisible = true;
166+
break;
167+
168+
case Accessibility.Internal:
169+
case Accessibility.ProtectedAndInternal:
170+
case Accessibility.Private:
171+
default:
172+
// The property is externally accessible, so the setter must be more accessible.
173+
getterVisible = false;
174+
break;
175+
}
176+
177+
switch (setterAccessibility)
178+
{
179+
case Accessibility.Public:
180+
case Accessibility.ProtectedOrInternal:
181+
case Accessibility.Protected:
182+
// Case 4
183+
setterVisible = true;
184+
break;
185+
186+
case Accessibility.Internal:
187+
case Accessibility.ProtectedAndInternal:
188+
case Accessibility.Private:
189+
default:
190+
// The property is externally accessible, so the getter must be more accessible.
191+
setterVisible = false;
192+
break;
193+
}
194+
}
158195
}
159196

160197
if (getterVisible && !setterVisible)

0 commit comments

Comments
 (0)