Skip to content

Commit

Permalink
null-propagation: fix assignment of null into a property
Browse files Browse the repository at this point in the history
  • Loading branch information
exyi committed Oct 5, 2023
1 parent e5a5d1a commit 7edbc96
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
11 changes: 10 additions & 1 deletion src/Framework/Framework/Compilation/Binding/TypeConversions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Samples/Tests/Tests/ErrorsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down
69 changes: 67 additions & 2 deletions src/Tests/Binding/BindingCompilationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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" };
Expand Down Expand Up @@ -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)]
Expand Down
19 changes: 18 additions & 1 deletion src/Tests/Binding/NullPropagationTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
}

Expand Down

0 comments on commit 7edbc96

Please sign in to comment.