Skip to content

Commit d5b2ca7

Browse files
sharwellsharwell
committed
Merge pull request #1813 from vweijsters/fix-1811
SA1623/1624 are now using the AccessLevelHelper
2 parents 3cb375e + 7dd1f8e commit d5b2ca7

4 files changed

Lines changed: 122 additions & 66 deletions

File tree

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

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,30 +20,43 @@ public class SA1623UnitTests : CodeFixVerifier
2020
/// <summary>
2121
/// Verifies that property documentation that does not start with the appropriate text will result in a diagnostic.
2222
/// </summary>
23+
/// <param name="accessibility">The accessibility of the property.</param>
2324
/// <param name="type">The type to use for the property.</param>
2425
/// <param name="accessors">The accessors for the property.</param>
2526
/// <param name="expectedArgument">The expected argument for the diagnostic message.</param>
2627
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
2728
[Theory]
28-
[InlineData("int", "{ get; set; }", "Gets or sets")]
29-
[InlineData("int", "{ get; }", "Gets")]
30-
[InlineData("int", "{ get; private set; }", "Gets")]
31-
[InlineData("int", "{ set { } }", "Sets")]
32-
[InlineData("int", "{ private get { return 0; } set { } }", "Sets")]
33-
[InlineData("bool", "{ get; set; }", "Gets or sets a value indicating whether")]
34-
[InlineData("bool", "{ get; }", "Gets a value indicating whether")]
35-
[InlineData("bool", "{ get; private set; }", "Gets a value indicating whether")]
36-
[InlineData("bool", "{ set { } }", "Sets a value indicating whether")]
37-
[InlineData("bool", "{ private get { return false; } set { } }", "Sets a value indicating whether")]
38-
public async Task VerifyDocumentationWithWrongStartingTextWillProduceDiagnosticAsync(string type, string accessors, string expectedArgument)
29+
[InlineData("public", "int", "{ get; set; }", "Gets or sets")]
30+
[InlineData("public", "int", "{ get; protected set; }", "Gets or sets")]
31+
[InlineData("public", "int", "{ get; protected internal set; }", "Gets or sets")]
32+
[InlineData("public", "int", "{ get; internal set; }", "Gets")]
33+
[InlineData("public", "int", "{ get; private set; }", "Gets")]
34+
[InlineData("public", "int", "{ get; }", "Gets")]
35+
[InlineData("public", "int", "{ set { } }", "Sets")]
36+
[InlineData("public", "int", "{ internal get { return 0; } set { } }", "Sets")]
37+
[InlineData("public", "int", "{ private get { return 0; } set { } }", "Sets")]
38+
[InlineData("public", "bool", "{ get; set; }", "Gets or sets a value indicating whether")]
39+
[InlineData("public", "bool", "{ get; }", "Gets a value indicating whether")]
40+
[InlineData("public", "bool", "{ get; private set; }", "Gets a value indicating whether")]
41+
[InlineData("public", "bool", "{ set { } }", "Sets a value indicating whether")]
42+
[InlineData("public", "bool", "{ private get { return false; } set { } }", "Sets a value indicating whether")]
43+
[InlineData("protected", "int", "{ get; private set; }", "Gets")]
44+
[InlineData("protected", "int", "{ private get { return 0; } set { } }", "Sets")]
45+
[InlineData("protected internal", "int", "{ get; internal set; }", "Gets")]
46+
[InlineData("protected internal", "int", "{ get; private set; }", "Gets")]
47+
[InlineData("protected internal", "int", "{ internal get { return 0; } set { } }", "Sets")]
48+
[InlineData("protected internal", "int", "{ private get { return 0; } set { } }", "Sets")]
49+
[InlineData("internal", "int", "{ get; private set; }", "Gets")]
50+
[InlineData("internal", "int", "{ private get { return 0; } set { } }", "Sets")]
51+
public async Task VerifyDocumentationWithWrongStartingTextWillProduceDiagnosticAsync(string accessibility, string type, string accessors, string expectedArgument)
3952
{
4053
var testCode = $@"
4154
public class TestClass
4255
{{
4356
/// <summary>
4457
/// The first test value.
4558
/// </summary>
46-
public {type} TestProperty {accessors}
59+
{accessibility} {type} TestProperty {accessors}
4760
}}
4861
";
4962

@@ -53,11 +66,11 @@ public class TestClass
5366
/// <summary>
5467
/// {expectedArgument} the first test value.
5568
/// </summary>
56-
public {type} TestProperty {accessors}
69+
{accessibility} {type} TestProperty {accessors}
5770
}}
5871
";
5972

60-
var expected = this.CSharpDiagnostic(PropertySummaryDocumentationAnalyzer.SA1623Descriptor).WithLocation(7, 13 + type.Length).WithArguments(expectedArgument);
73+
var expected = this.CSharpDiagnostic(PropertySummaryDocumentationAnalyzer.SA1623Descriptor).WithLocation(7, 7 + accessibility.Length + type.Length).WithArguments(expectedArgument);
6174

6275
await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
6376
await this.VerifyCSharpDiagnosticAsync(fixedTestCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);

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

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,26 +20,37 @@ public class SA1624UnitTests : CodeFixVerifier
2020
/// <summary>
2121
/// Verifies that documentation that starts with the proper text for multiple accessors will produce a diagnostic when one of the accessors has reduced visibility.
2222
/// </summary>
23+
/// <param name="accessibility">The accessibility of the property.</param>
2324
/// <param name="type">The type to use for the property.</param>
2425
/// <param name="accessors">The accessors for the property.</param>
2526
/// <param name="summaryPrefix">The prefix to use in the summary text.</param>
2627
/// <param name="expectedArgument1">The first expected argument for the diagnostic.</param>
2728
/// <param name="expectedArgument2">The second expected argument for the diagnostic.</param>
2829
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
2930
[Theory]
30-
[InlineData("int", "get; private set;", "Gets or sets", "get", "Gets")]
31-
[InlineData("int", "private get; set;", "Gets or sets", "set", "Sets")]
32-
[InlineData("bool", "get; private set;", "Gets or sets a value indicating whether", "get", "Gets a value indicating whether")]
33-
[InlineData("bool", "private get; set;", "Gets or sets a value indicating whether", "set", "Sets a value indicating whether")]
34-
public async Task VerifyThatInvalidDocumentationWillReportDiagnosticAsync(string type, string accessors, string summaryPrefix, string expectedArgument1, string expectedArgument2)
31+
[InlineData("public", "int", "get; internal set;", "Gets or sets", "get", "Gets")]
32+
[InlineData("public", "int", "get; private set;", "Gets or sets", "get", "Gets")]
33+
[InlineData("public", "int", "internal get; set;", "Gets or sets", "set", "Sets")]
34+
[InlineData("public", "int", "private get; set;", "Gets or sets", "set", "Sets")]
35+
[InlineData("public", "bool", "get; private set;", "Gets or sets a value indicating whether", "get", "Gets a value indicating whether")]
36+
[InlineData("public", "bool", "private get; set;", "Gets or sets a value indicating whether", "set", "Sets a value indicating whether")]
37+
[InlineData("protected", "int", "get; private set;", "Gets or sets", "get", "Gets")]
38+
[InlineData("protected", "int", "private get; set;", "Gets or sets", "set", "Sets")]
39+
[InlineData("protected internal", "int", "get; internal set;", "Gets or sets", "get", "Gets")]
40+
[InlineData("protected internal", "int", "get; private set;", "Gets or sets", "get", "Gets")]
41+
[InlineData("protected internal", "int", "internal get; set;", "Gets or sets", "set", "Sets")]
42+
[InlineData("protected internal", "int", "private get; set;", "Gets or sets", "set", "Sets")]
43+
[InlineData("internal", "int", "get; private set;", "Gets or sets", "get", "Gets")]
44+
[InlineData("internal", "int", "private get; set;", "Gets or sets", "set", "Sets")]
45+
public async Task VerifyThatInvalidDocumentationWillReportDiagnosticAsync(string accessibility, string type, string accessors, string summaryPrefix, string expectedArgument1, string expectedArgument2)
3546
{
3647
var testCode = $@"
3748
public class TestClass
3849
{{
3950
/// <summary>
4051
/// {summaryPrefix} the test property.
4152
/// </summary>
42-
public {type} TestProperty
53+
{accessibility} {type} TestProperty
4354
{{
4455
{accessors}
4556
}}
@@ -52,14 +63,14 @@ public class TestClass
5263
/// <summary>
5364
/// {expectedArgument2} the test property.
5465
/// </summary>
55-
public {type} TestProperty
66+
{accessibility} {type} TestProperty
5667
{{
5768
{accessors}
5869
}}
5970
}}
6071
";
6172

62-
var expected = this.CSharpDiagnostic(PropertySummaryDocumentationAnalyzer.SA1624Descriptor).WithLocation(7, 13 + type.Length).WithArguments(expectedArgument1, expectedArgument2);
73+
var expected = this.CSharpDiagnostic(PropertySummaryDocumentationAnalyzer.SA1624Descriptor).WithLocation(7, 7 + accessibility.Length + type.Length).WithArguments(expectedArgument1, expectedArgument2);
6374

6475
await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
6576
await this.VerifyCSharpDiagnosticAsync(fixedTestCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);

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

Lines changed: 40 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -113,10 +113,46 @@ private static void AnalyzeSummaryElement(SyntaxNodeAnalysisContext context, Xml
113113

114114
if (setter != null)
115115
{
116-
var getterIsVisible = IsPubliclyVisible(propertyDeclaration, getter);
117-
var setterIsVisible = IsPubliclyVisible(propertyDeclaration, setter);
116+
var getterAccessibility = getter.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
117+
var setterAccessibility = setter.GetEffectiveAccessibility(context.SemanticModel, context.CancellationToken);
118+
bool getterVisible;
119+
bool setterVisible;
118120

119-
if (getterIsVisible && !setterIsVisible)
121+
switch (getterAccessibility)
122+
{
123+
case Accessibility.Public:
124+
case Accessibility.ProtectedOrInternal:
125+
case Accessibility.Protected:
126+
getterVisible = true;
127+
break;
128+
129+
case Accessibility.Internal:
130+
getterVisible = setterAccessibility == Accessibility.Private;
131+
break;
132+
133+
default:
134+
getterVisible = false;
135+
break;
136+
}
137+
138+
switch (setterAccessibility)
139+
{
140+
case Accessibility.Public:
141+
case Accessibility.ProtectedOrInternal:
142+
case Accessibility.Protected:
143+
setterVisible = true;
144+
break;
145+
146+
case Accessibility.Internal:
147+
setterVisible = getterAccessibility == Accessibility.Private;
148+
break;
149+
150+
default:
151+
setterVisible = false;
152+
break;
153+
}
154+
155+
if (getterVisible && !setterVisible)
120156
{
121157
if (startsWithGetOrSet)
122158
{
@@ -130,7 +166,7 @@ private static void AnalyzeSummaryElement(SyntaxNodeAnalysisContext context, Xml
130166
context.ReportDiagnostic(Diagnostic.Create(SA1623Descriptor, diagnosticLocation, diagnosticProperties.ToImmutable(), startingTextGets));
131167
}
132168
}
133-
else if (!getterIsVisible && setterIsVisible)
169+
else if (!getterVisible && setterVisible)
134170
{
135171
if (startsWithGetOrSet)
136172
{
@@ -171,45 +207,5 @@ private static void AnalyzeSummaryElement(SyntaxNodeAnalysisContext context, Xml
171207
}
172208
}
173209
}
174-
175-
private static bool IsPubliclyVisible(PropertyDeclarationSyntax propertyDeclaration, AccessorDeclarationSyntax accessor)
176-
{
177-
if (accessor == null)
178-
{
179-
return false;
180-
}
181-
182-
if (accessor.Modifiers.Any(SyntaxKind.PrivateKeyword))
183-
{
184-
return false;
185-
}
186-
187-
var current = accessor.Parent;
188-
while (current != null)
189-
{
190-
switch (current.Kind())
191-
{
192-
case SyntaxKind.ClassDeclaration:
193-
if (((ClassDeclarationSyntax)current).Modifiers.Any(SyntaxKind.PrivateKeyword))
194-
{
195-
return false;
196-
}
197-
198-
break;
199-
200-
case SyntaxKind.StructDeclaration:
201-
if (((StructDeclarationSyntax)current).Modifiers.Any(SyntaxKind.PrivateKeyword))
202-
{
203-
return false;
204-
}
205-
206-
break;
207-
}
208-
209-
current = current.Parent;
210-
}
211-
212-
return true;
213-
}
214210
}
215211
}

‎StyleCop.Analyzers/StyleCop.Analyzers/Helpers/AccessLevelHelper.cs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,21 @@ internal static Accessibility GetDeclaredAccessibility(this BasePropertyDeclarat
234234
return declaredSymbol?.DeclaredAccessibility ?? Accessibility.NotApplicable;
235235
}
236236

237+
internal static Accessibility GetDeclaredAccessibility(this AccessorDeclarationSyntax syntax, SemanticModel semanticModel, CancellationToken cancellationToken)
238+
{
239+
Requires.NotNull(syntax, nameof(syntax));
240+
Requires.NotNull(semanticModel, nameof(semanticModel));
241+
242+
AccessLevel accessLevel = GetAccessLevel(syntax.Modifiers);
243+
if (accessLevel != AccessLevel.NotSpecified)
244+
{
245+
return accessLevel.ToAccessibility();
246+
}
247+
248+
ISymbol declaredSymbol = semanticModel.GetDeclaredSymbol(syntax, cancellationToken);
249+
return declaredSymbol?.DeclaredAccessibility ?? Accessibility.NotApplicable;
250+
}
251+
237252
internal static Accessibility GetDeclaredAccessibility(this BaseFieldDeclarationSyntax syntax, SemanticModel semanticModel, CancellationToken cancellationToken)
238253
{
239254
Requires.NotNull(syntax, nameof(syntax));
@@ -343,6 +358,27 @@ internal static Accessibility GetEffectiveAccessibility(this BasePropertyDeclara
343358
return CombineEffectiveAccessibility(declaredAccessibility, enclosingAccessibility);
344359
}
345360

361+
internal static Accessibility GetEffectiveAccessibility(this AccessorDeclarationSyntax syntax, SemanticModel semanticModel, CancellationToken cancellationToken)
362+
{
363+
Requires.NotNull(syntax, nameof(syntax));
364+
Requires.NotNull(semanticModel, nameof(semanticModel));
365+
366+
Accessibility declaredAccessibility = syntax.GetDeclaredAccessibility(semanticModel, cancellationToken);
367+
if (declaredAccessibility <= Accessibility.Private)
368+
{
369+
return declaredAccessibility;
370+
}
371+
372+
BasePropertyDeclarationSyntax enclosingProperty = syntax.Parent.Parent as BasePropertyDeclarationSyntax;
373+
if (enclosingProperty == null)
374+
{
375+
return declaredAccessibility;
376+
}
377+
378+
Accessibility enclosingAccessibility = enclosingProperty.GetEffectiveAccessibility(semanticModel, cancellationToken);
379+
return CombineEffectiveAccessibility(declaredAccessibility, enclosingAccessibility);
380+
}
381+
346382
internal static Accessibility GetEffectiveAccessibility(this BaseFieldDeclarationSyntax syntax, SemanticModel semanticModel, CancellationToken cancellationToken)
347383
{
348384
Requires.NotNull(syntax, nameof(syntax));

0 commit comments

Comments
 (0)