Fix bugs in generated remove params callers

Author: Hosch250

RetailCoder.VBE/Refactorings/RemoveParameters/RemoveParametersRefactoring.cs

@@ -96,7 +96,6 @@ private void AdjustReferences(IEnumerable<IdentifierReference> references, Decla
         {
             foreach (var reference in references.Where(item => item.Context != method.Context))
             {
-                var proc = (dynamic)reference.Context;
                 var module = reference.QualifiedModuleName.Component.CodeModule;
                 VBAParser.ArgumentListContext argumentList = null;
                 var callStmt = ParserRuleContextHelper.GetParent<VBAParser.CallStmtContext>(reference.Context);
@@ -111,7 +110,7 @@ private void AdjustReferences(IEnumerable<IdentifierReference> references, Decla
 
         private void RemoveCallParameter(VBAParser.ArgumentListContext paramList, CodeModule module)
         {
-            List<string> paramNames = new List<string>();
+            var paramNames = new List<string>();
             if (paramList.positionalOrNamedArgumentList().positionalArgumentOrMissing() != null)
             {
                 paramNames.AddRange(paramList.positionalOrNamedArgumentList().positionalArgumentOrMissing().Select(p =>
@@ -120,10 +119,8 @@ private void RemoveCallParameter(VBAParser.ArgumentListContext paramList, CodeMo
                     {
                         return ((VBAParser.SpecifiedPositionalArgumentContext)p).positionalArgument().GetText();
                     }
-                    else
-                    {
-                        return string.Empty;
-                    }
+
+                    return string.Empty;
                 }).ToList());
             }
             if (paramList.positionalOrNamedArgumentList().namedArgumentList() != null)
@@ -136,48 +133,46 @@ private void RemoveCallParameter(VBAParser.ArgumentListContext paramList, CodeMo
             }
             var lineCount = paramList.Stop.Line - paramList.Start.Line + 1; // adjust for total line count
 
-            var newContent = module.Lines[paramList.Start.Line, lineCount].Replace(" _" + Environment.NewLine, string.Empty).RemoveExtraSpacesLeavingIndentation();
-            var currentStringIndex = 0;
+            var newContent = module.Lines[paramList.Start.Line, lineCount];
+            newContent = newContent.Remove(paramList.Start.Column, paramList.GetText().Length);
 
-            foreach (
-                var param in
-                    _model.Parameters.Where(item => item.IsRemoved && item.Index < paramNames.Count)
-                        .Select(item => item.Declaration))
+            var savedParamNames = paramNames;
+            for (var index = _model.Parameters.Count - 1; index >= 0; index--)
             {
-                var paramIndex = _model.Parameters.FindIndex(item => item.Declaration.Context.GetText() == param.Context.GetText());
-                if (paramIndex >= paramNames.Count) { return; }
-
-                do
+                var param = _model.Parameters[index];
+                if (!param.IsRemoved)
                 {
-                    var paramToRemoveName = paramNames.ElementAt(0).Contains(":=")
-                        ? paramNames.Find(item => item.Contains(param.IdentifierName + ":="))
-                        : paramNames.ElementAt(paramIndex);
+                    continue;
+                }
 
-                    if (paramToRemoveName == null || !newContent.Contains(paramToRemoveName))
+                if (param.Name.Contains("ParamArray"))
+                {
+                    // handle param arrays
+                    while (savedParamNames.Count > index)
                     {
-                        continue;
+                        savedParamNames.RemoveAt(index);
                     }
-
-                    var valueToRemove = paramToRemoveName != paramNames.Last()
-                        ? paramToRemoveName + ","
-                        : paramToRemoveName;
-
-                    var parameterStringIndex = newContent.IndexOf(valueToRemove, currentStringIndex, StringComparison.Ordinal);
-                    if (parameterStringIndex <= -1) { continue; }
-
-                    newContent = newContent.Remove(parameterStringIndex, valueToRemove.Length);
-
-                    currentStringIndex = parameterStringIndex;
-
-                    if (paramToRemoveName == paramNames.Last() && newContent.LastIndexOf(',') != -1)
+                }
+                else
+                {
+                    if (index < savedParamNames.Count && !savedParamNames[index].StripStringLiterals().Contains(":="))
                     {
-                        newContent = newContent.Remove(newContent.LastIndexOf(','), 1);
+                        savedParamNames.RemoveAt(index);
                     }
-                } while (paramIndex >= _model.Parameters.Count - 1 && ++paramIndex < paramNames.Count &&
-                         newContent.Contains(paramNames.ElementAt(paramIndex)));
+                    else
+                    {
+                        var paramIndex = savedParamNames.FindIndex(s => s.StartsWith(param.Declaration.IdentifierName + ":="));
+                        if (paramIndex != -1 && paramIndex < savedParamNames.Count)
+                        {
+                            savedParamNames.RemoveAt(paramIndex);
+                        }
+                    }
+                }
             }
 
-            module.ReplaceLine(paramList.Start.Line, newContent);
+            newContent = newContent.Insert(paramList.Start.Column, string.Join(", ", savedParamNames));
+
+            module.ReplaceLine(paramList.Start.Line, newContent.Replace(" _" + Environment.NewLine, string.Empty));
             module.DeleteLines(paramList.Start.Line + 1, lineCount - 1);
         }
 
@@ -251,26 +246,6 @@ private string GetOldSignature(Declaration target)
             return rewriter.GetText(new Interval(firstTokenIndex, lastTokenIndex));
         }
 
-        private string ReplaceCommas(string signature, int indexParamRemoved)
-        {
-            if (signature.Count(c => c == ',') > indexParamRemoved) { indexParamRemoved++; }
-
-            for (int i = 0, commaCounter = 0; i < signature.Length && indexParamRemoved != 0; i++)
-            {
-                if (signature.ElementAt(i) == ',')
-                {
-                    commaCounter++;
-                }
-
-                if (commaCounter == indexParamRemoved)
-                {
-                    return signature.Remove(i, 1);
-                }
-            }
-
-            return signature;
-        }
-
         private void AdjustSignatures()
         {
             var proc = (dynamic)_model.TargetDeclaration.Context;

RubberduckTests/Refactoring/RemoveParametersTests.cs

@@ -215,7 +215,7 @@ End Sub
 End Sub
 
 Public Sub Goo()
-    Foo arg2:=""test44"",  arg1:=3
+    Foo arg2:=""test44"", arg1:=3
 End Sub
 ";
 
@@ -248,6 +248,59 @@ End Sub
             Assert.AreEqual(expectedCode, module.Lines());
         }
 
+        [TestMethod]
+        public void RemoveParametersRefactoring_CallerArgNameContainsOtherArgName()
+        {
+            //Input
+            const string inputCode =
+@"Sub foo(a, b, c)
+
+End Sub
+
+Sub goo()
+    foo asd, sdf, s
+End Sub";
+            var selection = new Selection(1, 23, 1, 27);
+
+            //Expectation
+            const string expectedCode =
+@"Sub foo(a, b)
+
+End Sub
+
+Sub goo()
+    foo asd, sdf
+End Sub";
+
+            //Arrange
+            var builder = new MockVbeBuilder();
+            VBComponent component;
+            var vbe = builder.BuildFromSingleStandardModule(inputCode, out component, selection);
+            var module = component.CodeModule;
+            var mockHost = new Mock<IHostApplication>();
+            mockHost.SetupAllProperties();
+            var parser = MockParser.Create(vbe.Object, new RubberduckParserState(new Mock<ISinks>().Object));
+
+            parser.Parse(new CancellationTokenSource());
+            if (parser.State.Status >= ParserState.Error) { Assert.Inconclusive("Parser Error"); }
+
+            var qualifiedSelection = new QualifiedSelection(new QualifiedModuleName(component), selection);
+
+            //Specify Param(s) to remove
+            var model = new RemoveParametersModel(parser.State, qualifiedSelection, null);
+            model.Parameters[2].IsRemoved = true;
+
+            //SetupFactory
+            var factory = SetupFactory(model);
+
+            //Act
+            var refactoring = new RemoveParametersRefactoring(vbe.Object, factory.Object);
+            refactoring.Refactor(qualifiedSelection);
+
+            //Assert
+            Assert.AreEqual(expectedCode, module.Lines());
+        }
+
         [TestMethod]
         public void RemoveParametersRefactoring_RemoveLastFromFunction()
         {
@@ -354,7 +407,7 @@ End Sub
 End Function
 
 Private Sub Goo(ByVal arg1 As Integer, ByVal arg2 As String)
-    Foo  
+    Foo 
 End Sub
 ";
 
@@ -406,7 +459,7 @@ Private Sub goo()
 End Sub
 
 Private Sub goo()
-    foo 1, 2,   5,  7
+    foo 1, 2, 5, 7
 End Sub";
 
             //Arrange
@@ -589,7 +642,7 @@ End Sub
 End Sub
 
 Private Sub Bar()
-    Foo  ""Hello""
+    Foo ""Hello""
 End Sub
 ";
 
@@ -642,9 +695,9 @@ End Sub
 End Sub
 
 Private Sub Bar()
-    Foo 10 
+    Foo 10
 End Sub
-"; //note: The IDE strips out the extra whitespace, you can't see it but there's a space after "Foo 10 "
+";
 
             //Arrange
             var builder = new MockVbeBuilder();
@@ -707,9 +760,9 @@ Public Sub Goo(ByVal arg1 As Integer, _
                ByVal arg5 As Integer, _
                ByVal arg6 As Integer)
               
-    Foo ""test""      
+    Foo ""test""
 End Sub
-"; //note: The IDE strips out the extra whitespace, you can't see it but there are several spaces after " ParamArrayTest ""test""      "
+";
 
             //Arrange
             var builder = new MockVbeBuilder();
@@ -945,6 +998,59 @@ Private Sub Goo(ByVal arg1 As Integer)
             Assert.AreEqual(expectedCode, module.Lines());
         }
 
+        [TestMethod]
+        public void RemoveParametersRefactoring_RemoveOptionalParam()
+        {
+            //Input
+            const string inputCode =
+@"Private Sub Foo(ByVal arg1 As Integer, Optional ByVal arg2 As String)
+End Sub
+
+Private Sub Goo(ByVal arg1 As Integer)
+    Foo arg1
+    Foo 1, ""test""
+End Sub";
+            var selection = new Selection(1, 23, 1, 27);
+
+            //Expectation
+            const string expectedCode =
+@"Private Sub Foo(ByVal arg1 As Integer)
+End Sub
+
+Private Sub Goo(ByVal arg1 As Integer)
+    Foo arg1
+    Foo 1
+End Sub";
+
+            //Arrange
+            var builder = new MockVbeBuilder();
+            VBComponent component;
+            var vbe = builder.BuildFromSingleStandardModule(inputCode, out component, selection);
+            var module = component.CodeModule;
+            var mockHost = new Mock<IHostApplication>();
+            mockHost.SetupAllProperties();
+            var parser = MockParser.Create(vbe.Object, new RubberduckParserState(new Mock<ISinks>().Object));
+
+            parser.Parse(new CancellationTokenSource());
+            if (parser.State.Status >= ParserState.Error) { Assert.Inconclusive("Parser Error"); }
+
+            var qualifiedSelection = new QualifiedSelection(new QualifiedModuleName(component), selection);
+
+            //Specify Params to remove
+            var model = new RemoveParametersModel(parser.State, qualifiedSelection, null);
+            model.Parameters[1].IsRemoved = true;
+
+            //SetupFactory
+            var factory = SetupFactory(model);
+
+            //Act
+            var refactoring = new RemoveParametersRefactoring(vbe.Object, factory.Object);
+            refactoring.Refactor(qualifiedSelection);
+
+            //Assert
+            Assert.AreEqual(expectedCode, module.Lines());
+        }
+
         [TestMethod]
         public void RemoveParametersRefactoring_SignatureOnMultipleLines()
         {
@@ -1150,7 +1256,7 @@ End Sub
 
 Private Sub Goo(ByVal arg1 as Integer, ByVal arg2 As String, ByVal arg3 As Date)
 
-    Foo  arg2, arg3
+    Foo arg2, arg3
 
 End Sub
 ";   // note: IDE removes excess spaces