From 4a8a61ee2e77ed4c34d93c413280c9cf3b562dec Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Wed, 29 May 2013 22:06:49 +0200 Subject: [PATCH] Fix false positives in SimplifyAnonymousMethodToDelegateIssue: the simplification is not possible when the lambda involves non-reference conversions. --- .../CodeIssues/RedundantNullCheckIssue.cs | 68 +++---- .../SimplifyAnonymousMethodToDelegateIssue.cs | 33 ++-- .../Refactoring/PatternHelper.cs | 24 +++ .../CodeIssues/RedundantNullCheckTests.cs | 1 - ...lifyAnonymousMethodToDelegateIssueTests.cs | 173 +++++++++++++++--- 5 files changed, 228 insertions(+), 71 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantNullCheckIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantNullCheckIssue.cs index ad15a25423..2755bb075f 100755 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantNullCheckIssue.cs +++ b/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 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("t").Single().Parent; + script.Replace(binaryOperatorExpression, isExpr); + }); return; } } diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/SimplifyAnonymousMethodToDelegateIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/SimplifyAnonymousMethodToDelegateIssue.cs index 8cf418cacb..65ab0ae22f 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/SimplifyAnonymousMethodToDelegateIssue.cs +++ b/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) { diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/PatternHelper.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/PatternHelper.cs index a85c59f207..ac11889f21 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/PatternHelper.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/PatternHelper.cs @@ -36,5 +36,29 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring new BinaryOperatorExpression(expr2.Clone(), op, expr1.Clone()) }; } + + /// + /// Optionally allows parentheses around the given expression. + /// + 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); + } + } } } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantNullCheckTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantNullCheckTests.cs index b45fddf8f2..418657ac4a 100755 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantNullCheckTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantNullCheckTests.cs @@ -82,7 +82,6 @@ class Test { }}}"); } - [Ignore("Missing")] [Test] public void TestCaseWithFullParens() { diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/SimplifyAnonymousMethodToDelegateIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/SimplifyAnonymousMethodToDelegateIssueTests.cs index dbc1b8b7f2..1f8f117308 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/SimplifyAnonymousMethodToDelegateIssueTests.cs +++ b/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 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 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 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 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 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 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 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 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 action = MyMethod; } }"); } @@ -134,7 +173,7 @@ class Foo void Bar (string str) { - var action = $() => str.Where (c => c != 'a').ToArray (); + Func 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 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 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 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 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 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 action = $foo => MyMethod(foo); + } + void MyMethod(object foo) {} +}"; + + TestRefactoringContext context; + var issues = GetIssues (new SimplifyAnonymousMethodToDelegateIssue (), input, out context); + Assert.AreEqual (0, issues.Count); + } } }