Skip to content

Commit

Permalink
Fix to #23472 - !boolean in query should convert to boolean == false …
Browse files Browse the repository at this point in the history
…rather than boolean != true where possible

When comparing something to bool constant, which is later negated we now flip the constant true -> false and false -> true, rather than flipping the operation from == to !=
  • Loading branch information
maumar committed Dec 17, 2020
1 parent a5158d8 commit 1ce7ef4
Show file tree
Hide file tree
Showing 12 changed files with 189 additions and 90 deletions.
28 changes: 28 additions & 0 deletions src/EFCore.Relational/Query/SqlNullabilityProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1490,6 +1490,34 @@ private SqlExpression OptimizeNonNullableNotExpression(SqlUnaryExpression sqlUna
sqlBinaryOperand.TypeMapping)!);
}

// use equality where possible
// !(a == true) -> a == false
// !(a == false) -> a == true
// !(true == a) -> false == a
// !(false == a) -> true == a
if (sqlBinaryOperand.OperatorType == ExpressionType.Equal)
{
if (sqlBinaryOperand.Left is SqlConstantExpression leftConstant
&& leftConstant.Type == typeof(bool))
{
return _sqlExpressionFactory.MakeBinary(
ExpressionType.Equal,
_sqlExpressionFactory.Constant(!(bool)leftConstant.Value!, leftConstant.TypeMapping),
sqlBinaryOperand.Right,
sqlBinaryOperand.TypeMapping)!;
}

if (sqlBinaryOperand.Right is SqlConstantExpression rightConstant
&& rightConstant.Type == typeof(bool))
{
return _sqlExpressionFactory.MakeBinary(
ExpressionType.Equal,
sqlBinaryOperand.Left,
_sqlExpressionFactory.Constant(!(bool)rightConstant.Value!, rightConstant.TypeMapping),
sqlBinaryOperand.TypeMapping)!;
}
}

// !(a == b) -> a != b
// !(a != b) -> a == b
// !(a > b) -> a <= b
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,22 +74,50 @@ private SqlExpression BuildCompareToExpression(SqlExpression sqlExpression)
sqlExpression,
_sqlExpressionFactory.Constant(true));

// !(a == b) -> (a != b)
// !(a != b) -> (a == b)

private SqlExpression SimplifyNegatedBinary(SqlExpression sqlExpression)
=> sqlExpression is SqlUnaryExpression sqlUnaryExpression
{
if (sqlExpression is SqlUnaryExpression sqlUnaryExpression
&& sqlUnaryExpression.OperatorType == ExpressionType.Not
&& sqlUnaryExpression.Type == typeof(bool)
&& sqlUnaryExpression.Operand is SqlBinaryExpression sqlBinaryOperand
&& (sqlBinaryOperand.OperatorType == ExpressionType.Equal || sqlBinaryOperand.OperatorType == ExpressionType.NotEqual)
? _sqlExpressionFactory.MakeBinary(
&& (sqlBinaryOperand.OperatorType == ExpressionType.Equal || sqlBinaryOperand.OperatorType == ExpressionType.NotEqual))
{
if (sqlBinaryOperand.Left.Type == typeof(bool)
&& sqlBinaryOperand.Right.Type == typeof(bool)
&& (sqlBinaryOperand.Left is SqlConstantExpression
|| sqlBinaryOperand.Right is SqlConstantExpression))
{
var constant = sqlBinaryOperand.Left as SqlConstantExpression ?? (SqlConstantExpression)sqlBinaryOperand.Right;
if (sqlBinaryOperand.Left is SqlConstantExpression)
{
return _sqlExpressionFactory.MakeBinary(
ExpressionType.Equal,
_sqlExpressionFactory.Constant(!(bool)constant.Value!, constant.TypeMapping),
sqlBinaryOperand.Right,
sqlBinaryOperand.TypeMapping)!;
}
else
{
return _sqlExpressionFactory.MakeBinary(
ExpressionType.Equal,
sqlBinaryOperand.Left,
_sqlExpressionFactory.Constant(!(bool)constant.Value!, constant.TypeMapping),
sqlBinaryOperand.TypeMapping)!;
}
}

return _sqlExpressionFactory.MakeBinary(
sqlBinaryOperand.OperatorType == ExpressionType.Equal
? ExpressionType.NotEqual
: ExpressionType.Equal,
sqlBinaryOperand.Left,
sqlBinaryOperand.Right,
sqlBinaryOperand.TypeMapping)!
: sqlExpression;
sqlBinaryOperand.TypeMapping)!;
}

return sqlExpression;
}

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down
11 changes: 11 additions & 0 deletions test/EFCore.Cosmos.FunctionalTests/CustomConvertersCosmosTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,17 @@ FROM root c
WHERE (c[""Discriminator""] IN (""Blog"", ""RssBlog"") AND (c[""IsVisible""] = ""Y""))");
}

[ConditionalFact]
public override void Where_negated_bool_gets_converted_to_equality_when_value_conversion_is_used()
{
base.Where_negated_bool_gets_converted_to_equality_when_value_conversion_is_used();

AssertSql(
@"SELECT c
FROM root c
WHERE (c[""Discriminator""] IN (""Blog"", ""RssBlog"") AND NOT((c[""IsVisible""] = ""Y"")))");
}

[ConditionalFact]
public override void Where_bool_gets_converted_to_equality_when_value_conversion_is_used_using_EFProperty()
{
Expand Down
10 changes: 10 additions & 0 deletions test/EFCore.Specification.Tests/CustomConvertersTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,16 @@ public virtual void Where_bool_gets_converted_to_equality_when_value_conversion_
Assert.Equal("http://blog.com", result.Url);
}

[ConditionalFact]
public virtual void Where_negated_bool_gets_converted_to_equality_when_value_conversion_is_used()
{
using var context = CreateContext();
var query = context.Set<Blog>().Where(b => !b.IsVisible).ToList();

var result = Assert.Single(query);
Assert.Equal("http://rssblog.com", result.Url);
}

[ConditionalFact]
public virtual void Where_bool_with_value_conversion_inside_comparison_doesnt_get_converted_twice()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,17 @@ FROM [Blog] AS [b]
WHERE [b].[IsVisible] = N'Y'");
}

[ConditionalFact]
public override void Where_negated_bool_gets_converted_to_equality_when_value_conversion_is_used()
{
base.Where_negated_bool_gets_converted_to_equality_when_value_conversion_is_used();

AssertSql(
@"SELECT [b].[BlogId], [b].[Discriminator], [b].[IndexerVisible], [b].[IsVisible], [b].[Url], [b].[RssUrl]
FROM [Blog] AS [b]
WHERE [b].[IsVisible] = N'N'");
}

public override void Where_bool_gets_converted_to_equality_when_value_conversion_is_used_using_EFProperty()
{
base.Where_bool_gets_converted_to_equality_when_value_conversion_is_used_using_EFProperty();
Expand All @@ -260,7 +271,7 @@ public override void Where_bool_gets_converted_to_equality_when_value_conversion
AssertSql(
@"SELECT [b].[BlogId], [b].[Discriminator], [b].[IndexerVisible], [b].[IsVisible], [b].[Url], [b].[RssUrl]
FROM [Blog] AS [b]
WHERE [b].[IndexerVisible] <> N'Aye'");
WHERE [b].[IndexerVisible] = N'Nay'");
}

public override void Object_to_string_conversion()
Expand Down
Loading

0 comments on commit 1ce7ef4

Please sign in to comment.