From f47fd7619e05c73424f8e5fca348f0024ebe8f07 Mon Sep 17 00:00:00 2001 From: Simon Lindgren Date: Tue, 24 Jul 2012 11:20:30 +0200 Subject: [PATCH] [CodeIssues] RedundantToStringIssue: Also check formatting calls. --- .../ICSharpCode.NRefactory.CSharp.csproj | 4 +- .../FormatStringIssues/FormatStringHelper.cs | 70 ++++++++++++ .../FormatStringIssue.cs | 40 ++----- .../CodeIssues/RedundantToStringIssue.cs | 108 ++++++++++++------ .../FormatStringTests.cs | 0 .../CodeIssues/RedundantToStringTests.cs | 85 ++++++++++++++ .../ICSharpCode.NRefactory.Tests.csproj | 3 +- 7 files changed, 238 insertions(+), 72 deletions(-) create mode 100644 ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssues/FormatStringHelper.cs rename ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/{ => FormatStringIssues}/FormatStringIssue.cs (73%) rename ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/{ => FormatStringIssues}/FormatStringTests.cs (100%) diff --git a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj index 4ee3cda294..b1a32abb74 100644 --- a/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj +++ b/ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj @@ -396,13 +396,14 @@ - + + @@ -417,6 +418,7 @@ + diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssues/FormatStringHelper.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssues/FormatStringHelper.cs new file mode 100644 index 0000000000..9531dd1243 --- /dev/null +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssues/FormatStringHelper.cs @@ -0,0 +1,70 @@ +// +// FormatStringHelper.cs +// +// Author: +// Simon Lindgren +// +// Copyright (c) 2012 Simon Lindgren +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +using System; +using System.Collections.Generic; +using ICSharpCode.NRefactory.CSharp.Resolver; +using System.Linq; +using ICSharpCode.NRefactory.TypeSystem; + +namespace ICSharpCode.NRefactory.CSharp +{ + class FormatStringHelper + { + static string[] parameterNames = { "format", "frmt", "fmt" }; + + public static bool TryGetFormattingParameters(CSharpInvocationResolveResult invocationResolveResult, InvocationExpression invocationExpression, + out Expression formatArgument, out TextLocation formatStart, out IList arguments, + Func argumentFilter) + { + if (argumentFilter == null) + argumentFilter = (p, e) => true; + + formatArgument = null; + formatStart = default(TextLocation); + arguments = new List(); + var argumentToParameterMap = invocationResolveResult.GetArgumentToParameterMap(); + var resolvedParameters = invocationResolveResult.Member.Parameters; + var allArguments = invocationExpression.Arguments.ToArray(); + for (int i = 0; i < allArguments.Length; i++) { + var parameterIndex = argumentToParameterMap[i]; + if (parameterIndex < 0 || parameterIndex >= resolvedParameters.Count) { + // No valid mapping for this parameter, skip it + continue; + } + var parameter = resolvedParameters[parameterIndex]; + var argument = allArguments[i]; + if (parameter.Type.IsKnownType(KnownTypeCode.String) && parameterNames.Contains(parameter.Name)) { + formatArgument = argument; + formatStart = argument.StartLocation; + } else if ((formatArgument != null || parameter.IsParams) && argumentFilter(parameter, argument)) { + arguments.Add(argument); + } + } + return formatArgument != null; + } + } +} + diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssues/FormatStringIssue.cs similarity index 73% rename from ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssue.cs rename to ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssues/FormatStringIssue.cs index c8afd65009..ae7778b53f 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssues/FormatStringIssue.cs @@ -25,7 +25,6 @@ // THE SOFTWARE. using System; using System.Collections.Generic; -using ICSharpCode.NRefactory.TypeSystem; using ICSharpCode.NRefactory.CSharp.Resolver; using System.Linq; using ICSharpCode.NRefactory.Utils; @@ -58,11 +57,17 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring var invocationResolveResult = context.Resolve(invocationExpression) as CSharpInvocationResolveResult; if (invocationResolveResult == null) return; - string format; + Expression formatArgument; IList formatArguments; TextLocation formatStart; - if (!TryGetFormattingParameters(invocationResolveResult, invocationExpression, out format, out formatStart, out formatArguments)) + if (!FormatStringHelper.TryGetFormattingParameters(invocationResolveResult, invocationExpression, + out formatArgument, out formatStart, out formatArguments, null)) { return; + } + var primitiveArgument = formatArgument as PrimitiveExpression; + if (primitiveArgument == null || !(primitiveArgument.Value is String)) + return; + var format = (string)primitiveArgument.Value; var parsingResult = context.ParseFormatString(format); CheckSegments(parsingResult.Segments, formatStart, formatArguments, invocationExpression); } @@ -99,35 +104,6 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring } } } - - static string[] parameterNames = { "format", "frmt", "fmt" }; - - bool TryGetFormattingParameters(CSharpInvocationResolveResult invocationResolveResult, InvocationExpression invocationExpression, - out string format, out TextLocation formatStart, out IList arguments) - { - format = null; - formatStart = default(TextLocation); - arguments = new List(); - var argumentToParameterMap = invocationResolveResult.GetArgumentToParameterMap(); - var resolvedParameters = invocationResolveResult.Member.Parameters; - var allArguments = invocationExpression.Arguments.ToArray(); - for (int i = 0; i < allArguments.Length; i++) { - var parameterIndex = argumentToParameterMap[i]; - if (parameterIndex < 0 || parameterIndex >= resolvedParameters.Count) { - // No valid mapping for this parameter, skip it - continue; - } - var parameter = resolvedParameters[parameterIndex]; - var argument = allArguments[i]; - if (parameter.Type.IsKnownType(KnownTypeCode.String) && parameterNames.Contains(parameter.Name) && argument is PrimitiveExpression) { - format = (string)((PrimitiveExpression)argument).Value; - formatStart = argument.StartLocation; - } else if (format != null || parameter.IsParams) { - arguments.Add(argument); - } - } - return format != null; - } } } } diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantToStringIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantToStringIssue.cs index 202a304c2e..ee0192f78c 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantToStringIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantToStringIssue.cs @@ -60,6 +60,29 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring } HashSet processedNodes = new HashSet(); + + void CheckExpressionInAutoCallContext(Expression expression) + { + if (expression is InvocationExpression) { + CheckInvocationInAutoCallContext((InvocationExpression)expression); + } + } + + void CheckInvocationInAutoCallContext(InvocationExpression invocationExpression) + { + var memberExpression = invocationExpression.Target as MemberReferenceExpression; + if (memberExpression == null) { + return; + } + var resolveResult = ctx.Resolve(invocationExpression) as CSharpInvocationResolveResult; + if (resolveResult == null) { + return; + } + if (resolveResult.Member.Name != "ToString") { + return; + } + AddRedundantToStringIssue(memberExpression, invocationExpression); + } void AddRedundantToStringIssue(MemberReferenceExpression memberExpression, InvocationExpression invocationExpression) { @@ -125,48 +148,41 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring expressions.Add(expression); } } - - void CheckExpressionInAutoCallContext(Expression expression) - { - if (expression is InvocationExpression) { - CheckInvocationInAutoCallContext((InvocationExpression)expression); - } - } - - void CheckInvocationInAutoCallContext(InvocationExpression invocationExpression) - { - var memberExpression = invocationExpression.Target as MemberReferenceExpression; - if (memberExpression == null) { - return; - } - var resolveResult = ctx.Resolve(invocationExpression) as CSharpInvocationResolveResult; - if (resolveResult == null) { - return; - } - if (resolveResult.Member.Name != "ToString") { - return; - } - AddRedundantToStringIssue(memberExpression, invocationExpression); - } #endregion - #region Invocation expressions + #region Invocation expression public override void VisitInvocationExpression(InvocationExpression invocationExpression) { base.VisitInvocationExpression(invocationExpression); - var memberExpression = invocationExpression.Target as MemberReferenceExpression; - if (memberExpression == null) { - return; - } - var targetResolveResult = ctx.Resolve(invocationExpression) as CSharpInvocationResolveResult; - if (targetResolveResult == null) { + var invocationResolveResult = ctx.Resolve(invocationExpression) as CSharpInvocationResolveResult; + if (invocationResolveResult == null) { return; } - if (targetResolveResult.TargetResult.Type.IsKnownType(KnownTypeCode.String) && targetResolveResult.Member.Name == "ToString") { - AddRedundantToStringIssue(memberExpression, invocationExpression); + IMember member = invocationResolveResult.Member; + + // "".ToString() + CheckTargetedObject(invocationExpression, invocationResolveResult.TargetResult.Type, member); + + // Check list of members that call ToString() automatically + CheckAutomaticToStringCallers(invocationExpression, member); + + // Check formatting calls + CheckFormattingCall(invocationExpression, invocationResolveResult); + } + + void CheckTargetedObject(InvocationExpression invocationExpression, IType type, IMember member) + { + var memberExpression = invocationExpression.Target as MemberReferenceExpression; + if (memberExpression != null) { + if (type.IsKnownType(KnownTypeCode.String) && member.Name == "ToString") { + AddRedundantToStringIssue(memberExpression, invocationExpression); + } } - IMember member = targetResolveResult.Member; + } + + void CheckAutomaticToStringCallers(InvocationExpression invocationExpression, IMember member) + { if (member.IsOverride) { member = InheritanceHelper.GetBaseMember(member); if (member == null) { @@ -175,12 +191,28 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring } var key = new Tuple(member.ReflectionName, invocationExpression.Arguments.Count); Tuple checkInfo; - if (!membersCallingToString.TryGetValue(key, out checkInfo)) { - return; + if (membersCallingToString.TryGetValue(key, out checkInfo)) { + var arguments = invocationExpression.Arguments.ToList(); + for (int i = checkInfo.Item1; i < Math.Min(invocationExpression.Arguments.Count, checkInfo.Item2 + 1); ++i) { + CheckExpressionInAutoCallContext(arguments[i]); + } } - var arguments = invocationExpression.Arguments.ToList(); - for (int i = checkInfo.Item1; i < Math.Min(invocationExpression.Arguments.Count, checkInfo.Item2 + 1); ++i) { - CheckExpressionInAutoCallContext(arguments[i]); + } + + void CheckFormattingCall(InvocationExpression invocationExpression, CSharpInvocationResolveResult invocationResolveResult) + { + Expression formatArgument; + IList formatArguments; + TextLocation formatStart; + // Only check parameters that are of type object: String means it is neccessary, others + // means that there is another problem (ie no matching overload of the method). + Func predicate = + (parameter, argument) => parameter.Type.GetDefinition().IsKnownType(KnownTypeCode.Object); + if (FormatStringHelper.TryGetFormattingParameters(invocationResolveResult, invocationExpression, + out formatArgument, out formatStart, out formatArguments, predicate)) { + foreach (var argument in formatArguments) { + CheckExpressionInAutoCallContext(argument); + } } } #endregion diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/FormatStringTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/FormatStringIssues/FormatStringTests.cs similarity index 100% rename from ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/FormatStringTests.cs rename to ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/FormatStringIssues/FormatStringTests.cs diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantToStringTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantToStringTests.cs index 7e346ce789..bbe384a340 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantToStringTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantToStringTests.cs @@ -103,6 +103,91 @@ class Foo }"); } + [Test] + public void FormatStringTests () + { + var input = @" +class Foo +{ + void Bar (int i) + { + string s = string.Format(""{0}"", i.ToString()); + } +}"; + + TestRefactoringContext context; + var issues = GetIssues (new RedundantToStringIssue (), input, out context); + Assert.AreEqual (1, issues.Count); + CheckFix (context, issues, @" +class Foo +{ + void Bar (int i) + { + string s = string.Format(""{0}"", i); + } +}"); + } + + [Test] + public void HandlesNonLiteralFormatParameter () + { + var input = @" +class Foo +{ + void Bar (int i) + { + string format = ""{0}""; + string s = string.Format(format, i.ToString()); + } +}"; + + TestRefactoringContext context; + var issues = GetIssues (new RedundantToStringIssue (), input, out context); + Assert.AreEqual (1, issues.Count); + CheckFix (context, issues, @" +class Foo +{ + void Bar (int i) + { + string format = ""{0}""; + string s = string.Format(format, i); + } +}"); + } + + [Test] + public void FormatStringWithNonObjectParameterTests () + { + var input = @" +class Foo +{ + void Bar (int i) + { + string s = FakeFormat(""{0} {1}"", i.ToString(), i.ToString()); + } + + void FakeFormat(string format, string arg0, object arg1) + { + } +}"; + + TestRefactoringContext context; + var issues = GetIssues (new RedundantToStringIssue (), input, out context); + Assert.AreEqual (1, issues.Count); + CheckFix (context, issues, @" +class Foo +{ + void Bar (int i) + { + string s = FakeFormat(""{0} {1}"", i.ToString(), i); + } + + void FakeFormat(string format, string arg0, object arg1) + { + } +}"); + } + [Test] public void DetectsBlacklistedCalls () { diff --git a/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj b/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj index 828ae243f3..4f0cfee082 100644 --- a/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj +++ b/ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj @@ -291,12 +291,12 @@ - + @@ -328,6 +328,7 @@ +