Merge pull request #4371 from BZngr/4349_RenameClash

Enhances refactor/rename name scope clash validation

Author: retailcoder

Rubberduck.CodeAnalysis/QuickFixes/AssignedByValParameterMakeLocalCopyQuickFix.cs

@@ -5,7 +5,6 @@
 using Rubberduck.Parsing.Symbols;
 using Rubberduck.UI.Refactorings;
 using Rubberduck.Common;
-using System.Collections.Generic;
 using System.Windows.Forms;
 using Antlr4.Runtime;
 using Rubberduck.Inspections.Abstract;
@@ -21,6 +20,7 @@ public sealed class AssignedByValParameterMakeLocalCopyQuickFix : QuickFixBase
     {
         private readonly IAssignedByValParameterQuickFixDialogFactory _dialogFactory;
         private readonly RubberduckParserState _parserState;
+        private Declaration _quickFixTarget;
 
         public AssignedByValParameterMakeLocalCopyQuickFix(RubberduckParserState state, IAssignedByValParameterQuickFixDialogFactory dialogFactory)
             : base(typeof(AssignedByValParameterInspection))
@@ -34,9 +34,9 @@ public override void Fix(IInspectionResult result)
             Debug.Assert(result.Target.Context.Parent is VBAParser.ArgListContext);
             Debug.Assert(null != ((ParserRuleContext)result.Target.Context.Parent.Parent).GetChild<VBAParser.EndOfStatementContext>());
 
-            var forbiddenNames = _parserState.DeclarationFinder.GetDeclarationsWithIdentifiersToAvoid(result.Target).Select(n => n.IdentifierName);
+            _quickFixTarget = result.Target;
 
-            var localIdentifier = PromptForLocalVariableName(result.Target, forbiddenNames.ToList());
+            var localIdentifier = PromptForLocalVariableName(result.Target);
             if (string.IsNullOrEmpty(localIdentifier))
             {
                 return;
@@ -53,16 +53,16 @@ public override void Fix(IInspectionResult result)
         public override bool CanFixInModule => false;
         public override bool CanFixInProject => false;
 
-        private string PromptForLocalVariableName(Declaration target, List<string> forbiddenNames)
+        private string PromptForLocalVariableName(Declaration target)
         {
             IAssignedByValParameterQuickFixDialog view = null;
             try
             {
-                view = _dialogFactory.Create(target.IdentifierName, target.DeclarationType.ToString(), forbiddenNames);
-                view.NewName = GetDefaultLocalIdentifier(target, forbiddenNames);
+                view = _dialogFactory.Create(target.IdentifierName, target.DeclarationType.ToString(), IsNameCollision);
+                view.NewName = GetDefaultLocalIdentifier(target);
                 view.ShowDialog();
 
-                if (view.DialogResult == DialogResult.Cancel || !IsValidVariableName(view.NewName, forbiddenNames))
+                if (view.DialogResult == DialogResult.Cancel || !IsValidVariableName(view.NewName))
                 {
                     return string.Empty;
                 }
@@ -75,29 +75,32 @@ private string PromptForLocalVariableName(Declaration target, List<string> forbi
             }
         }
 
-        private string GetDefaultLocalIdentifier(Declaration target, List<string> forbiddenNames)
+        private bool IsNameCollision(string newName)
+            => _parserState.DeclarationFinder.FindNewDeclarationNameConflicts(newName, _quickFixTarget).Any();
+
+        private string GetDefaultLocalIdentifier(Declaration target)
         {
             var newName = $"local{target.IdentifierName.CapitalizeFirstLetter()}";
-            if (IsValidVariableName(newName, forbiddenNames))
+            if (IsValidVariableName(newName))
             {
                 return newName;
             }
 
             for ( var attempt = 2; attempt < 10; attempt++)
             {
                 var result = newName + attempt;
-                if (IsValidVariableName(result, forbiddenNames))
+                if (IsValidVariableName(result))
                 {
                     return result;
                 }
             }
             return newName;
         }
 
-        private bool IsValidVariableName(string variableName, IEnumerable<string> forbiddenNames)
+        private bool IsValidVariableName(string variableName)
         {
             return VariableNameValidator.IsValidName(variableName)
-                && !forbiddenNames.Any(name => name.Equals(variableName, StringComparison.InvariantCultureIgnoreCase));
+                && !IsNameCollision(variableName);
         }
 
         private void ReplaceAssignedByValParameterReferences(IModuleRewriter rewriter, Declaration target, string localIdentifier)

Rubberduck.Core/UI/Refactorings/AssignedByValParameterQuickFixDialog.cs

@@ -1,21 +1,19 @@
 using System;
-using System.Collections.Generic;
 using System.Windows.Forms;
-using System.Linq;
 using Rubberduck.Common;
 using Rubberduck.Resources;
 
 namespace Rubberduck.UI.Refactorings
 {
     public partial class AssignedByValParameterQuickFixDialog : Form, IAssignedByValParameterQuickFixDialog
     {
-        private readonly IEnumerable<string> _forbiddenNames;
+        private readonly Func<string, bool> _isConflictingName;
 
-        public AssignedByValParameterQuickFixDialog(string identifier, string identifierType, IEnumerable<string> forbiddenNames)
+        public AssignedByValParameterQuickFixDialog(string identifier, string identifierType, Func<string, bool> nameCollisionChecker)
         {
             InitializeComponent();
             InitializeCaptions(identifier, identifierType);
-            _forbiddenNames = forbiddenNames;
+            _isConflictingName = nameCollisionChecker;
         }
 
         private void InitializeCaptions(string identifierName, string targetDeclarationType)
@@ -54,7 +52,7 @@ private string GetVariableNameFeedback()
             {
                 return string.Empty;
             }
-            if (_forbiddenNames.Any(name => name.Equals(NewName, StringComparison.OrdinalIgnoreCase)))
+            if (_isConflictingName(NewName))
             {
                 return string.Format(RubberduckUI.AssignedByValDialog_NewNameAlreadyUsedFormat, NewName);
             }
@@ -79,7 +77,7 @@ private string GetVariableNameFeedback()
 
         private void SetControlsProperties()
         {
-            var isValid = VariableNameValidator.IsValidName(NewName) && !_forbiddenNames.Any(name => name.Equals(NewName, StringComparison.OrdinalIgnoreCase));
+            var isValid = VariableNameValidator.IsValidName(NewName);
             OkButton.Visible = isValid;
             OkButton.Enabled = isValid;
             InvalidNameValidationIcon.Visible = !isValid;

Rubberduck.Core/UI/Refactorings/IAssignedByValParameterQuickFixDialogFactory.cs

@@ -1,11 +1,10 @@
-using Rubberduck.Parsing.Symbols;
-using System.Collections.Generic;
+using System;
 
 namespace Rubberduck.UI.Refactorings
 {
     public interface IAssignedByValParameterQuickFixDialogFactory
     {
-        IAssignedByValParameterQuickFixDialog Create(string identifier, string identifierType, IEnumerable<string> forbiddenNames);
+        IAssignedByValParameterQuickFixDialog Create(string identifier, string identifierType, Func<string, bool> nameCollisionChecker);
         void Release(IAssignedByValParameterQuickFixDialog dialog);
     }
 }

Rubberduck.Parsing/VBA/DeclarationCaching/DeclarationFinder.cs

@@ -1015,79 +1015,78 @@ private List<Declaration> FindAllEventHandlers()
             return handlers.ToList();
         }
 
-        public IEnumerable<Declaration> GetAccessibleUserDeclarations(Declaration target)
+        /// <summary>
+        /// Finds declarations that would be in conflict with the target declaration if renamed.
+        /// </summary>
+        /// <returns>Zero or more declarations that would be in conflict if the target declaration is renamed.</returns>
+        public IEnumerable<Declaration> FindNewDeclarationNameConflicts(string newName, Declaration renameTarget)
         {
-            if (target == null)
+            if (newName.Equals(renameTarget.IdentifierName))
             {
                 return Enumerable.Empty<Declaration>();
             }
 
-            return _userDeclarationsByType.AllValues()
-                .Where(callee => AccessibilityCheck.IsAccessible(
-                    Declaration.GetProjectParent(target),
-                    Declaration.GetModuleParent(target),
-                    target.ParentDeclaration,
-                    callee));
-        }
-
-        public IEnumerable<Declaration> GetDeclarationsWithIdentifiersToAvoid(Declaration target)
-        {
-            if (target == null)
+            var identifierMatches = MatchName(newName);
+            if (!identifierMatches.Any())
             {
                 return Enumerable.Empty<Declaration>();
             }
 
-            List<Declaration> declarationsToAvoid = GetNameCollisionDeclarations(target).ToList();
-
-            declarationsToAvoid.AddRange(GetNameCollisionDeclarations(target.References));
-
-            return declarationsToAvoid.Distinct();
-        }
-
-        private IEnumerable<Declaration> GetNameCollisionDeclarations(Declaration declaration)
-        {
-            if (declaration == null)
+            if (IsEnumOrUDTMemberDeclaration(renameTarget)) 
             {
-                return Enumerable.Empty<Declaration>();
+                var memberMatches = identifierMatches.Where(idm =>
+                    IsEnumOrUDTMemberDeclaration(idm) && idm.ParentDeclaration == renameTarget.ParentDeclaration);
+                if (memberMatches.Any())
+                {
+                    return memberMatches;
+                }
             }
 
-            //Filter accessible declarations to those that would result in name collisions or hiding
-            var declarationsToAvoid = GetAccessibleUserDeclarations(declaration).Where(candidate =>
-                                        (IsAccessibleInOtherProcedureModule(candidate,declaration)
-                                        || candidate.DeclarationType == DeclarationType.Project
-                                        || candidate.DeclarationType.HasFlag(DeclarationType.Module)
-                                        || IsDeclarationInSameProcedureScope(candidate, declaration)
-                                        )).ToHashSet();
-
-            //Add local variables when the target is a method or property
-            if(IsSubroutineOrProperty(declaration))
+            identifierMatches = identifierMatches.Where(nc => !IsEnumOrUDTMemberDeclaration(nc));
+            var referenceConflicts = identifierMatches.Where(idm =>
+                renameTarget.References
+                    .Any(renameTargetRef => renameTargetRef.ParentScoping == idm.ParentDeclaration
+                        || renameTarget.ParentDeclaration.DeclarationType != DeclarationType.ClassModule
+                            && idm == renameTargetRef.ParentScoping
+                            && !UsesScopeResolution(renameTargetRef.Context.Parent)
+                        || idm.References
+                            .Any(idmRef => idmRef.ParentScoping == renameTargetRef.ParentScoping
+                                && !UsesScopeResolution(renameTargetRef.Context.Parent)))
+                || idm.DeclarationType.HasFlag(DeclarationType.Variable)
+                    && idm.ParentDeclaration.DeclarationType.HasFlag(DeclarationType.Module)
+                    && renameTarget.References.Any(renameTargetRef => renameTargetRef.QualifiedModuleName == idm.ParentDeclaration.QualifiedModuleName));
+
+            if (referenceConflicts.Any())
             {
-                var localVariableDeclarations = _declarations.AllValues()
-                    .Where(dec => ReferenceEquals(declaration, dec.ParentDeclaration));
-                declarationsToAvoid.UnionWith(localVariableDeclarations);
+                return referenceConflicts;
             }
 
-            return declarationsToAvoid;
+            var renameTargetModule = Declaration.GetModuleParent(renameTarget);
+            var declarationConflicts = identifierMatches.Where(idm =>
+                renameTarget == idm.ParentDeclaration
+                || AccessibilityCheck.IsAccessible(
+                    Declaration.GetProjectParent(renameTarget),
+                    renameTargetModule,
+                    renameTarget.ParentDeclaration,
+                    idm)
+                    && IsConflictingMember(renameTarget, renameTargetModule, idm));
+
+            return declarationConflicts;
         }
 
-        private IEnumerable<Declaration> GetNameCollisionDeclarations(IEnumerable<IdentifierReference> references)
+        private bool IsEnumOrUDTMemberDeclaration(Declaration candidate)
         {
-            var declarationsToAvoid = new HashSet<Declaration>();
-            foreach (var reference in references)
-            {
-                if (!UsesScopeResolution(reference.Context.Parent))
-                {
-                    declarationsToAvoid.UnionWith(GetNameCollisionDeclarations(reference.ParentNonScoping));
-                }
-            }
-            return declarationsToAvoid;
+            return candidate.DeclarationType == DeclarationType.EnumerationMember
+                       || candidate.DeclarationType == DeclarationType.UserDefinedTypeMember;
         }
 
-        private bool IsAccessibleInOtherProcedureModule(Declaration candidate, Declaration declaration)
+        private bool IsConflictingMember(Declaration renameTarget, Declaration renameTargetModule, Declaration candidate)
         {
-            return IsInProceduralModule(declaration)
-                       && IsInProceduralModule(candidate)
-                       && candidate.Accessibility != Accessibility.Private;
+            var candidateModule = Declaration.GetModuleParent(candidate);
+            return renameTargetModule == candidateModule
+             || renameTargetModule.DeclarationType.HasFlag(DeclarationType.ProceduralModule)
+                && candidate.Accessibility != Accessibility.Private
+                && candidateModule.DeclarationType.HasFlag(DeclarationType.ProceduralModule);
         }
 
         private bool UsesScopeResolution(RuleContext ruleContext)
@@ -1096,25 +1095,6 @@ private bool UsesScopeResolution(RuleContext ruleContext)
                 || (ruleContext is VBAParser.MemberAccessExprContext);
         }
 
-        private bool IsInProceduralModule(Declaration candidateDeclaration)
-        {
-            var candidateModuleDeclaration = Declaration.GetModuleParent(candidateDeclaration);
-
-            return candidateModuleDeclaration?.DeclarationType == DeclarationType.ProceduralModule;
-        }
-
-        private bool IsDeclarationInSameProcedureScope(Declaration candidateDeclaration, Declaration scopingDeclaration)
-        {
-            return candidateDeclaration.ParentScope == scopingDeclaration.ParentScope;
-        }
-
-        private static bool IsSubroutineOrProperty(Declaration declaration)
-        {
-            return declaration.DeclarationType.HasFlag(DeclarationType.Property)
-                || declaration.DeclarationType == DeclarationType.Function
-                || declaration.DeclarationType == DeclarationType.Procedure;
-        }
-
         /// <summary>
         /// Creates a dictionary of identifier references, keyed by module.
         /// </summary>

Rubberduck.Refactorings/Rename/RenameRefactoring.cs

@@ -245,13 +245,11 @@ private bool TrySetNewName(IRenamePresenter presenter)
             var result = presenter.Show(_model.Target);
             if (result == null) { return false; }
 
-            var conflictDeclarations = _state.DeclarationFinder.GetDeclarationsWithIdentifiersToAvoid(_model.Target)
-                .Where(d => d.IdentifierName.Equals(_model.NewName, StringComparison.InvariantCultureIgnoreCase)).ToList();
+            var conflicts = _state.DeclarationFinder.FindNewDeclarationNameConflicts(_model.NewName, _model.Target);
 
-            if (conflictDeclarations.Any())
+            if (conflicts.Any())
             {
-                var message = string.Format(RubberduckUI.RenameDialog_ConflictingNames, _model.NewName,
-                    conflictDeclarations.First().IdentifierName);
+                var message = string.Format(RubberduckUI.RenameDialog_ConflictingNames, _model.NewName, _model.Target.IdentifierName);
 
                 return _messageBox?.ConfirmYesNo(message, RubberduckUI.RenameDialog_Caption) ?? false;
             }

Rubberduck.Resources/RubberduckUI.Designer.cs

@@ -484,8 +484,9 @@ Warning: All customized settings will be lost.  Your old file will be saved in '
     <value>New</value>
   </data>
   <data name="RenameDialog_ConflictingNames" xml:space="preserve">
-    <value>Renaming to '{0}' clashes with '{1}' in the same scope.
-Are you sure you want to proceed with this rename?</value>
+    <value>'{0}' conflicts with an existing name. Renaming '{1}' to '{0}' may result in uncompilable code or a change in logic.
+Do you want to proceed with this rename?</value>
+    <comment>0: NewName 1: CurrentName</comment>
   </data>
   <data name="RenameDialog_AmbiguousSelection" xml:space="preserve">
     <value>Please ensure that exactly 1 control is selected before renaming.</value>

Rubberduck.Resources/RubberduckUI.resx

@@ -1,5 +1,4 @@
 using NUnit.Framework;
-using System.Collections.Generic;
 using System.Linq;
 using System.Threading;
 using Rubberduck.Inspections.QuickFixes;
@@ -9,6 +8,7 @@
 using Rubberduck.UI.Refactorings;
 using System.Windows.Forms;
 using Rubberduck.Inspections.Concrete;
+using System;
 
 namespace RubberduckTests.QuickFixes
 {
@@ -392,7 +392,7 @@ private Mock<IAssignedByValParameterQuickFixDialogFactory> BuildMockDialogFactor
             mockDialog.SetupGet(m => m.DialogResult).Returns(() => DialogResult.OK);
 
             var mockDialogFactory = new Mock<IAssignedByValParameterQuickFixDialogFactory>();
-            mockDialogFactory.Setup(f => f.Create(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<IEnumerable<string>>())).Returns(mockDialog.Object);
+            mockDialogFactory.Setup(f => f.Create(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<Func<string,bool>>())).Returns(mockDialog.Object);
 
             return mockDialogFactory;
         }