From b34e331d636c5eea31b25257f5664d1ee7f7d321 Mon Sep 17 00:00:00 2001 From: "Joel Sallow (/u/ta11ow)" <32407840+vexx32@users.noreply.github.com> Date: Fri, 28 Jun 2019 14:39:34 -0400 Subject: [PATCH] Consider `DBNull.Value` and `NullString.Value` the same as `$null` when comparing with `$null` and casting to bool (#9794) - Adds `LanguagePrimitives.IsNullLike()` method to account for `DBNull.Value` and `NullString.Value` so that they can be considered the same as a null value where sensible in PowerShell. - Updates `-ne` and `-eq` binders to treat `DBNull.Value` and `NullString.Value` as equal to null/AutomationNull. - Update code paths for comparing objects in LanguagePrimitives to ensure consistency with how the `-eq` and `-ne` binders work when calling LanguagePrimitives methods to do the comparisons. - Make `LanguagePrimitives.IsNull()` and `LanguagePrimitives.IsNullLike()` public methods. - Added tests for null behaviours in `NullRepresentatives.Tests.ps1` --- .../engine/LanguagePrimitives.cs | 149 +++++++++--------- .../engine/runtime/Binding/Binders.cs | 17 +- .../Scripting/NullRepresentatives.Tests.ps1 | 89 +++++++++++ .../engine/Basic/SemanticVersion.Tests.ps1 | 59 +++---- 4 files changed, 209 insertions(+), 105 deletions(-) create mode 100644 test/powershell/Language/Scripting/NullRepresentatives.Tests.ps1 diff --git a/src/System.Management.Automation/engine/LanguagePrimitives.cs b/src/System.Management.Automation/engine/LanguagePrimitives.cs index 75974632b..618a23b9f 100644 --- a/src/System.Management.Automation/engine/LanguagePrimitives.cs +++ b/src/System.Management.Automation/engine/LanguagePrimitives.cs @@ -593,9 +593,7 @@ namespace System.Management.Automation /// Object to compare first to. /// True if first is equal to the second. public static new bool Equals(object first, object second) - { - return Equals(first, second, false, CultureInfo.InvariantCulture); - } + => Equals(first, second, false, CultureInfo.InvariantCulture); /// /// Used to compare two objects for equality converting the second to the type of the first, if required. @@ -606,9 +604,7 @@ namespace System.Management.Automation /// to specify the type of string comparison /// True if first is equal to the second. public static bool Equals(object first, object second, bool ignoreCase) - { - return Equals(first, second, ignoreCase, CultureInfo.InvariantCulture); - } + => Equals(first, second, ignoreCase, CultureInfo.InvariantCulture); /// /// Used to compare two objects for equality converting the second to the type of the first, if required. @@ -646,25 +642,28 @@ namespace System.Management.Automation if (first == null) { - if (second == null) return true; - return false; + return IsNullLike(second); } if (second == null) { - return false; // first is not null + return IsNullLike(first); } - 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(); @@ -708,24 +707,24 @@ namespace System.Management.Automation /// Helper method for [Try]Compare to determine object ordering with null. /// /// The numeric value to compare to null. - /// True if the number to compare is on the right hand side if the comparison. + /// True if the number to compare is on the right hand side in the comparison. private static int CompareObjectToNull(object value, bool numberIsRightHandSide) { var i = numberIsRightHandSide ? -1 : 1; // If it's a positive number, including 0, it's greater than null // for everything else it's less than zero... - switch (value) + return value switch { - 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; - } + 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, + _ => IsNullLike(value) ? 0 : i + }; } /// @@ -741,9 +740,7 @@ namespace System.Management.Automation /// to the type of . /// public static int Compare(object first, object second) - { - return LanguagePrimitives.Compare(first, second, false, CultureInfo.InvariantCulture); - } + => LanguagePrimitives.Compare(first, second, false, CultureInfo.InvariantCulture); /// /// Compare first and second, converting second to the @@ -759,9 +756,7 @@ namespace System.Management.Automation /// to the type of . /// 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); /// /// Compare first and second, converting second to the @@ -779,15 +774,12 @@ namespace System.Management.Automation /// 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) { - throw PSTraceSource.NewArgumentException("formatProvider"); + throw PSTraceSource.NewArgumentException(nameof(formatProvider)); } first = PSObject.Base(first); @@ -795,7 +787,7 @@ namespace System.Management.Automation if (first == null) { - return second == null ? 0 : CompareObjectToNull(second, true); + return CompareObjectToNull(second, true); } if (second == null) @@ -805,7 +797,7 @@ namespace System.Management.Automation if (first is string firstString) { - string secondString = second as string; + var secondString = second as string; if (secondString == null) { try @@ -814,19 +806,26 @@ namespace System.Management.Automation } 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); @@ -839,8 +838,12 @@ namespace System.Management.Automation } 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) @@ -855,7 +858,7 @@ namespace System.Management.Automation // 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()); } /// @@ -868,9 +871,7 @@ namespace System.Management.Automation /// zero if it is greater or zero if they are the same. /// True if the comparison was successful, false otherwise. 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); /// /// Tries to compare first and second, converting second to the type of the first, if necessary. @@ -882,9 +883,7 @@ namespace System.Management.Automation /// Less than zero if first is smaller than second, more than zero if it is greater or zero if they are the same. /// True if the comparison was successful, false otherwise. 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); /// /// Tries to compare first and second, converting second to the type of the first, if necessary. @@ -900,10 +899,7 @@ namespace System.Management.Automation 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)) { @@ -988,8 +984,10 @@ namespace System.Management.Automation public static bool IsTrue(object obj) { // null is a valid argument - it converts to false... - if (obj == null || obj == AutomationNull.Value) + if (IsNullLike(obj)) + { return false; + } obj = PSObject.Base(obj); @@ -1015,8 +1013,7 @@ namespace System.Management.Automation if (objType == typeof(SwitchParameter)) return ((SwitchParameter)obj).ToBool(); - IList objectArray = obj as IList; - if (objectArray != null) + if (obj is IList objectArray) { return IsTrue(objectArray); } @@ -1062,14 +1059,19 @@ namespace System.Management.Automation } /// - /// Internal routine that determines if an object meets any of our criteria for null. + /// Internal routine that determines if an object meets any of our criteria for true null. /// /// The object to test. /// True if the object is null. - internal static bool IsNull(object obj) - { - return (obj == null || obj == AutomationNull.Value); - } + public static bool IsNull(object obj) => obj == null || obj == AutomationNull.Value; + + /// + /// Internal routine that determines if an object meets any of our criteria for null. + /// This method additionally checks for and + /// + /// The object to test. + /// True if the object is null. + public static bool IsNullLike(object obj) => obj == DBNull.Value || obj == NullString.Value || IsNull(obj); /// /// Auxiliary for the cases where we want a new PSObject or null. @@ -3100,15 +3102,17 @@ namespace System.Management.Automation return AutomationNull.Value; } - private static bool ConvertClassToBool(object valueToConvert, - Type resultType, - bool recursion, - PSObject originalValueToConvert, - IFormatProvider formatProvider, - TypeTable backupTable) + private static bool ConvertClassToBool( + object valueToConvert, + Type resultType, + bool recursion, + PSObject originalValueToConvert, + IFormatProvider formatProvider, + TypeTable backupTable) { typeConversion.WriteLine("Converting ref to boolean."); - return valueToConvert != null; + // Both NullString and DBNull should be treated the same as true nulls for the purposes of this conversion. + return !IsNullLike(valueToConvert); } private static bool ConvertValueToBool(object valueToConvert, @@ -4724,10 +4728,11 @@ namespace System.Management.Automation { PSObject valueAsPsObj; Type originalType; - if (valueToConvert == null || valueToConvert == AutomationNull.Value) + + if (IsNull(valueToConvert)) { - valueAsPsObj = null; originalType = typeof(Null); + valueAsPsObj = null; } else { diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index f67cbab7a..0671c781d 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -3028,7 +3028,9 @@ namespace System.Management.Automation.Language if (target.Value == null) { return new DynamicMetaObject( - arg.Value == null ? ExpressionCache.BoxedTrue : ExpressionCache.BoxedFalse, + LanguagePrimitives.IsNullLike(arg.Value) + ? ExpressionCache.BoxedTrue + : ExpressionCache.BoxedFalse, target.CombineRestrictions(arg)); } @@ -3036,7 +3038,9 @@ namespace System.Management.Automation.Language if (enumerable == null && arg.Value == null) { return new DynamicMetaObject( - ExpressionCache.BoxedFalse, + LanguagePrimitives.IsNullLike(target.Value) + ? ExpressionCache.BoxedTrue + : ExpressionCache.BoxedFalse, target.CombineRestrictions(arg)); } @@ -3051,14 +3055,19 @@ namespace System.Management.Automation.Language if (target.Value == null) { return new DynamicMetaObject( - arg.Value == null ? ExpressionCache.BoxedFalse : ExpressionCache.BoxedTrue, + LanguagePrimitives.IsNullLike(arg.Value) + ? ExpressionCache.BoxedFalse + : ExpressionCache.BoxedTrue, target.CombineRestrictions(arg)); } var enumerable = PSEnumerableBinder.IsEnumerable(target); if (enumerable == null && arg.Value == null) { - return new DynamicMetaObject(ExpressionCache.BoxedTrue, + return new DynamicMetaObject( + LanguagePrimitives.IsNullLike(target.Value) + ? ExpressionCache.BoxedFalse + : ExpressionCache.BoxedTrue, target.CombineRestrictions(arg)); } diff --git a/test/powershell/Language/Scripting/NullRepresentatives.Tests.ps1 b/test/powershell/Language/Scripting/NullRepresentatives.Tests.ps1 new file mode 100644 index 000000000..4c1863bb6 --- /dev/null +++ b/test/powershell/Language/Scripting/NullRepresentatives.Tests.ps1 @@ -0,0 +1,89 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +using namespace System.Management.Automation.Internal + +Describe 'Null Representatives' -Tags 'CI' { + + Context 'Comparisons with $null' { + BeforeAll { + $TestValues = @( + @{ Value = { [AutomationNull]::Value } } + @{ Value = { [DBNull]::Value } } + @{ Value = { [NullString]::Value } } + ) + } + + It ' should be equivalent to $null (RHS: $null)' -TestCases $TestValues { + param($Value) + + $Value.InvokeReturnAsIs() -eq $null | Should -BeTrue + } + + It '$null should be equivalent to (LHS: $null)' -TestCases $TestValues { + param($Value) + + $null -eq $Value.InvokeReturnAsIs() | Should -BeTrue + } + } + + Context 'Comparisons with other null representatives' { + <# + The only unequal null-representatives are NullString and DBNull. + AutomationNull and $null are always considered equal already, so therefore NullString compares as + true with both of them, as does DBNull. + + However, as NullString and DBNull have different purposes, it makes more sense to consider them unequal + when directly compared with each other. + #> + It 'DBNull should not be equal to NullString' { + [DBNull]::Value -eq [NullString]::Value | Should -BeFalse + [NullString]::Value -eq [DBNull]::Value | Should -BeFalse + } + } + + Context 'Casting Behaviour' { + BeforeAll { + $TestValues = @( + @{ Value = { $null } } + @{ Value = { [DBNull]::Value } } + @{ Value = { [NullString]::Value } } + @{ Value = { [AutomationNull]::Value } } + ) + } + + It ' should cast to $false' -TestCases $TestValues { + param($Value) + + [bool]($Value.InvokeReturnAsIs()) | Should -BeFalse + } + + It '-not should be $true' -TestCases $TestValues { + param($Value) + + -not $Value.InvokeReturnAsIs() | Should -BeTrue + } + + It ' should be treated as $false by Where-Object' -TestCases $TestValues { + param($Value) + + 100 | Where-Object { $Value.InvokeReturnAsIs() } | Should -BeNullOrEmpty + } + } + + Context 'Collection Comparisons' { + BeforeAll { + $NullArray = $null, $null, [DBNull]::Value, $null, $null, [NullString]::Value + } + + It ' should correctly filter the array and return results' { + param($Value, $ExpectedCount) + + $NullArray -eq $Value | Should -HaveCount $ExpectedCount + } -TestCases @( + @{ Value = $null; ExpectedCount = 6 } + @{ Value = [DBNull]::Value; ExpectedCount = 5 } + @{ Value = [NullString]::Value; ExpectedCount = 5 } + ) + } +} diff --git a/test/powershell/engine/Basic/SemanticVersion.Tests.ps1 b/test/powershell/engine/Basic/SemanticVersion.Tests.ps1 index 65b6136b2..606b4a827 100644 --- a/test/powershell/engine/Basic/SemanticVersion.Tests.ps1 +++ b/test/powershell/engine/Basic/SemanticVersion.Tests.ps1 @@ -109,8 +109,8 @@ Describe "SemanticVersion api tests" -Tags 'CI' { @{ lhs = $v1_0_0_alpha; rhs = $v1_0_0_alpha2 } @{ lhs = $v1_0_0_alpha; rhs = $v1_0_0 } @{ lhs = $v1_0_0_beta; rhs = $v1_0_0 } - @{ lhs = $v2_1_0; rhs = "3.0"} - @{ lhs = "1.5"; rhs = $v2_1_0} + @{ lhs = $v2_1_0; rhs = "3.0" } + @{ lhs = "1.5"; rhs = $v2_1_0 } ) } @@ -176,38 +176,39 @@ Describe "SemanticVersion api tests" -Tags 'CI' { Context "Error handling" { It ": ''" -TestCases @( - @{ name = "Missing parts: 'null'"; errorId = "PSArgumentNullException";expectedResult = $false; version = $null } - @{ name = "Missing parts: 'NullString'"; errorId = "PSArgumentNullException";expectedResult = $false; version = [NullString]::Value } - @{ name = "Missing parts: 'EmptyString'";errorId = "FormatException"; expectedResult = $false; version = "" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "-" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "." } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "+" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "-alpha" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1..0" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.-alpha" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.+alpha" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0-alpha+" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0-+" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0+-" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0+" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0-" } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0." } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0." } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.." } - @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = ".0.0" } - @{ name = "Range check of versions"; errorId = "FormatException"; expectedResult = $false; version = "-1.0.0" } - @{ name = "Range check of versions"; errorId = "FormatException"; expectedResult = $false; version = "1.-1.0" } - @{ name = "Range check of versions"; errorId = "FormatException"; expectedResult = $false; version = "1.0.-1" } - @{ name = "Format errors"; errorId = "FormatException"; expectedResult = $false; version = "aa.0.0" } - @{ name = "Format errors"; errorId = "FormatException"; expectedResult = $false; version = "1.bb.0" } - @{ name = "Format errors"; errorId = "FormatException"; expectedResult = $false; version = "1.0.cc" } + @{ name = "Missing parts: 'null'"; errorId = "PSArgumentNullException"; expectedResult = $false; version = $null } + @{ name = "Missing parts: 'NullString'"; errorId = "PSArgumentNullException"; expectedResult = $false; version = [NullString]::Value } + @{ name = "Missing parts: 'EmptyString'"; errorId = "FormatException"; expectedResult = $false; version = "" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "-" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "." } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "+" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "-alpha" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1..0" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.-alpha" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.+alpha" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0-alpha+" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0-+" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0+-" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0+" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0-" } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.0." } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0." } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = "1.0.." } + @{ name = "Missing parts"; errorId = "FormatException"; expectedResult = $false; version = ".0.0" } + @{ name = "Range check of versions"; errorId = "FormatException"; expectedResult = $false; version = "-1.0.0" } + @{ name = "Range check of versions"; errorId = "FormatException"; expectedResult = $false; version = "1.-1.0" } + @{ name = "Range check of versions"; errorId = "FormatException"; expectedResult = $false; version = "1.0.-1" } + @{ name = "Format errors"; errorId = "FormatException"; expectedResult = $false; version = "aa.0.0" } + @{ name = "Format errors"; errorId = "FormatException"; expectedResult = $false; version = "1.bb.0" } + @{ name = "Format errors"; errorId = "FormatException"; expectedResult = $false; version = "1.0.cc" } ) { param($version, $expectedResult, $errorId) { [SemanticVersion]::new($version) } | Should -Throw -ErrorId $errorId - if ($version -eq $null) { + if ([LanguagePrimitives]::IsNull($version)) { # PowerShell convert $null to Empty string { [SemanticVersion]::Parse($version) } | Should -Throw -ErrorId "FormatException" - } else { + } + else { { [SemanticVersion]::Parse($version) } | Should -Throw -ErrorId $errorId } $semVer = $null