Fix issues with 'DeclareVariables' and adjust 'foreach' pattern to the new variable handling.

ICSharpCode.Decompiler/Ast/NRefactoryExtensions.cs

@@ -3,6 +3,7 @@
 
 using System;
 using ICSharpCode.NRefactory.CSharp;
+using ICSharpCode.NRefactory.CSharp.PatternMatching;
 
 namespace ICSharpCode.Decompiler.Ast
 {
@@ -29,6 +30,16 @@ namespace ICSharpCode.Decompiler.Ast
 			return node;
 		}
 		
+		public static Expression WithName(this Expression node, string patternGroupName)
+		{
+			return new NamedNode(patternGroupName, node);
+		}
+		
+		public static Statement WithName(this Statement node, string patternGroupName)
+		{
+			return new NamedNode(patternGroupName, node);
+		}
+		
 		public static void AddNamedArgument(this NRefactory.CSharp.Attribute attribute, string name, Expression argument)
 		{
 			attribute.Arguments.Add(new AssignmentExpression(new IdentifierExpression(name), argument));

ICSharpCode.Decompiler/Ast/Transforms/DeclareVariables.cs

@@ -72,7 +72,103 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 			// declarationPoint: The point where the variable would be declared, if we decide to declare it in this block
 			Statement declarationPoint = null;
 			// Check whether we can move down the variable into the sub-blocks
-			bool ok = true;
+			bool canMoveVariableIntoSubBlocks = FindDeclarationPoint(daa, variableName, allowPassIntoLoops, block, out declarationPoint);
+			if (declarationPoint == null) {
+				// The variable isn't used at all
+				return;
+			}
+			if (canMoveVariableIntoSubBlocks) {
+				// Declare the variable within the sub-blocks
+				foreach (Statement stmt in block.Statements) {
+					ForStatement forStmt = stmt as ForStatement;
+					if (forStmt != null && forStmt.Initializers.Count == 1) {
+						// handle the special case of moving a variable into the for initializer
+						if (TryConvertAssignmentExpressionIntoVariableDeclaration(forStmt.Initializers.Single(), type, variableName))
+							continue;
+					}
+					UsingStatement usingStmt = stmt as UsingStatement;
+					if (usingStmt != null && usingStmt.ResourceAcquisition is AssignmentExpression) {
+						// handle the special case of moving a variable into a using statement
+						if (TryConvertAssignmentExpressionIntoVariableDeclaration((Expression)usingStmt.ResourceAcquisition, type, variableName))
+							continue;
+					}
+					foreach (BlockStatement subBlock in stmt.Children.OfType<BlockStatement>()) {
+						DeclareVariableInBlock(daa, subBlock, type, variableName, allowPassIntoLoops);
+					}
+				}
+			} else {
+				// Try converting an assignment expression into a VariableDeclarationStatement
+				if (!TryConvertAssignmentExpressionIntoVariableDeclaration(declarationPoint, type, variableName)) {
+					// Declare the variable in front of declarationPoint
+					block.Statements.InsertBefore(
+						declarationPoint,
+						new VariableDeclarationStatement((AstType)type.Clone(), variableName)
+						.WithAnnotation(declaredVariableAnnotation));
+				}
+			}
+		}
+
+		bool TryConvertAssignmentExpressionIntoVariableDeclaration(Statement declarationPoint, AstType type, string variableName)
+		{
+			// convert the declarationPoint into a VariableDeclarationStatement
+			ExpressionStatement es = declarationPoint as ExpressionStatement;
+			if (es != null) {
+				AssignmentExpression ae = es.Expression as AssignmentExpression;
+				if (ae != null && ae.Operator == AssignmentOperatorType.Assign) {
+					IdentifierExpression ident = ae.Left as IdentifierExpression;
+					if (ident != null && ident.Identifier == variableName) {
+						declarationPoint.ReplaceWith(new VariableDeclarationStatement {
+						                             	Type = (AstType)type.Clone(),
+						                             	Variables = { new VariableInitializer(variableName, ae.Right.Detach()).CopyAnnotationsFrom(ae) }
+						                             }.CopyAnnotationsFrom(es).WithAnnotation(new DeclaredVariableAnnotation(es)));
+						return true;
+					}
+				}
+			}
+			return false;
+		}
+		
+		bool TryConvertAssignmentExpressionIntoVariableDeclaration(Expression expression, AstType type, string variableName)
+		{
+			AssignmentExpression ae = expression as AssignmentExpression;
+			if (ae != null && ae.Operator == AssignmentOperatorType.Assign) {
+				IdentifierExpression ident = ae.Left as IdentifierExpression;
+				if (ident != null && ident.Identifier == variableName) {
+					expression.ReplaceWith(new VariableDeclarationStatement {
+					                       	Type = (AstType)type.Clone(),
+					                       	Variables = { new VariableInitializer(variableName, ae.Right.Detach()).CopyAnnotationsFrom(ae) }
+					                       }.WithAnnotation(declaredVariableAnnotation));
+					return true;
+				}
+			}
+			return false;
+		}
+		
+		/// <summary>
+		/// Finds the declaration point for the variable within the specified block.
+		/// </summary>
+		/// <param name="daa">
+		/// Definite assignment analysis, must be prepared for 'block' or one of its parents.
+		/// </param>
+		/// <param name="varDecl">The variable to declare</param>
+		/// <param name="block">The block in which the variable should be declared</param>
+		/// <param name="declarationPoint">
+		/// Output parameter: the first statement within 'block' where the variable needs to be declared.
+		/// </param>
+		/// <returns>
+		/// Returns whether it is possible to move the variable declaration into sub-blocks.
+		/// </returns>
+		public static bool FindDeclarationPoint(DefiniteAssignmentAnalysis daa, VariableDeclarationStatement varDecl, BlockStatement block, out Statement declarationPoint)
+		{
+			string variableName = varDecl.Variables.Single().Name;
+			bool allowPassIntoLoops = varDecl.Variables.Single().Annotation<DelegateConstruction.CapturedVariableAnnotation>() == null;
+			return FindDeclarationPoint(daa, variableName, allowPassIntoLoops, block, out declarationPoint);
+		}
+		
+		static bool FindDeclarationPoint(DefiniteAssignmentAnalysis daa, string variableName, bool allowPassIntoLoops, BlockStatement block, out Statement declarationPoint)
+		{
+			// declarationPoint: The point where the variable would be declared, if we decide to declare it in this block
+			declarationPoint = null;
 			foreach (Statement stmt in block.Statements) {
 				if (UsesVariable(stmt, variableName)) {
 					if (declarationPoint == null)
@@ -80,8 +176,7 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 					if (!CanMoveVariableUseIntoSubBlock(stmt, variableName, allowPassIntoLoops)) {
 						// If it's not possible to move the variable use into a nested block,
 						// we need to declare the variable in this block
-						ok = false;
-						break;
+						return false;
 					}
 					// If we can move the variable into the sub-block, we need to ensure that the remaining code
 					// does not use the value that was assigend by the first sub-block
@@ -104,55 +199,19 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 						daa.SetAnalyzedRange(nextStatement, block);
 						daa.Analyze(variableName);
 						if (daa.UnassignedVariableUses.Count > 0) {
-							ok = false;
-							break;
+							return false;
 						}
 					}
 				}
 			}
-			if (declarationPoint == null) {
-				// The variable isn't used at all
-				return;
-			}
-			if (ok) {
-				// Declare the variable within the sub-blocks
-				foreach (Statement stmt in block.Statements) {
-					foreach (BlockStatement subBlock in stmt.Children.OfType<BlockStatement>()) {
-						DeclareVariableInBlock(daa, subBlock, type, variableName, allowPassIntoLoops);
-					}
-				}
-			} else {
-				// Try converting an assignment expression into a VariableDeclarationStatement
-				ExpressionStatement es = declarationPoint as ExpressionStatement;
-				if (es != null) {
-					AssignmentExpression ae = es.Expression as AssignmentExpression;
-					if (ae != null && ae.Operator == AssignmentOperatorType.Assign) {
-						IdentifierExpression ident = ae.Left as IdentifierExpression;
-						if (ident != null && ident.Identifier == variableName) {
-							// convert the declarationPoint into a VariableDeclarationStatement
-							declarationPoint.ReplaceWith(
-								new VariableDeclarationStatement {
-									Type = (AstType)type.Clone(),
-									Variables = {
-										new VariableInitializer(variableName, ae.Right.Detach()).CopyAnnotationsFrom(ae)
-									}
-								}.CopyAnnotationsFrom(es).WithAnnotation(new DeclaredVariableAnnotation(es)));
-							return;
-						}
-					}
-				}
-				// Declare the variable in front of declarationPoint
-				block.Statements.InsertBefore(
-					declarationPoint,
-					new VariableDeclarationStatement((AstType)type.Clone(), variableName)
-					.WithAnnotation(declaredVariableAnnotation));
-			}
+			return true;
 		}
 		
-		bool CanMoveVariableUseIntoSubBlock(Statement stmt, string variableName, bool allowPassIntoLoops)
+		static bool CanMoveVariableUseIntoSubBlock(Statement stmt, string variableName, bool allowPassIntoLoops)
 		{
 			if (!allowPassIntoLoops && (stmt is ForStatement || stmt is ForeachStatement || stmt is DoWhileStatement || stmt is WhileStatement))
 				return false;
+			
 			ForStatement forStatement = stmt as ForStatement;
 			if (forStatement != null && forStatement.Initializers.Count == 1) {
 				// for-statement is special case: we can move variable declarations into the initializer
@@ -167,6 +226,19 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 					}
 				}
 			}
+			
+			UsingStatement usingStatement = stmt as UsingStatement;
+			if (usingStatement != null) {
+				// using-statement is special case: we can move variable declarations into the initializer
+				AssignmentExpression ae = usingStatement.ResourceAcquisition as AssignmentExpression;
+				if (ae != null && ae.Operator == AssignmentOperatorType.Assign) {
+					IdentifierExpression ident = ae.Left as IdentifierExpression;
+					if (ident != null && ident.Identifier == variableName) {
+						return !UsesVariable(ae.Right, variableName);
+					}
+				}
+			}
+			
 			// We can move the variable into a sub-block only if the variable is used in only that sub-block
 			for (AstNode child = stmt.FirstChild; child != null; child = child.NextSibling) {
 				if (!(child is BlockStatement) && UsesVariable(child, variableName))
@@ -175,7 +247,7 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 			return true;
 		}
 		
-		bool UsesVariable(AstNode node, string variableName)
+		static bool UsesVariable(AstNode node, string variableName)
 		{
 			IdentifierExpression ie = node as IdentifierExpression;
 			if (ie != null && ie.Identifier == variableName)
@@ -195,61 +267,22 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 					return false; // no need to introduce the variable here
 			}
 			
+			UsingStatement usingStatement = node as UsingStatement;
+			if (usingStatement != null) {
+				VariableDeclarationStatement varDecl = usingStatement.ResourceAcquisition as VariableDeclarationStatement;
+				if (varDecl != null) {
+					foreach (VariableInitializer v in varDecl.Variables) {
+						if (v.Name == variableName)
+							return false; // no need to introduce the variable here
+					}
+				}
+			}
+			
 			for (AstNode child = node.FirstChild; child != null; child = child.NextSibling) {
 				if (UsesVariable(child, variableName))
 					return true;
 			}
 			return false;
 		}
-		
-		#region FindInsertPos
-		static AstNode FindInsertPos(AstNode node, string name, bool allowPassIntoLoops)
-		{
-			AstNode pos = null;
-			AstNode withinPos = null;
-			while (node != null) {
-				IdentifierExpression ident = node as IdentifierExpression;
-				if (ident != null && ident.Identifier == name && ident.TypeArguments.Count == 0)
-					return node;
-				
-				FixedStatement fixedStatement = node as FixedStatement;
-				if (fixedStatement != null) {
-					foreach (VariableInitializer v in fixedStatement.Variables) {
-						if (v.Name == name)
-							return null; // no need to introduce the variable here
-					}
-				}
-				ForeachStatement foreachStatement = node as ForeachStatement;
-				if (foreachStatement != null) {
-					if (foreachStatement.VariableName == name)
-						return null; // no need to introduce the variable here
-				}
-				
-				AstNode withinCurrent = FindInsertPos(node.FirstChild, name, allowPassIntoLoops);
-				if (withinCurrent != null) {
-					if (pos == null) {
-						pos = node;
-						withinPos = withinCurrent;
-					} else {
-						return pos;
-					}
-				}
-				node = node.NextSibling;
-			}
-			if (withinPos != null && withinPos.Role == BlockStatement.StatementRole && AllowPassInto(pos, allowPassIntoLoops))
-				return withinPos;
-			else
-				return pos;
-		}
-		
-		static bool AllowPassInto(AstNode node, bool allowPassIntoLoops)
-		{
-			if (node is AnonymousMethodExpression || node is LambdaExpression)
-				return false;
-			
-			return allowPassIntoLoops;
-			return true;
-		}
-		#endregion
 	}
 }

ICSharpCode.Decompiler/Ast/Transforms/IntroduceUsingDeclarations.cs

@@ -25,7 +25,7 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 		public void Run(AstNode compilationUnit)
 		{
 			// Don't show using when decompiling a single method or nested types:
-			if (context.CurrentType != null)
+			if (context.CurrentMethod != null || (context.CurrentType != null && context.CurrentType.DeclaringType != null))
 				return;
 			
 			// First determine all the namespaces that need to be imported:

ICSharpCode.Decompiler/Ast/Transforms/PatternStatementTransform.cs

@@ -7,6 +7,7 @@ using System.Diagnostics;
 using System.Linq;
 using ICSharpCode.Decompiler.ILAst;
 using ICSharpCode.NRefactory.CSharp;
+using ICSharpCode.NRefactory.CSharp.Analysis;
 using ICSharpCode.NRefactory.CSharp.PatternMatching;
 using Mono.Cecil;
 
@@ -155,24 +156,75 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 			AstNode tryCatch = node.NextSibling;
 			Match m2 = usingTryCatchPattern.Match(tryCatch);
 			if (m2 == null) return null;
-			if (m1.Get<IdentifierExpression>("variable").Single().Identifier == m2.Get<IdentifierExpression>("ident").Single().Identifier) {
+			string variableName = m1.Get<IdentifierExpression>("variable").Single().Identifier;
+			if (variableName == m2.Get<IdentifierExpression>("ident").Single().Identifier) {
 				if (m2.Has("valueType")) {
 					// if there's no if(x!=null), then it must be a value type
 					ILVariable v = m1.Get("variable").Single().Annotation<ILVariable>();
 					if (v == null || v.Type == null || !v.Type.IsValueType)
 						return null;
 				}
+				node.Remove();
 				BlockStatement body = m2.Get<BlockStatement>("body").Single();
 				UsingStatement usingStatement = new UsingStatement();
-				usingStatement.ResourceAcquisition = node.Detach();
+				usingStatement.ResourceAcquisition = node.Expression.Detach();
 				usingStatement.EmbeddedStatement = body.Detach();
 				tryCatch.ReplaceWith(usingStatement);
-				// TODO: Move the variable declaration into the resource acquisition, if possible
+				// Move the variable declaration into the resource acquisition, if possible
 				// This is necessary for the foreach-pattern to work on the result of the using-pattern
+				VariableDeclarationStatement varDecl = FindVariableDeclaration(usingStatement, variableName);
+				if (varDecl != null && varDecl.Parent is BlockStatement) {
+					Statement declarationPoint;
+					if (CanMoveVariableDeclarationIntoStatement(varDecl, usingStatement, out declarationPoint)) {
+						Debug.Assert(declarationPoint == usingStatement);
+						// Moving the variable into the UsingStatement is allowed:
+						usingStatement.ResourceAcquisition = new VariableDeclarationStatement {
+							Type = (AstType)varDecl.Type.Clone(),
+							Variables = {
+								new VariableInitializer {
+									Name = variableName,
+									Initializer = m1.Get<Expression>("initializer").Single().Detach()
+								}.CopyAnnotationsFrom(usingStatement.ResourceAcquisition)
+							}
+						}.CopyAnnotationsFrom(node);
+					}
+				}
 				return usingStatement;
 			}
 			return null;
 		}
+		
+		VariableDeclarationStatement FindVariableDeclaration(AstNode node, string identifier)
+		{
+			while (node != null) {
+				while (node.PrevSibling != null) {
+					node = node.PrevSibling;
+					VariableDeclarationStatement varDecl = node as VariableDeclarationStatement;
+					if (varDecl != null && varDecl.Variables.Count == 1 && varDecl.Variables.Single().Name == identifier) {
+						return varDecl;
+					}
+				}
+				node = node.Parent;
+			}
+			return null;
+		}
+		
+		bool CanMoveVariableDeclarationIntoStatement(VariableDeclarationStatement varDecl, Statement targetStatement, out Statement declarationPoint)
+		{
+			Debug.Assert(targetStatement.Ancestors.Contains(varDecl.Parent));
+			// Find all blocks between targetStatement and varDecl.Parent
+			List<BlockStatement> blocks = targetStatement.Ancestors.TakeWhile(block => block != varDecl.Parent).OfType<BlockStatement>().ToList();
+			blocks.Add((BlockStatement)varDecl.Parent); // also handle the varDecl.Parent block itself
+			blocks.Reverse(); // go from parent blocks to child blocks
+			DefiniteAssignmentAnalysis daa = new DefiniteAssignmentAnalysis(blocks[0], context.CancellationToken);
+			declarationPoint = null;
+			foreach (BlockStatement block in blocks) {
+				if (!DeclareVariables.FindDeclarationPoint(daa, varDecl, block, out declarationPoint)) {
+					return false;
+				}
+			}
+			return true;
+		}
 		#endregion
 		
 		#region foreach
@@ -190,19 +242,22 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 			},
 			EmbeddedStatement = new BlockStatement {
 				new Repeat(
-					new NamedNode("variableDeclaration", new VariableDeclarationStatement { Type = new AnyNode(), Variables = { new AnyNode() } })
+					new VariableDeclarationStatement { Type = new AnyNode(), Variables = { new VariableInitializer() } }.WithName("variablesOutsideLoop")
 				).ToStatement(),
 				new WhileStatement {
 					Condition = new IdentifierExpressionBackreference("enumeratorVariable").ToExpression().Invoke("MoveNext"),
 					EmbeddedStatement = new BlockStatement {
+						new Repeat(
+							new VariableDeclarationStatement { Type = new AnyNode(), Variables = { new VariableInitializer() } }.WithName("variablesInsideLoop")
+						).ToStatement(),
 						new AssignmentExpression {
-							Left = new NamedNode("itemVariable", new IdentifierExpression()),
+							Left = new IdentifierExpression().WithName("itemVariable"),
 							Operator = AssignmentOperatorType.Assign,
 							Right = new IdentifierExpressionBackreference("enumeratorVariable").ToExpression().Member("Current")
 						},
 						new Repeat(new AnyNode("statement")).ToStatement()
 					}
-				}
+				}.WithName("loop")
 			}};
 		
 		public ForeachStatement TransformForeach(UsingStatement node)
@@ -210,26 +265,45 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 			Match m = foreachPattern.Match(node);
 			if (m == null)
 				return null;
+			if (!(node.Parent is BlockStatement) && m.Has("variablesOutsideLoop")) {
+				// if there are variables outside the loop, we need to put those into the parent block, and that won't work if the direct parent isn't a block
+				return null;
+			}
 			VariableInitializer enumeratorVar = m.Get<VariableInitializer>("enumeratorVariable").Single();
-			ILVariable itemVar = m.Get("itemVariable").Single().Annotation<ILVariable>();
-			if (itemVar == null)
+			IdentifierExpression itemVar = m.Get<IdentifierExpression>("itemVariable").Single();
+			WhileStatement loop = m.Get<WhileStatement>("loop").Single();
+			
+			// Find the declaration of the item variable:
+			// Because we look only outside the loop, we won't make the mistake of moving a captured variable across the loop boundary
+			VariableDeclarationStatement itemVarDecl = FindVariableDeclaration(loop, itemVar.Identifier);
+			if (itemVarDecl == null || !(itemVarDecl.Parent is BlockStatement))
+				return null;
+			
+			// Now verify that we can move the variable declaration in front of the loop:
+			Statement declarationPoint;
+			CanMoveVariableDeclarationIntoStatement(itemVarDecl, loop, out declarationPoint);
+			// We ignore the return value because we don't care whether we can move the variable into the loop
+			// (that is possible only with non-captured variables).
+			// We just care that we can move it in front of the loop:
+			if (declarationPoint != loop)
 				return null;
-			return null; // TODO
-//			if (m.Has("itemVariableInsideLoop") && itemVar.Annotation<DelegateConstruction.CapturedVariableAnnotation>() != null) {
-//				// cannot move captured variables out of loops
-//				return null;
-//			}
+			
 			BlockStatement newBody = new BlockStatement();
+			foreach (Statement stmt in m.Get<Statement>("variablesInsideLoop"))
+				newBody.Add(stmt.Detach());
 			foreach (Statement stmt in m.Get<Statement>("statement"))
 				newBody.Add(stmt.Detach());
 			
 			ForeachStatement foreachStatement = new ForeachStatement {
-				VariableType = m.Get<AstType>("itemType").Single().Detach(),
-				VariableName = itemVar.Name,
+				VariableType = (AstType)itemVarDecl.Type.Clone(),
+				VariableName = itemVar.Identifier,
 				InExpression = m.Get<Expression>("collection").Single().Detach(),
 				EmbeddedStatement = newBody
 			};
 			node.ReplaceWith(foreachStatement);
+			foreach (Statement stmt in m.Get<Statement>("variablesOutsideLoop")) {
+				((BlockStatement)foreachStatement.Parent).Statements.InsertAfter(null, stmt);
+			}
 			return foreachStatement;
 		}
 		#endregion

ICSharpCode.Decompiler/Ast/Transforms/TransformationPipeline.cs

@@ -20,10 +20,10 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 				new PushNegation(),
 				new DelegateConstruction(context),
 				new PatternStatementTransform(context),
-				new ConvertConstructorCallIntoInitializer(),
 				new ReplaceMethodCallsWithOperators(),
 				new IntroduceUnsafeModifier(),
-				new DeclareVariables(context),
+				new DeclareVariables(context), // should run after most transforms that modify statements
+				new ConvertConstructorCallIntoInitializer(), // must run after DeclareVariables
 				new IntroduceUsingDeclarations(context)
 			};
 		}