Stack slots holding value types previously nevertheless often had type `object`

This was due to StackType.O doing double-duty as `object` and `other`.
While ExpressionBuilder would often improve the type of such locals, the `object` nevertheless ended up used in a couple of places, e.g. via the `typeHint`. This could result in value types being boxed even though the original IL didn't contain any `box` instruction.

This is an attempt to use better types for stack slot variables created by ILReader. The idea is: there aren't many IL instructions that produce "other" value types, and `InferType()` already handles pretty much all of them, so we can use that to assign types to our stack slots.

It's a bit more tricky if the stack is pushed to on multiple branches that join together before the value is used: here the variable type must be suitable for both assignments. In this case, we go back to the previously-used stacktype.

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/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.