Skip to content

CSHARP-5628: Add new boolean expression simplifications to PartialEvaluator #1803

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+318 −34
Open

CSHARP-5628: Add new boolean expression simplifications to PartialEvaluator #1803

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56a5854
CSHARP-5628: Add new boolean expression simplifications to PartialEva…
rstam Oct 28, 2025
631be11
CSHARP-5628: Requested changes.
rstam Oct 28, 2025
e29abd4
CSHARP-56j28: Requested changes
rstam Nov 10, 2025
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
150 changes: 120 additions & 30 deletions src/MongoDB.Driver/Linq/Linq3Implementation/Misc/PartialEvaluator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,60 +75,137 @@ public override Expression Visit(Expression expression)

protected override Expression VisitBinary(BinaryExpression node)
{
if (node.NodeType == ExpressionType.AndAlso)
var leftExpression = node.Left;
var rightExpression = node.Right;

if (leftExpression.Type == typeof(bool) && rightExpression.Type == typeof(bool))
{
var leftExpression = Visit(node.Left);
if (leftExpression is ConstantExpression constantLeftExpression )
if (node.NodeType == ExpressionType.AndAlso)
Copy link
Contributor Author

@rstam rstam Oct 28, 2025

Choose a reason for hiding this comment

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

Normally we like to do simplifications in the AstSimplifier but the existing simplifications for && and || were already being handled here.

VisitBinary is essentially the same as before but modified to use the new IsConstant helper method.

{
var value = (bool)constantLeftExpression.Value;
return value ? Visit(node.Right) : Expression.Constant(false);
leftExpression = Visit(leftExpression);
if (IsConstant<bool>(leftExpression, out var leftValue))
{
// true && Q => Q
// false && Q => false
return leftValue ? Visit(rightExpression) : Expression.Constant(false);
Copy link
Member

@sanych-sun sanych-sun Nov 10, 2025

Choose a reason for hiding this comment

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

Minor: can save a little on allocation probably by returning leftExpression instead of Expression.Constant(false).

Copy link
Contributor Author

@rstam rstam Nov 10, 2025

Choose a reason for hiding this comment

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

Using leftExpression is a bit subtle. It requires understanding why that works.

Instead I added __falseConstantExpression and __trueConstantExpression fields that can be used for both left and right sides.

}

rightExpression = Visit(rightExpression);
if (IsConstant<bool>(rightExpression, out var rightValue))
{
// P && true => P
// P && false => false
return rightValue ? leftExpression : Expression.Constant(false);
Copy link
Member

@sanych-sun sanych-sun Nov 10, 2025

Choose a reason for hiding this comment

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

Minor: same as above can return rightExpression instead of Expression.Constant(false).

Copy link
Contributor Author

@rstam rstam Nov 10, 2025

Choose a reason for hiding this comment

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

Used __falseConstantExpression and __trueConstantExpression instead.

}

return node.Update(leftExpression, conversion: null, rightExpression);
}

var rightExpression = Visit(node.Right);
if (rightExpression is ConstantExpression constantRightExpression)
if (node.NodeType == ExpressionType.OrElse)
{
var value = (bool)constantRightExpression.Value;
return value ? leftExpression : Expression.Constant(false);
leftExpression = Visit(leftExpression);
if (IsConstant<bool>(leftExpression, out var leftValue))
{
// true || Q => true
// false || Q => Q
return leftValue ? Expression.Constant(true) : Visit(rightExpression);
Copy link
Member

@sanych-sun sanych-sun Nov 10, 2025

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

@rstam rstam Nov 10, 2025

Choose a reason for hiding this comment

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

Used __falseConstantExpression and __trueConstantExpression instead.

}

rightExpression = Visit(rightExpression);
if (IsConstant<bool>(rightExpression, out var rightValue))
{
// P || true => true
// P || false => P
return rightValue ? Expression.Constant(true) : leftExpression;
Copy link
Member

@sanych-sun sanych-sun Nov 10, 2025

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

@rstam rstam Nov 10, 2025

Choose a reason for hiding this comment

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

Used __falseConstantExpression and __trueConstantExpression instead.

}

return node.Update(leftExpression, conversion: null, rightExpression);
}
}

return base.VisitBinary(node);
}

return node.Update(leftExpression, conversion: null, rightExpression);
protected override Expression VisitConditional(ConditionalExpression node)
{
var test = Visit(node.Test);

if (IsConstant<bool>(test, out var testValue))
{
// true ? A : B => A
// false ? A : B => B
return testValue ? Visit(node.IfTrue) : Visit(node.IfFalse);
Copy link
Contributor Author

@rstam rstam Oct 28, 2025

Choose a reason for hiding this comment

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

This was an existing simplification.

}

if (node.NodeType == ExpressionType.OrElse)
var ifTrue = Visit(node.IfTrue);
var ifFalse = Visit(node.IfFalse);

if (BothAreConstant<bool>(ifTrue, ifFalse, out var ifTrueValue, out var ifFalseValue))
{
var leftExpression = Visit(node.Left);
if (leftExpression is ConstantExpression constantLeftExpression)
return (ifTrueValue, ifFalseValue) switch
{
var value = (bool)constantLeftExpression.Value;
return value ? Expression.Constant(true) : Visit(node.Right);
}
(false, false) => Expression.Constant(false), // T ? false : false => false
(false, true) => Expression.Not(test), // T ? false : true => !T
(true, false) => test, // T ? true : false => T
(true, true) => Expression.Constant(true) // T ? true : true => true
};
}
else if (IsConstant<bool>(ifTrue, out ifTrueValue))
{
// T ? true : Q => T || Q
// T ? false : Q => !T && Q
return ifTrueValue
? Visit(Expression.OrElse(test, ifFalse))
: Visit(Expression.AndAlso(Expression.Not(test), ifFalse));
}
else if (IsConstant<bool>(ifFalse, out ifFalseValue))
{
// T ? P : true => !T || P
// T ? P : false => T && P
return ifFalseValue
? Visit(Expression.OrElse(Expression.Not(test), ifTrue))
: Visit(Expression.AndAlso(test, ifTrue));
}

var rightExpression = Visit(node.Right);
if (rightExpression is ConstantExpression constantRightExpression)
return node.Update(test, ifTrue, ifFalse);
}

protected override Expression VisitUnary(UnaryExpression node)
{
var operand = Visit(node.Operand);

if (node.Type == typeof(bool) &&
node.NodeType == ExpressionType.Not)
{
if (operand is UnaryExpression innerUnaryExpressionOperand &&
innerUnaryExpressionOperand.NodeType == ExpressionType.Not)
{
var value = (bool)constantRightExpression.Value;
return value ? Expression.Constant(true) : leftExpression;
// !!P => P
return innerUnaryExpressionOperand.Operand;
}

return node.Update(leftExpression, conversion: null, rightExpression);
}

return base.VisitBinary(node);
return node.Update(operand);
}

protected override Expression VisitConditional(ConditionalExpression node)
// private methods
private bool BothAreConstant<T>(Expression expression1, Expression expression2, out T constantValue1, out T constantValue2)
{
var test = Visit(node.Test);
if (test is ConstantExpression constantTestExpression)
if (expression1 is ConstantExpression constantExpression1 &&
expression2 is ConstantExpression constantExpression2 &&
constantExpression1.Type == typeof(T) &&
constantExpression2.Type == typeof(T))
{
var value = (bool)constantTestExpression.Value;
return value ? Visit(node.IfTrue) : Visit(node.IfFalse);
constantValue1 = (T)constantExpression1.Value;
constantValue2 = (T)constantExpression2.Value;
return true;
}

return node.Update(test, Visit(node.IfTrue), Visit(node.IfFalse));
constantValue1 = default;
constantValue2 = default;
return false;
}

// private methods
private Expression Evaluate(Expression expression)
{
if (expression.NodeType == ExpressionType.Constant)
Expand All @@ -139,6 +216,19 @@ private Expression Evaluate(Expression expression)
Delegate fn = lambda.Compile();
return Expression.Constant(fn.DynamicInvoke(null), expression.Type);
}

private bool IsConstant<T>(Expression expression, out T constantValue)
{
if (expression is ConstantExpression constantExpression1 &&
constantExpression1.Type == typeof(T))
{
constantValue = (T)constantExpression1.Value;
return true;
}

constantValue = default;
return false;
}
}

private class Nominator : ExpressionVisitor
Expand Down
8 changes: 4 additions & 4 deletions tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4337Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ public class CSharp4337Tests : LinqIntegrationTest<CSharp4337Tests.ClassFixture>
{
private static (Expression<Func<C, R<bool>>> Projection, string ExpectedStage, bool[] ExpectedResults)[] __predicate_should_use_correct_representation_test_cases = new (Expression<Func<C, R<bool>>> Projection, string ExpectedStage, bool[] ExpectedResults)[]
{
(d => new R<bool> { N = d.Id, V = d.I1 == E.E1 ? true : false }, "{ $project : { N : '$_id', V : { $cond : { if : { $eq : ['$I1', 1] }, then : true, else : false } }, _id : 0 } }", new[] { true, false }),
(d => new R<bool> { N = d.Id, V = d.S1 == E.E1 ? true : false }, "{ $project : { N : '$_id', V : { $cond : { if : { $eq : ['$S1', 'E1'] }, then : true, else : false } }, _id : 0 } }", new[] { true, false }),
(d => new R<bool> { N = d.Id, V = E.E1 == d.I1 ? true : false }, "{ $project : { N : '$_id', V : { $cond : { if : { $eq : [1, '$I1'] }, then : true, else : false } }, _id : 0 } }", new[] { true, false }),
(d => new R<bool> { N = d.Id, V = E.E1 == d.S1 ? true : false }, "{ $project : { N : '$_id', V : { $cond : { if : { $eq : ['E1', '$S1'] }, then : true, else : false } }, _id : 0 } }", new[] { true, false })
(d => new R<bool> { N = d.Id, V = d.I1 == E.E1 ? true : false }, "{ $project : { N : '$_id', V : { $eq : ['$I1', 1] }, _id : 0 } }", new[] { true, false }),
(d => new R<bool> { N = d.Id, V = d.S1 == E.E1 ? true : false }, "{ $project : { N : '$_id', V : { $eq : ['$S1', 'E1'] }, _id : 0 } }", new[] { true, false }),
(d => new R<bool> { N = d.Id, V = E.E1 == d.I1 ? true : false }, "{ $project : { N : '$_id', V : { $eq : [1, '$I1'] }, _id : 0 } }", new[] { true, false }),
(d => new R<bool> { N = d.Id, V = E.E1 == d.S1 ? true : false }, "{ $project : { N : '$_id', V : { $eq : ['E1', '$S1'] }, _id : 0 } }", new[] { true, false })
Copy link
Contributor Author

@rstam rstam Oct 28, 2025

Choose a reason for hiding this comment

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

This is the only existing test affected by these new simplifications.

};

public CSharp4337Tests(ClassFixture fixture)
Expand Down
Loading