From 15bbb603c6c050f447d909fe7067e80b55222d54 Mon Sep 17 00:00:00 2001 From: Sam Harwell Date: Fri, 27 Nov 2015 23:42:41 -0600 Subject: [PATCH] Improve IsValidNewMemberName handling of parameters and locals Fixes #1315 --- .../Helpers/RenameHelper.cs | 208 ++++++++++++++++-- .../RenameToLowerCaseCodeFixProvider.cs | 50 +++-- .../RenameToUpperCaseCodeFixProvider.cs | 25 +-- .../NamingRules/SA1302CodeFixProvider.cs | 2 +- .../NamingRules/SA1312UnitTests.cs | 10 +- .../NamingRules/SA1313UnitTests.cs | 12 +- 6 files changed, 240 insertions(+), 67 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/Helpers/RenameHelper.cs b/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/Helpers/RenameHelper.cs index 8b2fb06e7..626fbbd05 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/Helpers/RenameHelper.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/Helpers/RenameHelper.cs @@ -4,10 +4,13 @@ namespace StyleCop.Analyzers.Helpers { using System.Collections.Immutable; + using System.Linq; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CodeActions; + using Microsoft.CodeAnalysis.CSharp; + using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Rename; internal static class RenameHelper @@ -30,7 +33,7 @@ public static async Task RenameSymbolAsync(Document document, SyntaxNo return newSolution; } - public static bool IsValidNewMemberName(SemanticModel semanticModel, ISymbol symbol, string name) + public static async Task IsValidNewMemberNameAsync(SemanticModel semanticModel, ISymbol symbol, string name, CancellationToken cancellationToken) { if (symbol.Kind == SymbolKind.NamedType) { @@ -47,39 +50,204 @@ public static bool IsValidNewMemberName(SemanticModel semanticModel, ISymbol sym } } - var containingSymbol = symbol.ContainingSymbol as INamespaceOrTypeSymbol; - if (containingSymbol == null) + var containingSymbol = symbol.ContainingSymbol; + + var containingNamespaceOrTypeSymbol = containingSymbol as INamespaceOrTypeSymbol; + if (containingNamespaceOrTypeSymbol != null) { + if (containingNamespaceOrTypeSymbol.Kind == SymbolKind.Namespace) + { + // Make sure to use the compilation namespace so interfaces in referenced assemblies are considered + containingNamespaceOrTypeSymbol = semanticModel.Compilation.GetCompilationNamespace((INamespaceSymbol)containingNamespaceOrTypeSymbol); + } + else if (containingNamespaceOrTypeSymbol.Kind == SymbolKind.NamedType) + { + TypeKind typeKind = ((INamedTypeSymbol)containingNamespaceOrTypeSymbol).TypeKind; + + // If the containing type is a class or struct, the name can't be the same as the name of the containing + // type. + if ((typeKind == TypeKind.Class || typeKind == TypeKind.Struct) + && containingNamespaceOrTypeSymbol.Name == name) + { + return false; + } + } + + // The name can't be the same as the name of an other member of the same type. At this point no special + // consideration is given to overloaded methods. + ImmutableArray siblings = containingNamespaceOrTypeSymbol.GetMembers(name); + if (!siblings.IsDefaultOrEmpty) + { + return false; + } + return true; } - - if (containingSymbol.Kind == SymbolKind.Namespace) + else if (containingSymbol.Kind == SymbolKind.Method) { - // Make sure to use the compilation namespace so interfaces in referenced assemblies are considered - containingSymbol = semanticModel.Compilation.GetCompilationNamespace((INamespaceSymbol)containingSymbol); + IMethodSymbol methodSymbol = (IMethodSymbol)containingSymbol; + if (methodSymbol.Parameters.Any(i => i.Name == name) + || methodSymbol.TypeParameters.Any(i => i.Name == name)) + { + return false; + } + + IMethodSymbol outermostMethod = methodSymbol; + while (outermostMethod.ContainingSymbol.Kind == SymbolKind.Method) + { + outermostMethod = (IMethodSymbol)outermostMethod.ContainingSymbol; + if (outermostMethod.Parameters.Any(i => i.Name == name) + || outermostMethod.TypeParameters.Any(i => i.Name == name)) + { + return false; + } + } + + foreach (var syntaxReference in outermostMethod.DeclaringSyntaxReferences) + { + SyntaxNode syntaxNode = await syntaxReference.GetSyntaxAsync(cancellationToken).ConfigureAwait(false); + LocalNameFinder localNameFinder = new LocalNameFinder(name); + localNameFinder.Visit(syntaxNode); + if (localNameFinder.Found) + { + return false; + } + } + + return true; } - else if (containingSymbol.Kind == SymbolKind.NamedType) + else { - TypeKind typeKind = ((INamedTypeSymbol)containingSymbol).TypeKind; + return true; + } + } - // If the containing type is a class or struct, the name can't be the same as the name of the containing - // type. - if ((typeKind == TypeKind.Class || typeKind == TypeKind.Struct) - && containingSymbol.Name == name) + public static SyntaxNode GetParentDeclaration(SyntaxToken token) + { + SyntaxNode parent = token.Parent; + + while (parent != null) + { + switch (parent.Kind()) { - return false; + case SyntaxKind.VariableDeclarator: + case SyntaxKind.Parameter: + case SyntaxKind.TypeParameter: + case SyntaxKind.CatchDeclaration: + case SyntaxKind.ExternAliasDirective: + case SyntaxKind.QueryContinuation: + case SyntaxKind.FromClause: + case SyntaxKind.LetClause: + case SyntaxKind.JoinClause: + case SyntaxKind.JoinIntoClause: + case SyntaxKind.ForEachStatement: + case SyntaxKind.UsingDirective: + case SyntaxKind.LabeledStatement: + case SyntaxKind.AnonymousObjectMemberDeclarator: + return parent; + + default: + var declarationParent = parent as MemberDeclarationSyntax; + if (declarationParent != null) + { + return declarationParent; + } + + break; } + + parent = parent.Parent; + } + + return null; + } + + private class LocalNameFinder : CSharpSyntaxWalker + { + private readonly string name; + + public LocalNameFinder(string name) + { + this.name = name; + } + + public bool Found + { + get; + private set; } - // The name can't be the same as the name of an other member of the same type. At this point no special - // consideration is given to overloaded methods. - ImmutableArray siblings = containingSymbol.GetMembers(name); - if (!siblings.IsDefaultOrEmpty) + public override void VisitVariableDeclarator(VariableDeclaratorSyntax node) { - return false; + this.Found |= node.Identifier.ValueText == this.name; + base.VisitVariableDeclarator(node); } - return true; + public override void VisitParameter(ParameterSyntax node) + { + this.Found |= node.Identifier.ValueText == this.name; + base.VisitParameter(node); + } + + public override void VisitTypeParameter(TypeParameterSyntax node) + { + this.Found |= node.Identifier.ValueText == this.name; + base.VisitTypeParameter(node); + } + + public override void VisitCatchDeclaration(CatchDeclarationSyntax node) + { + this.Found |= node.Identifier.ValueText == this.name; + base.VisitCatchDeclaration(node); + } + + public override void VisitQueryContinuation(QueryContinuationSyntax node) + { + this.Found |= node.Identifier.ValueText == this.name; + base.VisitQueryContinuation(node); + } + + public override void VisitFromClause(FromClauseSyntax node) + { + this.Found |= node.Identifier.ValueText == this.name; + base.VisitFromClause(node); + } + + public override void VisitLetClause(LetClauseSyntax node) + { + this.Found |= node.Identifier.ValueText == this.name; + base.VisitLetClause(node); + } + + public override void VisitJoinClause(JoinClauseSyntax node) + { + this.Found |= node.Identifier.ValueText == this.name; + base.VisitJoinClause(node); + } + + public override void VisitJoinIntoClause(JoinIntoClauseSyntax node) + { + this.Found |= node.Identifier.ValueText == this.name; + base.VisitJoinIntoClause(node); + } + + public override void VisitForEachStatement(ForEachStatementSyntax node) + { + this.Found |= node.Identifier.ValueText == this.name; + base.VisitForEachStatement(node); + } + + public override void VisitLabeledStatement(LabeledStatementSyntax node) + { + this.Found |= node.Identifier.ValueText == this.name; + base.VisitLabeledStatement(node); + } + + public override void VisitAnonymousObjectMemberDeclarator(AnonymousObjectMemberDeclaratorSyntax node) + { + this.Found |= node.NameEquals?.Name?.Identifier.ValueText == this.name; + base.VisitAnonymousObjectMemberDeclarator(node); + } } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/RenameToLowerCaseCodeFixProvider.cs b/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/RenameToLowerCaseCodeFixProvider.cs index 0bf51be6f..9040e87d2 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/RenameToLowerCaseCodeFixProvider.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/RenameToLowerCaseCodeFixProvider.cs @@ -10,6 +10,7 @@ namespace StyleCop.Analyzers.NamingRules using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CodeActions; using Microsoft.CodeAnalysis.CodeFixes; + using Microsoft.CodeAnalysis.CSharp; /// /// Implements a code fix for diagnostics which are fixed by renaming a symbol to start with a lower case letter. @@ -44,22 +45,45 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) foreach (var diagnostic in context.Diagnostics) { var token = root.FindToken(diagnostic.Location.SourceSpan.Start); - if (!string.IsNullOrEmpty(token.ValueText)) + if (string.IsNullOrEmpty(token.ValueText)) { - var newName = token.ValueText.TrimStart('_'); + continue; + } - // only offer a codefix if the name does not consist of only underscores. - if (!string.IsNullOrEmpty(newName)) - { - newName = char.ToLower(newName[0]) + newName.Substring(1); - context.RegisterCodeFix( - CodeAction.Create( - string.Format(NamingResources.RenameToCodeFix, newName), - cancellationToken => RenameHelper.RenameSymbolAsync(document, root, token, newName, cancellationToken), - nameof(RenameToLowerCaseCodeFixProvider)), - diagnostic); - } + var originalName = token.ValueText; + var baseName = originalName.TrimStart('_'); + if (baseName.Length == 0) + { + // only offer a code fix if the name does not consist of only underscores. + continue; } + + baseName = char.ToLower(baseName[0]) + baseName.Substring(1); + int underscoreCount = originalName.Length - baseName.Length; + var newName = baseName; + var memberSyntax = RenameHelper.GetParentDeclaration(token); + + SemanticModel semanticModel = await document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); + + var declaredSymbol = semanticModel.GetDeclaredSymbol(memberSyntax); + if (declaredSymbol == null) + { + continue; + } + + int index = 0; + while (!await RenameHelper.IsValidNewMemberNameAsync(semanticModel, declaredSymbol, newName, context.CancellationToken).ConfigureAwait(false)) + { + index++; + newName = baseName + index; + } + + context.RegisterCodeFix( + CodeAction.Create( + string.Format(NamingResources.RenameToCodeFix, newName), + cancellationToken => RenameHelper.RenameSymbolAsync(document, root, token, newName, cancellationToken), + nameof(RenameToLowerCaseCodeFixProvider) + "_" + underscoreCount + "_" + index), + diagnostic); } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/RenameToUpperCaseCodeFixProvider.cs b/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/RenameToUpperCaseCodeFixProvider.cs index 03c37bb0c..122438fd1 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/RenameToUpperCaseCodeFixProvider.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/RenameToUpperCaseCodeFixProvider.cs @@ -57,7 +57,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) var token = root.FindToken(diagnostic.Location.SourceSpan.Start); var baseName = char.ToUpper(token.ValueText[0]) + token.ValueText.Substring(1); var newName = baseName; - var memberSyntax = GetParentTypeDeclaration(token); + var memberSyntax = RenameHelper.GetParentDeclaration(token); if (memberSyntax is NamespaceDeclarationSyntax) { @@ -90,14 +90,14 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) } bool usedSuffix = false; - if (declaredSymbol.Kind == SymbolKind.Field && !RenameHelper.IsValidNewMemberName(semanticModel, declaredSymbol, newName)) + if (declaredSymbol.Kind == SymbolKind.Field && !await RenameHelper.IsValidNewMemberNameAsync(semanticModel, declaredSymbol, newName, context.CancellationToken).ConfigureAwait(false)) { usedSuffix = true; newName = newName + Suffix; } int index = 0; - while (!RenameHelper.IsValidNewMemberName(semanticModel, declaredSymbol, newName)) + while (!await RenameHelper.IsValidNewMemberNameAsync(semanticModel, declaredSymbol, newName, context.CancellationToken).ConfigureAwait(false)) { usedSuffix = false; index++; @@ -113,24 +113,5 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) } } } - - private static SyntaxNode GetParentTypeDeclaration(SyntaxToken token) - { - SyntaxNode parent = token.Parent; - - while (parent != null) - { - var declarationParent = parent as MemberDeclarationSyntax - ?? (SyntaxNode)(parent as VariableDeclaratorSyntax); - if (declarationParent != null) - { - return declarationParent; - } - - parent = parent.Parent; - } - - return null; - } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/SA1302CodeFixProvider.cs b/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/SA1302CodeFixProvider.cs index 9ce831239..4053a6276 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/SA1302CodeFixProvider.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.CodeFixes/NamingRules/SA1302CodeFixProvider.cs @@ -54,7 +54,7 @@ private static async Task CreateChangedSolutionAsync(Document document var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); var declaredSymbol = semanticModel.GetDeclaredSymbol(token.Parent, cancellationToken); - while (!RenameHelper.IsValidNewMemberName(semanticModel, declaredSymbol, newName)) + while (!await RenameHelper.IsValidNewMemberNameAsync(semanticModel, declaredSymbol, newName, cancellationToken).ConfigureAwait(false)) { index++; newName = baseName + index; diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/NamingRules/SA1312UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/NamingRules/SA1312UnitTests.cs index 1e2a477b6..9cbdcb66a 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/NamingRules/SA1312UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/NamingRules/SA1312UnitTests.cs @@ -191,7 +191,7 @@ public void MethodName() await this.VerifyCSharpDiagnosticAsync(testCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } - [Fact(Skip = "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/1315")] + [Fact] public async Task TestRenameConflictsWithVariableAsync() { var testCode = @"public class TypeName @@ -203,7 +203,7 @@ public void MethodName() } }"; - DiagnosticResult expected = this.CSharpDiagnostic().WithArguments("Variable").WithLocation(5, 16); + DiagnosticResult expected = this.CSharpDiagnostic().WithArguments("Variable").WithLocation(6, 16); await this.VerifyCSharpDiagnosticAsync(testCode, expected, CancellationToken.None).ConfigureAwait(false); @@ -212,7 +212,7 @@ public void MethodName() public void MethodName() { string variable = ""Text""; - string variableValue = variable.ToString(); + string variable1 = variable.ToString(); } }"; @@ -247,7 +247,7 @@ public void MethodName() await this.VerifyCSharpFixAsync(testCode, fixedCode).ConfigureAwait(false); } - [Fact(Skip = "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/1315")] + [Fact] public async Task TestRenameConflictsWithParameterAsync() { var testCode = @"public class TypeName @@ -266,7 +266,7 @@ public void MethodName(int parameter) { public void MethodName(int parameter) { - string parameterValue = parameter.ToString(); + string parameter1 = parameter.ToString(); } }"; diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/NamingRules/SA1313UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/NamingRules/SA1313UnitTests.cs index ed3fb2097..1b64b1707 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/NamingRules/SA1313UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/NamingRules/SA1313UnitTests.cs @@ -140,7 +140,7 @@ public void MethodName(string Bar) await this.VerifyCSharpDiagnosticAsync(testCode, EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false); } - [Fact(Skip = "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/1315")] + [Fact] public async Task TestRenameConflictsWithVariableAsync() { var testCode = @"public class TypeName @@ -157,7 +157,7 @@ public void MethodName(string Parameter) var fixedCode = @"public class TypeName { - public void MethodName(string parameterValue) + public void MethodName(string parameter1) { string parameter = ""Text""; } @@ -192,7 +192,7 @@ public void MethodName(string @int) await this.VerifyCSharpFixAsync(testCode, fixedCode).ConfigureAwait(false); } - [Fact(Skip = "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/1315")] + [Fact] public async Task TestRenameConflictsWithLaterParameterAsync() { var testCode = @"public class TypeName @@ -208,7 +208,7 @@ public void MethodName(string Parameter, int parameter) var fixedCode = @"public class TypeName { - public void MethodName(string parameterValue, int parameter) + public void MethodName(string parameter1, int parameter) { } }"; @@ -217,7 +217,7 @@ public void MethodName(string parameterValue, int parameter) await this.VerifyCSharpFixAsync(testCode, fixedCode).ConfigureAwait(false); } - [Fact(Skip = "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/1315")] + [Fact] public async Task TestRenameConflictsWithEarlierParameterAsync() { var testCode = @"public class TypeName @@ -233,7 +233,7 @@ public void MethodName(string parameter, int Parameter) var fixedCode = @"public class TypeName { - public void MethodName(string parameter, int parameterValue) + public void MethodName(string parameter, int parameter1) { } }";