From 53d9b6e7eedf2305c8517ab3ff9fd6d67201fb6c Mon Sep 17 00:00:00 2001 From: Simon Lindgren Date: Wed, 11 Jul 2012 11:47:18 +0200 Subject: [PATCH] [CodeIssues] Fix some false positives in ParameterCanBeDemotedIssue. --- .../ParameterCanBeDemotedIssue.cs | 7 + .../TypeCriteriaCollector.cs | 34 +++-- .../ParameterCanBeDemotedTests.cs | 128 ++++++++++++++++++ 3 files changed, 158 insertions(+), 11 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ParameterCanBeDemotedIssue/ParameterCanBeDemotedIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ParameterCanBeDemotedIssue/ParameterCanBeDemotedIssue.cs index fdc3663ece..b64a87c446 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ParameterCanBeDemotedIssue/ParameterCanBeDemotedIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ParameterCanBeDemotedIssue/ParameterCanBeDemotedIssue.cs @@ -57,6 +57,13 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring { base.VisitMethodDeclaration(methodDeclaration); + var declarationResolveResult = context.Resolve(methodDeclaration) as MemberResolveResult; + if (declarationResolveResult == null) + return; + var member = declarationResolveResult.Member; + if (member.IsOverride || member.IsOverridable) + return; + var collector = new TypeCriteriaCollector(context); methodDeclaration.AcceptVisitor(collector); diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ParameterCanBeDemotedIssue/TypeCriteriaCollector.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ParameterCanBeDemotedIssue/TypeCriteriaCollector.cs index 0c6337c69b..b7cff61dca 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ParameterCanBeDemotedIssue/TypeCriteriaCollector.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/ParameterCanBeDemotedIssue/TypeCriteriaCollector.cs @@ -55,15 +55,33 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring TypeCriteria[variable] = new List(); TypeCriteria[variable].Add(criterion); } - + + public override void VisitMemberReferenceExpression(MemberReferenceExpression memberReferenceExpression) + { + base.VisitMemberReferenceExpression(memberReferenceExpression); + + var memberResolveResult = context.Resolve(memberReferenceExpression) as MemberResolveResult; + if (memberResolveResult == null) + return; + var targetResolveResult = memberResolveResult.TargetResult as LocalResolveResult; + if (targetResolveResult == null) + return; + var variable = targetResolveResult.Variable; + AddCriterion(variable, new HasMemberCriterion(memberResolveResult.Member)); + } + public override void VisitInvocationExpression(InvocationExpression invocationExpression) { base.VisitInvocationExpression(invocationExpression); + var resolveResult = context.Resolve(invocationExpression); var invocationResolveResult = resolveResult as InvocationResolveResult; if (invocationResolveResult == null) return; CollectRestrictionsFromNodes(invocationExpression.Arguments); + + // invocationExpression.Target resolves to a method group and VisitMemberReferenceExpression + // only handles members, so handle method groups here var targetResolveResult = invocationResolveResult.TargetResult as LocalResolveResult; if (targetResolveResult == null) return; @@ -73,18 +91,12 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring void CollectRestrictionsFromNodes(IEnumerable expressions) { - var identifiers = - from expression in expressions - from node in expression.DescendantsAndSelf - let identifier = node as IdentifierExpression - where identifier != null - select identifier; - foreach (var identifier in identifiers) { - var resolveResult2 = context.Resolve(identifier); - var argumentResolveResult = resolveResult2 as LocalResolveResult; + foreach (var expression in expressions) { + var resolveResult = context.Resolve(expression); + var argumentResolveResult = resolveResult as LocalResolveResult; if (argumentResolveResult == null) continue; - var expectedType = context.GetExpectedType(identifier); + var expectedType = context.GetExpectedType(expression); AddCriterion(argumentResolveResult.Variable, new IsTypeCriterion(expectedType)); } } diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterCanBeDemotedIssue/ParameterCanBeDemotedTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterCanBeDemotedIssue/ParameterCanBeDemotedTests.cs index a2cf688dc9..f18e35b91e 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterCanBeDemotedIssue/ParameterCanBeDemotedTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterCanBeDemotedIssue/ParameterCanBeDemotedTests.cs @@ -92,6 +92,88 @@ class A Assert.AreEqual(0, issues.Count); } + [Test] + public void IgnoresOverrides() + { + var input = @" +interface IA +{ + void Foo(); +} +class B : IA +{ + public virtual void Foo() {} + public virtual void Bar() {} +} +class TestBase +{ + public void F(B b) { + b.Foo(); + b.Bar(); + } +} +class TestClass : TestBase +{ + public override void F(B b) + { + b.Foo(); + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new ParameterCanBeDemotedIssue(), input, out context); + Assert.AreEqual(0, issues.Count); + } + + [Test] + public void IgnoresOverridables() + { + var input = @" +interface IA +{ + void Foo(); +} +class B : IA +{ + public virtual void Foo() {} + public virtual void Bar() {} +} +class TestClass +{ + public virtual void F(B b) + { + b.Foo(); + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new ParameterCanBeDemotedIssue(), input, out context); + Assert.AreEqual(0, issues.Count); + } + + [Test] + public void HandlesNeededProperties() + { + var input = @" +interface IA +{ + void Foo(string s); +} +class B : IA +{ + public virtual void Foo(string s) {} + public string Property { get; } +} +class TestClass +{ + public void F(B b) + { + b.Foo(b.Property); + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new ParameterCanBeDemotedIssue(), input, out context); + Assert.AreEqual(0, issues.Count); + } + [Test] public void InterfaceTest() { @@ -280,6 +362,52 @@ class Test void DemandType(D d) { } +}"); + } + + [Test] + public void RespectsOutgoingCallsTypeRestrictionsWhenPassingResultFromMember() + { + var input = @" +interface IA +{ + void Foo(string s); + string Property { get; } +} +class B : IA +{ + public virtual void Foo(string s) {} + public string Property { get; } +} +class TestClass +{ + public void F(B b) + { + b.Foo(b.Property); + } +}"; + TestRefactoringContext context; + var issues = GetIssues(new ParameterCanBeDemotedIssue(), input, out context); + Assert.AreEqual(1, issues.Count); + var issue = issues [0]; + Assert.AreEqual(1, issue.Actions.Count); + CheckFix(context, issues [0], @" +interface IA +{ + void Foo(string s); + string Property { get; } +} +class B : IA +{ + public virtual void Foo(string s) {} + public string Property { get; } +} +class TestClass +{ + public void F(IA b) + { + b.Foo(b.Property); + } }"); } }