Skip to content

Consider DBNull.Value and NullString.Value the same as $null #9794

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

Merged
daxian-dbw merged 16 commits into from
Jun 28, 2019
Merged

Consider DBNull.Value and NullString.Value the same as $null #9794

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
54ff36c
:wrench: Update binders for comparisons to $null
vexx32 Jun 4, 2019
6eca8a4
:wrench: Update comparison logic for various nulls
vexx32 Jun 10, 2019
a6d2c04
:white_check_mark: Add tests for spec behaviour
vexx32 Jun 11, 2019
5db70bc
:sparkles: IsTrue() => false for all null-likes
vexx32 Jun 17, 2019
475529f
:white_check_mark: update comparison tests
vexx32 Jun 17, 2019
2561a4a
:white_check_mark: Fix typo in nullrep tests
vexx32 Jun 17, 2019
cd2d8d0
:sparkles: Add IsNullLike() method
vexx32 Jun 18, 2019
455cec0
:sparkles: fix conversions to boolean
vexx32 Jun 18, 2019
ecccb08
:recycle: refactor with helper method
vexx32 Jun 18, 2019
805585d
:tada: make IsNull and IsNullLike public
vexx32 Jun 18, 2019
c305f67
:white_check_mark: Fix test null check
vexx32 Jun 18, 2019
96d906d
:white_check_mark: Update tests
vexx32 Jun 18, 2019
242c371
:recycle: pull null check into return statement
vexx32 Jun 22, 2019
aa09a21
:recycle: Address review comments
vexx32 Jun 22, 2019
99b50f5
:white_check_mark: Add collection comparison tests
vexx32 Jun 23, 2019
fe6e110
:white_check_mark: Add test case James mentioned
vexx32 Jun 27, 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
133 changes: 68 additions & 65 deletions src/System.Management.Automation/engine/LanguagePrimitives.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,9 +591,7 @@ public static PSDataCollection<PSObject> GetPSDataCollection(object inputValue)
/// <param name="second">Object to compare first to.</param>
Comment thread
vexx32 marked this conversation as resolved.
Outdated
Comment thread
vexx32 marked this conversation as resolved.
Outdated
Comment thread
vexx32 marked this conversation as resolved.
Comment thread
vexx32 marked this conversation as resolved.
Comment thread
vexx32 marked this conversation as resolved.
Comment thread
vexx32 marked this conversation as resolved.
Comment thread
vexx32 marked this conversation as resolved.
Comment thread
vexx32 marked this conversation as resolved.
Comment thread
vexx32 marked this conversation as resolved.
Comment thread
vexx32 marked this conversation as resolved.
/// <returns>True if first is equal to the second.</returns>
public static new bool Equals(object first, object second)
{
return Equals(first, second, false, CultureInfo.InvariantCulture);
}
=> Equals(first, second, false, CultureInfo.InvariantCulture);

/// <summary>
/// Used to compare two objects for equality converting the second to the type of the first, if required.
Expand All @@ -604,9 +602,7 @@ public static PSDataCollection<PSObject> GetPSDataCollection(object inputValue)
/// to specify the type of string comparison </param>
/// <returns>True if first is equal to the second.</returns>
public static bool Equals(object first, object second, bool ignoreCase)
{
return Equals(first, second, ignoreCase, CultureInfo.InvariantCulture);
}
=> Equals(first, second, ignoreCase, CultureInfo.InvariantCulture);

/// <summary>
/// Used to compare two objects for equality converting the second to the type of the first, if required.
Expand Down Expand Up @@ -644,25 +640,38 @@ public static bool Equals(object first, object second, bool ignoreCase, IFormatP

if (first == null)
{
if (second == null) return true;
if (second == null || second == DBNull.Value || second == NullString.Value)
{
return true;
}

return false;
Comment thread
vexx32 marked this conversation as resolved.
Outdated
}

if (second == null)
{
if (first == DBNull.Value || first == NullString.Value)
{
return true;
}

return false; // first is not null
Comment thread
vexx32 marked this conversation as resolved.
Outdated
}

string firstString = first as string;
string secondString;
if (firstString != null)
if (first is string firstString)
{
secondString = second as string ?? (string)LanguagePrimitives.ConvertTo(second, typeof(string), culture);
return (culture.CompareInfo.Compare(firstString, secondString,
ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None) == 0);
return culture.CompareInfo.Compare(
firstString,
secondString,
ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None) == 0;
}

if (first.Equals(second)) return true;
if (first.Equals(second))
{
return true;
}

Type firstType = first.GetType();
Type secondType = second.GetType();
Expand Down Expand Up @@ -713,17 +722,17 @@ private static int CompareObjectToNull(object value, bool numberIsRightHandSide)

Comment thread
vexx32 marked this conversation as resolved.
// If it's a positive number, including 0, it's greater than null
// for everything else it's less than zero...
switch (value)
{
case Int16 i16: return Math.Sign(i16) < 0 ? -i : i;
case Int32 i32: return Math.Sign(i32) < 0 ? -i : i;
case Int64 i64: return Math.Sign(i64) < 0 ? -i : i;
case sbyte sby: return Math.Sign(sby) < 0 ? -i : i;
case float f: return Math.Sign(f) < 0 ? -i : i;
case double d: return Math.Sign(d) < 0 ? -i : i;
case decimal de: return Math.Sign(de) < 0 ? -i : i;
default: return i;
}
return value switch
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Jun 20, 2019

Choose a reason for hiding this comment

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

switch expression!!! Yay :)

{
Int16 i16 => Math.Sign(i16) < 0 ? -i : i,
Int32 i32 => Math.Sign(i32) < 0 ? -i : i,
Int64 i64 => Math.Sign(i64) < 0 ? -i : i,
sbyte s => Math.Sign(s) < 0 ? -i : i,
float f => Math.Sign(f) < 0 ? -i : i,
double d => Math.Sign(d) < 0 ? -i : i,
decimal m => Math.Sign(m) < 0 ? -i : i,
_ => value == DBNull.Value || value == NullString.Value ? 0 : i
};
}

/// <summary>
Expand All @@ -739,9 +748,7 @@ private static int CompareObjectToNull(object value, bool numberIsRightHandSide)
/// to the type of <paramref name="first"/>.
/// </exception>
public static int Compare(object first, object second)
{
return LanguagePrimitives.Compare(first, second, false, CultureInfo.InvariantCulture);
}
=> LanguagePrimitives.Compare(first, second, false, CultureInfo.InvariantCulture);

/// <summary>
/// Compare first and second, converting second to the
Expand All @@ -757,9 +764,7 @@ public static int Compare(object first, object second)
/// to the type of <paramref name="first"/>.
/// </exception>
public static int Compare(object first, object second, bool ignoreCase)
{
return LanguagePrimitives.Compare(first, second, ignoreCase, CultureInfo.InvariantCulture);
}
=> LanguagePrimitives.Compare(first, second, ignoreCase, CultureInfo.InvariantCulture);

/// <summary>
/// Compare first and second, converting second to the
Expand All @@ -777,23 +782,19 @@ public static int Compare(object first, object second, bool ignoreCase)
/// </exception>
public static int Compare(object first, object second, bool ignoreCase, IFormatProvider formatProvider)
{
if (formatProvider == null)
{
formatProvider = CultureInfo.InvariantCulture;
}
formatProvider ??= CultureInfo.InvariantCulture;

var culture = formatProvider as CultureInfo;
if (culture == null)
if (!(formatProvider is CultureInfo culture))
Comment thread
vexx32 marked this conversation as resolved.
Outdated
{
throw PSTraceSource.NewArgumentException("formatProvider");
throw PSTraceSource.NewArgumentException(nameof(formatProvider));
}

first = PSObject.Base(first);
second = PSObject.Base(second);

if (first == null)
{
return second == null ? 0 : CompareObjectToNull(second, true);
return CompareObjectToNull(second, true);
}

if (second == null)
Expand All @@ -803,28 +804,34 @@ public static int Compare(object first, object second, bool ignoreCase, IFormatP

if (first is string firstString)
{
string secondString = second as string;
if (secondString == null)
if (!(second is string secondString))
Comment thread
vexx32 marked this conversation as resolved.
Outdated
{
try
{
secondString = (string)LanguagePrimitives.ConvertTo(second, typeof(string), culture);
}
catch (PSInvalidCastException e)
{
throw PSTraceSource.NewArgumentException("second", ExtendedTypeSystem.ComparisonFailure,
first.ToString(), second.ToString(), e.Message);
throw PSTraceSource.NewArgumentException(
nameof(second),
ExtendedTypeSystem.ComparisonFailure,
first.ToString(),
second.ToString(),
e.Message);
}
}

return culture.CompareInfo.Compare(firstString, secondString,
ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None);
return culture.CompareInfo.Compare(
firstString,
secondString,
ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None);
}

Type firstType = first.GetType();
Type secondType = second.GetType();
int firstIndex = LanguagePrimitives.TypeTableIndex(firstType);
int secondIndex = LanguagePrimitives.TypeTableIndex(secondType);

if ((firstIndex != -1) && (secondIndex != -1))
{
return LanguagePrimitives.NumericCompare(first, second, firstIndex, secondIndex);
Expand All @@ -837,8 +844,12 @@ public static int Compare(object first, object second, bool ignoreCase, IFormatP
}
catch (PSInvalidCastException e)
{
throw PSTraceSource.NewArgumentException("second", ExtendedTypeSystem.ComparisonFailure,
first.ToString(), second.ToString(), e.Message);
throw PSTraceSource.NewArgumentException(
nameof(second),
ExtendedTypeSystem.ComparisonFailure,
first.ToString(),
second.ToString(),
e.Message);
}

if (first is IComparable firstComparable)
Expand All @@ -853,7 +864,7 @@ public static int Compare(object first, object second, bool ignoreCase, IFormatP

// At this point, we know that they aren't equal but we have no way of
// knowing which should compare greater than the other so we throw an exception.
throw PSTraceSource.NewArgumentException("first", ExtendedTypeSystem.NotIcomparable, first.ToString());
throw PSTraceSource.NewArgumentException(nameof(first), ExtendedTypeSystem.NotIcomparable, first.ToString());
}

/// <summary>
Expand All @@ -866,9 +877,7 @@ public static int Compare(object first, object second, bool ignoreCase, IFormatP
/// zero if it is greater or zero if they are the same.</param>
/// <returns>True if the comparison was successful, false otherwise.</returns>
public static bool TryCompare(object first, object second, out int result)
{
return TryCompare(first, second, ignoreCase: false, CultureInfo.InvariantCulture, out result);
}
=> TryCompare(first, second, ignoreCase: false, CultureInfo.InvariantCulture, out result);

/// <summary>
/// Tries to compare first and second, converting second to the type of the first, if necessary.
Expand All @@ -880,9 +889,7 @@ public static bool TryCompare(object first, object second, out int result)
/// <param name="result">Less than zero if first is smaller than second, more than zero if it is greater or zero if they are the same.</param>
/// <returns>True if the comparison was successful, false otherwise.</returns>
public static bool TryCompare(object first, object second, bool ignoreCase, out int result)
{
return TryCompare(first, second, ignoreCase, CultureInfo.InvariantCulture, out result);
}
=> TryCompare(first, second, ignoreCase, CultureInfo.InvariantCulture, out result);

/// <summary>
/// Tries to compare first and second, converting second to the type of the first, if necessary.
Expand All @@ -898,10 +905,7 @@ public static bool TryCompare(object first, object second, bool ignoreCase, out
public static bool TryCompare(object first, object second, bool ignoreCase, IFormatProvider formatProvider, out int result)
{
result = 0;
if (formatProvider == null)
{
formatProvider = CultureInfo.InvariantCulture;
}
formatProvider ??= CultureInfo.InvariantCulture;

if (!(formatProvider is CultureInfo culture))
{
Expand Down Expand Up @@ -986,8 +990,10 @@ public static bool TryCompare(object first, object second, bool ignoreCase, IFor
public static bool IsTrue(object obj)
{
// null is a valid argument - it converts to false...
if (obj == null || obj == AutomationNull.Value)
if (IsNull(obj) || obj == DBNull.Value || obj == NullString.Value)
{
return false;
}

obj = PSObject.Base(obj);

Expand All @@ -1013,8 +1019,7 @@ public static bool IsTrue(object obj)
if (objType == typeof(SwitchParameter))
return ((SwitchParameter)obj).ToBool();

IList objectArray = obj as IList;
if (objectArray != null)
if (obj is IList objectArray)
{
return IsTrue(objectArray);
}
Expand Down Expand Up @@ -1064,10 +1069,7 @@ internal static bool IsTrue(IList objectArray)
/// </summary>
/// <param name="obj">The object to test.</param>
/// <returns>True if the object is null.</returns>
internal static bool IsNull(object obj)
{
return (obj == null || obj == AutomationNull.Value);
}
internal static bool IsNull(object obj) => obj == null || obj == AutomationNull.Value;

/// <summary>
/// Auxiliary for the cases where we want a new PSObject or null.
Expand Down Expand Up @@ -4711,10 +4713,11 @@ internal static IConversionData FigureConversion(object valueToConvert, Type res
{
PSObject valueAsPsObj;
Type originalType;
if (valueToConvert == null || valueToConvert == AutomationNull.Value)

if (IsNull(valueToConvert))
{
valueAsPsObj = null;
originalType = typeof(Null);
valueAsPsObj = null;
}
else
{
Expand Down
22 changes: 20 additions & 2 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 @@ -3028,13 +3028,22 @@ private DynamicMetaObject CompareEQ(DynamicMetaObject target,
if (target.Value == null)
{
return new DynamicMetaObject(
arg.Value == null ? ExpressionCache.BoxedTrue : ExpressionCache.BoxedFalse,
arg.Value == null || arg.Value == DBNull.Value || arg.Value == NullString.Value
? ExpressionCache.BoxedTrue
: ExpressionCache.BoxedFalse,
target.CombineRestrictions(arg));
}

var enumerable = PSEnumerableBinder.IsEnumerable(target);
if (enumerable == null && arg.Value == null)
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Jun 20, 2019

Choose a reason for hiding this comment

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

You don't handle the collection comparison case:

$s = @($null, $null, $null, [System.DBNull]::Value, [NullString]::Value)
$p = $s -eq $null
$p.Count

Copy link
Copy Markdown
Collaborator Author

@vexx32 vexx32 Jun 22, 2019

Choose a reason for hiding this comment

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

OK... it looks like it should be done here somewhere?
https://github.com/vexx32/PowerShell/blob/ecccb0872fefb9e3c8e8b0949bf82a4ba0c96479/src/System.Management.Automation/engine/runtime/Binding/Binders.cs#L3377

I... don't know where to start there. 😅

Copy link
Copy Markdown
Collaborator Author

@vexx32 vexx32 Jun 23, 2019

Choose a reason for hiding this comment

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

@daxian-dbw looks like we both spoke too soon. I have a lot of difficulty understanding all those code paths, but even without any editing yet, this works:

image

So... I would guess that somewhere in those paths we hook back into either LanguagePrimitives methods (which I've already handled) or back into the binder methods themselves as scalar comparisons.

I'll add some tests to make sure we don't regress. 😄

Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw Jun 24, 2019

Choose a reason for hiding this comment

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

Right :) I should have compile your branch and tried it myself.

PowerShell/src/System.Management.Automation/engine/runtime/Binding/Binders.cs

Lines 3360 to 3362 in 4967416

// If the target is enumerable, the we generate an object[] result with elements matching.
// The iteration will be done in a pre-compiled method, but the comparison is done with
// a dynamically generated lambda that uses a binder

Thanks for adding the collection comparison test!

{
if (target.Value == DBNull.Value || target.Value == NullString.Value)
{
return new DynamicMetaObject(
ExpressionCache.BoxedTrue,
target.CombineRestrictions(arg));
}
Comment thread
vexx32 marked this conversation as resolved.
Outdated

return new DynamicMetaObject(
ExpressionCache.BoxedFalse,
target.CombineRestrictions(arg));
Expand All @@ -3051,13 +3060,22 @@ private DynamicMetaObject CompareNE(DynamicMetaObject target,
if (target.Value == null)
{
return new DynamicMetaObject(
arg.Value == null ? ExpressionCache.BoxedFalse : ExpressionCache.BoxedTrue,
arg.Value == null || arg.Value == DBNull.Value || arg.Value == NullString.Value
? ExpressionCache.BoxedFalse
: ExpressionCache.BoxedTrue,
target.CombineRestrictions(arg));
}

var enumerable = PSEnumerableBinder.IsEnumerable(target);
if (enumerable == null && arg.Value == null)
{
if (target.Value == DBNull.Value || target.Value == NullString.Value)
{
return new DynamicMetaObject(
ExpressionCache.BoxedFalse,
target.CombineRestrictions(arg));
}
Comment thread
vexx32 marked this conversation as resolved.
Outdated

return new DynamicMetaObject(ExpressionCache.BoxedTrue,
target.CombineRestrictions(arg));
}
Expand Down
Loading