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

[vexx32]

PR Summary

Closes #9561

• 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

PR Context

DBNull.Value and NullString.Value are generally considered to be equivalent to null, and it makes the most sense to have PowerShell recognise them as such instead of treating them as strange class objects. See linked issue for more details.

/cc @daxian-dbw

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed

[vexx32]

@daxian-dbw Given how PowerShell handles conversions from null -> string (yielding string.Empty if I'm not mistaken) it may not make sense to also handle NullString.Value as a true null here as well -- either that or I need to be more careful with the handling in the string conversion paths so that a null string is a valid result when given a "null" value.

See the failing tests, where it expects empty string because the class definition has a [string]-type property and it is converting "expected" values into empty strings as a result here.

What would be appropriate here, do you think?

EDIT: Looks like FigureConversion() just needs to keep track that the original type was in fact NullString so that the conversion paths do their thing correctly. Verified that the tests seem to pass locally with this fix. 🙂

[daxian-dbw]

I think [DBNull]::Value and [NullString]::Value should be treated as null only when comparing with $null, but not in general.
With your change, gm -InputObject ([System.DBNull]::Value) will start to fail.

Quote @mklement0's comment in the original issue:

I wouldn't frame the issue in terms of $null, but in terms of (possibly implicit) to-Boolean conversion.
Secondarily, the question is how direct comparison with $null should be handled.

As for LanguagePrimitive.IsNull, I think it probably should be renamed to IsLiterallyNull() with a comment like "Unlike objects like DBNull.Value or NullString.Value that represent NULL in specific scenarios, this method check for literal NULL that is not considered to be an object" and keep the existing implementation unchanged.

[vexx32]

🤔 I guess that does make sense. I can do that.

Any chance you can point me in the general direction of where the direct comparison with $null would be implemented instead?

Also, given that, would we consider [DBNull]::Value -eq [NullString]::Value? And would it make sense to make IsLiterallyNull() public so that PS users can more easily distinguish between $null and [DBNull] etc., when they do need to?

[daxian-dbw]

Also, given that, would we consider [DBNull]::Value -eq [NullString]::Value?

I don't think we should consider them equal. The following shows how they should behave in my opinion:

## Equality comparison with $null should return true
$null -eq [AutomationNull].Value
> $true

[AutomationNull].Value -eq $null
> $true

$null -eq [DBNull].Value
> $true

[DBNull].Value -eq $null
> $true

$null -eq [NullString]::Value
> $true

[NullString]::Value -eq $null
> $true

## Equality comparison between '[AutomationNull].Value', '[DBNull].Value', and '[NullString].Value' should return false.
[AutomationNull].Value -eq [DBNull].Value
> $false

[DBNull].Value -eq [NullString]::Value
> $false

[NullString]::Value -eq [AutomationNull].Value
> $false

/cc @mklement0 for comments :)

would it make sense to make IsLiterallyNull() public

I agree to make it public, but we may need to chew on the name a bit more :)

Any chance you can point me in the general direction of where the direct comparison with $null would be implemented instead?

Sure, I will take a look at the code later today.

[vexx32]

Appreciate it! Personally I'm a fan of IsActuallyNull() 😉 (or maybe IsTrueNull())

Yeah, I figured those comparisons were what you had in mind. I think that might be inviting a bit of confusion, but it's still important to be able to check those against each other so I agree there.

[mklement0]

My vote is for IsTrueNull().

@daxian-dbw, I agree with the proposed behavior, thanks for hashing it out.

For someone who needs to distinguish between $null and one of the null-like values in an expression context (as an alternative to using Get-Member), they can use:

[NullString]::Value -is [NullString] 
[System.DBNull]::Value -is [System.DBNull] 

Unfortunately, we do not get the same behavior with [System.Management.Automation.Internal.AutomationNull]::Value, because it is seemingly converted to true $null during parameter binding and in expressions:

# FAILS - same as passing $null
Get-Member -InputObject ([System.Management.Automation.Internal.AutomationNull]::Value)

# $FALSE, paradoxically
[System.Management.Automation.Internal.AutomationNull]::Value -is [System.Management.Automation.Internal.AutomationNull]

Distinguishing between true $null and [System.Management.Automation.Internal.AutomationNull]::Value is currently only possible with obscure workarounds; e.g.:

# Determine if a given variable $v is true $null or AutomationNull
$isAutomationNull = $null -eq $v -and @($v).Count -eq 0
$isTrueNull = $null -eq $v -and @($v).Count -eq 1

Related: #7049 (comment)

[vexx32]

@daxian-dbw any luck finding an appropriate place to check for this? I've made a couple attempts myself but I've not found anything that gets the right behaviour as yet.

[daxian-dbw]

@vexx32 Sorry for being tardy. I have been focusing on some PSReadLine issues and lost track of this PR.
I think there are a few places that need to be updated for the desired behavior:

1. For binary comparison in language
   

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

   Lines 3024 to 3041 in 65a3e46

   	private DynamicMetaObject CompareEQ(DynamicMetaObject target,
   	DynamicMetaObject arg,
   	DynamicMetaObject errorSuggestion)
   	{
   	if (target.Value == null)
   	{
   	return new DynamicMetaObject(
   	arg.Value == null ? ExpressionCache.BoxedTrue : ExpressionCache.BoxedFalse,
   	target.CombineRestrictions(arg));
   	}
   	var enumerable = PSEnumerableBinder.IsEnumerable(target);
   	if (enumerable == null && arg.Value == null)
   	{
   	return new DynamicMetaObject(
   	ExpressionCache.BoxedFalse,
   	target.CombineRestrictions(arg));
   	}

   
   AND
   

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

   Lines 3047 to 3063 in 65a3e46

   	private DynamicMetaObject CompareNE(DynamicMetaObject target,
   	DynamicMetaObject arg,
   	DynamicMetaObject errorSuggestion)
   	{
   	if (target.Value == null)
   	{
   	return new DynamicMetaObject(
   	arg.Value == null ? ExpressionCache.BoxedFalse : ExpressionCache.BoxedTrue,
   	target.CombineRestrictions(arg));
   	}
   	var enumerable = PSEnumerableBinder.IsEnumerable(target);
   	if (enumerable == null && arg.Value == null)
   	{
   	return new DynamicMetaObject(ExpressionCache.BoxedTrue,
   	target.CombineRestrictions(arg));
   	}

2. For static methods in LanguagePrimitives

LanguagePrimitives.Equals
LanguagePrimitives.IsTrue
LanguagePrimitives.Compare // and other compare/trycompare overloads, to handle comparing $null and the special NULL representatives.
Review all methods in `LangaugePrimitives.cs` that are related to null comparison in names, like 'CompareObjectToNull'

The use of those static methods needs to be carefully reviewed as well.

[vexx32]

All hail @SeeminglyScience's eyeballs tbh. I missed that both binder methods needed some extra logic there.

This should work, I think. Let me know if we need to make further changes @daxian-dbw but I think this ought to do it. Verified that the test cases added appear to pass locally so we should be set to go! 🤞

[vexx32]

Okay, I see.

So @daxian-dbw the comparisons such as [AutomationNull]::Value -eq [DBNull]::Value are returning $true because that's kind of how [AutomationNull] is intended to behave in comparisons, typically. It's treated like a true null would be.

Is it worth digging in here and trying to tease apart that logic to make these all mutually not equal each other, do you think? It would be inconsistent in terms of how comparisons to automation null typically behave.

I'm thinking the way it's currently functioning is pretty sensible, given the typical cases you'll come across with AutomationNull, having it behave just like comparing these null-like values directly to null behaves. I'm also not really sure there's really value in having them all not equal each other. If each individually should be treated as "equal to" a true null for our purposes, then logically it makes the most sense to have them "equal" each other as well when tested in this manner.

After all, if you really wanted to check the "type" of the null value... we can still do $value -is [dbnull] and so forth, and get definitive results.

Would appreciate your thoughts here. 🙂

[daxian-dbw]

because that's kind of how [AutomationNull] is intended to behave in comparisons, typically. It's treated like a true null would be.

Good catch. Yes, [AutomationNull]::Value is supposed to behave just like $null with the only difference that $null can be passed through pipeline while [AutomationNull]::Value cannot.
Given that, I think it makes sense to have [AutomationNull]::Value -eq [DBNull]::Value returning $true (the same to [AutomationNull]::Value -eq [NullString]::Value).
@mklement0 Do you have any concerns?

[vexx32]

Well, you say that, but... depending on the situation it can actually be kind of tricky. Because technically a pipeline that doesn't emit output ends up storing [AutomationNull]::Value iirc.

Quick example would be:

$a = if ($False) { 2 }
@($a).Count # currently 0

Now, if we "fix" this, I think we may end up with a case where piping the $a value starts to invoke a pipeline and pass the $null value, or at least may seem like it should.

The farther you go, the trickier it becomes. 😉

I think the most sensible way to go is to accept that there are going to be cases where automationnull is detectable (after all, you can't just completely decouple engine behaviour and what the user sees in every case; one is dependent on the other), and for the few cases it can actually matter, give users some way to determine which is which.

[daxian-dbw]

It's definitely worth discussion and the behavior should depend on the real use cases where [AutomationNull]::Value has to be differentiated from $null.
I think you can open an issue to track the conversions around [AutomationNull]::Value. Once this PR is merged, the discussion here could be lost.

[SeeminglyScience]

@vexx32 Thanks for showing that example (I didn't know it before :)). The comment you put in the code snippet are reversed though, $null -is [psobject] returns false and the other one returns true.
Yes, that's inconsistent, and how to fix that (if we want to) depends on how we want to treat [AutomationNull]::Value -- more of an implementation detail, or something that makes sense for users to depend on the be able to tell.

Ideally if this were to be fixed it would just be removed from assignment, e.g. $obj = [AutomationNull]::Value would populate $obj with true null. The -is operator is one of the few things that never "lie" on occasion and I think it would be a detriment to change that.

You can also make [AutomationNull]::Value -is [AutomationNull] work by:

1. Removing the static keyword from the class decl
2. Make it inherit PSObject
3. Change the singleton instance to new AutomationNull()

That would allow $autoNull -is [psobject] to still work while enabling $autoNull -is [AutomationNull]. Though I think there are a few places in the compiler where it uses a pattern like if (expr.ExpressionType == typeof(PSObject) && expr.Value == AutomationNull.Value) so that might need to change.

[daxian-dbw]

@JamesWTruher Can you please quickly review this PR? Thanks!

[vexx32]

I'll write up a quick issue and link back here for the [automationnull] discussion & CC you guys so we can have a more focused discussion on that. 💖

(Thanks for the details, Patrick!)

[JamesWTruher]

although not specifically mentioned or explicitly tested, I assume that:

if ( -not [dbnull]::Value ) {
   "nothing up my sleeves"
}

will work as expected and emit nothing up my sleeves

[vexx32]

Yep! -not $var forces a conversion to Boolean if the object isn't already a Boolean value.

I can add an explicit test if you feel it's warranted?

[vexx32]

Done! 🙂

[daxian-dbw]

@PoshChan Please remind me in 2 hours

[PoshChan]

@daxian-dbw, this is the reminder you requested 2 hours ago

[iSazonov]

@vexx32 Thanks for the great work!

[ghost]

🎉v7.0.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes

[LaurentDardenne]

Why these behavior changes between version 5.1 and version 7.0, are not shown as a breaking change ?

[daxian-dbw]

@LaurentDardenne Good catch. Will add this to breaking changes in the release log.

[iSazonov]

@daxian-dbw Should it be CL-BreakingChange?