[CodeIssues] VariableDeclaredInWideScopeIssue: Handle nested scopes that are not BlockStatements and don't suggest moving declarations into closures

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

@@ -51,12 +51,14 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 				this.context = context;
 			}
 
-			static IList<Type> blockingStatements = new List<Type>() {
+			static IList<Type> moveTargetBlacklist = new List<Type>() {
 				typeof(WhileStatement),
 				typeof(ForeachStatement),
 				typeof(ForStatement),
 				typeof(DoWhileStatement),
-				typeof(TryCatchStatement)
+				typeof(TryCatchStatement),
+				typeof(AnonymousMethodExpression),
+				typeof(LambdaExpression)
 			};
 		
 			public override void VisitVariableDeclarationStatement(VariableDeclarationStatement variableDeclarationStatement)
@@ -86,7 +88,7 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 				var path = GetPath(rootNode, lowestCommonAncestor);
 
 				var firstLoopStatement = (from node in path
-				                          where blockingStatements.Contains(node.GetType())
+				                          where moveTargetBlacklist.Contains(node.GetType())
 				                          select node).FirstOrDefault();
 				IList<AstNode> possibleDestinationsPath;
 				if (firstLoopStatement == null) {
@@ -94,22 +96,54 @@ namespace ICSharpCode.NRefactory.CSharp.Refactoring
 				} else {
 					possibleDestinationsPath = GetPath(rootNode, firstLoopStatement);
 				}
-				var mostNestedBlockStatement = (from node in possibleDestinationsPath
+				var mostNestedEmbeddedStatement = (from node in possibleDestinationsPath
-				                                let block = node as BlockStatement
+				                                   let statement = node as Statement
-				                                where block != null
+				                                   let blockStatement = statement as BlockStatement
-				                                select block).LastOrDefault();
+				                                   where statement != null && IsNestedScope(statement)
+				                                   select statement).LastOrDefault();
 
-				if (mostNestedBlockStatement != null && mostNestedBlockStatement != rootNode) {
+				if (mostNestedEmbeddedStatement != null && mostNestedEmbeddedStatement != rootNode) {
 					AddIssue(variableDeclarationStatement, context.TranslateString("Variable could be moved to a nested scope"),
-					         GetActions(variableDeclarationStatement, mostNestedBlockStatement));
+					         GetActions(variableDeclarationStatement, mostNestedEmbeddedStatement));
 				}
 			}
 
-			IEnumerable<CodeAction> GetActions(VariableDeclarationStatement declaration, BlockStatement insertTarget)
+			bool IsNestedScope(AstNode node)
+			{
+				var blockStatement = node as BlockStatement;
+				if (blockStatement != null)
+					return true;
+
+				var statement = node as Statement;
+				if (statement == null)
+					return false;
+
+				var role = node.Role;
+				if (role == Roles.EmbeddedStatement ||
+					role == IfElseStatement.TrueRole ||
+					role == IfElseStatement.FalseRole) {
+					return true;
+				}
+				return false;
+			}
+
+			IEnumerable<CodeAction> GetActions(VariableDeclarationStatement declaration, Statement targetScope)
 			{
 				yield return new CodeAction(context.TranslateString("Move to nested scope"), script => {
+					var blockStatement = targetScope as BlockStatement;
+					if (blockStatement == null) {
+						var newBlockStatement = new BlockStatement {
+							Statements = {
+								declaration.Clone(),
+								targetScope.Clone()
+							}
+						};
+						script.Replace(targetScope, newBlockStatement);
+						script.FormatText(targetScope.Parent);
+					} else {
+						script.InsertBefore(blockStatement.Statements.First(), declaration.Clone());
+					}
 					script.Remove(declaration);
-					script.InsertBefore(insertTarget.Statements.First(), declaration.Clone());
 				});
 			}
 

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

@@ -288,6 +288,60 @@ class A
 			var issues = GetIssues(new VariableDeclaredInWideScopeIssue(), input, out context);
 			Assert.AreEqual(0, issues.Count);
 		}
+		
+		[Test]
+		public void DoesNotSuggestMovingIntoClosure ()
+		{	
+			var input = @"
+using System;
+class A
+{
+	void F()
+	{
+		int valA = 2;
+		Action a = () => Console.WriteLine(valA);
+
+		int valB = 2;
+		Action b = () => { Console.WriteLine(valB); };
+
+		int valC = 2;
+		Action c = delegate() { Console.WriteLine(valC); };
+	}
+}";
+			TestRefactoringContext context;
+			var issues = GetIssues(new VariableDeclaredInWideScopeIssue(), input, out context);
+			Assert.AreEqual(0, issues.Count);
+		}
+
+		[Test]
+		public void TestEmbeddedNonBlockStatement ()
+		{
+			var input = @"
+class A
+{
+	void F()
+	{
+		int val = 2;
+		if (true)
+			System.Console.WriteLine (val);
+	}
+}";
+			TestRefactoringContext context;
+			var issues = GetIssues(new VariableDeclaredInWideScopeIssue(), input, out context);
+			Assert.AreEqual(1, issues.Count);
+			
+			CheckFix(context, issues [0], @"
+class A
+{
+	void F()
+	{
+		if (true) {
+			int val = 2;
+			System.Console.WriteLine (val);
+		}
+	}
+}");
+		}
 	}
 }