Fix implicit conversion in expressions like C ? P : null

should fix part of #1694
riganti · Oct 18, 2023 · 72dffb2 · 72dffb2
1 parent 6d1a350
commit 72dffb2
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 3 deletions.
diff --git a/src/Framework/Framework/Compilation/Binding/ExpressionBuildingVisitor.cs b/src/Framework/Framework/Compilation/Binding/ExpressionBuildingVisitor.cs
@@ -13,6 +13,8 @@
 using DotVVM.Framework.Compilation.Parser.Binding.Parser.Annotations;
 using DotVVM.Framework.Binding;
 using DotVVM.Framework.Compilation.ControlTree.Resolved;
+using FastExpressionCompiler;
+using System.Diagnostics;
 
 namespace DotVVM.Framework.Compilation.Binding
 {
@@ -302,11 +304,43 @@ protected override Expression VisitConditionalExpression(ConditionalExpressionBi
 
             if (trueExpr.Type != falseExpr.Type)
             {
-                trueExpr = TypeConversion.ImplicitConversion(trueExpr, falseExpr.Type, allowToString: true) ?? trueExpr;
-                falseExpr = TypeConversion.ImplicitConversion(falseExpr, trueExpr.Type, allowToString: true) ?? falseExpr;
+                // implicit conversions are specified at https://github.com/ljw1004/csharpspec/blob/gh-pages/expressions.md#conditional-operator
+                // > If x has type X and y has type Y then
+                // > * If an implicit conversion (Implicit conversions) exists from X to Y, but not from Y to X, then Y is the type of the conditional expression.
+                // > * If an implicit conversion (Implicit conversions) exists from Y to X, but not from X to Y, then X is the type of the conditional expression.
+                // > * Otherwise, no expression type can be determined, and a compile-time error occurs.
+                // 
+
+                var trueConverted = TypeConversion.ImplicitConversion(trueExpr, falseExpr.Type, allowToString: true);
+                var falseConverted = TypeConversion.ImplicitConversion(falseExpr, trueExpr.Type, allowToString: true);
+
+                if (trueConverted is null && falseConverted is null)
+                    throw new BindingCompilationException($"Type of conditional expression '{node.ToDisplayString()}' cannot be determined because there is no implicit conversion between '{trueExpr.Type.ToCode()}' and '{falseExpr.Type.ToCode()}'", node);
+                else if (trueConverted is null)
+                    falseExpr = falseConverted;
+                else if (falseConverted is null)
+                    trueExpr = trueConverted;
+                else
+                {
+                    Debug.Assert(trueConverted.Type != falseConverted.Type);
+                    // 1. We represent some "typeless expressions" as expression of type object
+                    // 2. We allow conversion to string and also have the implicit conversion from string literal to enum
+                    // -> if we have an ambiguity, try to solve it by preferring the more specific type
+
+                    if (trueConverted.Type == typeof(object))
+                        falseExpr = falseConverted;
+                    else if (falseConverted.Type == typeof(object))
+                        trueExpr = trueConverted;
+                    else if (trueConverted.Type == typeof(string))
+                        falseExpr = falseConverted;
+                    else if (falseConverted.Type == typeof(string))
+                        trueExpr = trueConverted;
+                    else
+                        throw new BindingCompilationException($"Type of conditional expression '{node.ToDisplayString()}' cannot be determined because because '{trueExpr.Type.ToCode()}' and '{falseExpr.Type.ToCode()}' implicitly convert to one another", node);
+                }
             }
 
-            return Expression.Condition(condition!, trueExpr, falseExpr);
+            return Expression.Condition(condition!, trueExpr.NotNull(), falseExpr.NotNull());
         }
 
         protected override Expression VisitMemberAccess(MemberAccessBindingParserNode node)

diff --git a/src/Tests/Binding/BindingCompilationTests.cs b/src/Tests/Binding/BindingCompilationTests.cs
@@ -824,6 +824,26 @@ public void BindingCompiler_ImplicitConversion_ConditionalLongAndInt()
             Assert.AreEqual(5L, result);
         }
 
+        [TestMethod]
+        public void BindingCompiler_ImplicitConversion_ConditionalStringAndNull()
+        {
+            var resultNotNull = ExecuteBinding("_this.BoolProp ? _this.StringProp : null", new [] { new TestViewModel { StringProp = "test", BoolProp = true } }, expectedType: typeof(string));
+            Assert.AreEqual("test", resultNotNull);
+            resultNotNull = ExecuteBinding("!_this.BoolProp ? null : _this.StringProp", new [] { new TestViewModel { StringProp = "test", BoolProp = true } }, expectedType: typeof(string));
+            Assert.AreEqual("test", resultNotNull);
+            var resultNull = ExecuteBinding("_this.BoolProp ? _this.StringProp : null", new [] { new TestViewModel { StringProp = "test", BoolProp = false } }, expectedType: typeof(string));
+            Assert.IsNull(resultNull);
+        }
+
+        [TestMethod]
+        public void BindingCompiler_ImplicitConversion_ConditionalEnumAndLiteral()
+        {
+            var result = ExecuteBinding("_this.BoolProp ? _this.EnumProperty : 'A'", new [] { new TestViewModel { EnumProperty = TestEnum.B, BoolProp = false } }, expectedType: typeof(object));
+            Assert.AreEqual(TestEnum.A, result);
+            result = ExecuteBinding("_this.BoolProp ? 'A' : _this.EnumProperty", new [] { new TestViewModel { EnumProperty = TestEnum.B, BoolProp = false } }, expectedType: typeof(object));
+            Assert.AreEqual(TestEnum.B, result);
+        }
+
         [TestMethod]
         public void BindingCompiler_SimpleBlockExpression()
         {

diff --git a/src/Tests/Binding/ImplicitConversionTests.cs b/src/Tests/Binding/ImplicitConversionTests.cs
@@ -28,6 +28,13 @@ public void Conversion_IntToDouble()
             TypeConversion.EnsureImplicitConversion(Expression.Parameter(typeof(int)), typeof(double));
         }
 
+        [TestMethod]
+        public void Conversion_NullConversion()
+        {
+            TypeConversion.EnsureImplicitConversion(Expression.Constant(null, typeof(object)), typeof(string));
+            TypeConversion.EnsureImplicitConversion(Expression.Constant(null, typeof(object)), typeof(int?));
+        }
+
         [TestMethod]
         public void Conversion_ValidToString()
         {