Skip to content

Commit 0f37356

Browse files
dadhidadhi
committed
more things from #558: fix coalesce bug with nullable
1 parent 47a0678 commit 0f37356

4 files changed

Lines changed: 134 additions & 8 deletions

File tree

‎src/FastExpressionCompiler.LightExpression/Expression.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2179,7 +2179,7 @@ private static Type GetCoalesceType(Type left, Type right)
21792179

21802180
if (right.IsImplicitlyConvertibleTo(left))
21812181
return left;
2182-
if (left.IsImplicitlyConvertibleTo(right))
2182+
if ((underlyingLeft ?? left).IsImplicitlyConvertibleTo(right))
21832183
return right;
21842184
throw new ArgumentException($"Unable to coalesce arguments of left type of {left} and right type of {right}.");
21852185
}

‎src/FastExpressionCompiler/FastExpressionCompiler.cs‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2591,21 +2591,42 @@ private static bool TryEmitCoalesceOperator(BinaryExpression expr, ILGenerator i
25912591

25922592
il.Demit(OpCodes.Brfalse, labelFalse);
25932593

2594-
if (exprType == Nullable.GetUnderlyingType(leftType))
2594+
var underlyingLeftType = Nullable.GetUnderlyingType(leftType);
2595+
var underlyingExprType = Nullable.GetUnderlyingType(exprType) ?? exprType;
2596+
2597+
if (exprType == leftType)
2598+
EmitLoadLocalVariable(il, varIndex); // loading the value (not address) to return it
2599+
else
25952600
{
2596-
// if the target expression type is of underlying nullable, and the left operand is not null,
2597-
// then extract its underlying value
2601+
// the target expression type is the underlying type or a wider one (e.g. `int? ?? long`),
2602+
// then extract the underlying value and convert it if needed
25982603
EmitLoadLocalVariableAddress(il, varIndex);
25992604
il.Demit(OpCodes.Ldfld, leftType.GetNullableValueUnsafeAkaGetValueOrDefaultMethod());
2605+
if (underlyingLeftType != underlyingExprType &&
2606+
!TryEmitPrimitiveValueConvert(underlyingLeftType, underlyingExprType, il, isChecked: false))
2607+
return false;
2608+
if (exprType != underlyingExprType)
2609+
il.Demit(OpCodes.Newobj, exprType.GetNullableConstructor());
26002610
}
2601-
else
2602-
EmitLoadLocalVariable(il, varIndex); // loading the value (not address) to return it
26032611

26042612
il.Demit(OpCodes.Br, labelDone);
26052613
il.DmarkLabel(labelFalse);
26062614
if (!TryEmit(right, il, ref context, flags))
26072615
return false;
26082616

2617+
// the right operand may be of the narrower type than the result, e.g. `long? ?? int` or `int? ?? long?`
2618+
var rightType = right.Type;
2619+
if (rightType != exprType && rightType.IsValueType)
2620+
{
2621+
var underlyingRightType = Nullable.GetUnderlyingType(rightType);
2622+
if (underlyingRightType != null)
2623+
return false;
2624+
if (rightType != underlyingExprType && !TryEmitPrimitiveValueConvert(rightType, underlyingExprType, il, isChecked: false))
2625+
return false;
2626+
if (exprType != underlyingExprType)
2627+
il.Demit(OpCodes.Newobj, exprType.GetNullableConstructor());
2628+
}
2629+
26092630
il.DmarkLabel(labelDone);
26102631
}
26112632
else

‎test/FastExpressionCompiler.DotNetRuntimeTests/FastExpressionCompiler.DotNetRuntimeTests.csproj‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
<Nullable>disable</Nullable>
55
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
66
<EnableDefaultCompileItems>true</EnableDefaultCompileItems>
7-
<NoWarn>$(NoWarn);CS0618;CS0672;CS0649;CS0108;CS0659;CS0660;CS0661;xUnit1013;xUnit1026;xUnit1045;xUnit1044;xUnit2013</NoWarn>
7+
<NoWarn>$(NoWarn);CS0618;CS0672;CS0649;CS0108;CS0659;CS0660;CS0661;xUnit1013;xUnit1026;xUnit1045;xUnit1044;xUnit2013;xUnit2017</NoWarn>
88
</PropertyGroup>
99
<ItemGroup>
1010
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.14.1" />

‎test/FastExpressionCompiler.LightExpression.UnitTests/LightExpressionTests.cs‎

Lines changed: 106 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ public int Run()
6565
Can_compile_nullable_boolean_and();
6666
Can_compile_nullable_boolean_or();
6767
Nullable_boolean_logic_emits_the_same_IL_shape_as_system();
68+
Coalesce_type_is_the_same_as_in_system_expressions();
69+
Coalesce_with_nullable_left_emits_the_same_IL_shape_as_system();
6870
Can_compile_shifts_with_nullable_shift_count();
6971
Can_compile_nullable_boolean_and_also();
7072
Can_compile_nullable_boolean_or_else();
@@ -77,7 +79,7 @@ public int Run()
7779
Can_print_reference_comparison();
7880
Can_round_trip_new_expression_kinds_through_system_expressions();
7981

80-
return 58;
82+
return 60;
8183
}
8284

8385
private static int _sideEffectCalls;
@@ -540,6 +542,109 @@ void AssertResult(bool? leftValue, bool? rightValue, bool? expected)
540542
Asserts.Throws<InvalidOperationException>(() => And(left, right, incompatibleMethod));
541543
}
542544

545+
public void Coalesce_with_nullable_left_emits_the_same_IL_shape_as_system()
546+
{
547+
var ni = Parameter(typeof(int?), "ni");
548+
var nl = Parameter(typeof(long?), "nl");
549+
550+
Lambda<Func<int?, int>>(Coalesce(ni, Constant(5)), ni)
551+
.CompileFast(true, CompilerFlags.EnableDelegateDebugInfo).AssertOpCodes(
552+
OpCodes.Ldarg_1, OpCodes.Stloc_0, OpCodes.Ldloca_S, OpCodes.Call, OpCodes.Brfalse,
553+
OpCodes.Ldloca_S, OpCodes.Ldfld, OpCodes.Br, OpCodes.Ldc_I4_5, OpCodes.Ret);
554+
555+
Lambda<Func<int?, long>>(Coalesce(ni, Constant(5L)), ni)
556+
.CompileFast(true, CompilerFlags.EnableDelegateDebugInfo).AssertOpCodes(
557+
OpCodes.Ldarg_1, OpCodes.Stloc_0, OpCodes.Ldloca_S, OpCodes.Call, OpCodes.Brfalse,
558+
OpCodes.Ldloca_S, OpCodes.Ldfld, OpCodes.Conv_I8, OpCodes.Br, OpCodes.Ldc_I8, OpCodes.Ret);
559+
560+
Lambda<Func<int?, double>>(Coalesce(ni, Constant(5.5)), ni)
561+
.CompileFast(true, CompilerFlags.EnableDelegateDebugInfo).AssertOpCodes(
562+
OpCodes.Ldarg_1, OpCodes.Stloc_0, OpCodes.Ldloca_S, OpCodes.Call, OpCodes.Brfalse,
563+
OpCodes.Ldloca_S, OpCodes.Ldfld, OpCodes.Conv_R8, OpCodes.Br, OpCodes.Ldc_R8, OpCodes.Ret);
564+
565+
Lambda<Func<long?, long>>(Coalesce(nl, Constant(5)), nl)
566+
.CompileFast(true, CompilerFlags.EnableDelegateDebugInfo).AssertOpCodes(
567+
OpCodes.Ldarg_1, OpCodes.Stloc_0, OpCodes.Ldloca_S, OpCodes.Call, OpCodes.Brfalse,
568+
OpCodes.Ldloca_S, OpCodes.Ldfld, OpCodes.Br, OpCodes.Ldc_I4_5, OpCodes.Conv_I8, OpCodes.Ret);
569+
570+
Lambda<Func<int?, long?, long?>>(Coalesce(ni, nl), ni, nl)
571+
.CompileFast(true, CompilerFlags.EnableDelegateDebugInfo).AssertOpCodes(
572+
OpCodes.Ldarg_1, OpCodes.Stloc_0, OpCodes.Ldloca_S, OpCodes.Call, OpCodes.Brfalse,
573+
OpCodes.Ldloca_S, OpCodes.Ldfld, OpCodes.Conv_I8, OpCodes.Newobj, OpCodes.Br, OpCodes.Ldarg_2, OpCodes.Ret);
574+
}
575+
public void Coalesce_type_is_the_same_as_in_system_expressions()
576+
{
577+
var ni = Parameter(typeof(int?), "ni");
578+
var nl = Parameter(typeof(long?), "nl");
579+
var s = Parameter(typeof(string), "s");
580+
var sysNi = SysExpr.Parameter(typeof(int?), "ni");
581+
var sysNl = SysExpr.Parameter(typeof(long?), "nl");
582+
var sysS = SysExpr.Parameter(typeof(string), "s");
583+
584+
// int? ?? int -> int, int? ?? long -> long (widening of the underlying type), int? ?? double -> double
585+
var intOrInt = Coalesce(ni, Constant(5));
586+
Asserts.AreEqual(SysExpr.Coalesce(sysNi, SysExpr.Constant(5)).Type, intOrInt.Type);
587+
Asserts.AreEqual(typeof(int), intOrInt.Type);
588+
589+
var intOrLong = Coalesce(ni, Constant(5L));
590+
Asserts.AreEqual(SysExpr.Coalesce(sysNi, SysExpr.Constant(5L)).Type, intOrLong.Type);
591+
Asserts.AreEqual(typeof(long), intOrLong.Type);
592+
593+
var intOrDouble = Coalesce(ni, Constant(5.5));
594+
Asserts.AreEqual(SysExpr.Coalesce(sysNi, SysExpr.Constant(5.5)).Type, intOrDouble.Type);
595+
Asserts.AreEqual(typeof(double), intOrDouble.Type);
596+
597+
// int? ?? int? and long? ?? int stay nullable or widen as in BCL
598+
var intOrNullableInt = Coalesce(ni, Parameter(typeof(int?), "other"));
599+
Asserts.AreEqual(typeof(int?), intOrNullableInt.Type);
600+
601+
var nullableLongOrInt = Coalesce(nl, Constant(5));
602+
Asserts.AreEqual(SysExpr.Coalesce(sysNl, SysExpr.Constant(5)).Type, nullableLongOrInt.Type);
603+
Asserts.AreEqual(typeof(long), nullableLongOrInt.Type);
604+
605+
// int? ?? long? -> long?
606+
var intOrNullableLong = Coalesce(ni, nl);
607+
Asserts.AreEqual(SysExpr.Coalesce(sysNi, sysNl).Type, intOrNullableLong.Type);
608+
Asserts.AreEqual(typeof(long?), intOrNullableLong.Type);
609+
610+
// reference types
611+
var strOrObj = Coalesce(s, Constant(new object()));
612+
Asserts.AreEqual(typeof(object), strOrObj.Type);
613+
var strOrStr = Coalesce(s, Constant("x"));
614+
Asserts.AreEqual(typeof(string), strOrStr.Type);
615+
616+
// the result of compiled lambdas is the same as of the System ones
617+
var longLambda = Lambda<Func<int?, long>>(intOrLong, ni);
618+
var longSys = longLambda.CompileSys();
619+
var longFast = longLambda.CompileFast(true);
620+
Asserts.AreEqual(5L, longSys(null));
621+
Asserts.AreEqual(5L, longFast(null));
622+
Asserts.AreEqual(7L, longSys(7));
623+
Asserts.AreEqual(7L, longFast(7));
624+
625+
var doubleLambda = Lambda<Func<int?, double>>(intOrDouble, ni);
626+
var doubleSys = doubleLambda.CompileSys();
627+
var doubleFast = doubleLambda.CompileFast(true);
628+
Asserts.AreEqual(5.5, doubleSys(null));
629+
Asserts.AreEqual(5.5, doubleFast(null));
630+
Asserts.AreEqual(7.0, doubleSys(7));
631+
Asserts.AreEqual(7.0, doubleFast(7));
632+
633+
var nullableLongLambda = Lambda<Func<int?, long?, long?>>(intOrNullableLong, ni, nl);
634+
var nlSys = nullableLongLambda.CompileSys();
635+
var nlFast = nullableLongLambda.CompileFast(true);
636+
foreach (int? a in new int?[] { null, 3 })
637+
foreach (long? b in new long?[] { null, 9 })
638+
Asserts.AreEqual(nlSys(a, b), nlFast(a, b));
639+
640+
// the conversion to System expression keeps the type
641+
Asserts.AreEqual(typeof(long), intOrLong.ToExpression().Type);
642+
643+
// incompatible operands still throw
644+
Asserts.Throws<ArgumentException>(() => Coalesce(ni, Constant("x")));
645+
Asserts.Throws<ArgumentException>(() => Coalesce(Parameter(typeof(DateTime?), "d"), Constant(1)));
646+
}
647+
543648
public void Nullable_boolean_logic_emits_the_same_IL_shape_as_system()
544649
{
545650
var l = Parameter(typeof(bool?), "l");

0 commit comments

Comments
 (0)