From f829d5833c1244c90dcbbbe6d2bfb45a4847af54 Mon Sep 17 00:00:00 2001 From: Simon Lindgren Date: Sat, 8 Sep 2012 23:37:19 +0200 Subject: [PATCH 1/3] [CodeIssues] Don't mark calls to IFormattable.ToString() as redundant. --- .../CodeIssues/RedundantToStringIssue.cs | 3 ++- .../CSharp/CodeIssues/RedundantToStringTests.cs | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantToStringIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantToStringIssue.cs index 10e6f6d143..8d7df7b841 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantToStringIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/RedundantToStringIssue.cs @@ -79,7 +79,8 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring if (resolveResult == null) { return; } - if (resolveResult.Member.Name != "ToString") { + var member = resolveResult.Member; + if (member.Name != "ToString" || member.Parameters.Count != 0) { return; } AddRedundantToStringIssue(memberExpression, invocationExpression); diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantToStringTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantToStringTests.cs index 1bd2dbc6e0..7925e405c9 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantToStringTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/RedundantToStringTests.cs @@ -76,6 +76,23 @@ class Foo Assert.AreEqual (0, issues.Count); } + [Test] + public void IgnoresCallsToIFormattableToString () + { + var input = @" +class Foo +{ + void Bar (System.DateTime dt) + { + string s = dt.ToString("""", CultureInfo.InvariantCulture) + string.Empty; + } +}"; + + TestRefactoringContext context; + var issues = GetIssues (new RedundantToStringIssue (), input, out context); + Assert.AreEqual (0, issues.Count); + } + [Test] public void StringTarget () { From 3b5e5731eae3b04de80c4f1714106ccacdfecc06 Mon Sep 17 00:00:00 2001 From: Simon Lindgren Date: Sun, 9 Sep 2012 01:17:47 +0200 Subject: [PATCH 2/3] [CodeIssues] Don't warn for unused parameters on overrides and interface implementations. --- .../ParameterNotUsedIssue.cs | 20 +++++++++++++ .../CodeIssues/ParameterNotUsedIssueTests.cs | 30 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableNotUsedIssues/ParameterNotUsedIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableNotUsedIssues/ParameterNotUsedIssue.cs index 1a07c73eac..d8b24b8b56 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableNotUsedIssues/ParameterNotUsedIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableNotUsedIssues/ParameterNotUsedIssue.cs @@ -25,6 +25,8 @@ // THE SOFTWARE. using ICSharpCode.NRefactory.Semantics; +using System.Linq; +using ICSharpCode.NRefactory.TypeSystem; namespace ICSharpCode.NRefactory.CSharp.Refactoring { @@ -51,6 +53,24 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring this.unit = unit; } + public override void VisitMethodDeclaration(MethodDeclaration methodDeclaration) + { + // Only some methods are candidates for the warning + + if (methodDeclaration.Body.IsNull) + return; + var methodResolveResult = ctx.Resolve(methodDeclaration) as MemberResolveResult; + if (methodResolveResult == null) + return; + var member = methodResolveResult.Member; + if (member.IsOverride) + return; + if (member.ImplementedInterfaceMembers.Any ()) + return; + + base.VisitMethodDeclaration(methodDeclaration); + } + public override void VisitParameterDeclaration (ParameterDeclaration parameterDeclaration) { base.VisitParameterDeclaration (parameterDeclaration); diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterNotUsedIssueTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterNotUsedIssueTests.cs index 81da14b795..e939c3cfa0 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterNotUsedIssueTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/ParameterNotUsedIssueTests.cs @@ -43,6 +43,36 @@ class TestClass { }"; Test (input, 1); } + + [Test] + public void TestInterfaceImplementation () + { + var input = @" +interface ITestClass { + void TestMethod (int i); +} +class TestClass : ITestClass { + public void TestMethod (int i) + { + } +}"; + Test (input, 0); + } + + [Test] + public void TestAbstractMethodImplementation () + { + var input = @" +abstract class TestBase { + public abstract void TestMethod (int i); +} +class TestClass : TestBase { + public override void TestMethod (int i) + { + } +}"; + Test (input, 0); + } [Test] public void TestUsedParameter () From b0e1fac6b6a1618b9be19b3e241efabac050daa4 Mon Sep 17 00:00:00 2001 From: Simon Lindgren Date: Tue, 11 Sep 2012 01:02:23 +0200 Subject: [PATCH 3/3] [CodeIssues] Don't suggest moving declarations into invalid or weird places. --- .../VariableDeclaredInWideScopeIssue.cs | 36 +++++++++++++++---- .../VariableDeclaredInWideScopeTests.cs | 21 +++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs index 9746aaa371..83facbfdd9 100644 --- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs +++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs @@ -88,14 +88,15 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring var path = GetPath(rootNode, deepestCommonAncestor); // The node that will follow the moved declaration statement - AstNode anchorNode = GetInitialAnchorNode (rootNode, identifiers, path); + AstNode anchorNode = GetInitialAnchorNode(rootNode, identifiers, path); // Restrict path to only those where the initializer has not changed var pathToCheck = path.Skip(1).ToList(); var firstInitializerChangeNode = GetFirstInitializerChange(variableDeclarationStatement, pathToCheck, variableInitializer.Initializer); if (firstInitializerChangeNode != null) { - // The initializer and usage nodes may not be in the same path, so merge them - // instead of just making a path to firstInitializerChange + // The node changing the initializer expression may not be on the path + // to the actual usages of the variable, so we need to merge the paths + // so we get the part of the paths that are common between them var pathToChange = GetPath(rootNode, firstInitializerChangeNode); var deepestCommonIndex = GetLowestCommonAncestorIndex(path, pathToChange); anchorNode = pathToChange [deepestCommonIndex + 1]; @@ -109,9 +110,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring anchorNode = firstBlackListedNode; } - // Get the parent statement - while (anchorNode != null && !(anchorNode is Statement)) - anchorNode = anchorNode.Parent; + anchorNode = GetInsertionPoint(anchorNode); if (anchorNode != null && anchorNode != rootNode && anchorNode.Parent != rootNode) { AddIssue(variableDeclarationStatement, context.TranslateString("Variable could be moved to a nested scope"), @@ -119,6 +118,31 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring } } + static bool IsBannedInsertionPoint(AstNode anchorNode) + { + var parent = anchorNode.Parent; + + // Don't split 'else if ...' into else { if ... } + if (parent is IfElseStatement && anchorNode is IfElseStatement) + return true; + // Don't allow moving the declaration into the resource aquisition of a using statement + if (parent is UsingStatement) + return true; + return false; + } + + static AstNode GetInsertionPoint(AstNode node) + { + while (true) { + if (node == null) + break; + if (node is Statement && !IsBannedInsertionPoint(node)) + break; + node = node.Parent; + } + return node; + } + AstNode GetInitialAnchorNode (BlockStatement rootNode, List identifiers, IList path) { if (identifiers.Count > 1) { diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs index 397374bfb4..cfa1c737e2 100644 --- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs +++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs @@ -379,6 +379,27 @@ class A } "); } + + [Test] + public void DoesNotExpandElseClause () + { + TestStatements(@" + string o = ""Hello World !""; + if (false) { + } else if (!o.Contains (""Foo"")) { + } +", 0); + } + + [Test] + public void DoesNotInsertBlockStatementInResourceAquisition () + { + TestStatements(@" + string o = ""Hello World !""; + using (var file = File.Open(o, FileMode.Open)) + return; +", 0); + } } }