Merge pull request #4071 from icsharpcode/better-types-for-stack-slots

Improve types for variables generated from stack slots

ICSharpCode.Decompiler.Tests/TestCases/Correctness/Switch.cs

@@ -17,6 +17,7 @@
 // DEALINGS IN THE SOFTWARE.
 
 using System;
+using System.Text;
 
 namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness
 {
@@ -37,6 +38,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness
 			SwitchWithGoto(2);
 			SwitchWithGoto(3);
 			SwitchWithGoto(4);
+			JsonPathTest();
 		}
 
 		static void TestCase<T>(Func<T, string> target, params T[] args)
@@ -245,5 +247,35 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness
 					break;
 			}
 		}
+
+		public static void JsonPathTest()
+		{
+			Console.WriteLine("JsonPathTest:");
+			for (int i = 0; i < 5; i++)
+			{
+				for (int j = 0; j < 5; j++)
+				{
+					Console.WriteLine("JsonPath({0}, {1}) = {2}", i, j, JsonPath(i, j));
+				}
+			}
+		}
+
+		public static string JsonPath(int continuationCount, int count)
+		{
+			StringBuilder sb = new StringBuilder("$");
+#if CS80
+			(int, bool) pair = continuationCount switch {
+				0 => (count - 1, true),
+				1 => (0, true),
+				_ => (continuationCount, false)
+			};
+			(int frameCount, bool includeCurrent) = pair;
+			for (int i = 0; i < frameCount; i++)
+				sb.Append(i);
+			if (includeCurrent)
+				sb.Append('c');
+#endif
+			return sb.ToString();
+		}
 	}
 }

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

@@ -8,35 +8,35 @@ public static class Issue684
 		int num = int.Parse(Console.ReadLine());
 		// Point of this test was to ensure the stack slot here uses an appropriate type,
 		// (bool instead of int). Unfortunately our type fixup runs too late to affect variable names.
-		bool num2 = num >= 1000;
+		bool flag = num >= 1000;
-		if (!num2)
+		if (!flag)
 		{
-			num2 = num < 2;
+			flag = num < 2;
 		}
-		if (num2)
+		if (flag)
 		{
 			Console.WriteLine(-1);
 		}
 		else
 		{
 			int i = 2;
-			for (int num3 = 2; num3 <= num; num3 = i)
+			for (int num2 = 2; num2 <= num; num2 = i)
 			{
-				Console.WriteLine(num3);
+				Console.WriteLine(num2);
-				for (; i <= num; i += num3)
+				for (; i <= num; i += num2)
 				{
-					int num4 = 1;
+					int num3 = 1;
-					array[i] = num4;
+					array[i] = num3;
 				}
-				i = num3;
+				i = num2;
 				while (true)
 				{
-					bool num5 = i <= num;
+					bool flag2 = i <= num;
-					if (num5)
+					if (flag2)
 					{
-						num5 = array[i] != 0;
+						flag2 = array[i] != 0;
 					}
-					if (!num5)
+					if (!flag2)
 					{
 						break;
 					}

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

@@ -40,11 +40,7 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 
 		private void SimpleArray()
 		{
-#if ROSLYN && OPT
-			var obj = new[] {
-#else
 			var array = new[] {
-#endif
 				new {
 					X = 5,
 					Y = 2,
@@ -57,13 +53,8 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 				}
 			};
 
-#if ROSLYN && OPT
-			Console.WriteLine(obj[0].X);
-			Console.WriteLine(obj[1].X);
-#else
 			Console.WriteLine(array[0].X);
 			Console.WriteLine(array[1].X);
-#endif
 		}
 #if !MCS
 		private void JaggedArray()
@@ -80,19 +71,11 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 					Z = -6
 				}
 			};
-#if ROSLYN && OPT
-			var obj = new[] { array, array };
-
-			Console.WriteLine(array[0].X);
-			Console.WriteLine(array[1].X);
-			Console.WriteLine(obj.Length);
-#else
 			var array2 = new[] { array, array };
 
 			Console.WriteLine(array[0].X);
 			Console.WriteLine(array[1].X);
 			Console.WriteLine(array2.Length);
-#endif
 		}
 #endif
 #if CS70

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

@@ -623,17 +623,9 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 		// leaf instead of becoming a nested designation.
 		public void LocalVariable_TupleInner_ElementUsedOutside()
 		{
-#if OPT
-			(int, (int, int)) tuple = GetTuple<int, (int, int)>();
-			int item = tuple.Item1;
-			(int, int) item2 = tuple.Item2;
-			Console.WriteLine(item);
-			Console.WriteLine(item2.Item1);
-#else
 			var (value, tuple2) = GetTuple<int, (int, int)>();
 			Console.WriteLine(value);
 			Console.WriteLine(tuple2.Item1);
-#endif
 		}
 
 		// Same, but the escaping element is in the first position. Every leaf of the

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

@@ -647,19 +647,11 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 
 		public unsafe void ForEachOverMultiDimArray3(int*[,] items)
 		{
-#if ROSLYN && OPT
-			foreach (int* intPtr in items)
-			{
-				Console.WriteLine(*intPtr);
-				Console.WriteLine(*intPtr);
-			}
-#else
 			foreach (int* ptr in items)
 			{
 				Console.WriteLine(*ptr);
 				Console.WriteLine(*ptr);
 			}
-#endif
 		}
 #endif
 

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

@@ -264,21 +264,12 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 
 		public static IEnumerable<char> Issue1310a(bool test)
 		{
-#if ROSLYN && OPT
-			IEnumerable<char> obj = (test ? (from c in Enumerable.Range(0, 255)
-											 where char.IsLetter((char)c)
-											 select (char)c) : (from c in Enumerable.Range(0, 255)
-																where char.IsDigit((char)c)
-																select (char)c));
-			return obj.Concat(obj);
-#else
 			IEnumerable<char> enumerable = (test ? (from c in Enumerable.Range(0, 255)
 													where char.IsLetter((char)c)
 													select (char)c) : (from c in Enumerable.Range(0, 255)
 																	   where char.IsDigit((char)c)
 																	   select (char)c));
 			return enumerable.Concat(enumerable);
-#endif
 		}
 
 		public static Maybe<TB> Cast<TA, TB>(Maybe<TA> a) where TB : class

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

@@ -253,6 +253,15 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 			}).HasValue;
 		}
 
+		public static bool TupleWithImmediateUse(int i)
+		{
+			return (i switch {
+				0 => (0, true),
+				1 => (1, true),
+				_ => (i, false),
+			}).Item2;
+		}
+
 		public static void ThrowDifferentExceptions(int i)
 		{
 			throw i switch {

ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs

@@ -4340,7 +4340,11 @@ namespace ICSharpCode.Decompiler.CSharp
 			}
 			else
 			{
-				resultType = compilation.FindType(inst.ResultType);
+				resultType = inst.InferType(compilation);
+				if (resultType.Kind == TypeKind.Unknown || resultType.GetStackType() != inst.ResultType)
+				{
+					resultType = compilation.FindType(inst.ResultType);
+				}
 			}
 
 			var expressionsForTypeInference = new List<TranslatedExpression>();

ICSharpCode.Decompiler/IL/ILReader.cs

@@ -507,7 +507,7 @@ namespace ICSharpCode.Decompiler.IL
 
 			// Merge different variables for same stack slot:
 			var unionFind = CheckOutgoingEdges();
-			var visitor = new CollectStackVariablesVisitor(unionFind);
+			var visitor = new CollectStackVariablesVisitor(unionFind, compilation);
 			foreach (var block in blocksByOffset.Values)
 			{
 				block.Block.AcceptVisitor(visitor);
@@ -1298,13 +1298,16 @@ namespace ICSharpCode.Decompiler.IL
 
 		sealed class CollectStackVariablesVisitor : ILVisitor<ILInstruction>
 		{
+			readonly ICompilation compilation;
 			readonly UnionFind<ILVariable> unionFind;
 			internal readonly HashSet<ILVariable> variables = new HashSet<ILVariable>();
 
-			public CollectStackVariablesVisitor(UnionFind<ILVariable> unionFind)
+			public CollectStackVariablesVisitor(UnionFind<ILVariable> unionFind, ICompilation compilation)
 			{
 				Debug.Assert(unionFind != null);
+				Debug.Assert(compilation != null);
 				this.unionFind = unionFind;
+				this.compilation = compilation;
 			}
 
 			protected override ILInstruction Default(ILInstruction inst)
@@ -1318,15 +1321,26 @@ namespace ICSharpCode.Decompiler.IL
 				return inst;
 			}
 
+			ILVariable MapVar(ILVariable v1)
+			{
+				var v2 = unionFind.Find(v1);
+				if (variables.Add(v2))
+				{
+					v2.Name = $"S_{variables.Count - 1}";
+				}
+				if (v1 != v2 && !v1.Type.Equals(v2.Type) && !v2.Type.CannotBeReconstructedFromStackType())
+				{
+					v2.Type = compilation.FindType(v1.StackType);
+				}
+				return v2;
+			}
+
 			protected internal override ILInstruction VisitLdLoc(LdLoc inst)
 			{
 				base.VisitLdLoc(inst);
 				if (inst.Variable.Kind == VariableKind.StackSlot)
 				{
-					var variable = unionFind.Find(inst.Variable);
+					inst.Variable = MapVar(inst.Variable);
-					if (variables.Add(variable))
-						variable.Name = $"S_{variables.Count - 1}";
-					return new LdLoc(variable).WithILRange(inst);
 				}
 				return inst;
 			}
@@ -1336,10 +1350,7 @@ namespace ICSharpCode.Decompiler.IL
 				base.VisitStLoc(inst);
 				if (inst.Variable.Kind == VariableKind.StackSlot)
 				{
-					var variable = unionFind.Find(inst.Variable);
+					inst.Variable = MapVar(inst.Variable);
-					if (variables.Add(variable))
-						variable.Name = $"S_{variables.Count - 1}";
-					return new StLoc(variable, inst.Value).WithILRange(inst);
 				}
 				return inst;
 			}
@@ -2107,7 +2118,20 @@ namespace ICSharpCode.Decompiler.IL
 			foreach (var inst in expressionStack)
 			{
 				Debug.Assert(inst.ResultType != StackType.Void);
-				IType type = compilation.FindType(inst.ResultType);
+				// Use InferType() for an improved type for these stackslot locals.
+				// This is crucial for value types, where FindType(StackType.O)
+				// would incorrectly use `object`.
+				// It's also highly useful for ref-locals,
+				// and shouldn't hurt for other types -- this type of
+				// stackslot-variable is never reassigned, so even types
+				// like `bool` shouldn't hurt.
+				// (note: if the variable is merged across control-flow branches,
+				//  we'll reset the type to be based on the StackType)
+				IType type = inst.InferType(compilation);
+				if (type.GetStackType() != inst.ResultType)
+				{
+					type = compilation.FindType(inst.ResultType);
+				}
 				var v = new ILVariable(VariableKind.StackSlot, type, inst.ResultType);
 				v.HasGeneratedName = true;
 				currentStack = currentStack.Push(v);

ICSharpCode.Decompiler/IL/ILTypeExtensions.cs

@@ -17,6 +17,8 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
 // DEALINGS IN THE SOFTWARE.
 
+using System.Linq;
+
 using ICSharpCode.Decompiler.TypeSystem;
 
 namespace ICSharpCode.Decompiler.IL
@@ -166,6 +168,14 @@ namespace ICSharpCode.Decompiler.IL
 		/// 
 		/// Returns SpecialType.UnknownType for unsupported instructions.
 		/// </summary>
+		/// <remarks>
+		/// For instructions with StackType.O that produce a value type, or
+		/// instructions with StackType.Ref, we should aim to return the actual type
+		/// instead of SpecialType.UnknownType.
+		/// 
+		/// If not returning UnknownType, must return a type that can store
+		/// the result of the instruction without loss of information.
+		/// </remarks>
 		public static IType InferType(this ILInstruction inst, ICompilation? compilation)
 		{
 			switch (inst)
@@ -242,6 +252,35 @@ namespace ICSharpCode.Decompiler.IL
 					return defaultValue.Type;
 				case ILFunction func when func.DelegateType != null:
 					return func.DelegateType;
+				case IfInstruction ifInst:
+					// For structs and byrefs, we don't want to return Unknown as a fallback to
+					// to FindType(StackType) wouldn't work. Valid IL should have the same
+					// type on both branches so we just return the first that works.
+					var thenType = ifInst.TrueInst.InferType(compilation);
+					if (thenType.CannotBeReconstructedFromStackType())
+					{
+						return thenType;
+					}
+					var elseType = ifInst.FalseInst.InferType(compilation);
+					if (elseType.CannotBeReconstructedFromStackType())
+					{
+						return elseType;
+					}
+					if (thenType.Equals(elseType))
+					{
+						return thenType;
+					}
+					return SpecialType.UnknownType;
+				case SwitchInstruction switchInst:
+					foreach (var section in switchInst.Sections)
+					{
+						var bodyType = section.Body.InferType(compilation);
+						if (bodyType.CannotBeReconstructedFromStackType())
+						{
+							return bodyType;
+						}
+					}
+					return SpecialType.UnknownType;
 				default:
 					return SpecialType.UnknownType;
 			}

ICSharpCode.Decompiler/TypeSystem/TypeUtils.cs

@@ -316,6 +316,23 @@ namespace ICSharpCode.Decompiler.TypeSystem
 			}
 		}
 
+		/// <summary>
+		/// Returns true for types where compilation.FindType(type.GetStackType()) will
+		/// be completely unsuitable (e.g. lead to miscompilation if the stack type
+		/// alone is used for when a variable is created for a stack slot):
+		///  * managed reference types
+		///  * value types with StackType.O
+		/// </summary>
+		public static bool CannotBeReconstructedFromStackType(this IType type)
+		{
+			var stackType = type.GetStackType();
+			if (stackType == StackType.Ref)
+			{
+				return true;
+			}
+			return stackType == StackType.O && type.IsReferenceType == false;
+		}
+
 		/// <summary>
 		/// If type is an enumeration type, returns the underlying type.
 		/// Otherwise, returns type unmodified.