From 092a18f26e8abde9e0e615b0c6bd915d400a36c3 Mon Sep 17 00:00:00 2001 From: Simon Lindgren Date: Thu, 19 Jul 2012 12:27:01 +0200 Subject: [PATCH] [CodeIssues] Check redundant optional arguments of constructors. --- .../OptionalParameterCouldBeSkippedIssue.cs | 93 +++++++++++++------ .../OptionalParameterCouldBeSkippedTests.cs | 35 +++++++ 2 files changed, 100 insertions(+), 28 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/OptionalParameterCouldBeSkippedIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/OptionalParameterCouldBeSkippedIssue.cs index a4995c22c6..d1339cbae3 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/OptionalParameterCouldBeSkippedIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/OptionalParameterCouldBeSkippedIssue.cs @@ -27,13 +27,15 @@ using System.Collections.Generic; using ICSharpCode.NRefactory.CSharp.Resolver; using System.Linq; using ICSharpCode.NRefactory.Semantics; +using ICSharpCode.NRefactory.TypeSystem; namespace ICSharpCode.NRefactory.CSharp.Refactoring { [IssueDescription("Optional argument has default value and can be skipped", Description = "Finds calls to functions where optional parameters are used and the passed argument is the same as the default.", Category = IssueCategories.Redundancies, - Severity = Severity.Hint)] + Severity = Severity.Hint, + IssueMarker = IssueMarker.GrayOut)] public class OptionalParameterCouldBeSkippedIssue : ICodeIssueProvider { public IEnumerable GetIssues(BaseRefactoringContext context) @@ -43,63 +45,98 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring class GatherVisitor : GatherVisitorBase { - readonly BaseRefactoringContext context; - public GatherVisitor(BaseRefactoringContext context) : base (context) { - this.context = context; + } + + 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 }); + } } public override void VisitInvocationExpression(InvocationExpression invocationExpression) { base.VisitInvocationExpression(invocationExpression); - var invocationResolveResult = context.Resolve(invocationExpression) as CSharpInvocationResolveResult; + + var invocationResolveResult = ctx.Resolve(invocationExpression) as CSharpInvocationResolveResult; if (invocationResolveResult == null) return; + var arguments = invocationExpression.Arguments.ToArray(); + + 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) + .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 }); + } + } + + IEnumerable GetRedundantArguments(Expression[] arguments, CSharpInvocationResolveResult invocationResolveResult) + { var argumentToParameterMap = invocationResolveResult.GetArgumentToParameterMap(); var resolvedParameters = invocationResolveResult.Member.Parameters; - var arguments = invocationExpression.Arguments.ToArray(); for (int i = arguments.Length - 1; i >= 0; i--) { var parameter = resolvedParameters[argumentToParameterMap[i]]; if (parameter.IsParams) - // before params optional arguments are needed, or some of the + // 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) - continue; + // There can be no optional parameters preceding a required one + break; var argument = arguments[i] as PrimitiveExpression; if (argument == null) continue; - var argumentResolveResult = context.Resolve(argument) as ConstantResolveResult; + var argumentResolveResult = ctx.Resolve(argument) as ConstantResolveResult; if (parameter.ConstantValue != argumentResolveResult.ConstantValue) // Stop here since any arguments before this one has to be there - // enable passing this argument + // to enable the passing of this argument break; - var message = context.TranslateString("Argument is identical to the default value"); - AddIssue(argument, message, GetRemoveNodeAction(invocationExpression, argument)); + yield return argument; } } - IEnumerable GetRemoveNodeAction(InvocationExpression invocationExpression, Expression firstArgumentToRemove) + string GetActionTitle(IEnumerable arguments, Expression firstArgumentToRemove) { - var argumentsToRemove = invocationExpression.Arguments - .Where(arg => arg.StartLocation >= firstArgumentToRemove.StartLocation).ToArray(); + var redundantArgumentCount = arguments + .Where(arg => arg.StartLocation >= firstArgumentToRemove.StartLocation) + .Count(); - string message; - if (argumentsToRemove.Length == 1) { - message = context.TranslateString("Remove this argument"); - } else { - message = context.TranslateString("Remove this and the following arguments"); + if (redundantArgumentCount == 1) { + return ctx.TranslateString("Remove this argument"); + } + else { + return ctx.TranslateString("Remove this and the following arguments"); } - yield return new CodeAction(message, script => { - var newArgumentList = invocationExpression.Arguments - .Where(arg => !argumentsToRemove.Contains(arg)) - .Select(argument => argument.Clone()); - var newInvocation = new InvocationExpression(invocationExpression.Target.Clone(), newArgumentList); - script.Replace(invocationExpression, newInvocation); - - }); } } } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/OptionalParameterCouldBeSkippedTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/OptionalParameterCouldBeSkippedTests.cs index 37776a9623..580f8c5c70 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/OptionalParameterCouldBeSkippedTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/OptionalParameterCouldBeSkippedTests.cs @@ -68,6 +68,41 @@ class TestClass }"); } + [Test] + public void ChecksConstructors() + { + var input = @" +class TestClass +{ + public TestClass(string a1 = ""a1"") + { + } + + void Bar() + { + var foo = new TestClass (""a1""); + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new OptionalParameterCouldBeSkippedIssue(), input, out context); + Assert.AreEqual(1, issues.Count); + var issue = issues [0]; + Assert.AreEqual(1, issue.Actions.Count); + + CheckFix(context, issues [0], @" +class TestClass +{ + public TestClass(string a1 = ""a1"") + { + } + + void Bar() + { + var foo = new TestClass (); + } +}"); + } + [Test] public void IgnoresAllParametersPreceedingANeededOne() {