Skip to content

Improve Null-Like behaviour in comparisons #10422

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

Closed
vexx32 wants to merge 7 commits into from
Closed

Improve Null-Like behaviour in comparisons #10422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
61690d3
:sparkles: Handle null-like as null for -gt/-lt
vexx32 Aug 22, 2019
f9a95d9
:white_check_mark: Add tests
vexx32 Aug 22, 2019
adf6faf
:recycle: Refactor IsNullLike for fast path
vexx32 Aug 23, 2019
748557d
:recycle: Invert confusing ternary expressions
vexx32 Aug 30, 2019
c4b0525
:white_check_mark: Update tests
vexx32 Oct 15, 2019
079e31b
:bug: Fix issue with binders
vexx32 Oct 15, 2019
f8a5fe7
:white_check_mark: Add mirror tests for comparison
vexx32 Oct 15, 2019
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
2 changes: 1 addition & 1 deletion src/System.Management.Automation/engine/LanguagePrimitives.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,7 +1071,7 @@ internal static bool IsTrue(IList objectArray)
/// </summary>
/// <param name="obj">The object to test.</param>
/// <returns>True if the object is null.</returns>
public static bool IsNullLike(object obj) => obj == DBNull.Value || obj == NullString.Value || IsNull(obj);
public static bool IsNullLike(object obj) => IsNull(obj) || obj == DBNull.Value || obj == NullString.Value;

/// <summary>
/// Auxiliary for the cases where we want a new PSObject or null.
Expand Down
100 changes: 60 additions & 40 deletions src/System.Management.Automation/engine/runtime/Binding/Binders.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3170,18 +3170,23 @@ private static Expression CompareWithZero(DynamicMetaObject target, Func<Express
return comparer(target.Expression.Cast(target.LimitType), ExpressionCache.Constant(0).Cast(target.LimitType)).Cast(typeof(object));
}

private DynamicMetaObject CompareLT(DynamicMetaObject target,
DynamicMetaObject arg,
DynamicMetaObject errorSuggestion)
private DynamicMetaObject CompareLT(
DynamicMetaObject target,
DynamicMetaObject arg,
DynamicMetaObject errorSuggestion)
{
var enumerable = PSEnumerableBinder.IsEnumerable(target);
if (enumerable == null && (target.Value == null || arg.Value == null))
{
Expression result =
target.LimitType.IsNumeric() ? CompareWithZero(target, Expression.LessThan)
: arg.LimitType.IsNumeric() ? CompareWithZero(arg, Expression.GreaterThanOrEqual)
: arg.Value != null ? ExpressionCache.BoxedTrue
: ExpressionCache.BoxedFalse;
if (enumerable == null
&& (LanguagePrimitives.IsNullLike(target.Value)
|| LanguagePrimitives.IsNullLike(arg.Value)))
{
Expression result = target.LimitType.IsNumeric()
? CompareWithZero(target, Expression.LessThan)
: arg.LimitType.IsNumeric()
? CompareWithZero(arg, Expression.GreaterThanOrEqual)
: LanguagePrimitives.IsNullLike(arg.Value)
? ExpressionCache.BoxedFalse
: ExpressionCache.BoxedTrue;

return new DynamicMetaObject(result, target.CombineRestrictions(arg));
}
Expand All @@ -3190,18 +3195,23 @@ private DynamicMetaObject CompareLT(DynamicMetaObject target,
?? BinaryComparison(target, arg, e => Expression.LessThan(e, ExpressionCache.Constant(0)));
}

private DynamicMetaObject CompareLE(DynamicMetaObject target,
DynamicMetaObject arg,
DynamicMetaObject errorSuggestion)
private DynamicMetaObject CompareLE(
DynamicMetaObject target,
DynamicMetaObject arg,
DynamicMetaObject errorSuggestion)
{
var enumerable = PSEnumerableBinder.IsEnumerable(target);
if (enumerable == null && (target.Value == null || arg.Value == null))
{
Expression result =
target.LimitType.IsNumeric() ? CompareWithZero(target, Expression.LessThan)
: arg.LimitType.IsNumeric() ? CompareWithZero(arg, Expression.GreaterThanOrEqual)
: target.Value != null ? ExpressionCache.BoxedFalse
: ExpressionCache.BoxedTrue;
if (enumerable == null
&& (LanguagePrimitives.IsNullLike(target.Value)
|| LanguagePrimitives.IsNullLike(arg.Value)))
{
Expression result = target.LimitType.IsNumeric()
? CompareWithZero(target, Expression.LessThanOrEqual)
: arg.LimitType.IsNumeric()
? CompareWithZero(arg, Expression.GreaterThanOrEqual)
: LanguagePrimitives.IsNullLike(target.Value)
? ExpressionCache.BoxedTrue
: ExpressionCache.BoxedFalse;

return new DynamicMetaObject(result, target.CombineRestrictions(arg));
}
Expand All @@ -3210,20 +3220,25 @@ private DynamicMetaObject CompareLE(DynamicMetaObject target,
?? BinaryComparison(target, arg, e => Expression.LessThanOrEqual(e, ExpressionCache.Constant(0)));
}

private DynamicMetaObject CompareGT(DynamicMetaObject target,
DynamicMetaObject arg,
DynamicMetaObject errorSuggestion)
private DynamicMetaObject CompareGT(
DynamicMetaObject target,
DynamicMetaObject arg,
DynamicMetaObject errorSuggestion)
{
// Handle a null operand as a special case here unless the target is enumerable or if one of the operands is numeric,
// in which case null is converted to 0 and regular numeric comparison is done.
var enumerable = PSEnumerableBinder.IsEnumerable(target);
if (enumerable == null && (target.Value == null || arg.Value == null))
{
Expression result =
target.LimitType.IsNumeric() ? CompareWithZero(target, Expression.GreaterThanOrEqual)
: arg.LimitType.IsNumeric() ? CompareWithZero(arg, Expression.LessThan)
: target.Value != null ? ExpressionCache.BoxedTrue
: ExpressionCache.BoxedFalse;
if (enumerable == null
&& (LanguagePrimitives.IsNullLike(target.Value)
|| LanguagePrimitives.IsNullLike(arg.Value)))
{
Expression result = target.LimitType.IsNumeric()
? CompareWithZero(target, Expression.GreaterThanOrEqual)
: arg.LimitType.IsNumeric()
? CompareWithZero(arg, Expression.LessThan)
: LanguagePrimitives.IsNullLike(target.Value)
? ExpressionCache.BoxedFalse
: ExpressionCache.BoxedTrue;

return new DynamicMetaObject(result, target.CombineRestrictions(arg));
}
Expand All @@ -3232,20 +3247,25 @@ private DynamicMetaObject CompareGT(DynamicMetaObject target,
?? BinaryComparison(target, arg, e => Expression.GreaterThan(e, ExpressionCache.Constant(0)));
}

private DynamicMetaObject CompareGE(DynamicMetaObject target,
DynamicMetaObject arg,
DynamicMetaObject errorSuggestion)
private DynamicMetaObject CompareGE(
DynamicMetaObject target,
DynamicMetaObject arg,
DynamicMetaObject errorSuggestion)
{
// Handle a null operand as a special case here unless the target is enumerable or if one of the operands is numeric,
// in which case null is converted to 0 and regular numeric comparison is done.
var enumerable = PSEnumerableBinder.IsEnumerable(target);
if (enumerable == null && (target.Value == null || arg.Value == null))
{
Expression result =
target.LimitType.IsNumeric() ? CompareWithZero(target, Expression.GreaterThanOrEqual)
: arg.LimitType.IsNumeric() ? CompareWithZero(arg, Expression.LessThan)
: arg.Value != null ? ExpressionCache.BoxedFalse
: ExpressionCache.BoxedTrue;
if (enumerable == null
&& (LanguagePrimitives.IsNullLike(target.Value)
|| LanguagePrimitives.IsNullLike(arg.Value)))
{
Expression result = target.LimitType.IsNumeric()
? CompareWithZero(target, Expression.GreaterThanOrEqual)
: arg.LimitType.IsNumeric()
? CompareWithZero(arg, Expression.LessThanOrEqual)
: LanguagePrimitives.IsNullLike(arg.Value)
? ExpressionCache.BoxedTrue
: ExpressionCache.BoxedFalse;

return new DynamicMetaObject(result, target.CombineRestrictions(arg));
}
Expand Down
43 changes: 43 additions & 0 deletions test/powershell/Language/Scripting/NullRepresentatives.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,47 @@ Describe 'Null Representatives' -Tags 'CI' {
@{ Value = [NullString]::Value; ExpectedCount = 5 }
)
}

Context 'Numeric Comparisons' {
BeforeAll {
$TestValues = @(
@{ Value = { $null } }
@{ Value = { [DBNull]::Value } }
@{ Value = { [NullString]::Value } }
@{ Value = { [AutomationNull]::Value } }
)
}

It '<Value> should be considered greater than or equal to 0' -TestCases $TestValues {
param($Value)

$TestValue = $Value.InvokeReturnAsIs()
$TestValue -ge 0 | Should -BeTrue
0 -le $TestValue | Should -BeTrue
}

It '<Value> should be considered less than or equal to 0' -TestCases $TestValues {
param($Value)

$TestValue = $Value.InvokeReturnAsIs()
$TestValue -le 0 | Should -BeTrue
0 -ge $TestValue | Should -BeTrue
}

It '<Value> should be less than 1' -TestCases $TestValues {
param($Value)

$TestValue = $Value.InvokeReturnAsIs()
$TestValue -lt 1 | Should -BeTrue
1 -gt $TestValue | Should -BeTrue
}

It '<Value> should be greater than -1' -TestCases $TestValues {
param($Value)

$TestValue = $Value.InvokeReturnAsIs()
$TestValue -gt -1 | Should -BeTrue
-1 -lt $TestValue | Should -BeTrue
}
}
}