Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
150 changes: 120 additions & 30 deletions src/MongoDB.Driver/Linq/Linq3Implementation/Misc/PartialEvaluator.cs
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;
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

Extra space between '=' and 'node.Right'. Should be single space for consistency.

Suggested change
var rightExpression = node.Right;
var rightExpression = node.Right;

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


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

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

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

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

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

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

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

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

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

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

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);
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

Extra space between '=' and 'Visit'. Should be single space for consistency.

Suggested change
var ifFalse = Visit(node.IfFalse);
var ifFalse = Visit(node.IfFalse);

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


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.Or(test, ifFalse))
: Visit(Expression.And(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.Or(Expression.Not(test), ifTrue))
: Visit(Expression.And(test, ifTrue));
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

Using Expression.Or and Expression.And instead of Expression.OrElse and Expression.AndAlso may cause issues. These methods create bitwise operations for integral types, not short-circuit logical operations. Use Expression.OrElse and Expression.AndAlso for boolean logic to ensure proper short-circuit evaluation.

Suggested change
? Visit(Expression.Or(test, ifFalse))
: Visit(Expression.And(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.Or(Expression.Not(test), ifTrue))
: Visit(Expression.And(test, ifTrue));
? 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));

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

Using Expression.Or and Expression.And instead of Expression.OrElse and Expression.AndAlso may cause issues. These methods create bitwise operations for integral types, not short-circuit logical operations. Use Expression.OrElse and Expression.AndAlso for boolean logic to ensure proper short-circuit evaluation.

Suggested change
? Visit(Expression.Or(test, ifFalse))
: Visit(Expression.And(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.Or(Expression.Not(test), ifTrue))
: Visit(Expression.And(test, ifTrue));
? 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));

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

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
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

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
Loading