[CodeIssues] RedundantToStringIssue: Also check formatting calls.

ICSharpCode.NRefactory.CSharp/ICSharpCode.NRefactory.CSharp.csproj

@@ -396,13 +396,14 @@
     <Compile Include="Refactoring\CodeIssues\ParameterCanBeDemotedIssue\TypeCriteriaCollector.cs" />
     <Compile Include="Refactoring\CodeIssues\ReferenceToStaticMemberViaDerivedTypeIssue.cs" />
     <Compile Include="Refactoring\CodeIssues\ParameterCanBeDemotedIssue\IsTypeCriterion.cs" />
-    <Compile Include="Refactoring\CodeIssues\FormatStringIssue.cs" />
     <Compile Include="Refactoring\CodeIssues\OptionalParameterCouldBeSkippedIssue.cs" />
     <Compile Include="Refactoring\CodeIssues\RedundantCatchIssue.cs" />
     <Compile Include="Refactoring\CodeIssues\RedundantToStringIssue.cs" />
     <Compile Include="Refactoring\CodeIssues\CallToObjectEqualsViaBaseIssue.cs" />
     <Compile Include="Refactoring\CodeIssues\IncorrectCallToObjectGetHashCodeIssue.cs" />
     <Compile Include="Refactoring\CodeActions\ExtractMethod\VariableUsageAnalyzation.cs" />
+    <Compile Include="Refactoring\CodeIssues\FormatStringIssues\FormatStringIssue.cs" />
+    <Compile Include="Refactoring\CodeIssues\FormatStringIssues\FormatStringHelper.cs" />
   </ItemGroup>
   <ItemGroup>
     <ProjectReference Include="..\ICSharpCode.NRefactory\ICSharpCode.NRefactory.csproj">
@@ -417,6 +418,7 @@
     <Folder Include="Refactoring\CodeIssues\InconsistentNamingIssue\" />
     <Folder Include="Refactoring\CodeActions\ExtractMethod\" />
     <Folder Include="Refactoring\CodeActions\ConvertToInitializer\" />
+    <Folder Include="Refactoring\CodeIssues\FormatStringIssues\" />
   </ItemGroup>
   <ProjectExtensions>
     <MonoDevelop>

ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/FormatStringIssues/FormatStringHelper.cs

@@ -0,0 +1,70 @@
+//
+// FormatStringHelper.cs
+//
+// Author:
+//       Simon Lindgren <simon.n.lindgren@gmail.com>
+//
+// 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<Expression> arguments,
+		                                              Func<IParameter, Expression, bool> argumentFilter)
+		{
+			if (argumentFilter == null)
+				argumentFilter = (p, e) => true;
+
+			formatArgument = null;
+			formatStart = default(TextLocation);
+			arguments = new List<Expression>();
+			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;
+		}
+	}
+}
+

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<Expression> 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<Expression> arguments)
-			{
-				format = null;
-				formatStart = default(TextLocation);
-				arguments = new List<Expression>();
-				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;
-			}
 		}
 	}
 }

ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantToStringIssue.cs

@@ -60,6 +60,29 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 			}
 			
 			HashSet<AstNode> processedNodes = new HashSet<AstNode>();
+
+			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;
+				var invocationResolveResult = ctx.Resolve(invocationExpression) as CSharpInvocationResolveResult;
-				if (memberExpression == null) {
+				if (invocationResolveResult == null) {
-					return;
-				}
-				var targetResolveResult = ctx.Resolve(invocationExpression) as CSharpInvocationResolveResult;
-				if (targetResolveResult == null) {
 					return;
 				}
-				if (targetResolveResult.TargetResult.Type.IsKnownType(KnownTypeCode.String) && targetResolveResult.Member.Name == "ToString") {
+				IMember member = invocationResolveResult.Member;
-					AddRedundantToStringIssue(memberExpression, invocationExpression);
+
+				// "".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<string, int>(member.ReflectionName, invocationExpression.Arguments.Count);
 				Tuple<int, int> checkInfo;
-				if (!membersCallingToString.TryGetValue(key, out checkInfo)) {
+				if (membersCallingToString.TryGetValue(key, out checkInfo)) {
-					return;
+					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<Expression> 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<IParameter, Expression, bool> 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

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 ()
 		{

ICSharpCode.NRefactory.Tests/ICSharpCode.NRefactory.Tests.csproj

@@ -291,12 +291,12 @@
     <Compile Include="CSharp\CodeIssues\ReferenceToStaticMemberViaDerivedTypeTests.cs" />
     <Compile Include="CSharp\CodeIssues\ParameterCanBeDemotedIssue\IsTypeCriterionTests.cs" />
     <Compile Include="Utils\CompositeFormatStringParser\CompositeFormatStringParserTests.cs" />
-    <Compile Include="CSharp\CodeIssues\FormatStringTests.cs" />
     <Compile Include="CSharp\CodeIssues\OptionalParameterCouldBeSkippedTests.cs" />
     <Compile Include="CSharp\CodeIssues\RedundantCatchTests.cs" />
     <Compile Include="CSharp\CodeIssues\RedundantToStringTests.cs" />
     <Compile Include="CSharp\CodeIssues\CallToObjectEqualsViaBaseTests.cs" />
     <Compile Include="CSharp\CodeIssues\IncorrectCallToGetHashCodeTests.cs" />
+    <Compile Include="CSharp\CodeIssues\FormatStringIssues\FormatStringTests.cs" />
   </ItemGroup>
   <ItemGroup>
     <ProjectReference Include="..\..\Mono.Cecil\Mono.Cecil.csproj">
@@ -328,6 +328,7 @@
     <Folder Include="CSharp\CodeActions\ConvertToInitializer\" />
     <Folder Include="CSharp\CodeIssues\ParameterCanBeDemotedIssue\" />
     <Folder Include="Utils\CompositeFormatStringParser\" />
+    <Folder Include="CSharp\CodeIssues\FormatStringIssues\" />
   </ItemGroup>
   <Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
   <ProjectExtensions>