From 551869bd41e80d08472734aa49dbd5fcc178372e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mike=20Kr=C3=BCger?= <mkrueger@xamarin.com>
Date: Wed, 13 Feb 2013 08:13:11 +0100
Subject: [PATCH] VariableDeclaredInWideScopeIssue no longer suggests moving
 method call initializers.

---
 .../VariableDeclaredInWideScopeIssue.cs       | 31 ++++++++-
 .../VariableDeclaredInWideScopeTests.cs       | 68 ++++++-------------
 2 files changed, 50 insertions(+), 49 deletions(-)

diff --git a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs
index aa59d96a64..4635c1b04c 100644
--- a/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs
+++ b/ICSharpCode.NRefactory.CSharp/Refactoring/CodeIssues/VariableDeclaredInWideScopeIssue.cs
@@ -63,7 +63,33 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 				typeof(LambdaExpression),
 				typeof(LockStatement)
 			};
-		
+
+			class CheckInitializer : DepthFirstAstVisitor
+			{
+				public bool IsValid {
+					get;
+					private set;
+				}
+
+				public CheckInitializer()
+				{
+					IsValid = true;
+				}
+
+				public override void VisitInvocationExpression(InvocationExpression invocationExpression)
+				{
+					base.VisitInvocationExpression(invocationExpression);
+					IsValid = false;
+				}
+			}
+			
+			bool CheckForInvocations(Expression initializer)
+			{
+				var visitor = new CheckInitializer();
+				initializer.AcceptVisitor(visitor);
+				return visitor.IsValid;
+			}
+
 			public override void VisitVariableDeclarationStatement(VariableDeclarationStatement variableDeclarationStatement)
 			{
 				base.VisitVariableDeclarationStatement(variableDeclarationStatement);
@@ -84,6 +110,9 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 					// variable is not used
 					return;
 
+				if (!CheckForInvocations(variableInitializer.Initializer))
+					return;
+
 				AstNode deepestCommonAncestor = GetDeepestCommonAncestor(rootNode, identifiers);
 				var path = GetPath(rootNode, deepestCommonAncestor);
 
diff --git a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs
index d842626ca7..c81f56e5e8 100644
--- a/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs
+++ b/ICSharpCode.NRefactory.Tests/CSharp/CodeIssues/VariableDeclaredInWideScopeTests.cs
@@ -187,7 +187,7 @@ class A
 		public void IfElseIf()
 		{
 			TestStatements(@"
-		int val = GetValue ();
+		int val = 12;
 		if (condition) {
 			int val2 = GetValue ();
 			Console.WriteLine(val2);
@@ -199,7 +199,7 @@ class A
 			int val2 = GetValue ();
 			Console.WriteLine(val2);
 		} else if (!condition) {
-			int val = GetValue ();
+			int val = 12;
 			Console.WriteLine(val);
 		}
 ");
@@ -302,58 +302,18 @@ class A
 ");
 		}
 		
-		[Test]
-		public void SuggestsMovingMethodCallInitializer ()
-		{	
-				TestStatements(@"
-		int j = GetValue ();
-		if (true) {
-			Console.WriteLine(j);
-		}
-", 1, @"
-		if (true) {
-			int j = GetValue ();
-			Console.WriteLine(j);
-		}
-");
-		}
-
-		[Test]
-		public void DoesNotReorderMethodCalls ()
-		{	
-			TestStatements(@"
-		int k = GetValue ();
-		if (b > 3) {
-			if (a < 2) {
-				// This should block any move past this point
-				SetValue (2);
-			}
-			Console.WriteLine(k);
-		}
-", 1, @"
-		if (b > 3) {
-			int k = GetValue ();
-			if (a < 2) {
-				// This should block any move past this point
-				SetValue (2);
-			}
-			Console.WriteLine(k);
-		}
-");
-		}
-		
 		[Test]
 		public void CommonParentStatement ()
 		{	
 			TestStatements(@"
-		int k = GetValue ();
+		int k = 12;
 		{
 			SetValue (k);
 			SetValue (k);
 		}
 ", 1, @"
 		{
-			int k = GetValue ();
+			int k = 12;
 			SetValue (k);
 			SetValue (k);
 		}
@@ -364,7 +324,7 @@ class A
 		public void CaseTarget ()
 		{	
 			TestStatements(@"
-		int j = GetValue ();
+		int j = 12;
 		switch (a) {
 		case 2:
 			SetValue (j + 1);
@@ -373,7 +333,7 @@ class A
 ", 1, @"
 		switch (a) {
 		case 2:
-			int j = GetValue ();
+			int j = 12;
 			SetValue (j + 1);
 			break;
 		}
@@ -395,14 +355,14 @@ class A
 		public void DoesNotExpandForStatement ()
 		{	
 			TestStatements(@"
-		var val = GetValue ();
+		var val = 12;
 		if (false) {
 			for (var i = GetValue (); ; i++)
 				System.Console.WriteLine (val);
 		}
 ", 1, @"
 		if (false) {
-			var val = GetValue ();
+			var val = 12;
 			for (var i = GetValue (); ; i++)
 				System.Console.WriteLine (val);
 		}
@@ -453,6 +413,18 @@ class A
 			Assert.AreEqual(0, issues.Count);
 		}
 
+		
+		[Test]
+		public void DoesNotSuggestMovingMethodCallInitializer()
+		{
+			// Method calls can have side effects therefore it's generally not possible to move them.
+			TestStatements(@"
+		int val = GetValue ();
+		if (true) {
+			System.Console.WriteLine(val);
+		}
+");
+		}
 
 
 	}