Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InMemory: Add translation for object.Equals when object is null #23391

Merged
merged 1 commit into from
Nov 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -903,12 +903,31 @@ static Expression RemapLambda(GroupingElementExpression groupingElement, LambdaE
arguments);

result = ConvertToNullable(result);
result = Expression.Condition(
Expression.Equal(@object, Expression.Constant(null, @object.Type)),
Expression.Constant(null, result.Type),
result);
var objectNullCheck = Expression.Equal(@object, Expression.Constant(null, @object.Type));
// instance.Equals(argument) should translate to
// instance == null ? argument == null : instance.Equals(argument)
if (method.Name == nameof(object.Equals))
{
var argument = arguments[0];
if (argument.NodeType == ExpressionType.Convert
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also ConvertChecked

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we are here we are basically matching instance.Equals(param) method.
By default there would be method coming from object, Equals(object), the Type can also add overload of Equals(Type).
Now, we always flow null forward except for parameter values of the a method call since the method may not be take nullable parameters so we convert all the parameter to match their types if they are non-nullable type. Which can happen for the latter overload if any. Since we do this conversion (by calling ConvertToNonNullable function), it would always be a Convert node from nullable to non-nullable value, which we want to remove here since ((bool?)(bool)nullableBoolColumn) == null would throw exception even before comparing to null. Hence I put Convert node. Do you still think we need ConvertChecked here? (This is speculative future proofing), the tests work fine without this if block too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case let's keep Convert only. Perhaps add a comment in case we decide to sweep thru entire codebase and unify convert+convertchecked checks. But I don't feel strongly.

&& argument is UnaryExpression unaryExpression
&& argument.Type == unaryExpression.Operand.Type.UnwrapNullableType())
{
argument = unaryExpression.Operand;
}

return result;
if (!argument.Type.IsNullableType())
{
argument = Expression.Convert(argument, argument.Type.MakeNullable());
}

return Expression.Condition(
objectNullCheck,
ConvertToNullable(Expression.Equal(argument, Expression.Constant(null, argument.Type))),
result);
}

return Expression.Condition(objectNullCheck, Expression.Constant(null, result.Type), result);
}

// TODO-Nullable bug
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Linq;
using System.Linq.Expressions;
using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.TestModels.Northwind;
using Microsoft.EntityFrameworkCore.TestUtilities;
using Xunit;
Expand Down Expand Up @@ -4209,6 +4210,18 @@ public override Task Skip_0_Take_0_works_when_constant(bool async)
return base.Skip_0_Take_0_works_when_constant(async);
}

public override Task Using_static_string_Equals_with_StringComparison_throws_informative_error(bool async)
{
return AssertTranslationFailedWithDetails(() => base.Using_static_string_Equals_with_StringComparison_throws_informative_error(async),
CoreStrings.QueryUnableToTranslateStringEqualsWithStringComparison);
}

public override Task Using_string_Equals_with_StringComparison_throws_informative_error(bool async)
{
return AssertTranslationFailedWithDetails(() => base.Using_string_Equals_with_StringComparison_throws_informative_error(async),
CoreStrings.QueryUnableToTranslateStringEqualsWithStringComparison);
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1151,10 +1151,10 @@ public override async Task Projecting_after_navigation_and_distinct_throws(bool
AssertSql(" ");
}

public override Task Reverse_without_explicit_ordering_throws(bool async)
public override Task Reverse_without_explicit_ordering(bool async)
{
return AssertTranslationFailedWithDetails(
() => base.Reverse_without_explicit_ordering_throws(async), CosmosStrings.MissingOrderingInSelectExpression);
() => base.Reverse_without_explicit_ordering(async), CosmosStrings.MissingOrderingInSelectExpression);
}

[ConditionalTheory(Skip = "Cross collection join Issue#17246")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2141,6 +2141,11 @@ FROM root c
WHERE ((c[""Discriminator""] = ""Order"") AND c[""OrderID""] IN (10248, 10249))");
}

public override Task Where_equals_method_string_with_ignore_case(bool async)
{
return AssertTranslationFailed(() => base.Where_equals_method_string_with_ignore_case(async));
}

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@ public ComplexNavigationsQueryInMemoryTest(ComplexNavigationsQueryInMemoryFixtur
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

[ConditionalTheory(Skip = "issue #17386")]
public override Task Complex_query_with_optional_navigations_and_client_side_evaluation(bool async)
{
return base.Complex_query_with_optional_navigations_and_client_side_evaluation(async);
}

[ConditionalFact(Skip = "issue #18194")]
public override void Member_pushdown_chain_3_levels_deep_entity()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,6 @@ public ComplexNavigationsWeakQueryInMemoryTest(
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Complex_query_with_optional_navigations_and_client_side_evaluation(bool async)
{
return base.Complex_query_with_optional_navigations_and_client_side_evaluation(async);
}

[ConditionalTheory(Skip = "Issue#17539")]
public override Task Join_navigations_in_inner_selector_translated_without_collision(bool async)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.TestModels.GearsOfWarModel;
using Xunit;
using Xunit.Abstractions;

Expand All @@ -15,21 +17,11 @@ public GearsOfWarQueryInMemoryTest(GearsOfWarQueryInMemoryFixture fixture, ITest
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

[ConditionalTheory(Skip = "issue #17386")]
public override Task Correlated_collection_order_by_constant_null_of_non_mapped_type(bool async)
=> base.Correlated_collection_order_by_constant_null_of_non_mapped_type(async);

[ConditionalTheory(Skip = "issue #17386")]
public override Task Client_side_equality_with_parameter_works_with_optional_navigations(bool async)
=> base.Client_side_equality_with_parameter_works_with_optional_navigations(async);

[ConditionalTheory(Skip = "issue #17386")]
public override Task Where_coalesce_with_anonymous_types(bool async)
=> base.Where_coalesce_with_anonymous_types(async);

[ConditionalTheory(Skip = "issue #17386")]
public override Task GetValueOrDefault_on_DateTimeOffset(bool async)
=> base.GetValueOrDefault_on_DateTimeOffset(async);
public override Task Client_member_and_unsupported_string_Equals_in_the_same_query(bool async)
{
return AssertTranslationFailedWithDetails(() => base.Client_member_and_unsupported_string_Equals_in_the_same_query(async),
CoreStrings.QueryUnableToTranslateMember(nameof(Gear.IsMarcus), nameof(Gear)));
}

[ConditionalFact(Skip = "issue #17537")]
public override void Include_on_GroupJoin_SelectMany_DefaultIfEmpty_with_coalesce_result1()
Expand All @@ -53,14 +45,6 @@ public override Task Select_subquery_projecting_single_constant_inside_anonymous
public override Task Group_by_on_StartsWith_with_null_parameter_as_argument(bool async)
=> base.Group_by_on_StartsWith_with_null_parameter_as_argument(async);

[ConditionalTheory(Skip = "issue #17386")]
public override Task Client_member_and_unsupported_string_Equals_in_the_same_query(bool async)
=> base.Client_member_and_unsupported_string_Equals_in_the_same_query(async);

[ConditionalTheory(Skip = "issue #17386")]
public override Task Client_eval_followed_by_set_operation_throws_meaningful_exception(bool async)
=> base.Client_eval_followed_by_set_operation_throws_meaningful_exception(async);

[ConditionalTheory(Skip = "issue #17537")]
public override Task SelectMany_predicate_with_non_equality_comparison_with_Take_doesnt_convert_to_join(bool async)
=> base.SelectMany_predicate_with_non_equality_comparison_with_Take_doesnt_convert_to_join(async);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,6 @@ public override Task Collection_Last_member_access_in_projection_translated(bool
() => base.Collection_Last_member_access_in_projection_translated(async));
}

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Contains_with_local_tuple_array_closure(bool async)
{
return base.Contains_with_local_tuple_array_closure(async);
}

[ConditionalFact(Skip = "Issue#20023")]
public override void Contains_over_keyless_entity_throws()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore.TestUtilities;
using Xunit;

namespace Microsoft.EntityFrameworkCore.Query
{
Expand All @@ -13,14 +11,5 @@ public NorthwindCompiledQueryInMemoryTest(NorthwindQueryInMemoryFixture<NoopMode
: base(fixture)
{
}

[ConditionalFact(Skip = "See issue #17386")]
public override void Query_with_array_parameter()
{
}

[ConditionalFact(Skip = "See issue #17386")]
public override Task Query_with_array_parameter_async()
=> null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.TestModels.Northwind;
using Microsoft.EntityFrameworkCore.TestUtilities;
using Xunit;
Expand Down Expand Up @@ -66,50 +65,6 @@ public override void Client_code_using_instance_in_static_method()
public override void Client_code_using_instance_method_throws()
=> base.Client_code_using_instance_method_throws();

[ConditionalTheory(Skip = "Issue#17386")]
public override Task OrderBy_multiple_queries(bool async)
=> base.OrderBy_multiple_queries(async);

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Random_next_is_not_funcletized_1(bool async)
=> base.Random_next_is_not_funcletized_1(async);

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Random_next_is_not_funcletized_2(bool async)
=> base.Random_next_is_not_funcletized_2(async);

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Random_next_is_not_funcletized_3(bool async)
=> base.Random_next_is_not_funcletized_3(async);

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Random_next_is_not_funcletized_4(bool async)
=> base.Random_next_is_not_funcletized_4(async);

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Random_next_is_not_funcletized_5(bool async)
=> base.Random_next_is_not_funcletized_5(async);

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Random_next_is_not_funcletized_6(bool async)
=> base.Random_next_is_not_funcletized_6(async);

[ConditionalTheory(Skip = "issue#17386")]
public override Task Where_query_composition5(bool async)
=> base.Where_query_composition5(async);

[ConditionalTheory(Skip = "issue#17386")]
public override Task Where_query_composition6(bool async)
=> base.Where_query_composition6(async);

[ConditionalTheory(Skip = "issue #17386")]
public override Task Using_string_Equals_with_StringComparison_throws_informative_error(bool async)
=> base.Using_string_Equals_with_StringComparison_throws_informative_error(async);

[ConditionalTheory(Skip = "issue #17386")]
public override Task Using_static_string_Equals_with_StringComparison_throws_informative_error(bool async)
=> base.Using_static_string_Equals_with_StringComparison_throws_informative_error(async);

public override async Task Max_on_empty_sequence_throws(bool async)
{
using var context = CreateContext();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,5 @@ public NorthwindNavigationsQueryInMemoryTest(
{
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Where_subquery_on_navigation_client_eval(bool async)
{
return base.Where_subquery_on_navigation_client_eval(async);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,28 +20,10 @@ public NorthwindSelectQueryInMemoryTest(
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Select_bool_closure_with_order_by_property_with_cast_to_nullable(bool async)
{
return base.Select_bool_closure_with_order_by_property_with_cast_to_nullable(async);
}

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Projection_when_arithmetic_mixed_subqueries(bool async)
{
return base.Projection_when_arithmetic_mixed_subqueries(async);
}

[ConditionalTheory(Skip = "Issue#17536")]
public override Task SelectMany_correlated_with_outer_3(bool async)
{
return base.SelectMany_correlated_with_outer_3(async);
}

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Reverse_without_explicit_ordering_throws(bool async)
{
return base.Reverse_without_explicit_ordering_throws(async);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,6 @@ public NorthwindWhereQueryInMemoryTest(
//TestLoggerFactory.TestOutputHelper = testOutputHelper;
}

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Where_bool_client_side_negated(bool async)
{
return base.Where_bool_client_side_negated(async);
}

[ConditionalTheory(Skip = "Issue#17386")]
public override Task Where_equals_method_string_with_ignore_case(bool async)
{
return base.Where_equals_method_string_with_ignore_case(async);
}

[ConditionalTheory(Skip = "issue #17386")]
public override Task Where_equals_on_null_nullable_int_types(bool async)
{
return base.Where_equals_on_null_nullable_int_types(async);
}

public override async Task<string> Where_simple_closure(bool async)
{
var queryString = await base.Where_simple_closure(async);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Microsoft.EntityFrameworkCore.TestUtilities;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.EntityFrameworkCore.Query
Expand All @@ -22,11 +21,5 @@ public class QueryFilterFuncletizationInMemoryFixture : QueryFilterFuncletizatio
protected override ITestStoreFactory TestStoreFactory
=> InMemoryTestStoreFactory.Instance;
}

[ConditionalFact(Skip = "issue #17386")]
public override void DbContext_list_is_parameterized()
{
base.DbContext_list_is_parameterized();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,11 @@ public virtual Task Projecting_collection_with_FirstOrDefault_without_split_work
});
}

public override Task Complex_query_with_optional_navigations_and_client_side_evaluation(bool async)
{
return AssertTranslationFailed(() => base.Complex_query_with_optional_navigations_and_client_side_evaluation(async));
}

protected virtual bool CanExecuteQueryString
=> false;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,11 @@ public virtual async Task Filtered_include_calling_methods_directly_on_parameter
.AsSplitQuery()))).Message;
}

public override Task Complex_query_with_optional_navigations_and_client_side_evaluation(bool async)
{
return AssertTranslationFailed(() => base.Complex_query_with_optional_navigations_and_client_side_evaluation(async));
}

protected virtual bool CanExecuteQueryString
=> false;

Expand Down
Loading