From cdfde8bcefbbd6c85fa124d5c3d0eabd47d5c530 Mon Sep 17 00:00:00 2001 From: Simon Lindgren Date: Thu, 2 Aug 2012 16:49:27 +0200 Subject: [PATCH] [CodeActions] Greatly improved OptionalParameterCouldBeSkippedIssue. It now handles named parameters much better, has more actions, and less code duplication. --- .../OptionalParameterCouldBeSkippedIssue.cs | 137 ++++++++++-------- .../OptionalParameterCouldBeSkippedTests.cs | 120 ++++++++++++++- 2 files changed, 190 insertions(+), 67 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/OptionalParameterCouldBeSkippedIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/OptionalParameterCouldBeSkippedIssue.cs index 55f18db704..6c7f378a4d 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/OptionalParameterCouldBeSkippedIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/OptionalParameterCouldBeSkippedIssue.cs @@ -28,6 +28,7 @@ using ICSharpCode.NRefactory.CSharp.Resolver; using System.Linq; using ICSharpCode.NRefactory.Semantics; using ICSharpCode.NRefactory.TypeSystem; +using System; namespace ICSharpCode.NRefactory.CSharp.Refactoring { @@ -52,50 +53,66 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring public override void VisitObjectCreateExpression(ObjectCreateExpression objectCreateExpression) { base.VisitObjectCreateExpression(objectCreateExpression); - - var invocationResolveResult = ctx.Resolve(objectCreateExpression) as CSharpInvocationResolveResult; - if (invocationResolveResult == null) - return; - var arguments = objectCreateExpression.Arguments.ToArray(); - var redundantArguments = GetRedundantArguments(arguments, invocationResolveResult); - foreach (var argument in redundantArguments) { - string actionMessage = GetActionTitle(objectCreateExpression.Arguments, argument); - - var action = new CodeAction(actionMessage, script => { - var newArgumentList = objectCreateExpression.Arguments - .Where(arg => arg.StartLocation < argument.StartLocation) - .Select(arg => arg.Clone()); - var newInvocation = new ObjectCreateExpression(objectCreateExpression.Type.Clone(), newArgumentList); - script.Replace(objectCreateExpression, newInvocation); - }); - var issueMessage = ctx.TranslateString("Argument is identical to the default value"); - AddIssue(argument, issueMessage, new [] { action }); - } + CheckMethodCall(objectCreateExpression, objectCreateExpression.Arguments, + (objectCreation, args) => new ObjectCreateExpression(objectCreation.Type.Clone(), args)); } public override void VisitInvocationExpression(InvocationExpression invocationExpression) { base.VisitInvocationExpression(invocationExpression); + + CheckMethodCall(invocationExpression, invocationExpression.Arguments, + (invocation, args) => new InvocationExpression(invocation.Target.Clone(), args)); + } - var invocationResolveResult = ctx.Resolve(invocationExpression) as CSharpInvocationResolveResult; + void CheckMethodCall (T node, IEnumerable arguments, Func, T> generateReplacement) where T: AstNode + { + var invocationResolveResult = ctx.Resolve(node) as CSharpInvocationResolveResult; if (invocationResolveResult == null) return; - var arguments = invocationExpression.Arguments.ToArray(); + + string actionMessage = ctx.TranslateString("Remove redundant arguments"); + + var redundantArguments = GetRedundantArguments(arguments.ToArray(), invocationResolveResult); + var action = new CodeAction(actionMessage, script => { + var newArgumentList = arguments + .Where(arg => !redundantArguments.Contains(arg)) + .Select(arg => arg.Clone()); + var newInvocation = generateReplacement(node, newArgumentList); + script.Replace(node, newInvocation); + }); + var issueMessage = ctx.TranslateString("Argument is identical to the default value"); + var lastPositionalArgument = redundantArguments.FirstOrDefault(expression => !(expression is NamedArgumentExpression)); + bool hasNamedArguments = false; - var redundantArguments = GetRedundantArguments(arguments, invocationResolveResult); foreach (var argument in redundantArguments) { - string actionMessage = GetActionTitle(invocationExpression.Arguments, argument); - - var action = new CodeAction(actionMessage, script => { - var newArgumentList = invocationExpression.Arguments - .Where(arg => arg.StartLocation < argument.StartLocation) + var localArgument = argument; + var actions = new List(); + actions.Add(action); + + if (localArgument is NamedArgumentExpression || localArgument == lastPositionalArgument) { + var title = ctx.TranslateString("Remove this argument"); + actions.Add(new CodeAction(title, script => { + var newArgumentList = arguments + .Where(arg => arg != localArgument) .Select(arg => arg.Clone()); - var newInvocation = new InvocationExpression(invocationExpression.Target.Clone(), newArgumentList); - script.Replace(invocationExpression, newInvocation); - }); - var issueMessage = ctx.TranslateString("Argument is identical to the default value"); - AddIssue(argument, issueMessage, new [] { action }); + var newInvocation = generateReplacement(node, newArgumentList); + script.Replace(node, newInvocation); + })); + hasNamedArguments = true; + } else { + var title = ctx.TranslateString("Remove this and the following positional arguments"); + actions.Add(new CodeAction(title, script => { + var newArgumentList = arguments + .Where(arg => arg.StartLocation < localArgument.StartLocation && !(arg is NamedArgumentExpression)) + .Select(arg => arg.Clone()); + var newInvocation = generateReplacement(node, newArgumentList); + script.Replace(node, newInvocation); + })); + } + + AddIssue(localArgument, issueMessage, actions); } } @@ -110,36 +127,34 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring // This particular parameter is an error, but keep trying the other ones continue; var parameter = resolvedParameters[parameterIndex]; - if (parameter.IsParams) - // before params all optional arguments are needed, otherwise some of the - // param arguments will be shifted out of the params into the fixed parameters - break; - if (!parameter.IsOptional) - // There can be no optional parameters preceding a required one + var argument = arguments[i]; + if (argument is PrimitiveExpression) { + if (parameter.IsParams) + // before positional params arguments all optional arguments are needed, otherwise some of the + // param arguments will be shifted out of the params into the fixed parameters + break; + if (!parameter.IsOptional) + // There can be no optional parameters preceding a required one + break; + var argumentResolveResult = ctx.Resolve(argument) as ConstantResolveResult; + if (argumentResolveResult == null || parameter.ConstantValue != argumentResolveResult.ConstantValue) + // Stop here since any arguments before this one has to be there + // to enable the passing of this argument + break; + yield return argument; + } else if (argument is NamedArgumentExpression) { + var expression = ((NamedArgumentExpression)argument).Expression as PrimitiveExpression; + if (expression == null) + continue; + var expressionResolveResult = ctx.Resolve(expression) as ConstantResolveResult; + if (expressionResolveResult == null || parameter.ConstantValue != expressionResolveResult.ConstantValue) + // continue, since there can still be more arguments that are redundant + continue; + yield return argument; + } else { + // This is a non-constant positional argument => no more redundancies are possible break; - var argument = arguments[i] as PrimitiveExpression; - if (argument == null) - continue; - var argumentResolveResult = ctx.Resolve(argument) as ConstantResolveResult; - if (parameter.ConstantValue != argumentResolveResult.ConstantValue) - // Stop here since any arguments before this one has to be there - // to enable the passing of this argument - break; - yield return argument; - } - } - - string GetActionTitle(IEnumerable arguments, Expression firstArgumentToRemove) - { - var redundantArgumentCount = arguments - .Where(arg => arg.StartLocation >= firstArgumentToRemove.StartLocation) - .Count(); - - if (redundantArgumentCount == 1) { - return ctx.TranslateString("Remove this argument"); - } - else { - return ctx.TranslateString("Remove this and the following arguments"); + } } } } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/OptionalParameterCouldBeSkippedTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/OptionalParameterCouldBeSkippedTests.cs index 580f8c5c70..335cfb4494 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/OptionalParameterCouldBeSkippedTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/OptionalParameterCouldBeSkippedTests.cs @@ -52,7 +52,7 @@ class TestClass var issues = GetIssues(new OptionalParameterCouldBeSkippedIssue(), input, out context); Assert.AreEqual(1, issues.Count); var issue = issues [0]; - Assert.AreEqual(1, issue.Actions.Count); + Assert.AreEqual(2, issue.Actions.Count); CheckFix(context, issues [0], @" class TestClass @@ -87,7 +87,7 @@ class TestClass var issues = GetIssues(new OptionalParameterCouldBeSkippedIssue(), input, out context); Assert.AreEqual(1, issues.Count); var issue = issues [0]; - Assert.AreEqual(1, issue.Actions.Count); + Assert.AreEqual(2, issue.Actions.Count); CheckFix(context, issues [0], @" class TestClass @@ -122,7 +122,7 @@ class TestClass var issues = GetIssues(new OptionalParameterCouldBeSkippedIssue(), input, out context); Assert.AreEqual(1, issues.Count); var issue = issues [0]; - Assert.AreEqual(1, issue.Actions.Count); + Assert.AreEqual(2, issue.Actions.Count); CheckFix(context, issues [0], @" class TestClass @@ -156,8 +156,9 @@ class TestClass TestRefactoringContext context; var issues = GetIssues(new OptionalParameterCouldBeSkippedIssue(), input, out context); Assert.AreEqual(3, issues.Count); - var issue = issues [0]; - Assert.AreEqual(1, issue.Actions.Count); + Assert.AreEqual(2, issues[0].Actions.Count); + Assert.AreEqual(2, issues[1].Actions.Count); + Assert.AreEqual(2, issues[2].Actions.Count); CheckFix(context, issues [2], @" class TestClass @@ -172,7 +173,7 @@ class TestClass } }"); } - + [Test] public void IgnoresIfParamsAreUsed() { @@ -192,6 +193,113 @@ class TestClass var issues = GetIssues(new OptionalParameterCouldBeSkippedIssue(), input, out context); Assert.AreEqual(0, issues.Count); } + + [Test] + public void NamedArgument() + { + var input = @" +class TestClass +{ + void Foo(string a1 = ""a1"", string a2 = ""a2"") + { + } + + void Bar() + { + Foo (a2: ""a2""); + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new OptionalParameterCouldBeSkippedIssue(), input, out context); + Assert.AreEqual(1, issues.Count); + Assert.AreEqual(2, issues[0].Actions.Count); + + CheckFix(context, issues [0], @" +class TestClass +{ + void Foo(string a1 = ""a1"", string a2 = ""a2"") + { + } + + void Bar() + { + Foo (); + } +}"); + } + + [Test] + public void DoesNotStopAtDifferingNamedParameters() + { + var input = @" +class TestClass +{ + void Foo(string a1 = ""a1"", string a2 = ""a2"", string a3 = ""a3"") + { + } + + void Bar() + { + Foo (""a1"", ""a2"", a3: ""non-default""); + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new OptionalParameterCouldBeSkippedIssue(), input, out context); + Assert.AreEqual(2, issues.Count); + Assert.AreEqual(2, issues[0].Actions.Count); + Assert.AreEqual(2, issues[1].Actions.Count); + + CheckFix(context, issues [1], @" +class TestClass +{ + void Foo(string a1 = ""a1"", string a2 = ""a2"", string a3 = ""a3"") + { + } + + void Bar() + { + Foo (a3: ""non-default""); + } +}"); + } + + [Test] + public void DoesNotStopAtNamedParamsArray() + { + var input = @" +class TestClass +{ + void Foo(string a1 = ""a1"", string a2 = ""a2"", params string[] extras) + { + } + + void Bar() + { + Foo (""a1"", ""a2"", extras: new [] { ""extra1"" }); + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new OptionalParameterCouldBeSkippedIssue(), input, out context); + Assert.AreEqual(2, issues.Count); + Assert.AreEqual(2, issues[0].Actions.Count); + Assert.AreEqual(2, issues[1].Actions.Count); + + // TODO: Fix formatting + CheckFix(context, issues [1], @" +class TestClass +{ + void Foo(string a1 = ""a1"", string a2 = ""a2"", params string[] extras) + { + } + + void Bar() + { + Foo (extras: new[] { + ""extra1"" +}); + } +}"); + } } }