From e5a5d1a2c1a5933091596d5d73b80dd8dd22e000 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Herceg?= Date: Fri, 29 Sep 2023 17:18:44 +0200 Subject: [PATCH 1/2] Repro for a bug with null assignment --- .../Common/DotVVM.Samples.Common.csproj | 1 + .../PropertyNullAssignmentViewModel.cs | 24 ++++++++++++++++++ .../PropertyNullAssignment.dothtml | 21 ++++++++++++++++ .../Abstractions/SamplesRouteUrls.designer.cs | 1 + .../Feature/ViewModelDeserializationTests.cs | 25 +++++++++++++++++++ 5 files changed, 72 insertions(+) create mode 100644 src/Samples/Common/ViewModels/FeatureSamples/ViewModelDeserialization/PropertyNullAssignmentViewModel.cs create mode 100644 src/Samples/Common/Views/FeatureSamples/ViewModelDeserialization/PropertyNullAssignment.dothtml diff --git a/src/Samples/Common/DotVVM.Samples.Common.csproj b/src/Samples/Common/DotVVM.Samples.Common.csproj index 4602ac2f7a..bd6baaecd1 100644 --- a/src/Samples/Common/DotVVM.Samples.Common.csproj +++ b/src/Samples/Common/DotVVM.Samples.Common.csproj @@ -127,6 +127,7 @@ + diff --git a/src/Samples/Common/ViewModels/FeatureSamples/ViewModelDeserialization/PropertyNullAssignmentViewModel.cs b/src/Samples/Common/ViewModels/FeatureSamples/ViewModelDeserialization/PropertyNullAssignmentViewModel.cs new file mode 100644 index 0000000000..154d660a92 --- /dev/null +++ b/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; + } + } +} + diff --git a/src/Samples/Common/Views/FeatureSamples/ViewModelDeserialization/PropertyNullAssignment.dothtml b/src/Samples/Common/Views/FeatureSamples/ViewModelDeserialization/PropertyNullAssignment.dothtml new file mode 100644 index 0000000000..7674deee2a --- /dev/null +++ b/src/Samples/Common/Views/FeatureSamples/ViewModelDeserialization/PropertyNullAssignment.dothtml @@ -0,0 +1,21 @@ +@viewModel DotVVM.Samples.Common.ViewModels.FeatureSamples.ViewModelDeserialization.PropertyNullAssignmentViewModel, DotVVM.Samples.Common + + + + + + + + + + +

{{value: NullableDateTime}}

+ + + + + + + + + diff --git a/src/Samples/Tests/Abstractions/SamplesRouteUrls.designer.cs b/src/Samples/Tests/Abstractions/SamplesRouteUrls.designer.cs index 899b5ad55c..89dde243e6 100644 --- a/src/Samples/Tests/Abstractions/SamplesRouteUrls.designer.cs +++ b/src/Samples/Tests/Abstractions/SamplesRouteUrls.designer.cs @@ -381,6 +381,7 @@ public partial class SamplesRouteUrls public const string FeatureSamples_ViewModelCache_ViewModelCacheMiss = "FeatureSamples/ViewModelCache/ViewModelCacheMiss"; public const string FeatureSamples_ViewModelDeserialization_DoesNotDropObject = "FeatureSamples/ViewModelDeserialization/DoesNotDropObject"; public const string FeatureSamples_ViewModelDeserialization_NegativeLongNumber = "FeatureSamples/ViewModelDeserialization/NegativeLongNumber"; + public const string FeatureSamples_ViewModelDeserialization_PropertyNullAssignment = "FeatureSamples/ViewModelDeserialization/PropertyNullAssignment"; public const string FeatureSamples_ViewModelNesting_NestedViewModel = "FeatureSamples/ViewModelNesting/NestedViewModel"; public const string FeatureSamples_ViewModelProtection_ComplexViewModelProtection = "FeatureSamples/ViewModelProtection/ComplexViewModelProtection"; public const string FeatureSamples_ViewModelProtection_NestedSignatures = "FeatureSamples/ViewModelProtection/NestedSignatures"; diff --git a/src/Samples/Tests/Tests/Feature/ViewModelDeserializationTests.cs b/src/Samples/Tests/Tests/Feature/ViewModelDeserializationTests.cs index 83a93fe586..207220877a 100644 --- a/src/Samples/Tests/Tests/Feature/ViewModelDeserializationTests.cs +++ b/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) { } From 7edbc9656a1300459ffd876a7b1808a71640c5bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Sun, 1 Oct 2023 17:53:48 +0200 Subject: [PATCH 2/2] null-propagation: fix assignment of null into a property --- .../ExpressionNullPropagationVisitor.cs | 21 +++--- .../Compilation/Binding/TypeConversions.cs | 11 ++- src/Samples/Tests/Tests/ErrorsTests.cs | 2 +- src/Tests/Binding/BindingCompilationTests.cs | 69 ++++++++++++++++++- src/Tests/Binding/NullPropagationTests.cs | 19 ++++- 5 files changed, 107 insertions(+), 15 deletions(-) diff --git a/src/Framework/Framework/Compilation/Binding/ExpressionNullPropagationVisitor.cs b/src/Framework/Framework/Compilation/Binding/ExpressionNullPropagationVisitor.cs index 51ead1e4ee..a33c85b1e9 100644 --- a/src/Framework/Framework/Compilation/Binding/ExpressionNullPropagationVisitor.cs +++ b/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) { diff --git a/src/Framework/Framework/Compilation/Binding/TypeConversions.cs b/src/Framework/Framework/Compilation/Binding/TypeConversions.cs index 0b2d47c600..1f04527a48 100644 --- a/src/Framework/Framework/Compilation/Binding/TypeConversions.cs +++ b/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; } diff --git a/src/Samples/Tests/Tests/ErrorsTests.cs b/src/Samples/Tests/Tests/ErrorsTests.cs index 45aef6e60c..4057b6e87b 100644 --- a/src/Samples/Tests/Tests/ErrorsTests.cs +++ b/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")); diff --git a/src/Tests/Binding/BindingCompilationTests.cs b/src/Tests/Binding/BindingCompilationTests.cs index 206eabafb8..3deb4eaeea 100755 --- a/src/Tests/Binding/BindingCompilationTests.cs +++ b/src/Tests/Binding/BindingCompilationTests.cs @@ -936,7 +936,7 @@ 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" }; @@ -944,7 +944,7 @@ public void BindingCompiler_MultiBlockExpression_EmptyBlockAtEnd_Throws() } [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(() => + 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(() => + ExecuteBinding("DateTime = null", new TestViewModel()) + ); + XAssert.Contains("Cannot convert null to System.DateTime", ex.Message); + } + [DataTestMethod] [DataRow("IntProp + 1L", 101L)] [DataRow("1L + IntProp", 101L)] diff --git a/src/Tests/Binding/NullPropagationTests.cs b/src/Tests/Binding/NullPropagationTests.cs index 7cf948af98..8a89309c4b 100644 --- a/src/Tests/Binding/NullPropagationTests.cs +++ b/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(Expression> e) => e; @@ -201,6 +208,16 @@ Func 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; + } } }