Merge pull request #112 from riviti/gsoc-fixes

... More gsoc fixes

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);

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

@@ -94,8 +94,9 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 				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
+					// The node changing the initializer expression may not be on the path
-					// instead of just making a path to firstInitializerChange
+					// 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
+				anchorNode = GetInsertionPoint(anchorNode);
-				while (anchorNode != null && !(anchorNode is Statement))
-					anchorNode = anchorNode.Parent;
 
 				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<IdentifierExpression> identifiers, IList<AstNode> path)
 			{
 				if (identifiers.Count > 1) {

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);

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

@@ -44,6 +44,36 @@ class TestClass {
 			Test<ParameterNotUsedIssue> (input, 1);
 		}
 		
+		[Test]
+		public void TestInterfaceImplementation ()
+		{
+			var input = @"
+interface ITestClass {
+	void TestMethod (int i);
+}
+class TestClass : ITestClass {
+	public void TestMethod (int i)
+	{
+	}
+}";
+			Test<ParameterNotUsedIssue> (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<ParameterNotUsedIssue> (input, 0);
+		}
+
 		[Test]
 		public void TestUsedParameter ()
 		{

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

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);
+		}
 	}
 }