Merge pull request #3243 from icsharpcode/improved-variable-names

Rewrite AssignVariableNames

ICSharpCode.Decompiler.Tests/ICSharpCode.Decompiler.Tests.csproj

@@ -93,6 +93,7 @@
     <None Include="TestCases\ILPretty\GuessAccessors.cs" />
     <None Include="TestCases\ILPretty\GuessAccessors.il" />
     <None Include="TestCases\ILPretty\Issue2260SwitchString.il" />
+    <None Include="TestCases\ILPretty\MonoFixed.il" />
     <None Include="TestCases\ILPretty\UnknownTypes.cs" />
     <None Include="TestCases\ILPretty\UnknownTypes.il" />
     <None Include="TestCases\ILPretty\EvalOrder.cs" />
@@ -128,6 +129,7 @@
     <Compile Include="Output\InsertParenthesesVisitorTests.cs" />
     <Compile Include="ProjectDecompiler\TargetFrameworkTests.cs" />
     <Compile Include="TestAssemblyResolver.cs" />
+    <Compile Include="TestCases\ILPretty\MonoFixed.cs" />
     <Compile Include="Util\ResourceReaderWriterTests.cs" />
     <None Include="TestCases\VBPretty\VBAutomaticEvents.vb" />
     <Compile Include="TestCases\VBPretty\VBAutomaticEvents.cs" />

ICSharpCode.Decompiler.Tests/TestCases/ILPretty/MonoFixed.cs

@@ -6,13 +6,13 @@ public class MonoFixed
 	{
 		fixed (char* ptr = text)
 		{
-			fixed (char* ptr2 = Environment.UserName)
+			fixed (char* userName = Environment.UserName)
 			{
-				fixed (char* ptr3 = text)
+				fixed (char* ptr2 = text)
 				{
 					*ptr = 'c';
-					*ptr2 = 'd';
-					*ptr3 = 'e';
+					*userName = 'd';
+					*ptr2 = 'e';
 				}
 			}
 		}

ICSharpCode.Decompiler.Tests/TestCases/Pretty/Async.cs

@@ -250,7 +250,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 			{
 				throw null;
 			}
-			catch (Exception ex2) when (i == 0)
+			catch (Exception ex) when (i == 0)
 			{
 				Console.WriteLine("First!");
 				if (i == 1)
@@ -258,9 +258,9 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 					throw;
 				}
 				await Task.Yield();
-				Console.WriteLine(ex2.StackTrace);
+				Console.WriteLine(ex.StackTrace);
 			}
-			catch (Exception ex3) when (True())
+			catch (Exception ex2) when (True())
 			{
 				Console.WriteLine("Second!");
 				if (i == 1)
@@ -268,9 +268,9 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 					throw;
 				}
 				await Task.Yield();
-				Console.WriteLine(ex3.StackTrace);
+				Console.WriteLine(ex2.StackTrace);
 			}
-			catch (Exception ex)
+			catch (Exception ex3)
 			{
 				Console.WriteLine("Third!");
 				if (i == 1)
@@ -278,7 +278,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 					throw;
 				}
 				await Task.Yield();
-				Console.WriteLine(ex.StackTrace);
+				Console.WriteLine(ex3.StackTrace);
 			}
 			catch when (i == 0)
 			{

ICSharpCode.Decompiler.Tests/TestCases/Pretty/DelegateConstruction.cs

@@ -353,19 +353,19 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty.DelegateConstruction
 
 		public static void NameConflict()
 		{
-			// i is captured variable,
-			// j is parameter in anonymous method
+			// i is local in main method,
+			// j is captured variable,
+			// k is parameter in anonymous method
 			// l is local in anonymous method,
-			// k is local in main method
 			// Ensure that the decompiler doesn't introduce name conflicts
 			List<Action<int>> list = new List<Action<int>>();
-			for (int k = 0; k < 10; k++)
+			for (int i = 0; i < 10; i++)
 			{
-				int i;
-				for (i = 0; i < 10; i++)
+				int j;
+				for (j = 0; j < 10; j++)
 				{
-					list.Add(delegate (int j) {
-						for (int l = 0; l < i; l += j)
+					list.Add(delegate (int k) {
+						for (int l = 0; l < j; l += k)
 						{
 							Console.WriteLine();
 						}

ICSharpCode.Decompiler.Tests/TestCases/Pretty/LocalFunctions.cs

@@ -39,10 +39,10 @@ namespace LocalFunctions
 #pragma warning disable CS0219
 				T2 t2 = default(T2);
 				object z = this;
-				for (int j = 0; j < 10; j++)
+				for (int i = 0; i < 10; i++)
 				{
-					int i = 0;
-					i += NonStaticMethod6<object>(0);
+					int i2 = 0;
+					i2 += NonStaticMethod6<object>(0);
 #if CS90
 					[My]
 					[return: My]
@@ -56,7 +56,7 @@ namespace LocalFunctions
 						return NonStaticMethod6_1<T1>() + NonStaticMethod6_1<T2>() + z.GetHashCode();
 						int NonStaticMethod6_1<T4>()
 						{
-							return i + l + NonStaticMethod6<T4>(0) + StaticMethod1<decimal>();
+							return i2 + l + NonStaticMethod6<T4>(0) + StaticMethod1<decimal>();
 						}
 					}
 				}
@@ -119,10 +119,10 @@ namespace LocalFunctions
 			{
 				T2 t2 = default(T2);
 				object z = this;
-				for (int j = 0; j < 10; j++)
+				for (int i = 0; i < 10; i++)
 				{
-					int i = 0;
-					i += StaticInvokeAsFunc(NonStaticMethod6<object>);
+					int i2 = 0;
+					i2 += StaticInvokeAsFunc(NonStaticMethod6<object>);
 					int NonStaticMethod6<T3>()
 					{
 						t2 = default(T2);
@@ -130,7 +130,7 @@ namespace LocalFunctions
 						return StaticInvokeAsFunc(NonStaticMethod6_1<T1>) + StaticInvokeAsFunc(NonStaticMethod6_1<T2>) + z.GetHashCode();
 						int NonStaticMethod6_1<T4>()
 						{
-							return i + l + StaticInvokeAsFunc(NonStaticMethod6<T4>) + StaticInvokeAsFunc(StaticMethod1<decimal>);
+							return i2 + l + StaticInvokeAsFunc(NonStaticMethod6<T4>) + StaticInvokeAsFunc(StaticMethod1<decimal>);
 						}
 					}
 				}
@@ -743,12 +743,12 @@ namespace LocalFunctions
 				int ZZZ_1()
 				{
 					t0 = 0;
-					int t = t0;
+					int t3 = t0;
 					return new Func<int>(ZZZ_1_0)();
 					int ZZZ_1_0()
 					{
 						t0 = 0;
-						t = 0;
+						t3 = 0;
 						return 0;
 					}
 				}
@@ -779,14 +779,14 @@ namespace LocalFunctions
 				int ZZZ_1()
 				{
 					t0 = 0;
-					int t1 = t0;
+					int t3 = t0;
 #if !OPT
 					Func<int> func = delegate {
 #else
 					return ((Func<int>)delegate {
 #endif
 						t0 = 0;
-						t1 = 0;
+						t3 = 0;
 						return 0;
 #if !OPT
 					};
@@ -822,14 +822,14 @@ namespace LocalFunctions
 				int ZZZ_1()
 				{
 					t0 = 0;
-					int t1 = t0;
+					int t3 = t0;
 #if !OPT
 					Func<int> func = delegate {
 #else
 					return ((Func<int>)delegate {
 #endif
 						t0 = 0;
-						t1 = 0;
+						t3 = 0;
 						return 0;
 #if !OPT
 					};

ICSharpCode.Decompiler.Tests/TestCases/Pretty/UnsafeCode.cs

@@ -431,9 +431,9 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 
 		public unsafe void PinFixedMember(ref StructWithFixedSizeMembers m)
 		{
-			fixed (int* ptr = m.Integers)
+			fixed (int* integers = m.Integers)
 			{
-				UsePointer(ptr);
+				UsePointer(integers);
 			}
 		}
 

ICSharpCode.Decompiler.Tests/TestCases/Pretty/VariableNamingWithoutSymbols.cs

@@ -1,4 +1,7 @@
-namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
+using System;
+using System.Collections.Generic;
+
+namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 {
 	internal class VariableNamingWithoutSymbols
 	{
@@ -25,5 +28,62 @@
 			string text2 = c.Text;
 #endif
 		}
+
+		private static IDisposable GetData()
+		{
+			return null;
+		}
+
+		private static void UseData(IDisposable data)
+		{
+
+		}
+
+		private static IEnumerable<int> GetItems()
+		{
+			throw null;
+		}
+
+		private static byte[] GetMemory()
+		{
+			throw null;
+		}
+
+		private static void Test(int item)
+		{
+			foreach (int item2 in GetItems())
+			{
+				Console.WriteLine(item2);
+			}
+		}
+
+		private static void Test(IDisposable data)
+		{
+#if CS80
+			using IDisposable data2 = GetData();
+			UseData(data2);
+#else
+			using (IDisposable data2 = GetData())
+			{
+				UseData(data2);
+			}
+#endif
+		}
+
+		private unsafe static void Test(byte[] memory)
+		{
+			fixed (byte* memory2 = GetMemory())
+			{
+				Console.WriteLine(*memory2);
+			}
+		}
+
+		private static void ForLoopNamingConflict(int i)
+		{
+			for (int j = 0; j < i; j++)
+			{
+				Console.WriteLine(i + " of " + j);
+			}
+		}
 	}
 }

ICSharpCode.Decompiler.Tests/TestCases/Ugly/AggressiveScalarReplacementOfAggregates.Expected.cs

@@ -96,25 +96,25 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Ugly
 				thisField = this,
 				field1 = i
 			};
-			int field1 = default(int);
-			string field2 = default(string);
+			int field5 = default(int);
+			string field4 = default(string);
 			DisplayClass field3 = default(DisplayClass);
 			while (true)
 			{
 				switch (Rand())
 				{
 					case 1:
-						field1 = Rand();
+						field5 = Rand();
 						continue;
 					case 2:
-						field2 = Rand().ToString();
+						field4 = Rand().ToString();
 						continue;
 					case 3:
 						field3 = displayClass;
 						continue;
 				}
-				Console.WriteLine(field1);
-				Console.WriteLine(field2);
+				Console.WriteLine(field5);
+				Console.WriteLine(field4);
 				Console.WriteLine(field3);
 			}
 		}

ICSharpCode.Decompiler/IL/Transforms/AssignVariableNames.cs

@@ -55,7 +55,6 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 		ILTransformContext context;
 		List<string> currentLowerCaseTypeOrMemberNames;
 		Dictionary<string, int> reservedVariableNames;
-		Dictionary<MethodDefinitionHandle, string> localFunctionMapping;
 		HashSet<ILVariable> loopCounters;
 		const char maxLoopVariableName = 'n';
 		int numDisplayClassLocals;
@@ -75,7 +74,6 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 				currentLowerCaseTypeOrMemberNames.Add(name);
 				AddExistingName(reservedVariableNames, name);
 			}
-			localFunctionMapping = new Dictionary<MethodDefinitionHandle, string>();
 			loopCounters = CollectLoopCounters(function);
 			foreach (var f in function.Descendants.OfType<ILFunction>())
 			{
@@ -153,10 +151,7 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 				}
 			}
 			numDisplayClassLocals = 0;
-			foreach (ILFunction f in function.Descendants.OfType<ILFunction>().Reverse())
-			{
-				PerformAssignment(f);
-			}
+			PerformAssignment(function);
 		}
 
 		static IEnumerable<string> CollectAllLowerCaseMemberNames(ITypeDefinition type)
@@ -195,128 +190,131 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 
 		void PerformAssignment(ILFunction function)
 		{
-			// remove unused variables before assigning names
-			function.Variables.RemoveDead();
-			Dictionary<int, string> assignedLocalSignatureIndices = new Dictionary<int, string>();
-			foreach (var v in function.Variables.OrderBy(v => v.Name))
-			{
-				switch (v.Kind)
-				{
-					case VariableKind.Parameter:
-						// Parameter names are handled in ILReader.CreateILVariable
-						// and CSharpDecompiler.FixParameterNames
-						break;
-					case VariableKind.InitializerTarget: // keep generated names
-						AddExistingName(reservedVariableNames, v.Name);
-						break;
-					case VariableKind.DisplayClassLocal:
-						v.Name = "CS$<>8__locals" + (numDisplayClassLocals++);
-						break;
-					case VariableKind.Local when v.Index != null:
-						if (assignedLocalSignatureIndices.TryGetValue(v.Index.Value, out string name))
-						{
-							// make sure all local ILVariables that refer to the same slot in the locals signature
-							// are assigned the same name.
-							v.Name = name;
-						}
-						else
-						{
-							AssignName();
-							// Remember the newly assigned name:
-							assignedLocalSignatureIndices.Add(v.Index.Value, v.Name);
-						}
-						break;
-					default:
-						AssignName();
-						break;
-				}
+			var localFunctionMapping = new Dictionary<MethodDefinitionHandle, string>();
+			var variableMapping = new Dictionary<ILVariable, string>(ILVariableEqualityComparer.Instance);
+			var assignedLocalSignatureIndices = new Dictionary<(ILFunction, int), string>();
 
-				void AssignName()
+			foreach (var inst in function.Descendants)
+			{
+				if (inst is ILFunction { Kind: ILFunctionKind.LocalFunction } localFunction)
 				{
-					if (v.HasGeneratedName || !IsValidName(v.Name) || ConflictWithLocal(v))
+					// assign names to local functions
+					if (!LocalFunctionDecompiler.ParseLocalFunctionName(localFunction.Name, out _, out var newName) || !IsValidName(newName))
+						newName = null;
+					if (newName == null)
 					{
-						// don't use the name from the debug symbols if it looks like a generated name
-						v.Name = null;
-					}
-					else
-					{
-						// use the name from the debug symbols and update index appended to duplicates
-						string nameWithoutNumber = SplitName(v.Name, out int newIndex);
+						string nameWithoutNumber = "f";
 						if (!reservedVariableNames.TryGetValue(nameWithoutNumber, out int currentIndex))
 						{
 							currentIndex = 1;
 						}
-						reservedVariableNames[nameWithoutNumber] = Math.Max(newIndex, currentIndex);
+						int count = Math.Max(1, currentIndex) + 1;
+						reservedVariableNames[nameWithoutNumber] = count;
+						if (count > 1)
+						{
+							newName = nameWithoutNumber + count.ToString();
+						}
+						else
+						{
+							newName = nameWithoutNumber;
+						}
+					}
+					localFunction.Name = newName;
+					localFunction.ReducedMethod.Name = newName;
+					localFunctionMapping[(MethodDefinitionHandle)localFunction.ReducedMethod.MetadataToken] = newName;
+				}
+				else if (inst is IInstructionWithVariableOperand i)
+				{
+					var v = i.Variable;
+					// if there is already a valid name for the variable slot, just use it
+					if (variableMapping.TryGetValue(v, out string name))
+					{
+						v.Name = name;
+						continue;
+					}
+					switch (v.Kind)
+					{
+						case VariableKind.Parameter:
+							// Parameter names are handled in ILReader.CreateILVariable
+							// and CSharpDecompiler.FixParameterNames
+							break;
+						case VariableKind.InitializerTarget: // keep generated names
+							AddExistingName(reservedVariableNames, v.Name);
+							break;
+						case VariableKind.DisplayClassLocal:
+							v.Name = "CS$<>8__locals" + (numDisplayClassLocals++);
+							break;
+						case VariableKind.Local when v.Index != null:
+							if (assignedLocalSignatureIndices.TryGetValue((v.Function, v.Index.Value), out name))
+							{
+								// make sure all local ILVariables that refer to the same slot in the locals signature
+								// are assigned the same name.
+								v.Name = name;
+							}
+							else
+							{
+								v.Name = AssignName(v, variableMapping);
+								// Remember the newly assigned name:
+								assignedLocalSignatureIndices.Add((v.Function, v.Index.Value), v.Name);
+							}
+							break;
+						default:
+							v.Name = AssignName(v, variableMapping);
+							break;
+					}
+				}
+				else if (inst is (Call or LdFtn) and IInstructionWithMethodOperand m)
+				{
+					// update references to local functions
+					if (m.Method is LocalFunctionMethod lf
+						&& localFunctionMapping.TryGetValue((MethodDefinitionHandle)lf.MetadataToken, out var name))
+					{
+						lf.Name = name;
 					}
 				}
 			}
-			foreach (var localFunction in function.LocalFunctions)
+		}
+
+		string AssignName(ILVariable v, Dictionary<ILVariable, string> variableMapping)
+		{
+			// variable has no valid name
+			string newName = v.Name;
+			if (v.HasGeneratedName || !IsValidName(newName))
 			{
-				if (!LocalFunctionDecompiler.ParseLocalFunctionName(localFunction.Name, out _, out var newName) || !IsValidName(newName))
-					newName = null;
-				localFunction.Name = newName;
-				localFunction.ReducedMethod.Name = newName;
+				// don't use the name from the debug symbols if it looks like a generated name
+				// generate a new one based on how the variable is used
+				newName = GenerateNameForVariable(v);
 			}
-			// Now generate names:
-			var mapping = new Dictionary<ILVariable, string>(ILVariableEqualityComparer.Instance);
-			foreach (var inst in function.Descendants.OfType<IInstructionWithVariableOperand>())
+			// use the existing name and update index appended to future conflicts
+			string nameWithoutNumber = SplitName(newName, out int newIndex);
+			if (reservedVariableNames.TryGetValue(nameWithoutNumber, out int lastUsedIndex))
 			{
-				var v = inst.Variable;
-				if (!mapping.TryGetValue(v, out string name))
+				// name without number was already used
+				if (v.Type.IsKnownType(KnownTypeCode.Int32) && loopCounters.Contains(v))
 				{
-					if (string.IsNullOrEmpty(v.Name))
-						v.Name = GenerateNameForVariable(v);
-					mapping.Add(v, v.Name);
+					// special case for loop counters,
+					// we don't want them to be named i, i2, ..., but i, j, ...
+					newName = GenerateNameForVariable(v);
 				}
 				else
 				{
-					v.Name = name;
-				}
-			}
-			foreach (var localFunction in function.LocalFunctions)
-			{
-				var newName = localFunction.Name;
-				if (newName == null)
-				{
-					string nameWithoutNumber = "f";
-					if (!reservedVariableNames.TryGetValue(nameWithoutNumber, out int currentIndex))
+					if (newIndex > lastUsedIndex)
 					{
-						currentIndex = 1;
-					}
-					int count = Math.Max(1, currentIndex) + 1;
-					reservedVariableNames[nameWithoutNumber] = count;
-					if (count > 1)
-					{
-						newName = nameWithoutNumber + count.ToString();
+						// new index is larger than last, so we can use it
 					}
 					else
 					{
-						newName = nameWithoutNumber;
+						// new index is smaller or equal, so we use the next value
+						newIndex = lastUsedIndex + 1;
 					}
+					// resolve conflicts by appending the index to the new name:
+					newName = nameWithoutNumber + newIndex.ToString();
 				}
-				localFunction.Name = newName;
-				localFunction.ReducedMethod.Name = newName;
-				localFunctionMapping[(MethodDefinitionHandle)localFunction.ReducedMethod.MetadataToken] = newName;
-			}
-			foreach (var inst in function.Descendants)
-			{
-				LocalFunctionMethod localFunction;
-				switch (inst)
-				{
-					case Call call:
-						localFunction = call.Method as LocalFunctionMethod;
-						break;
-					case LdFtn ldftn:
-						localFunction = ldftn.Method as LocalFunctionMethod;
-						break;
-					default:
-						localFunction = null;
-						break;
-				}
-				if (localFunction == null || !localFunctionMapping.TryGetValue((MethodDefinitionHandle)localFunction.MetadataToken, out var name))
-					continue;
-				localFunction.Name = name;
 			}
+			// update the last used index
+			reservedVariableNames[nameWithoutNumber] = newIndex;
+			variableMapping.Add(v, newName);
+			return newName;
 		}
 
 		/// <remarks>
@@ -326,6 +324,7 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 		{
 			switch (arg)
 			{
+				case GetPinnableReference _:
 				case LdObj _:
 				case LdFlda _:
 				case LdsFlda _:
@@ -336,16 +335,6 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 			}
 		}
 
-		bool ConflictWithLocal(ILVariable v)
-		{
-			if (v.Kind == VariableKind.UsingLocal || v.Kind == VariableKind.ForeachLocal)
-			{
-				if (reservedVariableNames.ContainsKey(v.Name))
-					return true;
-			}
-			return false;
-		}
-
 		internal static bool IsValidName(string varName)
 		{
 			if (string.IsNullOrWhiteSpace(varName))
@@ -431,6 +420,12 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 						if (!currentLowerCaseTypeOrMemberNames.Contains(name))
 							proposedNameForStores.Add(name);
 					}
+					else if (store is PinnedRegion pinnedRegion)
+					{
+						var name = GetNameFromInstruction(pinnedRegion.Init);
+						if (!currentLowerCaseTypeOrMemberNames.Contains(name))
+							proposedNameForStores.Add(name);
+					}
 				}
 				if (proposedNameForStores.Count == 1)
 				{
@@ -462,32 +457,21 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 				proposedName = GetNameByType(variable.Type);
 			}
 
-			// remove any numbers from the proposed name
-			proposedName = SplitName(proposedName, out int number);
-
-			if (!reservedVariableNames.ContainsKey(proposedName))
-			{
-				reservedVariableNames.Add(proposedName, 0);
-			}
-			int count = ++reservedVariableNames[proposedName];
-			Debug.Assert(!string.IsNullOrWhiteSpace(proposedName));
-			if (count > 1)
-			{
-				return proposedName + count.ToString();
-			}
-			else
-			{
-				return proposedName;
-			}
+			// for generated names remove number-suffixes
+			return SplitName(proposedName, out _);
 		}
 
 		static string GetNameFromInstruction(ILInstruction inst)
 		{
 			switch (inst)
 			{
+				case GetPinnableReference getPinnableReference:
+					return GetNameFromInstruction(getPinnableReference.Argument);
 				case LdObj ldobj:
 					return GetNameFromInstruction(ldobj.Target);
 				case LdFlda ldflda:
+					if (ldflda.Field.IsCompilerGeneratedOrIsInCompilerGeneratedClass())
+						return GetNameFromInstruction(ldflda.Target);
 					return CleanUpVariableName(ldflda.Field.Name);
 				case LdsFlda ldsflda:
 					return CleanUpVariableName(ldsflda.Field.Name);
@@ -567,6 +551,8 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 				return true;
 			if (m.Name == "Concat" && m.DeclaringType.IsKnownType(KnownTypeCode.String))
 				return true;
+			if (m.Name == "GetPinnableReference")
+				return true;
 			return false;
 		}
 

ICSharpCode.Decompiler/IL/Transforms/ExpressionTransforms.cs

@@ -898,7 +898,9 @@ namespace ICSharpCode.Decompiler.IL.Transforms
 			}
 			context.Step("TransformCatchVariable", entryPoint.Instructions[0]);
 			exceptionVar.Kind = VariableKind.ExceptionLocal;
+			exceptionVar.Name = handler.Variable.Name;
 			exceptionVar.Type = handler.Variable.Type;
+			exceptionVar.HasGeneratedName = handler.Variable.HasGeneratedName;
 			handler.Variable = exceptionVar;
 			if (isCatchBlock)
 			{