Merge pull request #1706 from riganti/fix/nullable-command-change

Repro for a bug with null assignment

src/Framework/Framework/Compilation/Binding/ExpressionNullPropagationVisitor.cs

@@ -47,32 +47,33 @@ Expression createExpr(Expression left)
             {
                 return CheckForNull(Visit(node.Right), right =>
                 {
-                    // When assigning values to nullable types, convert value to nullable first
-                    if (node.NodeType == ExpressionType.Assign)
-                    {
-                        if (ReflectionUtils.IsNullableType(left.Type) && !ReflectionUtils.IsNullableType(right.Type))
-                            right = Expression.Convert(right, left.Type);
-                    }
-
                     return Expression.MakeBinary(node.NodeType, left, right, false, node.Method, node.Conversion);
                 }, checkReferenceTypes: false);
             }
 
             if (node.NodeType.ToString().EndsWith("Assign"))
             {
+                var right = Visit(node.Right);
+                // Convert non-nullable to nullable
+                // and unwrap nullable to non-nullable - throw a runtime NullReferenceException when right is null
+                if (right.Type != node.Right.Type && right.Type.UnwrapNullableType() == right.Type.UnwrapNullableType())
+                {
+                    right = Expression.Convert(right, node.Right.Type);
+                }
+
                 // only check for left target's null, assignment to null is perfectly valid
                 if (node.Left is MemberExpression memberExpression)
                 {
                     return CheckForNull(Visit(memberExpression.Expression), memberTarget =>
-                        createExpr(memberExpression.Update(memberTarget)));
+                        node.Update(memberExpression.Update(memberTarget), node.Conversion, right));
                 }
                 else if (node.Left is IndexExpression indexer)
                 {
                     return CheckForNull(Visit(indexer.Object), memberTarget =>
-                        createExpr(indexer.Update(memberTarget, indexer.Arguments)));
+                        node.Update(indexer.Update(memberTarget, indexer.Arguments), node.Conversion, right));
                 }
                 // this should only be ParameterExpression
-                else return createExpr(node.Left);
+                else return node.Update(node.Left, node.Conversion, right);
             }
             else if (node.NodeType == ExpressionType.ArrayIndex)
             {

src/Framework/Framework/Compilation/Binding/TypeConversions.cs

@@ -130,7 +130,16 @@ public static Expression EnsureImplicitConversion(Expression src, Type destType,
             {
                 result = ToStringConversion(src);
             }
-            if (throwException && result == null) throw new InvalidOperationException($"Could not implicitly convert expression of type { src.Type.ToCode() } to { destType.ToCode() }.");
+            if (throwException && result == null)
+            {
+                if (src is ConstantExpression { Value: null } && destType.IsValueType)
+                    throw new InvalidOperationException($"Cannot convert null to {destType.ToCode()} because it is a non-nullable value type.");
+                else if (src is ConstantExpression { Value: var value })
+                    // constants have quite different conversion rules, so we include the value in the error message
+                    throw new InvalidOperationException($"Cannot convert '{value}' of type {src.Type.ToCode()} to {destType.ToCode()}.");
+
+                throw new InvalidOperationException($"Cannot implicitly convert expression of type { src.Type.ToCode() } to { destType.ToCode() }.");
+            }
             return result;
         }
 

src/Samples/Common/DotVVM.Samples.Common.csproj

@@ -127,6 +127,7 @@
     <None Remove="Views\FeatureSamples\Validation\RangeClientSideValidation.dothtml" />
     <None Remove="Views\FeatureSamples\Validation\ValidationTargetIsCollection.dothtml" />
     <None Remove="Views\FeatureSamples\Validation\ValidatorValueComplexExpressions.dothtml" />
+    <None Remove="Views\FeatureSamples\ViewModelDeserialization\PropertyNullAssignment.dothtml" />
     <None Remove="Views\FeatureSamples\ViewModules\Incrementer.dotcontrol" />
     <None Remove="Views\FeatureSamples\ViewModules\IncrementerInRepeater.dothtml" />
     <None Remove="Views\FeatureSamples\ViewModules\InnerModuleControl.dotcontrol" />

src/Samples/Common/ViewModels/FeatureSamples/ViewModelDeserialization/PropertyNullAssignmentViewModel.cs

@@ -0,0 +1,24 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using DotVVM.Framework.ViewModel;
+using DotVVM.Framework.Hosting;
+
+namespace DotVVM.Samples.Common.ViewModels.FeatureSamples.ViewModelDeserialization
+{
+    public class PropertyNullAssignmentViewModel : DotvvmViewModelBase
+    {
+
+        public DateTime? NullableDateTime { get; set; }
+
+        public DateTime Value { get; set; } = new DateTime(2023, 1, 2, 3, 4, 5, 678);
+
+        public void SetNullableDateTimeToNull()
+        {
+            NullableDateTime = null;
+        }
+    }
+}
+

src/Samples/Common/Views/FeatureSamples/ViewModelDeserialization/PropertyNullAssignment.dothtml

@@ -0,0 +1,21 @@
+@viewModel DotVVM.Samples.Common.ViewModels.FeatureSamples.ViewModelDeserialization.PropertyNullAssignmentViewModel, DotVVM.Samples.Common
+
+<!DOCTYPE html>
+
+<html lang="en" xmlns="http://www.w3.org/1999/xhtml">
+<head>
+    <meta charset="utf-8" />
+    <title></title>
+</head>
+<body>
+
+    <p class="result">{{value: NullableDateTime}}</p>
+
+    <dot:Button Text="Set value" Click="{command: NullableDateTime = Value}" />
+    <dot:Button Text="Set null" Click="{command: NullableDateTime = null}" />
+    <dot:Button Text="Set null with command" Click="{command: SetNullableDateTimeToNull()}" />
+
+</body>
+</html>
+
+

src/Samples/Tests/Tests/ErrorsTests.cs

@@ -99,7 +99,7 @@ public void Error_WrongPropertyValue()
             RunInAllBrowsers(browser => {
                 browser.NavigateToUrl(SamplesRouteUrls.Errors_WrongPropertyValue);
                 AssertUI.InnerText(browser.First("[class='exceptionMessage']")
-                    , s => s.Contains("Could not implicitly convert expression"));
+                    , s => s.Contains("Cannot convert 'NotAllowedValue' of type string to bool?"));
 
                 AssertUI.InnerText(browser.First("[class='errorUnderline']")
                     , s => s.Contains("NotAllowedValue"));

src/Samples/Tests/Tests/Feature/ViewModelDeserializationTests.cs

@@ -52,6 +52,31 @@ public void Feature_ViewModelDeserialization_NegativeLongNumber()
             });
         }
 
+        [Fact]
+        public void Feature_ViewModelDeserialization_PropertyNullAssignment()
+        {
+            RunInAllBrowsers(browser => {
+                browser.NavigateToUrl(SamplesRouteUrls.FeatureSamples_ViewModelDeserialization_PropertyNullAssignment);
+
+                var value = browser.Single(".result");
+                var buttons = browser.FindElements("input[type=button]");
+
+                AssertUI.InnerTextEquals(value, "");
+
+                buttons[0].Click();
+                AssertUI.InnerTextEquals(value, "1/2/2023 3:04:05 AM");
+
+                buttons[1].Click();
+                AssertUI.InnerTextEquals(value, "");
+
+                buttons[0].Click();
+                AssertUI.InnerTextEquals(value, "1/2/2023 3:04:05 AM");
+
+                buttons[2].Click();
+                AssertUI.InnerTextEquals(value, "");
+            });
+        }
+
         public ViewModelDeserializationTests(ITestOutputHelper output) : base(output)
         {
         }

src/Tests/Binding/BindingCompilationTests.cs

@@ -936,15 +936,15 @@ public void BindingCompiler_MultiBlockExpression_EnumAtEnd_CorrectResult()
         }
 
         [TestMethod]
-        [ExpectedExceptionMessageSubstring(typeof(BindingPropertyException), "Could not implicitly convert expression of type void to object")]
+        [ExpectedExceptionMessageSubstring(typeof(BindingPropertyException), "Cannot implicitly convert expression of type void to object")]
         public void BindingCompiler_MultiBlockExpression_EmptyBlockAtEnd_Throws()
         {
             TestViewModel vm = new TestViewModel { StringProp = "a" };
             var result = ExecuteBinding("GetEnum();", new[] { vm });
         }
 
         [TestMethod]
-        [ExpectedExceptionMessageSubstring(typeof(BindingPropertyException), "Could not implicitly convert expression of type void to object")]
+        [ExpectedExceptionMessageSubstring(typeof(BindingPropertyException), "Cannot implicitly convert expression of type void to object")]
         public void BindingCompiler_MultiBlockExpression_WhitespaceBlockAtEnd_Throws()
         {
             TestViewModel vm = new TestViewModel { StringProp = "a" };
@@ -1103,6 +1103,71 @@ public void Error_DifferentDataContext()
             );
         }
 
+        [TestMethod]
+        public void NullableIntAssignment()
+        {
+            var vm = new TestViewModel() { NullableIntProp = 11 };
+            ExecuteBinding("NullableIntProp = null", vm);
+            Assert.AreEqual(null, vm.NullableIntProp);
+            ExecuteBinding("NullableIntProp = 3", vm);
+            Assert.AreEqual(3, vm.NullableIntProp);
+        }
+
+        [TestMethod]
+        public void NullableDateAssignment()
+        {
+            var vm = new TestViewModel() {
+                DateFrom = new DateTime(2000, 1, 1, 11, 11, 11),
+                DateTime = new DateTime(203, 3, 3, 3, 3, 3),
+                DateOnly = new DateOnly(2000, 1, 1),
+                NullableDateOnly = new DateOnly(2000, 1, 1)
+            };
+            ExecuteBinding("DateFrom = null; NullableDateOnly = null", vm);
+            Assert.AreEqual(null, vm.DateFrom);
+            Assert.AreEqual(null, vm.NullableDateOnly);
+
+            ExecuteBinding("DateFrom = DateTime; NullableDateOnly = DateOnly", vm);
+            Assert.AreEqual(new DateTime(203, 3, 3, 3, 3, 3), vm.DateFrom);
+            Assert.AreEqual(vm.DateTime, vm.DateFrom);
+            Assert.AreEqual(new DateOnly(2000, 1, 1), vm.NullableDateOnly);
+            Assert.AreEqual(vm.DateOnly, vm.NullableDateOnly);
+        }
+
+        [TestMethod]
+        public void NullableToNonnullableAssignment()
+        {
+            var vm = new TestViewModel() { NullableIntProp = 11 };
+            ExecuteBinding("IntProp = NullableIntProp", vm);
+            Assert.AreEqual(11, vm.IntProp);
+            ExecuteBinding("IntProp = NullableIntProp.Value", vm);
+            Assert.AreEqual(11, vm.IntProp);
+
+            vm.NullableIntProp = null;
+            var exception = Assert.ThrowsException<InvalidOperationException>(() =>
+                ExecuteBinding("IntProp = NullableIntProp", vm));
+            Assert.AreEqual("Nullable object must have a value.", exception.Message);
+        }
+
+        [TestMethod]
+        public void NullableStringAssignment()
+        {
+            var vm = new TestViewModel() { StringProp2 = "A", StringProp = "B" };
+            ExecuteBinding("StringProp2 = null", vm);
+            Assert.AreEqual(null, vm.StringProp2);
+            ExecuteBinding("StringProp2 = StringProp", vm);
+            Assert.AreEqual("B", vm.StringProp2);
+            Assert.AreEqual(vm.StringProp, vm.StringProp2);
+        }
+
+        [TestMethod]
+        public void Error_ValueTypeIntAssignment()
+        {
+            var ex = XAssert.ThrowsAny<Exception>(() =>
+                ExecuteBinding("DateTime = null", new TestViewModel())
+            );
+            XAssert.Contains("Cannot convert null to System.DateTime", ex.Message);
+        }
+
         [DataTestMethod]
         [DataRow("IntProp + 1L", 101L)]
         [DataRow("1L + IntProp", 101L)]

src/Tests/Binding/NullPropagationTests.cs

@@ -62,7 +62,14 @@ public class NullPropagationTests
             Create((DateTime d) => d.ToString()),
             Create((double a) => TimeSpan.FromSeconds(a)),
             Create((TestViewModel vm) => (TimeSpan)vm.Time),
-            Create((TestViewModel vm, TimeSpan time, int integer, double number, DateTime d) => new TestViewModel{ EnumProperty = TestEnum.B, BoolMethodExecuted = vm.BoolMethodExecuted, StringProp = time + ": " + vm.StringProp, StringProp2 = integer + vm.StringProp2 + number, TestViewModel2 = vm.TestViewModel2, DateFrom = d}),
+            Create((TestViewModel vm, TimeSpan time, int integer, double number, DateTime d) => new TestViewModel {
+                EnumProperty = TestEnum.B,
+                BoolMethodExecuted = (bool?)vm.BoolMethodExecuted ?? false,
+                StringProp = time + ": " + vm.StringProp,
+                StringProp2 = integer + vm.StringProp2 + number,
+                TestViewModel2 = vm.TestViewModel2,
+                DateFrom = d
+            }),
         };
 
         private static LambdaExpression Create<T1, T2>(Expression<Func<T1, T2>> e) => e;
@@ -201,6 +208,16 @@ Func<TestViewModel[], object> compile(Expression e) =>
                     // and it must only occur for value types
                     Assert.IsTrue(new [] { "System.Int", "System.TimeSpan", "System.Char", "System.Double" }.Any(ex.Message.Contains));
                 }
+                catch (Exception e)
+                {
+                    Console.WriteLine($"Original expression: {expr.ToCSharpString()}");
+                    Console.WriteLine($"Null-checked expression {expr.ToCSharpString()}");
+                    Console.WriteLine();
+                    Console.WriteLine(expr.ToExpressionString());
+                    Console.WriteLine(e.ToString());
+                    Assert.Fail($"Exception {e.Message} while executing null-checked {expr.ToCSharpString()}");
+                    return;
+                }
             }
         }