Fix false positives in SimplifyAnonymousMethodToDelegateIssue: the simplification is not possible when the lambda involves non-reference conversions.

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

@@ -35,42 +35,43 @@ using ICSharpCode.NRefactory.Refactoring;
 namespace ICSharpCode.NRefactory.CSharp.Refactoring
 {
 	[IssueDescription("Redundant null check",
-					  Description = "When 'is' keyword is used, which implicitly check null.",
-					  Category = IssueCategories.Redundancies,
-					  Severity = Severity.Suggestion,
+	                  Description = "When 'is' keyword is used, which implicitly check null.",
+	                  Category = IssueCategories.Redundancies,
+	                  Severity = Severity.Suggestion,
 	                  ResharperDisableKeyword = "RedundantNullCheck",
-					  IssueMarker = IssueMarker.GrayOut)]
+	                  IssueMarker = IssueMarker.GrayOut)]
 	public class RedundantNullCheckIssue : ICodeIssueProvider
 	{
 		private static readonly Pattern pattern1
 			= new Choice {
-			//  x is Record && x!= null	 
+			//  a is Record && a != null
 			new BinaryOperatorExpression(
-				new IsExpression
-					{
+				PatternHelper.OptionalParentheses(
+					new IsExpression {
 						Expression = new AnyNode("a"),
 						Type = new AnyNode("t")
-					}, BinaryOperatorType.ConditionalAnd,
-				PatternHelper.CommutativeOperator(new Backreference("a"),
-													BinaryOperatorType.
-														InEquality,
-													new NullReferenceExpression
-														())
+					}),
+				BinaryOperatorType.ConditionalAnd,
+				PatternHelper.OptionalParentheses(
+					PatternHelper.CommutativeOperator(new Backreference("a"),
+					                                  BinaryOperatorType.InEquality,
+					                                  new NullReferenceExpression())
 				)
-		};
-		private static readonly Pattern pattern2
-		   = new Choice {
-			//  x != null && x is Record
+			),
+			//  a != null && a is Record
 			new BinaryOperatorExpression (
-				PatternHelper.CommutativeOperator (new AnyNode("a"), 
-														BinaryOperatorType.InEquality, 
-													new NullReferenceExpression())			
-				, BinaryOperatorType.ConditionalAnd,
-				new IsExpression {
-					Expression = new Backreference("a"),
-					Type = new AnyNode("t")
-				}
-			)	
+				PatternHelper.OptionalParentheses(
+					PatternHelper.CommutativeOperator(new AnyNode("a"),
+					                                  BinaryOperatorType.InEquality,
+					                                  new NullReferenceExpression())
+				),
+				BinaryOperatorType.ConditionalAnd,
+				PatternHelper.OptionalParentheses(
+					new IsExpression {
+						Expression = new Backreference("a"),
+						Type = new AnyNode("t")
+					})
+			)
 		};
 
 		public IEnumerable<CodeIssue> GetIssues(BaseRefactoringContext context)
@@ -91,18 +92,9 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 				Match m1 = pattern1.Match(binaryOperatorExpression);
 				if (m1.Success) {
 					AddIssue(binaryOperatorExpression, ctx.TranslateString("Remove redundant IsNULL check"), script => {
-						Expression expr = binaryOperatorExpression.Left;
-						script.Replace(binaryOperatorExpression, expr);
-					});
-					return;
-				}
-
-				Match m2 = pattern2.Match(binaryOperatorExpression);
-				if (m2.Success) {
-					AddIssue(binaryOperatorExpression, ctx.TranslateString("Remove redundant IsNULL check"), script => {
-						Expression expr = binaryOperatorExpression.Right;
-						script.Replace(binaryOperatorExpression, expr);
-					});
+					         	var isExpr = m1.Get<AstType>("t").Single().Parent;
+					         	script.Replace(binaryOperatorExpression, isExpr);
+					         });
 					return;
 				}
 			}

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

@@ -27,6 +27,7 @@ using System;
 using System.Collections.Generic;
 using System.Linq;
 using ICSharpCode.NRefactory.PatternMatching;
+using ICSharpCode.NRefactory.Semantics;
 using ICSharpCode.NRefactory.CSharp.Resolver;
 using ICSharpCode.NRefactory.TypeSystem;
 using ICSharpCode.NRefactory.Refactoring;
@@ -86,26 +87,28 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 					return;
 				if (!IsSimpleTarget (invocation.Target))
 					return;
+				var rr = ctx.Resolve (invocation) as CSharpInvocationResolveResult;
+				if (rr == null)
+					return;
 				var lambdaParameters = parameters.ToList();
-				if (lambdaParameters.Count != invocation.Arguments.Count)
+				var arguments = rr.GetArgumentsForCall();
+				if (lambdaParameters.Count != arguments.Count)
 					return;
-				int i = 0;
-				foreach (var param in invocation.Arguments) {
-					var id = param as IdentifierExpression;
-					if (id == null)
-						return;
-					if (lambdaParameters[i].Name != id.Identifier)
+				for (int i = 0; i < arguments.Count; i++) {
+					var arg = UnpackImplicitIdentityOrReferenceConversion(arguments[i]) as LocalResolveResult;
+					if (arg == null || arg.Variable.Name != lambdaParameters[i].Name)
 						return;
-					i++;
 				}
+				var returnConv = ctx.GetConversion(invocation);
+				if (returnConv.IsExplicit || !(returnConv.IsIdentityConversion || returnConv.IsReferenceConversion))
+					return;
 				AddIssue(expression, ctx.TranslateString("Expression can be reduced to delegate"), script =>  {
 					var validTypes = CreateFieldAction.GetValidTypes (ctx.Resolver, expression).ToList ();
 					if (validTypes.Any (t => t.FullName == "System.Func" && t.TypeParameterCount == 1 + parameters.Count) && validTypes.Any (t => t.FullName == "System.Action")) {
-						var rr = ctx.Resolve (invocation) as CSharpInvocationResolveResult;
-						if (rr != null && rr.Member.ReturnType.Kind != ICSharpCode.NRefactory.TypeSystem.TypeKind.Void) {
+						if (rr != null && rr.Member.ReturnType.Kind != TypeKind.Void) {
 							var builder = ctx.CreateTypeSytemAstBuilder (expression);
 							var type = builder.ConvertType(new TopLevelTypeName("System", "Func", 1));
-							var args = type is SimpleType ? ((SimpleType)type).TypeArguments : ((MemberType)type).TypeArguments;
+							var args = type.GetChildrenByRole(Roles.TypeArgument);
 							args.Clear ();
 							foreach (var pde in parameters) {
 								args.Add (builder.ConvertType (ctx.Resolve (pde).Type));
@@ -118,6 +121,14 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 					script.Replace(expression, invocation.Target.Clone());
 				});
 			}
+			
+			static ResolveResult UnpackImplicitIdentityOrReferenceConversion(ResolveResult rr)
+			{
+				var crr = rr as ConversionResolveResult;
+				if (crr != null && crr.Conversion.IsImplicit && (crr.Conversion.IsIdentityConversion || crr.Conversion.IsReferenceConversion))
+					return crr.Input;
+				return rr;
+			}
 
 			public override void VisitLambdaExpression(LambdaExpression lambdaExpression)
 			{

ICSharpCode.NRefactory.CSharp/Refactoring/PatternHelper.cs

@@ -36,5 +36,29 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 				new BinaryOperatorExpression(expr2.Clone(), op, expr1.Clone())
 			};
 		}
+		
+		/// <summary>
+		/// Optionally allows parentheses around the given expression.
+		/// </summary>
+		public static Expression OptionalParentheses(Expression expr)
+		{
+			return new OptionalParenthesesPattern(expr);
+		}
+		
+		sealed class OptionalParenthesesPattern : Pattern
+		{
+			readonly INode child;
+			
+			public OptionalParenthesesPattern(INode child)
+			{
+				this.child = child;
+			}
+			
+			public override bool DoMatch(INode other, Match match)
+			{
+				INode unpacked = ParenthesizedExpression.UnpackParenthesizedExpression(other as Expression);
+				return child.DoMatch(unpacked, match);
+			}
+		}
 	}
 }

ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantNullCheckTests.cs

@@ -82,7 +82,6 @@ class Test {
 		}}}");
 		}
 
-		[Ignore("Missing")]
 		[Test]
 		public void TestCaseWithFullParens()
 		{

ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/SimplifyAnonymousMethodToDelegateIssueTests.cs

@@ -34,60 +34,97 @@ namespace ICSharpCode.NRefactory.CSharp.CodeIssues
 	public class SimplifyAnonymousMethodToDelegateIssueTests : InspectionActionTestBase
 	{
 		[Test]
-		public void TestSimpleLambda ()
+		public void TestSimpleVoidLambda ()
 		{
-			var input = @"class Foo
+			var input = @"using System;
+class Foo
+{
+	void Bar (string str)
+	{
+		Action<int, int> action = $(foo, bar) => MyMethod (foo, bar);
+	}
+	void MyMethod(int foo, int bar) {}
+}";
+			
+			TestRefactoringContext context;
+			var issues = GetIssues (new SimplifyAnonymousMethodToDelegateIssue (), input, out context);
+			Assert.AreEqual (1, issues.Count);
+			CheckFix (context, issues, @"using System;
+class Foo
+{
+	void Bar (string str)
+	{
+		Action<int, int> action = MyMethod;
+	}
+	void MyMethod(int foo, int bar) {}
+}");
+		}
+		
+		[Test]
+		public void TestSimpleBoolLambda ()
+		{
+			var input = @"using System;
+class Foo
 {
 	void Bar (string str)
 	{
-		var action = $(foo, bar) => MyMethod (foo, bar);
+		Func<int, int, bool> action = $(foo, bar) => MyMethod (foo, bar);
 	}
+	bool MyMethod(int foo, int bar) {}
 }";
 			
 			TestRefactoringContext context;
 			var issues = GetIssues (new SimplifyAnonymousMethodToDelegateIssue (), input, out context);
 			Assert.AreEqual (1, issues.Count);
-			CheckFix (context, issues, @"class Foo
+			CheckFix (context, issues, @"using System;
+class Foo
 {
 	void Bar (string str)
 	{
-		var action = MyMethod;
+		Func<int, int, bool> action = MyMethod;
 	}
+	bool MyMethod(int foo, int bar) {}
 }");
 		}
 
 		[Test]
 		public void TestLambdaWithBody ()
 		{
-			var input = @"class Foo
+			var input = @"using System;
+class Foo
 {
 	void Bar (string str)
 	{
-		var action = $(foo, bar) => { return MyMethod (foo, bar); };
+		Action<int, int> action = $(foo, bar) => { return MyMethod (foo, bar); };
 	}
+	void MyMethod(int foo, int bar) {}
 }";
 			
 			TestRefactoringContext context;
 			var issues = GetIssues (new SimplifyAnonymousMethodToDelegateIssue (), input, out context);
 			Assert.AreEqual (1, issues.Count);
-			CheckFix (context, issues, @"class Foo
+			CheckFix (context, issues, @"using System;
+class Foo
 {
 	void Bar (string str)
 	{
-		var action = MyMethod;
+		Action<int, int> action = MyMethod;
 	}
+	void MyMethod(int foo, int bar) {}
 }");
 		}
 
 		[Test]
-		public void TestSimpleInvalidLambda ()
+		public void Lambda_SwapParameterOrder ()
 		{
-			var input = @"class Foo
+			var input = @"using System;
+class Foo
 {
 	void Bar (string str)
 	{
-		var action = $(foo, bar) => MyMethod (bar, foo);
+		Action<int, int> action = $(foo, bar) => MyMethod (bar, foo);
 	}
+	void MyMethod(int foo, int bar) {}
 }";
 			
 			TestRefactoringContext context;
@@ -98,26 +135,28 @@ namespace ICSharpCode.NRefactory.CSharp.CodeIssues
 		[Test]
 		public void TestSimpleAnonymousMethod ()
 		{
-			var input = @"class Foo
+			var input = @"using System;
+class Foo
 {
 	int MyMethod (int x, int y) { return x * y; }
 
 	void Bar (string str)
 	{
-		var action = $delegate(int foo, int bar) { return MyMethod (foo, bar); };
+		Func<int, int, int> action = $delegate(int foo, int bar) { return MyMethod (foo, bar); };
 	}
 }";
 			
 			TestRefactoringContext context;
 			var issues = GetIssues (new SimplifyAnonymousMethodToDelegateIssue (), input, out context);
 			Assert.AreEqual (1, issues.Count);
-			CheckFix (context, issues, @"class Foo
+			CheckFix (context, issues, @"using System;
+class Foo
 {
 	int MyMethod (int x, int y) { return x * y; }
 
 	void Bar (string str)
 	{
-		var action = MyMethod;
+		Func<int, int, int> action = MyMethod;
 	}
 }");
 		}
@@ -134,7 +173,7 @@ class Foo
 
 	void Bar (string str)
 	{
-		var action = $() => str.Where (c => c != 'a').ToArray ();
+		Func<char[]> action = $() => str.Where (c => c != 'a').ToArray ();
 	}
 }";
 			
@@ -144,15 +183,35 @@ class Foo
 		}
 
 		[Test]
-		public void TestComplexCase ()
+		public void CallInvolvesOptionalParameter ()
 		{
-			var input = @"class Foo
+			var input = @"using System;
+class Foo
 {
-	int MyMethod (int x, int y) { return x * y; }
+	int MyMethod (int x, int y = 1) { return x * y; }
 
 	void Bar (string str)
 	{
-		var action = $delegate(int foo, int bar) { return MyMethod (bar, foo); };
+		Func<int, int> action = $foo => MyMethod (foo);
+	}
+}";
+			
+			TestRefactoringContext context;
+			var issues = GetIssues (new SimplifyAnonymousMethodToDelegateIssue (), input, out context);
+			Assert.AreEqual (0, issues.Count);
+		}
+
+		[Test]
+		public void CallExpandsParams ()
+		{
+			var input = @"using System;
+class Foo
+{
+	int MyMethod (params object[] args) { return 0; }
+
+	void Bar (string str)
+	{
+		Func<string, int> action = $foo => MyMethod (foo);
 	}
 }";
 			
@@ -197,6 +256,78 @@ class C
 }");
 
 		}
+		
+		[Test]
+		public void Return_ReferenceConversion ()
+		{
+			var input = @"using System;
+class Foo
+{
+	void Bar (string str)
+	{
+		Func<int, object> action = $foo => MyMethod(foo);
+	}
+	string MyMethod(int foo) {}
+}";
+			
+			TestRefactoringContext context;
+			var issues = GetIssues (new SimplifyAnonymousMethodToDelegateIssue (), input, out context);
+			Assert.AreEqual (1, issues.Count);
+		}
+		
+		[Test]
+		public void Return_BoxingConversion ()
+		{
+			var input = @"using System;
+class Foo
+{
+	void Bar (string str)
+	{
+		Func<int, object> action = $foo => MyMethod(foo);
+	}
+	bool MyMethod(int foo) {}
+}";
+			
+			TestRefactoringContext context;
+			var issues = GetIssues (new SimplifyAnonymousMethodToDelegateIssue (), input, out context);
+			Assert.AreEqual (0, issues.Count);
+		}
+		
+		[Test]
+		public void Parameter_ReferenceConversion ()
+		{
+			var input = @"using System;
+class Foo
+{
+	void Bar (string str)
+	{
+		Action<string> action = $foo => MyMethod(foo);
+	}
+	void MyMethod(object foo) {}
+}";
+			
+			TestRefactoringContext context;
+			var issues = GetIssues (new SimplifyAnonymousMethodToDelegateIssue (), input, out context);
+			Assert.AreEqual (1, issues.Count);
+		}
+		
+		[Test]
+		public void Parameter_BoxingConversion ()
+		{
+			var input = @"using System;
+class Foo
+{
+	void Bar (string str)
+	{
+		Action<int> action = $foo => MyMethod(foo);
+	}
+	void MyMethod(object foo) {}
+}";
+			
+			TestRefactoringContext context;
+			var issues = GetIssues (new SimplifyAnonymousMethodToDelegateIssue (), input, out context);
+			Assert.AreEqual (0, issues.Count);
+		}
 	}
 }