Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SA1308CodeFixProvider creating empty identifier and add regression test #2338

Merged
merged 6 commits into from
Jun 20, 2017
Merged

Fix SA1308CodeFixProvider creating empty identifier and add regression test #2338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1e86fc9
Fix logic in SA1308CodeFixProvider
jamesqo Apr 10, 2017
8b3beda
Add regression tests for changes, improve existing ones
jamesqo Apr 10, 2017
f2e3a7a
Fix compiler errors
jamesqo Apr 10, 2017
3620d2c
Unescape prefixes before handing to WithArguments
jamesqo Apr 10, 2017
1c15322
Escaped too much
jamesqo Apr 10, 2017
b7e8a57
[PR feedback] Remove dead branch
jamesqo Jun 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 13 additions & 16 deletions StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/SA1308CodeFixProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,18 +42,12 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var token = root.FindToken(diagnostic.Location.SourceSpan.Start);

// 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.
if (token.ValueText.Length <= 2)
{
continue;
}

var numberOfCharsToRemove = 2;
var numberOfCharsToRemove = 0;

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

if (!string.IsNullOrEmpty(token.ValueText))
// 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.
if (token.ValueText.Length == numberOfCharsToRemove)
{
var newName = token.ValueText.Substring(numberOfCharsToRemove);
context.RegisterCodeFix(
CodeAction.Create(
string.Format(NamingResources.RenameToCodeFix, newName),
cancellationToken => RenameHelper.RenameSymbolAsync(document, root, token, newName, cancellationToken),
nameof(SA1308CodeFixProvider)),
diagnostic);
continue;
}

var newName = token.ValueText.Substring(numberOfCharsToRemove);
context.RegisterCodeFix(
CodeAction.Create(
string.Format(NamingResources.RenameToCodeFix, newName),
cancellationToken => RenameHelper.RenameSymbolAsync(document, root, token, newName, cancellationToken),
nameof(SA1308CodeFixProvider)),
diagnostic);
}
}
}
Expand Down
110 changes: 88 additions & 22 deletions StyleCop.Analyzers/StyleCop.Analyzers.Test/NamingRules/SA1308UnitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,26 @@ namespace StyleCop.Analyzers.Test.NamingRules

public class SA1308UnitTests : CodeFixVerifier
{
private const string UnderscoreEscapeSequence = @"\u005F";

private readonly string[] modifiers = new[] { "public", "private", "protected", "public readonly", "internal readonly", "public static", "private static" };

public static IEnumerable<object[]> PrefixesData()
{
yield return new object[] { "m_" };
yield return new object[] { "s_" };
yield return new object[] { "t_" };
yield return new object[] { $"m{UnderscoreEscapeSequence}" };
yield return new object[] { $"s{UnderscoreEscapeSequence}" };
yield return new object[] { $"t{UnderscoreEscapeSequence}" };
}

public static IEnumerable<object[]> MultipleDistinctPrefixesData()
{
yield return new object[] { "m_t_s_", "m_" };
yield return new object[] { $"s{UnderscoreEscapeSequence}m{UnderscoreEscapeSequence}t{UnderscoreEscapeSequence}", "s_" };
}

[Fact]
public async Task TestFieldStartingWithPrefixesToTriggerDiagnosticAsync()
{
Expand All @@ -24,9 +42,9 @@ public async Task TestFieldStartingWithPrefixesToTriggerDiagnosticAsync()
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "m_", "m_").ConfigureAwait(false);
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "s_", "s_").ConfigureAwait(false);
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "t_", "t_").ConfigureAwait(false);
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "m\\u005F", "m_").ConfigureAwait(false);
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "s\\u005F", "s_").ConfigureAwait(false);
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, "t\\u005F", "t_").ConfigureAwait(false);
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, $"m{UnderscoreEscapeSequence}", "m_").ConfigureAwait(false);
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, $"s{UnderscoreEscapeSequence}", "s_").ConfigureAwait(false);
await this.TestFieldSpecifyingModifierAndPrefixAsync(modifier, $"t{UnderscoreEscapeSequence}", "t_").ConfigureAwait(false);
}
}

Expand Down Expand Up @@ -71,56 +89,102 @@ public async Task TestFieldInsideNativeMethodsClassAsync()
/// <summary>
/// This is a regression test for DotNetAnalyzers/StyleCopAnalyzers#627.
/// </summary>
/// <param name="prefix">The prefix to repeat in the variable name.</param>
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
/// <seealso href="https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/627">#627: Code Fixes For Naming
/// Rules SA1308 and SA1309 Do Not Always Fix The Name Entirely</seealso>
[Fact]
public async Task TestFixingMultipleIdenticalPrefixesAsync()
[Theory]
[MemberData(nameof(PrefixesData))]
public async Task TestFixingMultipleIdenticalPrefixesAsync(string prefix)
{
var testCode = @"public class Foo
{
private string m_m_bar = ""baz"";
}";
var testCode = $@"public class Foo
{{
private string {prefix}{prefix}bar = ""baz"";
}}";

string diagnosticPrefix = UnescapeUnderscores(prefix);
DiagnosticResult expected =
this.CSharpDiagnostic()
.WithArguments("m_m_bar", "m_")
.WithArguments($"{diagnosticPrefix}{diagnosticPrefix}bar", diagnosticPrefix)
.WithLocation(3, 20);

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

var fixedCode = testCode.Replace("m_", string.Empty);
var fixedCode = testCode.Replace(prefix, string.Empty);
await this.VerifyCSharpFixAsync(testCode, fixedCode).ConfigureAwait(false);
}

[Theory]
[MemberData(nameof(PrefixesData))]
public async Task TestMultipleIdenticalPrefixesOnlyAsync(string prefix)
{
var testCode = $@"public class Foo
{{
private string {prefix}{prefix} = ""baz"";
}}";

string diagnosticPrefix = UnescapeUnderscores(prefix);
DiagnosticResult expected =
this.CSharpDiagnostic()
.WithArguments($"{diagnosticPrefix}{diagnosticPrefix}", diagnosticPrefix)
.WithLocation(3, 20);

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

// A code fix is not offered as removing the prefixes would create an empty identifier.
await this.VerifyCSharpFixAsync(testCode, testCode).ConfigureAwait(false);
}

/// <summary>
/// This is a regression test for DotNetAnalyzers/StyleCopAnalyzers#627.
/// </summary>
/// <param name="prefixes">The prefixes to prepend to the variable name.</param>
/// <param name="diagnosticPrefix">The prefix that should be reported in the diagnostic.</param>
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
/// <seealso href="https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/627">#627: Code Fixes For Naming
/// Rules SA1308 and SA1309 Do Not Always Fix The Name Entirely</seealso>
[Fact]
public async Task TestFixingMultipleIndependentPrefixesAsync()
[Theory]
[MemberData(nameof(MultipleDistinctPrefixesData))]
public async Task TestFixingMultipleDistinctPrefixesAsync(string prefixes, string diagnosticPrefix)
{
var testCode = @"public class Foo
{
private string m_t_s_bar = ""baz"";
}";
var testCode = $@"public class Foo
{{
private string {prefixes}bar = ""baz"";
}}";

string diagnosticPrefixes = UnescapeUnderscores(prefixes);
DiagnosticResult expected =
this.CSharpDiagnostic()
.WithArguments("m_t_s_bar", "m_")
.WithArguments($"{diagnosticPrefixes}bar", diagnosticPrefix)
.WithLocation(3, 20);

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

var fixedCode = testCode.Replace("m_", string.Empty);
fixedCode = fixedCode.Replace("s_", string.Empty);
fixedCode = fixedCode.Replace("t_", string.Empty);

var fixedCode = testCode.Replace(prefixes, string.Empty);
await this.VerifyCSharpFixAsync(testCode, fixedCode).ConfigureAwait(false);
}

[Theory]
[MemberData(nameof(MultipleDistinctPrefixesData))]
public async Task TestMultipleDistinctPrefixesOnlyAsync(string prefixes, string diagnosticPrefix)
{
var testCode = $@"public class Foo
{{
private string {prefixes} = ""baz"";
}}";

string diagnosticPrefixes = UnescapeUnderscores(prefixes);
DiagnosticResult expected =
this.CSharpDiagnostic()
.WithArguments(diagnosticPrefixes, diagnosticPrefix)
.WithLocation(3, 20);

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

// A code fix is not offered as removing the prefixes would create an empty identifier.
await this.VerifyCSharpFixAsync(testCode, testCode).ConfigureAwait(false);
}

protected override IEnumerable<DiagnosticAnalyzer> GetCSharpDiagnosticAnalyzers()
{
yield return new SA1308VariableNamesMustNotBePrefixed();
Expand All @@ -131,6 +195,8 @@ protected override CodeFixProvider GetCSharpCodeFixProvider()
return new SA1308CodeFixProvider();
}

private static string UnescapeUnderscores(string identifier) => identifier.Replace(UnderscoreEscapeSequence, "_");

private async Task TestFieldSpecifyingModifierAndPrefixAsync(string modifier, string codePrefix, string diagnosticPrefix)
{
var originalCode = @"public class Foo
Expand Down