Resolve variable naming conflicts that occur with anonymous methods.

ICSharpCode.Decompiler/Ast/DecompilerContext.cs

@@ -2,6 +2,7 @@
 // This code is distributed under MIT X11 license (for details please see \doc\license.txt)
 
 using System;
+using System.Collections.Generic;
 using System.Threading;
 using Mono.Cecil;
 
@@ -14,9 +15,16 @@ namespace ICSharpCode.Decompiler
 		public MethodDefinition CurrentMethod;
 		public DecompilerSettings Settings = new DecompilerSettings();
 		
+		/// <summary>
+		/// Used to pass variable names from a method to its anonymous methods.
+		/// </summary>
+		internal List<string> ReservedVariableNames = new List<string>();
+		
 		public DecompilerContext Clone()
 		{
-			return (DecompilerContext)MemberwiseClone();
+			DecompilerContext ctx = (DecompilerContext)MemberwiseClone();
+			ctx.ReservedVariableNames = new List<string>(ctx.ReservedVariableNames);
+			return ctx;
 		}
 	}
 }

ICSharpCode.Decompiler/Ast/NameVariables.cs

@@ -36,8 +36,16 @@ namespace ICSharpCode.Decompiler.Ast
 			NameVariables nv = new NameVariables();
 			nv.context = context;
 			nv.fieldNamesInCurrentType = context.CurrentType.Fields.Select(f => f.Name).ToList();
-			nv.AddExistingNames(parameters.Select(p => p.Name));
-			nv.AddExistingNames(variables.Where(v => v.IsGenerated).Select(v => v.Name));
+			// First mark existing variable names as reserved.
+			foreach (string name in context.ReservedVariableNames)
+				nv.AddExistingName(name);
+			foreach (var p in parameters)
+				nv.AddExistingName(p.Name);
+			foreach (var v in variables) {
+				if (v.IsGenerated)
+					nv.AddExistingName(v.Name);
+			}
+			// Now generate names:
 			foreach (ILVariable p in parameters) {
 				if (string.IsNullOrEmpty(p.Name))
 					p.Name = nv.GenerateNameForVariable(p, methodBody);
@@ -53,30 +61,59 @@ namespace ICSharpCode.Decompiler.Ast
 		List<string> fieldNamesInCurrentType;
 		Dictionary<string, int> typeNames = new Dictionary<string, int>();
 		
-		void AddExistingNames(IEnumerable<string> existingNames)
+		public void AddExistingName(string name)
 		{
-			foreach (string name in existingNames) {
-				if (string.IsNullOrEmpty(name))
-					continue;
-				// First, identify whether the name already ends with a number:
-				int pos = name.Length;
-				while (pos > 0 && name[pos-1] >= '0' && name[pos-1] <= '9')
-					pos--;
-				if (pos < name.Length) {
-					int number;
-					if (int.TryParse(name.Substring(pos), out number)) {
-						string nameWithoutDigits = name.Substring(0, pos);
-						int existingNumber;
-						if (typeNames.TryGetValue(nameWithoutDigits, out existingNumber)) {
-							typeNames[nameWithoutDigits] = Math.Max(number, existingNumber);
-						} else {
-							typeNames.Add(nameWithoutDigits, number);
-						}
-						continue;
+			if (string.IsNullOrEmpty(name))
+				return;
+			int number;
+			string nameWithoutDigits = SplitName(name, out number);
+			int existingNumber;
+			if (typeNames.TryGetValue(nameWithoutDigits, out existingNumber)) {
+				typeNames[nameWithoutDigits] = Math.Max(number, existingNumber);
+			} else {
+				typeNames.Add(nameWithoutDigits, number);
+			}
+		}
+		
+		string SplitName(string name, out int number)
+		{
+			// First, identify whether the name already ends with a number:
+			int pos = name.Length;
+			while (pos > 0 && name[pos-1] >= '0' && name[pos-1] <= '9')
+				pos--;
+			if (pos < name.Length) {
+				if (int.TryParse(name.Substring(pos), out number)) {
+					return name.Substring(0, pos);
+				}
+			}
+			number = 1;
+			return name;
+		}
+		
+		const char maxLoopVariableName = 'n';
+		
+		public string GetAlternativeName(string oldVariableName)
+		{
+			if (oldVariableName.Length == 1 && oldVariableName[0] >= 'i' && oldVariableName[0] <= maxLoopVariableName) {
+				for (char c = 'i'; c <= maxLoopVariableName; c++) {
+					if (!typeNames.ContainsKey(c.ToString())) {
+						typeNames.Add(c.ToString(), 1);
+						return c.ToString();
 					}
 				}
-				if (!typeNames.ContainsKey(name))
-					typeNames.Add(name, 1);
+			}
+			
+			int number;
+			string nameWithoutDigits = SplitName(oldVariableName, out number);
+			
+			if (!typeNames.ContainsKey(nameWithoutDigits)) {
+				typeNames.Add(nameWithoutDigits, 0);
+			}
+			int count = ++typeNames[nameWithoutDigits];
+			if (count > 1) {
+				return nameWithoutDigits + count.ToString();
+			} else {
+				return nameWithoutDigits;
 			}
 		}
 		
@@ -106,7 +143,7 @@ namespace ICSharpCode.Decompiler.Ast
 				}
 				if (isLoopCounter) {
 					// For loop variables, use i,j,k,l,m,n
-					for (char c = 'i'; c <= 'n'; c++) {
+					for (char c = 'i'; c <= maxLoopVariableName; c++) {
 						if (!typeNames.ContainsKey(c.ToString())) {
 							proposedName = c.ToString();
 							break;

ICSharpCode.Decompiler/Ast/Transforms/DelegateConstruction.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using System.Threading;
 using ICSharpCode.Decompiler;
@@ -37,6 +38,8 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 		{
 		}
 		
+		List<string> currentlyUsedVariableNames = new List<string>();
+		
 		public DelegateConstruction(DecompilerContext context) : base(context)
 		{
 		}
@@ -119,10 +122,16 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 			ame.Parameters.AddRange(AstBuilder.MakeParameters(method.Parameters));
 			ame.HasParameterList = true;
 			
+			// rename variables so that they don't conflict with the parameters:
+			foreach (ParameterDeclaration pd in ame.Parameters) {
+				EnsureVariableNameIsAvailable(objectCreateExpression, pd.Name);
+			}
+			
 			// Decompile the anonymous method:
 			
 			DecompilerContext subContext = context.Clone();
 			subContext.CurrentMethod = method;
+			subContext.ReservedVariableNames.AddRange(currentlyUsedVariableNames);
 			BlockStatement body = AstMethodBodyBuilder.CreateMethodBody(method, subContext, ame.Parameters);
 			TransformationPipeline.RunTransformationsUntil(body, v => v is DelegateConstruction, subContext);
 			body.AcceptVisitor(this, null);
@@ -179,6 +188,76 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 			return true;
 		}
 		
+		#region Track current variables
+		public override object VisitMethodDeclaration(MethodDeclaration methodDeclaration, object data)
+		{
+			Debug.Assert(currentlyUsedVariableNames.Count == 0);
+			try {
+				currentlyUsedVariableNames.AddRange(methodDeclaration.Parameters.Select(p => p.Name));
+				return base.VisitMethodDeclaration(methodDeclaration, data);
+			} finally {
+				currentlyUsedVariableNames.Clear();
+			}
+		}
+		
+		public override object VisitOperatorDeclaration(OperatorDeclaration operatorDeclaration, object data)
+		{
+			Debug.Assert(currentlyUsedVariableNames.Count == 0);
+			try {
+				currentlyUsedVariableNames.AddRange(operatorDeclaration.Parameters.Select(p => p.Name));
+				return base.VisitOperatorDeclaration(operatorDeclaration, data);
+			} finally {
+				currentlyUsedVariableNames.Clear();
+			}
+		}
+		
+		public override object VisitConstructorDeclaration(ConstructorDeclaration constructorDeclaration, object data)
+		{
+			Debug.Assert(currentlyUsedVariableNames.Count == 0);
+			try {
+				currentlyUsedVariableNames.AddRange(constructorDeclaration.Parameters.Select(p => p.Name));
+				return base.VisitConstructorDeclaration(constructorDeclaration, data);
+			} finally {
+				currentlyUsedVariableNames.Clear();
+			}
+		}
+		
+		public override object VisitIndexerDeclaration(IndexerDeclaration indexerDeclaration, object data)
+		{
+			Debug.Assert(currentlyUsedVariableNames.Count == 0);
+			try {
+				currentlyUsedVariableNames.AddRange(indexerDeclaration.Parameters.Select(p => p.Name));
+				return base.VisitIndexerDeclaration(indexerDeclaration, data);
+			} finally {
+				currentlyUsedVariableNames.Clear();
+			}
+		}
+		
+		public override object VisitAccessor(Accessor accessor, object data)
+		{
+			try {
+				currentlyUsedVariableNames.Add("value");
+				return base.VisitAccessor(accessor, data);
+			} finally {
+				currentlyUsedVariableNames.RemoveAt(currentlyUsedVariableNames.Count - 1);
+			}
+		}
+		
+		public override object VisitVariableDeclarationStatement(VariableDeclarationStatement variableDeclarationStatement, object data)
+		{
+			foreach (VariableInitializer v in variableDeclarationStatement.Variables)
+				currentlyUsedVariableNames.Add(v.Name);
+			return base.VisitVariableDeclarationStatement(variableDeclarationStatement, data);
+		}
+		
+		public override object VisitFixedStatement(FixedStatement fixedStatement, object data)
+		{
+			foreach (VariableInitializer v in fixedStatement.Variables)
+				currentlyUsedVariableNames.Add(v.Name);
+			return base.VisitFixedStatement(fixedStatement, data);
+		}
+		#endregion
+		
 		static readonly ExpressionStatement displayClassAssignmentPattern =
 			new ExpressionStatement(new AssignmentExpression(
 				new NamedNode("variable", new IdentifierExpression()),
@@ -187,6 +266,7 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 		
 		public override object VisitBlockStatement(BlockStatement blockStatement, object data)
 		{
+			int numberOfVariablesOutsideBlock = currentlyUsedVariableNames.Count;
 			base.VisitBlockStatement(blockStatement, data);
 			foreach (ExpressionStatement stmt in blockStatement.Statements.OfType<ExpressionStatement>().ToArray()) {
 				Match displayClassAssignmentMatch = displayClassAssignmentPattern.Match(stmt);
@@ -261,6 +341,8 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 				foreach (FieldDefinition field in type.Fields) {
 					if (dict.ContainsKey(field))
 						continue;
+					EnsureVariableNameIsAvailable(blockStatement, field.Name);
+					currentlyUsedVariableNames.Add(field.Name);
 					variablesToDeclare.Add(Tuple.Create(AstBuilder.ConvertType(field.FieldType, field), field.Name));
 					dict[field] = new IdentifierExpression(field.Name);
 				}
@@ -283,7 +365,53 @@ namespace ICSharpCode.Decompiler.Ast.Transforms
 					blockStatement.Statements.InsertBefore(insertionPoint, newVarDecl);
 				}
 			}
+			currentlyUsedVariableNames.RemoveRange(numberOfVariablesOutsideBlock, currentlyUsedVariableNames.Count - numberOfVariablesOutsideBlock);
 			return null;
 		}
+		
+		void EnsureVariableNameIsAvailable(AstNode currentNode, string name)
+		{
+			int pos = currentlyUsedVariableNames.IndexOf(name);
+			if (pos < 0) {
+				// name is still available
+				return;
+			}
+			// Naming conflict. Let's rename the existing variable so that the field keeps the name from metadata.
+			NameVariables nv = new NameVariables();
+			// Add currently used variable and parameter names
+			foreach (string nameInUse in currentlyUsedVariableNames)
+				nv.AddExistingName(nameInUse);
+			// variables declared in child nodes of this block
+			foreach (VariableInitializer vi in currentNode.Descendants.OfType<VariableInitializer>())
+				nv.AddExistingName(vi.Name);
+			// parameters in child lambdas
+			foreach (ParameterDeclaration pd in currentNode.Descendants.OfType<ParameterDeclaration>())
+				nv.AddExistingName(pd.Name);
+			
+			string newName = nv.GetAlternativeName(name);
+			currentlyUsedVariableNames[pos] = newName;
+			
+			// find top-most block
+			AstNode topMostBlock = currentNode.Ancestors.OfType<BlockStatement>().LastOrDefault() ?? currentNode;
+			
+			// rename identifiers
+			foreach (IdentifierExpression ident in topMostBlock.Descendants.OfType<IdentifierExpression>()) {
+				if (ident.Identifier == name) {
+					ident.Identifier = newName;
+					ILVariable v = ident.Annotation<ILVariable>();
+					if (v != null)
+						v.Name = newName;
+				}
+			}
+			// rename variable declarations
+			foreach (VariableInitializer vi in topMostBlock.Descendants.OfType<VariableInitializer>()) {
+				if (vi.Name == name) {
+					vi.Name = newName;
+					ILVariable v = vi.Annotation<ILVariable>();
+					if (v != null)
+						v.Name = newName;
+				}
+			}
+		}
 	}
 }

ICSharpCode.Decompiler/Tests/DelegateConstruction.cs

@@ -77,4 +77,45 @@ public static class DelegateConstruction
 			Console.WriteLine();
 		};
 	}
+	
+	public static void NameConflict()
+	{
+		// i is captured variable,
+		// j is parameter in anonymous method
+		// k is local in anonymous method,
+		// l is local in main method
+		// Ensure that the decompiler doesn't introduce name conflicts
+		List<Action<int>> list = new List<Action<int>>();
+		for (int l = 0; l < 10; l++) {
+			int i;
+			for (i = 0; i < 10; i++) {
+				list.Add(
+					delegate (int j) {
+						for (int k = 0; k < i; k += j) {
+							Console.WriteLine();
+						}
+					});
+			}
+		}
+	}
+	
+	public static void NameConflict2(int j)
+	{
+		List<Action<int>> list = new List<Action<int>>();
+		for (int k = 0; k < 10; k++) {
+			list.Add(
+				delegate(int i) {
+					Console.WriteLine(i);
+				});
+		}
+	}
+	
+	public static Action<int> NameConflict3(int i)
+	{
+		return delegate(int j) {
+			for (int k = 0; k < j; k++) {
+				Console.WriteLine(k);
+			}
+		};
+	}
 }