Use correct overload for Write-Object in Get-Variable

[vexx32]

PR Summary

Currently Get-Variable sends output to the pipeline without enumerating it when using -ValueOnly. This is at odds with almost every other cmdlet, and furthermore is at odds with the intuitive behaviour — I think it is reasonable to expect (indeed, that the intent for this switch is for) the variable value to be returned in the same manner as it is when simply calling the variable.

In other words, it appears reasonable to expect that the two statements following the declaration should be equivalent:

$a = 1, 2, 3, 4, 5
$a | Get-Member
Get-Variable -Name a -ValueOnly | Get-Member

If this behaviour is instead actually intentional, then it needs to be documented properly (it isn't), but I suspect it is more a matter of oversight than design here.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• 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: to the beginning of the title and remove the prefix when the PR is ready.
• User-facing changes
  • Not Applicable
  • OR
  • User-facing Documentation needed
    • Issue filed - Issue link:
• Testing - New and feature
  • Not Applicable 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
    • Add [feature] if the change is significant or affects feature tests

[iSazonov]

Formally it is a breaking change. /cc @SteveL-MSFT

@mklement0 Could you please look the change too?

[iSazonov]

We don't cover the code by tests. 😕
@vexx32 please add tests.

[mklement0]

The motivation for the change makes perfect sense to me and the change looks good. Nice catch, @vexx32.

[vexx32]

I can't really take the credit. @HumanEquivalentUnit stumbled across it trying to creatively troll someone on Reddit, apparently. 😂

I'll get some tests together during lunch. 😄

[PaulHigin]

Looks good if breaking change is approved and tests are added.

[vexx32]

@iSazonov @PaulHigin For a minute I was thinking "typical Should tests won't account for this... will they?"

And yet, despite a horrifically confusing error message, they do:

/cc @nohwnd

[PaulHigin]

This test will be very confusing for someone if it fails. I think that this should do an explicit check to ensure a Get-Variable -ValueOnly collection is enumerated to the pipe, and not depend on Pester 'Should'. Maybe something similar to the original repro where the two cases are piped to Get-Member, then check type name.

Also, I think the 'It' description should be made more clear, or at least add a comment providing a clearer explanation of the test. Something about -ValueOnly should enumerate collections to output pipe.

[vexx32]

Get-Member is a bit of a chunky way to go, generating a lot of data we don't need... we ought to be able to do $v | % GetType and gv v | % GetType and compare instead with the same result.

I'll try to clarify the It name. 😄

[vexx32]

Okay, I think that ought to be clear enough... let me know if I need to rethink it. 😄

[iSazonov]

And yet, despite a horrifically confusing error message, they do

Perhaps it could be reported to Pester.

[vexx32]

I have notified one of the maintainers both here and in the PowerShell Slack. I'll file an issue as well, just to be absolutely sure it's filed somewhere and not forgotten. 😄

[iSazonov]

Current test with checking types isn't good for me.

Get-Variable -Name a -ValueOnly | Get-Member

I think a key in the test is that Get-Member has parameter with ValueFromPipeline = true
Seems we could use Compare-Object with DifferenceObject parameter like
Get-Variable -Name a -ValueOnly | Compare-Object -ReferenceObject $expected | Should ...

[vexx32]

@iSazonov This is the only especially sensible method I can think of apart from the Get-Member test. Interestingly, Compare-Object does not notice the difference; running that Compare-Object line (omitting the Should) gives no output. Appending -IncludeEqual indicates that it thinks both collections are equal and contain the same items.

Which is perfectly correct, I suppose, but it doesn't seem to distinguish the enumerated/not enumerated like one can by checking the type directly; indeed, it seems to me that this may be the only straightforward method to do so.

It's a bit of a tricky problem, and I acknowledge that there aren't a lot of good and easy ways to test it due to many cmdlets simply having an implicit assumption that piped objects will be enumerated and quietly handling those that don't, in an effort to make things like $a | Compare-Object -ReferenceObject $b and Compare-Object -ReferenceObject $a -DifferenceObject $b functionally equivalent.

[mklement0]

A pragmatic solution would be:

(Get-Variable var1 -ValueOnly | Measure-Object).Count | Should -Be 3

If the array contained in $var1 were unexpectedly sent to the pipeline as a single object, the result would be 1.

[iSazonov]

@mklement0 Thanks! It would nice test.

[nohwnd]

I also think that the approach that @mklement0 proposed is the easiest and most reliable. The reason for the assertion failure is the same. Internally Should -Be compares the count of items on both sides and fails when they differ.

Depending on your version of Pester you might consider adding a -Because that would better explain what is the reason for the failure is.

[PaulHigin]

LGTM

[stale]

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

[vexx32]

Pending committee decision.

[SteveL-MSFT]

@PowerShell/powershell-committee reviewed this. Agree that there is inconsistency, but there isn't clear value to taking this breaking change (likely breaking Pester usage which is highly impactful and may represent wider usage of existing behavior). Need to verify we have sufficient documentation on existing behavior.

[vexx32]

@SteveL-MSFT can you expand upon how that would break Pester?

[SteveL-MSFT]

@vexx32 looking at your example, we believed that Pester relies on current behavior. Correct us if that is not true.

[vexx32]

As far as I know (/cc @nohwnd as I'm sure he knows more than I where Pester's internals are concerned), Pester doesn't currently rely on Get-Variable -ValueOnly to compare items in any way.

Which example are you referring to? The example in the PR description doesn't have anything to do with Pester itself:

$a = 1, 2, 3, 4, 5
$a | Get-Member
Get-Variable -Name a -ValueOnly | Get-Member

These simply differ because with Get-Variable (and as far as I know, apart from ConvertFrom-Json where it is intentional due to JSON structure, it's the only cmdlet that does this) doesn't enumerate its output, so there's no difference between gv -val a | gm and gm -inp (gv -val a)

The initial Pester test that I used to verify the behaviour is this:

Get-Variable -Name var1 -ValueOnly | Should -Be $var1

And the reason this currently fails is simply because Should is expecting that a pipeline enumerates the output, so it deliberately assumes that any input that comes in implicitly has an already-discarded collection wrapper and matches things to the -Be provided parameter values accordingly.

The only way this breaks Pester itself would be if someone was using that exact test format and relying on the behaviour of Get-Variable -ValueOnly itself over the pipeline, which may certainly be a concern, but to my knowledge Pester doesn't use that cmdlet specifically in this way, so it should remain unaffected. (Were I to need this, after all, I would opt instead for Get-Variable without -ValueOnly, so that I could simply get the returned object and examine its .Value property, which is guaranteed to be true to life, pipeline or not.

If it did rely on that behaviour, I think I would expect the test to continue to fail once the change is made in the Get-Variable cmdlet, and we definitely saw the behaviour change as expected judging by the CI results and my own local tests.

I will of course respect the committee's decision, but I just want to make sure we're not retaining inconsistent behaviour without giving it a thorough look-see. 😄

[SteveL-MSFT]

@vexx32 thanks for the details. I think the overall conclusion from @PowerShell/powershell-committee is that we don't know how much impact there is with users relying on the current behavior. A simple search on GItHub turned up a number of scripts using Get-Variable -ValueOnly, but it's not easy to see how many use this in a pipeline (directly or indirectly). The Pester remark may have been incorrect, but wasn't the deciding factor. It's also not clear how much value this provides (other than consistency) to justify the breaking change.

[vexx32]

Roger that, I appreciate you taking the time to clarify! Personally, I think consistency is super valuable but I also recognise that it's... dicey... at best to change long-standing behaviour for its sole sake. 😄

[nohwnd]

@vexx32 you analysis is correct. (If Pester relied on that behavior we could work around it by adding a case that would change based on the version of PowerShell, there are few conditional fixes and features like this already, but I also respect the decision of the committee of course :) )

[vexx32]

@SteveL-MSFT Mostly for completeness' sake:

I'm not an expert with GH search, but from what I can see, it appears that Get-Variable -ValueOnly is almost never used in a pipeline context that would be affected, at least on Github (potentially because of this confusion?)

https://github.com/search?q=language%3APowerShell+%22Get-Variable+-ValueOnly+%7C%22&type=Code

Scrolling through these results, I see a few common patterns:

• (Get-Variable myVar -ValueOnly) | ... - not affected due to parentheses forcing collection of output and re-output anyway (see: (Get-Variable a -ValueOnly) | Should -Be $a)
• Get-Variable -Name <reserved scalar variable e.g., MyInvocation> -ValueOnly
• Use in non-pipeline contexts, or in pipeline contexts with scalar values (which are unaffected because there is no need to enumerate them on output to pipeline)

It seems that in a majority of cases I can find, people are using it to code defensively (safely getting values in strict mode, for example) and as such are well aware of the limitation; I note around one instance of (Get-Variable -Name myVar -ValueOnly) | Some-Command every couple of pages, and the only instances where it is used without parentheses seem to be ones where the variable has been recently explicitly declared as a scalar value.

That said, however, I don't think this is enough to go on. There are a fair few alias-forms of the command and parameter that can be used, and in general I would expect less carefully-coded forms of the command to crop up more frequently in a search for those. For now I think we can probably leave it as-is -- but we absolutely should document this oddity. 😄

[iSazonov]

Such fixes is good candidates for next LTS version if we'll produce LTS versions. We could mark such PRs with "Next-LTS" label.

[SteveL-MSFT]

Perhaps add this to #6745