Skip to content

Commit 1f8c520

Browse files
sharwellsharwell
committed
Merge pull request #2338 from jamesqo/sa1308
Fix SA1308CodeFixProvider creating empty identifier and add regression test
2 parents f2d2cc3 + b7e8a57 commit 1f8c520

2 files changed

Lines changed: 101 additions & 38 deletions

File tree

‎StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/SA1308CodeFixProvider.cs‎

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -42,18 +42,12 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
4242
{
4343
var token = root.FindToken(diagnostic.Location.SourceSpan.Start);
4444

45-
// The variable name is the full suffix. In this case we cannot generate a valid variable name and thus will not offer a code fix.
46-
if (token.ValueText.Length <= 2)
47-
{
48-
continue;
49-
}
50-
51-
var numberOfCharsToRemove = 2;
45+
var numberOfCharsToRemove = 0;
5246

5347
// If a variable contains multiple prefixes that would result in this diagnostic,
5448
// we detect that and remove all of the bad prefixes such that after
5549
// the fix is applied there are no more violations of this rule.
56-
for (int i = 2; i < token.ValueText.Length; i += 2)
50+
for (int i = 0; i < token.ValueText.Length; i += 2)
5751
{
5852
if (string.Compare("m_", 0, token.ValueText, i, 2, StringComparison.Ordinal) == 0
5953
|| string.Compare("s_", 0, token.ValueText, i, 2, StringComparison.Ordinal) == 0
@@ -66,16 +60,19 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
6660
break;
6761
}
6862

69-
if (!string.IsNullOrEmpty(token.ValueText))
63+
// The prefix is the full variable name. In this case we cannot generate a valid variable name and thus will not offer a code fix.
64+
if (token.ValueText.Length == numberOfCharsToRemove)
7065
{
71-
var newName = token.ValueText.Substring(numberOfCharsToRemove);
72-
context.RegisterCodeFix(
73-
CodeAction.Create(
74-
string.Format(NamingResources.RenameToCodeFix, newName),
75-
cancellationToken => RenameHelper.RenameSymbolAsync(document, root, token, newName, cancellationToken),
76-
nameof(SA1308CodeFixProvider)),
77-
diagnostic);
66+
continue;
7867
}
68+
69+
var newName = token.ValueText.Substring(numberOfCharsToRemove);
70+
context.RegisterCodeFix(
71+
CodeAction.Create(
72+
string.Format(NamingResources.RenameToCodeFix, newName),
73+
cancellationToken => RenameHelper.RenameSymbolAsync(document, root, token, newName, cancellationToken),
74+
nameof(SA1308CodeFixProvider)),
75+
diagnostic);
7976
}
8077
}
8178
}

‎StyleCop.Analyzers/StyleCop.Analyzers.Test/NamingRules/SA1308UnitTests.cs‎

Lines changed: 88 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,26 @@ namespace StyleCop.Analyzers.Test.NamingRules
1414

1515
public class SA1308UnitTests : CodeFixVerifier
1616
{
17+
private const string UnderscoreEscapeSequence = @"\u005F";
18+
1719
private readonly string[] modifiers = new[] { "public", "private", "protected", "public readonly", "internal readonly", "public static", "private static" };
1820

21+
public static IEnumerable<object[]> PrefixesData()
22+
{
23+
yield return new object[] { "m_" };
24+
yield return new object[] { "s_" };
25+
yield return new object[] { "t_" };
26+
yield return new object[] { $"m{UnderscoreEscapeSequence}" };
27+
yield return new object[] { $"s{UnderscoreEscapeSequence}" };
28+
yield return new object[] { $"t{UnderscoreEscapeSequence}" };
29+
}
30+
31+
public static IEnumerable<object[]> MultipleDistinctPrefixesData()
32+
{
33+
yield return new object[] { "m_t_s_", "m_" };
34+
yield return new object[] { $"s{UnderscoreEscapeSequence}m{UnderscoreEscapeSequence}t{UnderscoreEscapeSequence}", "s_" };
35+
}
36+
1937
[Fact]
2038
public async Task TestFieldStartingWithPrefixesToTriggerDiagnosticAsync()
2139
{
@@ -24,9 +42,9 @@ public async Task TestFieldStartingWithPrefixesToTriggerDiagnosticAsync()
2442
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "m_", "m_").ConfigureAwait(false);
2543
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "s_", "s_").ConfigureAwait(false);
2644
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "t_", "t_").ConfigureAwait(false);
27-
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "m\\u005F", "m_").ConfigureAwait(false);
28-
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "s\\u005F", "s_").ConfigureAwait(false);
29-
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "t\\u005F", "t_").ConfigureAwait(false);
45+
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, $"m{UnderscoreEscapeSequence}", "m_").ConfigureAwait(false);
46+
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, $"s{UnderscoreEscapeSequence}", "s_").ConfigureAwait(false);
47+
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, $"t{UnderscoreEscapeSequence}", "t_").ConfigureAwait(false);
3048
}
3149
}
3250

@@ -71,56 +89,102 @@ public async Task TestFieldInsideNativeMethodsClassAsync()
7189
/// <summary>
7290
/// This is a regression test for DotNetAnalyzers/StyleCopAnalyzers#627.
7391
/// </summary>
92+
/// <param name="prefix">The prefix to repeat in the variable name.</param>
7493
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
7594
/// <seealso href="https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/627">#627: Code Fixes For Naming
7695
/// Rules SA1308 and SA1309 Do Not Always Fix The Name Entirely</seealso>
77-
[Fact]
78-
public async Task TestFixingMultipleIdenticalPrefixesAsync()
96+
[Theory]
97+
[MemberData(nameof(PrefixesData))]
98+
public async Task TestFixingMultipleIdenticalPrefixesAsync(string prefix)
7999
{
80-
var testCode = @"public class Foo
81-
{
82-
private string m_m_bar = ""baz"";
83-
}";
100+
var testCode = $@"public class Foo
101+
{{
102+
private string {prefix}{prefix}bar = ""baz"";
103+
}}";
84104

105+
string diagnosticPrefix = UnescapeUnderscores(prefix);
85106
DiagnosticResult expected =
86107
this.CSharpDiagnostic()
87-
.WithArguments("m_m_bar", "m_")
108+
.WithArguments($"{diagnosticPrefix}{diagnosticPrefix}bar", diagnosticPrefix)
88109
.WithLocation(3, 20);
89110

90111
await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
91112

92-
var fixedCode = testCode.Replace("m_", string.Empty);
113+
var fixedCode = testCode.Replace(prefix, string.Empty);
93114
await this.VerifyCSharpFixAsync(testCode, fixedCode).ConfigureAwait(false);
94115
}
95116

117+
[Theory]
118+
[MemberData(nameof(PrefixesData))]
119+
public async Task TestMultipleIdenticalPrefixesOnlyAsync(string prefix)
120+
{
121+
var testCode = $@"public class Foo
122+
{{
123+
private string {prefix}{prefix} = ""baz"";
124+
}}";
125+
126+
string diagnosticPrefix = UnescapeUnderscores(prefix);
127+
DiagnosticResult expected =
128+
this.CSharpDiagnostic()
129+
.WithArguments($"{diagnosticPrefix}{diagnosticPrefix}", diagnosticPrefix)
130+
.WithLocation(3, 20);
131+
132+
await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
133+
134+
// A code fix is not offered as removing the prefixes would create an empty identifier.
135+
await this.VerifyCSharpFixAsync(testCode, testCode).ConfigureAwait(false);
136+
}
137+
96138
/// <summary>
97139
/// This is a regression test for DotNetAnalyzers/StyleCopAnalyzers#627.
98140
/// </summary>
141+
/// <param name="prefixes">The prefixes to prepend to the variable name.</param>
142+
/// <param name="diagnosticPrefix">The prefix that should be reported in the diagnostic.</param>
99143
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
100144
/// <seealso href="https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/627">#627: Code Fixes For Naming
101145
/// Rules SA1308 and SA1309 Do Not Always Fix The Name Entirely</seealso>
102-
[Fact]
103-
public async Task TestFixingMultipleIndependentPrefixesAsync()
146+
[Theory]
147+
[MemberData(nameof(MultipleDistinctPrefixesData))]
148+
public async Task TestFixingMultipleDistinctPrefixesAsync(string prefixes, string diagnosticPrefix)
104149
{
105-
var testCode = @"public class Foo
106-
{
107-
private string m_t_s_bar = ""baz"";
108-
}";
150+
var testCode = $@"public class Foo
151+
{{
152+
private string {prefixes}bar = ""baz"";
153+
}}";
109154

155+
string diagnosticPrefixes = UnescapeUnderscores(prefixes);
110156
DiagnosticResult expected =
111157
this.CSharpDiagnostic()
112-
.WithArguments("m_t_s_bar", "m_")
158+
.WithArguments($"{diagnosticPrefixes}bar", diagnosticPrefix)
113159
.WithLocation(3, 20);
114160

115161
await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
116162

117-
var fixedCode = testCode.Replace("m_", string.Empty);
118-
fixedCode = fixedCode.Replace("s_", string.Empty);
119-
fixedCode = fixedCode.Replace("t_", string.Empty);
120-
163+
var fixedCode = testCode.Replace(prefixes, string.Empty);
121164
await this.VerifyCSharpFixAsync(testCode, fixedCode).ConfigureAwait(false);
122165
}
123166

167+
[Theory]
168+
[MemberData(nameof(MultipleDistinctPrefixesData))]
169+
public async Task TestMultipleDistinctPrefixesOnlyAsync(string prefixes, string diagnosticPrefix)
170+
{
171+
var testCode = $@"public class Foo
172+
{{
173+
private string {prefixes} = ""baz"";
174+
}}";
175+
176+
string diagnosticPrefixes = UnescapeUnderscores(prefixes);
177+
DiagnosticResult expected =
178+
this.CSharpDiagnostic()
179+
.WithArguments(diagnosticPrefixes, diagnosticPrefix)
180+
.WithLocation(3, 20);
181+
182+
await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false);
183+
184+
// A code fix is not offered as removing the prefixes would create an empty identifier.
185+
await this.VerifyCSharpFixAsync(testCode, testCode).ConfigureAwait(false);
186+
}
187+
124188
protected override IEnumerable<DiagnosticAnalyzer> GetCSharpDiagnosticAnalyzers()
125189
{
126190
yield return new SA1308VariableNamesMustNotBePrefixed();
@@ -131,6 +195,8 @@ protected override CodeFixProvider GetCSharpCodeFixProvider()
131195
return new SA1308CodeFixProvider();
132196
}
133197

198+
private static string UnescapeUnderscores(string identifier) => identifier.Replace(UnderscoreEscapeSequence, "_");
199+
134200
private async Task TestFieldSpecifyingModifierAndPrefixAsync(string modifier, string codePrefix, string diagnosticPrefix)
135201
{
136202
var originalCode = @"public class Foo

0 commit comments

Comments
 (0)