From a98d13a98e7bcfb7cb75eeef97f6d4cd3333db59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Sat, 1 Jul 2023 15:36:04 +0200 Subject: [PATCH] Fix equality operator resolution --- .../Compilation/Binding/OperatorResolution.cs | 77 +++++++------------ .../Tests/Tests/DotVVM.Samples.Tests.csproj | 4 +- src/Tests/Binding/BindingCompilationTests.cs | 36 ++++++++- .../Binding/JavascriptCompilationTests.cs | 30 +++++--- .../Parser/Binding/BindingParserTests.cs | 2 +- 5 files changed, 81 insertions(+), 68 deletions(-) diff --git a/src/Framework/Framework/Compilation/Binding/OperatorResolution.cs b/src/Framework/Framework/Compilation/Binding/OperatorResolution.cs index 3665478c9f..ba475be74d 100644 --- a/src/Framework/Framework/Compilation/Binding/OperatorResolution.cs +++ b/src/Framework/Framework/Compilation/Binding/OperatorResolution.cs @@ -93,7 +93,6 @@ public static Expression GetBinaryOperator( _ => null }; - // Try to find user defined operator if (customOperator != null && (!leftType.IsPrimitive || !rightType.IsPrimitive)) { @@ -107,18 +106,30 @@ public static Expression GetBinaryOperator( throw new InvalidOperationException($"Cannot apply {operation} operator to two different enum types: {leftType.Name}, {rightType.Name}."); } - // numeric operations - if (operation == ExpressionType.LeftShift) + if (operation is ExpressionType.Equal or ExpressionType.NotEqual && !leftType.IsValueType && !rightType.IsValueType) { - return Expression.LeftShift(left, ConvertToMaybeNullable(right, typeof(int), true)!); + // https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/expressions.md#11117-reference-type-equality-operators + // Every class type C implicitly provides the following predefined reference type equality operators: + // bool operator ==(C x, C y); + // bool operator !=(C x, C y); + return ReferenceEquality(left, right, operation == ExpressionType.NotEqual); } - else if (operation == ExpressionType.RightShift) + + if (operation is ExpressionType.LeftShift or ExpressionType.RightShift) { - return Expression.RightShift(left, ConvertToMaybeNullable(right, typeof(int), true)!); + // https://github.com/dotnet/csharpstandard/blob/standard-v6/standard/expressions.md#1110-shift-operators + // * shift operators always take int32 as the second argument + var rightConverted = ConvertToMaybeNullable(right, typeof(int), true)!; + // * the first argument is int, uint, long, ulong (in this order) + var leftConverted = ConvertToMaybeNullable(left, typeof(int), false) ?? ConvertToMaybeNullable(left, typeof(uint), false) ?? ConvertToMaybeNullable(left, typeof(long), false) ?? ConvertToMaybeNullable(left, typeof(ulong), false)!; + if (leftConverted is null) + throw new InvalidOperationException($"Cannot apply {operation} operator to type {leftType.ToCode()}. The type must be convertible to an integer or have a custom operator defined."); + return operation == ExpressionType.LeftShift ? Expression.LeftShift(leftConverted, rightConverted) : Expression.RightShift(leftConverted, rightConverted); } // List of types in order of precendence var enumType = leftType.IsEnum ? leftType : rightType.IsEnum ? rightType : null; + // all operators have defined "overloads" for two var typeList = operation switch { ExpressionType.Or or ExpressionType.And or ExpressionType.ExclusiveOr => new[] { typeof(bool), enumType, typeof(int), typeof(uint), typeof(long), typeof(ulong) }, @@ -152,56 +163,22 @@ public static Expression GetBinaryOperator( // if (left.Type.IsNullable() || right.Type.IsNullable()) // return GetBinaryOperator(expressionFactory, left.UnwrapNullable(), right.UnwrapNullable(), operation); - // as a fallback, try finding overridden Equals method - if (operation == ExpressionType.Equal) return EqualsMethod(expressionFactory, left, right); - if (operation == ExpressionType.NotEqual) return Expression.Not(EqualsMethod(expressionFactory, left, right)); - throw new InvalidOperationException($"Cannot apply {operation} operator to types {left.Type.Name} and {right.Type.Name}."); } - public static Expression EqualsMethod( - MemberExpressionFactory expressionFactory, - Expression left, - Expression right - ) + static Expression ReferenceEquality(Expression left, Expression right, bool not) { - Expression? equatable = null; - Expression? theOther = null; - if (typeof(IEquatable<>).IsAssignableFrom(left.Type)) - { - equatable = left; - theOther = right; - } - else if (typeof(IEquatable<>).IsAssignableFrom(right.Type)) - { - equatable = right; - theOther = left; - } - - if (equatable != null) + // * It is a binding-time error to use the predefined reference type equality operators to compare two references that are known to be different at binding-time. For example, if the binding-time types of the operands are two class types, and if neither derives from the other, then it would be impossible for the two operands to reference the same object. Thus, the operation is considered a binding-time error. + var leftT = left.Type; + var rightT = right.Type; + if (leftT != rightT && !(leftT.IsAssignableFrom(rightT) || rightT.IsAssignableFrom(leftT))) { - var m = expressionFactory.CallMethod(equatable, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.FlattenHierarchy, "Equals", null, new[] { theOther! }); - if (m != null) return m; + if (!leftT.IsInterface && rightT.IsInterface) + throw new InvalidOperationException($"Cannot compare types {leftT.ToCode()} and {rightT.ToCode()}, because the classes are unrelated."); + if (leftT.IsSealed || rightT.IsSealed) + throw new InvalidOperationException($"Cannot compare types {leftT.ToCode()} and {rightT.ToCode()}, because {(leftT.IsSealed ? leftT : rightT).ToCode(stripNamespace: true)} is sealed and does not implement {rightT.ToCode(stripNamespace: true)}."); } - - if (left.Type.IsValueType) - { - equatable = left; - theOther = right; - } - else if (left.Type.IsValueType) - { - equatable = right; - theOther = left; - } - - if (equatable != null) - { - theOther = TypeConversion.ImplicitConversion(theOther!, equatable.Type); - if (theOther != null) return Expression.Equal(equatable, theOther); - } - - return expressionFactory.CallMethod(left, BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.FlattenHierarchy, "Equals", null, new[] { right }); + return not ? Expression.ReferenceNotEqual(left, right) : Expression.ReferenceEqual(left, right); } static Expression? ConvertToMaybeNullable( diff --git a/src/Samples/Tests/Tests/DotVVM.Samples.Tests.csproj b/src/Samples/Tests/Tests/DotVVM.Samples.Tests.csproj index ced2bba49f..1e4167c204 100644 --- a/src/Samples/Tests/Tests/DotVVM.Samples.Tests.csproj +++ b/src/Samples/Tests/Tests/DotVVM.Samples.Tests.csproj @@ -9,8 +9,8 @@ - - + + all runtime; build; native; contentfiles; analyzers; buildtransitive diff --git a/src/Tests/Binding/BindingCompilationTests.cs b/src/Tests/Binding/BindingCompilationTests.cs index 2d7010d0ba..d607114948 100755 --- a/src/Tests/Binding/BindingCompilationTests.cs +++ b/src/Tests/Binding/BindingCompilationTests.cs @@ -1123,11 +1123,41 @@ public void Error_DifferentDataContext() [DataRow("DoubleProp - 1", 0.5)] [DataRow("DoubleProp + ShortProp", short.MaxValue + 1.5)] [DataRow("NullableDoubleProp + ShortProp", null)] - public void BindingCompiler_OperatorType(string expr, object expectedResult) - { - var vm = new TestViewModel { IntProp = 100, DoubleProp = 1.5 }; + [DataRow("ByteProp | ByteProp", (byte)255)] + [DataRow("DateTime == DateTime", true)] + [DataRow("NullableTimeOnly == NullableTimeOnly", true)] + [DataRow("NullableTimeOnly != NullableTimeOnly", false)] + [DataRow("NullableTimeOnly == TimeOnly", false)] + [DataRow("EnumList[0] > EnumList[1]", false)] + [DataRow("EnumList[0] < EnumList[1]", true)] + [DataRow("EnumList[0] == 'A'", true)] + [DataRow("EnumList[0] < 'C'", true)] + [DataRow("(EnumList[1] | 'C') == 'C'", false)] + [DataRow("(EnumList[2] & 1) != 0", false)] + public void BindingCompiler_BinaryOperator_ResultType(string expr, object expectedResult) + { + var vm = new TestViewModel { IntProp = 100, DoubleProp = 1.5, EnumList = new () { TestEnum.A, TestEnum.B, TestEnum.C, TestEnum.D } }; Assert.AreEqual(expectedResult, ExecuteBinding(expr, vm)); } + + [TestMethod] + [ExpectedExceptionMessageSubstring(typeof(BindingPropertyException), "Reference equality is not defined for the types 'DotVVM.Framework.Tests.Binding.TestViewModel2' and 'DotVVM.Framework.Tests.Binding.TestViewModel'")] + public void BindingCompiler_InvalidReferenceComparison() => + ExecuteBinding("TestViewModel2 == _this", new TestViewModel()); + + [TestMethod] + [ExpectedExceptionMessageSubstring(typeof(BindingPropertyException), "Cannot apply Equal operator to types DateTime and Object.")] + public void BindingCompiler_InvalidStructReferenceComparison() => + ExecuteBinding("DateTime == Time", new TestViewModel()); + + [TestMethod] + [ExpectedExceptionMessageSubstring(typeof(BindingPropertyException), "Cannot apply Equal operator to types DateTime and Object.")] + public void BindingCompiler_InvalidStructComparison() => + ExecuteBinding("DateTime == Time", new TestViewModel()); + [TestMethod] + [ExpectedExceptionMessageSubstring(typeof(BindingPropertyException), "Cannot apply And operator to types TestEnum and Boolean")] + public void BindingCompiler_InvalidBitAndComparison() => + ExecuteBinding("EnumProperty & 2 == 0", new TestViewModel()); } class TestViewModel { diff --git a/src/Tests/Binding/JavascriptCompilationTests.cs b/src/Tests/Binding/JavascriptCompilationTests.cs index d6da0eafda..193b4103d3 100644 --- a/src/Tests/Binding/JavascriptCompilationTests.cs +++ b/src/Tests/Binding/JavascriptCompilationTests.cs @@ -165,19 +165,25 @@ public void JavascriptCompilation_Parent() } [DataTestMethod] - // [DataRow("2+2", "2+2", DisplayName = "2+2")] - // [DataRow("2+2+2", "2+2+2", DisplayName = "2+2+2")] - // [DataRow("(2+2)+2", "2+2+2", DisplayName = "(2+2)+2")] - // [DataRow("2+(2+2)", "2+(2+2)", DisplayName = "2+(2+2)")] - // [DataRow("2+(2*2)", "2+2*2", DisplayName = "2+(2*2)")] - // [DataRow("2*(2+2)", "2*(2+2)", DisplayName = "2*(2+2)")] - // [DataRow("IntProp & (2+2)", "IntProp()&2+2", DisplayName = "IntProp & (2+2)")] - // [DataRow("IntProp & 2+2", "IntProp()&2+2", DisplayName = "IntProp & 2+2")] - // [DataRow("IntProp & -1", "IntProp()&-1", DisplayName = "IntProp & -1")] - // [DataRow("'a' + 'b'", "\"ab\"", DisplayName = "'a' + 'b'")] - // [DataRow("IntProp ^ 1", "IntProp()^1", DisplayName = "IntProp ^ 1")] - // [DataRow("'xx' + IntProp", "\"xx\"+IntProp()", DisplayName = "'xx' + IntProp")] + [DataRow("2+2", "2+2", DisplayName = "2+2")] + [DataRow("2+2+2", "2+2+2", DisplayName = "2+2+2")] + [DataRow("(2+2)+2", "2+2+2", DisplayName = "(2+2)+2")] + [DataRow("2+(2+2)", "2+(2+2)", DisplayName = "2+(2+2)")] + [DataRow("2+(2*2)", "2+2*2", DisplayName = "2+(2*2)")] + [DataRow("2*(2+2)", "2*(2+2)", DisplayName = "2*(2+2)")] + [DataRow("IntProp & (2+2)", "IntProp()&2+2", DisplayName = "IntProp & (2+2)")] + [DataRow("IntProp & 2+2", "IntProp()&2+2", DisplayName = "IntProp & 2+2")] + [DataRow("IntProp & -1", "IntProp()&-1", DisplayName = "IntProp & -1")] + [DataRow("'a' + 'b'", "\"ab\"", DisplayName = "'a' + 'b'")] + [DataRow("IntProp ^ 1", "IntProp()^1", DisplayName = "IntProp ^ 1")] + [DataRow("'xx' + IntProp", "\"xx\"+IntProp()", DisplayName = "'xx' + IntProp")] [DataRow("true == (IntProp == 1)", "true==(IntProp()==1)", DisplayName = "true == (IntProp == 1)")] + [DataRow("TestViewModel2 == null", "TestViewModel2()==null", DisplayName = "TestViewModel2 == null")] + [DataRow("NullableDateOnly == null", "NullableDateOnly()==null", DisplayName = "NullableDateOnly == null")] + [DataRow("NullableDateOnly == NullableDateOnly", "NullableDateOnly()==NullableDateOnly()", DisplayName = "NullableDateOnly == NullableDateOnly")] + [DataRow("null != StringProp", "null!=StringProp()", DisplayName = "null != StringProp")] + [DataRow("(EnumProperty & 2) == 0", "dotvvm.translations.enums.fromInt(dotvvm.translations.enums.toInt(EnumProperty(),\"nEayAzHQ5xyCfSP6\")&2,\"nEayAzHQ5xyCfSP6\")==\"A\"", DisplayName = "(EnumProperty & 2) == 0")] + [DataRow("EnumProperty == 'B'", "EnumProperty()==\"B\"", DisplayName = "EnumProperty & 2 == 0")] public void JavascriptCompilation_BinaryExpressions(string expr, string expectedJs) { var js = CompileBinding(expr, new [] { typeof(TestViewModel) }); diff --git a/src/Tests/Parser/Binding/BindingParserTests.cs b/src/Tests/Parser/Binding/BindingParserTests.cs index 550ee53584..4db7414a04 100644 --- a/src/Tests/Parser/Binding/BindingParserTests.cs +++ b/src/Tests/Parser/Binding/BindingParserTests.cs @@ -803,7 +803,7 @@ public void BindingParser_GenericExpression_MultipleInside() var parser = bindingParserNodeFactory.SetupParser(originalString); var node = parser.ReadExpression(); - Assert.IsTrue(node is TypeOrFunctionReferenceBindingParserNode); + Assert.IsInstanceOfType(node, typeof(TypeOrFunctionReferenceBindingParserNode)); Assert.IsTrue(string.Equals(originalString, node.ToDisplayString())); }