Skip to content

Conversation

KirkMunro
Copy link
Contributor

@KirkMunro KirkMunro commented Oct 4, 2019

PR Summary

Fix #10656.

PR Context

Mads Torgersen rightly pointed out at this point in a .NET Conf 2019 demonstration he delivered that testing for $null using -is is more accurate than testing for $null using -eq.

PowerShell would benefit from having the same support, such that users can check if an object is or is not null by invoking $x -is $null or $x -isnot $null, or Where-Object Property -is $null or Where-Object Property -isnot $null, for two reasons:

  1. Code written this way is very discriptive, and matches the way we speak about what is actually happening when we test for $null.
  2. More importantly, this allows for comparisons of any object to $null, including collections. Before this change, if you want to test whether or not a collection is $null, you have to place the $null on the left-hand side of the operator, which is not possible in Where-Object. With this change in place, you can simply keep $null on the right-hand side of the -is and -isnot operators, regardless of the type of object on the left-hand side that you are comparing to $null.

Here are the new patterns that will be supported once this PR is merged:

$x -is $null
$y -isnot $null
$collection | where Property -is $null
$collection | where Property -isnot $null

Note that -is $null will return $true if the left hand side is $null, [DBNull]::Value, [NullString]::Value, or [System.Management.Automation.Internal.AutomationNull]::Value.

Also note that this only works when you use a null literal on the right-hand side of -is and -isnot, regardless of whether you use the operators or Where-Object. The explicit check for a null literal is necessary to prevent the use of other variables or subexpressions on the right-hand side.

PR Checklist

@vexx32
Copy link
Collaborator

vexx32 commented Oct 4, 2019

@KirkMunro should we provide support here also for the null-like values [dbnull]::Value and [NullString]::Value (or indeed possibly even [AutomationNull]::Value?)

Given the decisions in #9561, I would say that we probably should have this operation also treat the null-representatives as null via LanguagePrimitives.IsNullLike(value). If distinction is required, $value -is [dbnull] will still be functional to make the distinction, but in most cases we don't need that distinction, I would think?

/cc @daxian-dbw

@KirkMunro
Copy link
Contributor Author

@PoshChan Please restart macos

@PoshChan
Copy link
Collaborator

PoshChan commented Oct 4, 2019

@KirkMunro, successfully started retry of PowerShell-CI-macOS

@KirkMunro
Copy link
Contributor Author

KirkMunro commented Oct 4, 2019

@vexx32 So you're proposing this update invoke LanguagePrimitives.IsNullLike instead of just checking if the lhs is null? That seems to make sense to me, but technically that would make this a breaking change. It would be good to get @daxian-dbw's opinion to be sure that's the desired/expected behavior.

Aside: LanguagePrimitives.IsNullLike should probably check IsNull first, before checking DBNull.Value or NullString.Value, right, since the most common matches in PowerShell would be null or AutomationNull.Value. I'd also put NullString.Value before DBNull.Value. That way the comparisons are in order from most likely to least likely.

@vexx32
Copy link
Collaborator

vexx32 commented Oct 4, 2019

@KirkMunro yep, that's the conclusion we came to in #10422 which is currently stalled pending a question on how we determine equality and what matters to it when using -ge/-le instead of -eq; I believe the same change you recommend is made there, but that PR is yet to be merged.

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 8, 2019

I'd expect consistency with -eq for NullDB and etc.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 8, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.5 milestone Oct 8, 2019
@KirkMunro
Copy link
Contributor Author

KirkMunro commented Oct 9, 2019

@iSazonov Are you're suggesting that we should support [NullString]::Value, [DBNull]::Value and [AutomationNull]::Value on the right-hand side of -is as well? Because there is a slight difference between $null and these values.

When using -eq, to test for equality with these other null values, you need to use the singleton returned from the Value static member. When using -is, however, it is more natural and accurate to use the object type on the right-hand side. For example:

$x = [AutomationNull]::Value
$x -is [AutomationNull] # returns true
$x -is [AutomationNull]::Value # returns an error because the right-hand side is not a type or null

I believe the above behavior is desirable and expected.

The only outstanding question I have on this PR is whether or not $x -is $null should return true when $x is one of the singleton nulls.

@vexx32
Copy link
Collaborator

vexx32 commented Oct 9, 2019

@KirkMunro i would that that if given the type itself to test, it would follow current behaviour, and if given a null singleton value, it should follow the IsNullLike pattern. Reason being is that $nullLike -eq $null will return true for all of them, and it would a point of confusion for this semantic to differ here, I think.

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 9, 2019

$x -is [AutomationNull]::Value # returns an error because the right-hand side is not a type or null

null is not type too. So it is under question too. If we allow null then we should allow [AutomationNull]::Value too. Or disable null.
And if we allow values why do not we allow:

$x="abc"
$x -is "abc" # -> True

@KirkMunro
Copy link
Contributor Author

KirkMunro commented Oct 9, 2019

Using the -is operator is the most accurate and correct way to test for $null, because it avoids running user code (operator overloads), and simply checks if a value is null. That's why this PR was implemented. C# added support for is null in C# 7, and that is the best practice there. We're just following their lead. It also promotes better code because it keeps the operand on the right-hand side (collection unwrapping does not come into play with -is).

The other nulls already work using -is, with their types.

Your question @iSazonov highlights exactly why I think we shouldn't support [AutomationNull]::Value and the other non-$null nulls on the right-hand side of -is: because it opens a can of worms. If we're going to support one value on the right-hand side of -is, then why not support all values on the right-hand side of -is? i.e. After checking for $null or instances of Type, why not fall back to an equality comparison?

The answer to that question is simple: intent/ambiguity.

-is $null is not a test for equality. It's a check to see if something is null.
-is [string] is not a test for equality either. It's a test to see if something is a string.

But what about the following syntax:

is 'string' is a test to see if something is a string. The 'string' string gets converted into a type.

If you want to fall back to -eq when the right-hand side is not null and not a type, suddenly you have an ambiguity issue. When the right-hand side is a string, which is supported today, and when that string is also a type name are you checking if something is of type string or if something is a string that contains the value "string"?

By forcing tests for value equality to be done using the -eq or -neq operators, the intent is clear and unambiguous.

@KirkMunro
Copy link
Contributor Author

KirkMunro commented Oct 9, 2019

@vexx32 To your point, I think it makes sense when checking for -is $null to also match on the other null types. e.g. [AutomationNull]::value -is $null should return true IMHO. That allows scripters to avoid thinking about which null they are dealing with. If they want to test for a specific null other than $null, they can always use the type (i.e. [NullString], [DBNull], etc.).

@rjmholt
Copy link
Collaborator

rjmholt commented Oct 9, 2019

Is the final proposal then that:

  • The RHS of -is must be either a type object or $null
  • Other RHS expressions, including DBNull, AutomationNull and NullString, are errors
  • When the RHS is a type, it checks the LHS expression for being that type (established behaviour)
  • When the RHS is $null, it checks the LHS for being null-like (null, DBNull, AutomationNull, NullString) (new behaviour)

@KirkMunro
Copy link
Contributor Author

KirkMunro commented Oct 9, 2019

@rjmholt That's almost 100% correct.

  • Other RHS expressions, including DBNull, AutomationNull and NullString, are errors

[DBNull]::Value, [NullString]::Value, and [S.M.A.Internal.AutomationNull]::Value on the right-hand side are errors, just like any non-null, non-type value. The [DBNull], [NullString], and [AutomationNull] types are supported on the right-hand side though, because they are types.

The other statements you listed were accurate, and the code and tests were just updated to support that proposal.

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 9, 2019

If they want to test for a specific null other than $null, they can always use the type (i.e. [AutomationNull], [DBNull], etc.).

For consistency we could define [Null] type.
It is not clear why do we follow C# pattern.

@rjmholt
Copy link
Collaborator

rjmholt commented Oct 9, 2019

@KirkMunro sorry, was using the type name as a shorthand for the singleton value.

Ok, that sounds good to me. I certainly agree that not allowing the alt-null values on the RHS is the way to go.

@KirkMunro

This comment has been minimized.

@KirkMunro
Copy link
Contributor Author

This PR is still awaiting a decision or a merge.

@ghost ghost added the Review - Needed The PR is being reviewed label May 27, 2020
@ghost
Copy link

ghost commented May 27, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@rjmholt
Copy link
Collaborator

rjmholt commented Jun 2, 2020

I know this has already been discussed, but I want to register that I'm worried about a cmdlet having a different behaviour between the $null literal and another variable that is null. At runtime, my feeling is that the behaviour around a value should not depend on the syntax used to create it. I think that introduces complexity in both implementation and understanding and hurts our separations of concern between the parser and the PowerShell runtime.

This PR does great work, and I think the addition of -is $null makes sense both because of the .NET concepts at work and also to alleviate the pain of -eq $null. But I think we should design very carefully around:

  • -is $null vs -is $variableThatIsNull vs -is $uninstantiatedVariable -- in particular I'm not sure of anywhere else in PowerShell where accessing a variable (in $ form) is different in those three scenarios
  • -is $null vs Where-Object -is $null (vs Where-Object -IsNull)

I'm not sure I fully agree that the breaking change that the literal $null behaviour differentiation was intended to avoid is something we have considered to be breaking in the past (for example, a similar breakage could be claimed by defining most command names). I definitely understand the scenario there though, and I think it's a valuable point.

As I say, I think -is $null is a great addition. My concern is only that differentiating the $null literal from a null value puts a corner case into PowerShell that may not belong there.

I just want to make these points in writing so that maintainers or the @PowerShell/powershell-committee can review the design carefully.

@rjmholt rjmholt added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jun 2, 2020
@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jun 3, 2020
@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee reviewed this, we prefer adding -isnull operator which can be used with -not instead of overloading -is which is expected to be used against types and would be a breaking change.

@ghost ghost removed the Review - Needed The PR is being reviewed label Jun 3, 2020
@vexx32
Copy link
Collaborator

vexx32 commented Jun 3, 2020

@SteveL-MSFT That would effectively make it the only postfix operator currently existing in PS, at least if we're still talking $object -isnull

I'd be concerned about whether that would overcomplicate the parsing logic for operators.

@ghost ghost added the Review - Needed The PR is being reviewed label Jun 11, 2020
@ghost
Copy link

ghost commented Jun 11, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@mklement0
Copy link
Contributor

We could make it a regular prefix unary operator with a -isnotnull variant, I suppose, which would also work with Where-Object (where the argument representing an operator can be placed in any position):

# Expressions
-isnull $var
-isnotnull $var

# Where-Object
... | Where-Object -isnull Foo
... | Where-Object -isnotnull Foo
... | Where-Object -isnotnull   # once #8357  gets implemented

@vexx32
Copy link
Collaborator

vexx32 commented Jun 11, 2020

🤔 having experimented with Where-Object's parameter sets before... I genuinely don't think that you can make that binding work without overhauling the entire parameter binder.

So that request is, at least for Where-Object, currently impossible @SteveL-MSFT @mklement0

I stand corrected. 🙂

@mklement0
Copy link
Contributor

mklement0 commented Jun 11, 2020

It works with the unary -not operator, so wouldn't it work with other unary operators too?

PS> [pscustomobject] @{ foo = 0 } | Where-Object -Not foo

foo
---
  0

Note that, for the reasons mentioned, Where-Object foo -Not works too (though is awkward, though perhaps less so with Where-Object foo -isnull, but it's generally advisable to stick with the order required in expressions).

@KirkMunro
Copy link
Contributor Author

My only objection to any of the arguments made above and to what the Committee decided on is that we should have -isnotnull instead of -not -isnull for the same reason that we have paired -eq/-ne, -is/-isnot, and so many other operators that have positive/negative comparisons associated with them.

I also feel strongly that these should be postfix operators, not prefix operators so that the expressions can be read and more easily understood, so I'm going to move forward with that approach. Part of the value with these changes is eliminating the need to put the $null value you are comparing against first, which is not intuitive for non-developers and which never reads well.

$array -isnull # Intuitive, reads exactly like what it checks

-isnull $array # Not as readable

I've never liked that PSScriptAnalyzer always complains and tells me to put my $null values first in comparisons such as $null -eq $value.

@KirkMunro
Copy link
Contributor Author

KirkMunro commented Jun 14, 2020

@SteveL-MSFT That would effectively make it the only postfix operator currently existing in PS, at least if we're still talking $object -isnull

I'd be concerned about whether that would overcomplicate the parsing logic for operators.

@vexx32: You're forgetting about the ++ and -- postfix operators. Adding additional postfix operators won't overcomplicate the parsing logic.

@mklement0
Copy link
Contributor

I can see the appeal of postfix -isnull / -isnotnull for symmetry with -is / -isnot.

However, I still think that -is $null / -isnot $null is by far preferable to having separate operators.

I definitely understand @rjmholt's concern about the special-casing of only supporting a verbatim $null as the RHS.

However, he also states:

I'm not sure I fully agree that the breaking change that the literal $null behaviour differentiation was intended to avoid is something we have considered to be breaking in the past

So my plea is:

Let us stick with -is $null / -isnot $null, irrespective of the syntactic form of the RHS, which eliminates the special casing and also simplifies the implementation.

I think there's a reasonable case to be made that the technically breaking change that this constitutes falls into Bucket 3: Unlikely Grey Area:

  • I've gone through about 30 pages of the top 3 matches among the 203,107 uses of the -is operator (https://github.com/search?p=20&q=language%3APowerShell+-is&type=Code) and didn't find a single instance of using a variable as the RHS, which is the scenario @lzybkr was concerned about (the vast majority of these literal RHSs were type literals (e.g, [string]), with only very few instances of strings (e.g., 'string').

  • Even the case of code that contains "" -is $Typo (see above) relying on a statement-terminating error strikes me as unlikely:

    • With $Typo accidentally evaluating to $null (as the naming suggests), the bug would likely have been discovered and fixed before deployment.
    • Without an explicit try / catch / $ErrorActionPreference = 'Stop', execution would continue even with the statement-terminating error (although with the proposed change an error would no longer be emitted).

@KirkMunro
Copy link
Contributor Author

Closing. See #10238 (comment).

@KirkMunro KirkMunro closed this Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision Review - Needed The PR is being reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow $null on right-hand side of -is and -isnot operators.