Fix -NoEnumerate behaviour in Write-Output by vexx32 · Pull Request #9069 · PowerShell/PowerShell · GitHub
Skip to content

Fix -NoEnumerate behaviour in Write-Output#9069

Merged
iSazonov merged 5 commits intoPowerShell:masterfrom
vexx32:WriteOutputNoEnumerate
Mar 13, 2019
Merged

Fix -NoEnumerate behaviour in Write-Output#9069
iSazonov merged 5 commits intoPowerShell:masterfrom
vexx32:WriteOutputNoEnumerate

Conversation

@vexx32
Copy link
Copy Markdown
Collaborator

@vexx32 vexx32 commented Mar 6, 2019

PR Summary

Write-Output's InputObject parameter was typed as PSObject[]. This was forcing all collections to be enumerated during parameter binding, making -NoEnumerate essentially useless; use of the switch would result in a PSObject[]-typed output collection instead of the usual object[], but it was completely impossible to retain the original collection(s) via the switch.

Fixes #5955, a long-standing regression from Windows PowerShell.

Adds regression test.

As I was digging through the underlying code before I came upon the true cause, I also came across a pair of methods with names that are just asking for misinterpretation. I renamed one of the pairs, the pair with only one reference each. As they are private methods, I doubt this will affect anything much, but it should make maintaining those code paths less confusing going forward. Moved to #9074.

PR Context

See issue #5955 for context.

PR Checklist

Comment thread src/System.Management.Automation/engine/MshCommandRuntime.cs Outdated
⏪ Moving commit to another PR

This reverts commit d30ffd6.
@vexx32
Copy link
Copy Markdown
Collaborator Author

vexx32 commented Mar 6, 2019

@vexx32 vexx32 marked this pull request as ready for review March 6, 2019 14:38
@iSazonov iSazonov added Review - Committee The PR/Issue needs a review from the PowerShell Committee Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log labels Mar 6, 2019
@SteveL-MSFT
Copy link
Copy Markdown
Member

@PowerShell/powershell-committee reviewed this. We agree that we should fix this to get back to the Windows PowerShell behavior which is the correct behavior.

@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 Mar 6, 2019
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Mar 7, 2019

@vexx32 Please open new issue in Docs repo if needed.

Copy link
Copy Markdown
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM

@iSazonov iSazonov self-assigned this Mar 8, 2019
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Mar 8, 2019

@mklement0 Could you please look the PR before we merge?

@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Mar 8, 2019

The parameter type of InputObject for Write-Output is psobject[] on Windows PowerShell,

PS:2> Get-Command Write-Output -Syntax

Write-Output [-InputObject] <psobject[]> [-NoEnumerate] [<CommonParameters>]

and it behaves as expected:

PS:3> (Write-Output -NoEnumerate 1, 2).GetType().Name
Object[]
PS:4> (Write-Output -NoEnumerate ([System.Collections.ArrayList] (1, 2))).GetType().Name
ArrayList

So I wonder what is the actual change that breaks it on PowerShell Core ...

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Mar 8, 2019

So I wonder what is the actual change that breaks it on PowerShell Core …

Perhaps #2035

@vexx32
Copy link
Copy Markdown
Collaborator Author

vexx32 commented Mar 8, 2019

@daxian-dbw Yeah, I can see where that's confusing... And knowing what PSObject is for more than I used to, I think it makes more sense behaving as it currently does... but I am very sure that sentiment wouldn't necessarily be shared by most PowerShell users.

@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Mar 8, 2019

Yes, the regression was caused by #2038. See another related issue: #5122
Unfortunately, due to #2038, Write-Output still doesn't behave exactly the same as on Windows PowerShell even with this PR:

# on windows powershell
(Write-Output -NoEnumerate 1).GetType().FullName
> System.Int32
# on powershell core with this PR
(Write-Output -NoEnumerate 1).GetType().FullName
> System.Collections.Generic.List`1[[System.Object, System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]

@vexx32
Copy link
Copy Markdown
Collaborator Author

vexx32 commented Mar 8, 2019

@daxian-dbw I shall do some digging to see where that occurs and what can be done about it.

@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Mar 8, 2019

@vexx32 After #2038, the remaining argument are handled in a similar way as params in C#:
if the remaining argument is a collection, then use that collection directly, otherwise, wrap the argument(s) to a List<object>.
I double there is anything we can do in Write-Output to make it behave the same as in Windows PowerShell in those particular cases :(

@vexx32
Copy link
Copy Markdown
Collaborator Author

vexx32 commented Mar 8, 2019

Hmm. I guess that makes some sense.

It does seem rather unlikely that folks would deliberately use -NoEnumerate and then not expect a collection to be the result. 🤔

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Mar 9, 2019

Make alternative internal attribute?

@PrzemyslawKlys
Copy link
Copy Markdown

Maybe instead of fixing Write-Output -NoEnumerate (which would be nice of course) behavior for [OutputType()] could be changed to keep output type untouched if it's defined. Right now even without [OutputType()] defined you still get unwrap for a single object.

@vexx32
Copy link
Copy Markdown
Collaborator Author

vexx32 commented Mar 11, 2019

Not sure I follow what you mean there. That attribute generally is metadata and more of a documentation attribute that anything else. As far as I know, it has no effect during execution.

Nor do I really see how it would help with this current situation, where -NoEnumerate is simply broken?

@PrzemyslawKlys
Copy link
Copy Markdown

I see. Well, I thought that it's more than metadata. And the only reason I would use NoEnumerate is to preserve Array on function return. If it has other uses, that's cool too.

What I mean is, if that change wouldn't go thru because of other reasons I would still want some other way to have that feature. If it goes thru, that's great.

@vexx32
Copy link
Copy Markdown
Collaborator Author

vexx32 commented Mar 11, 2019

@daxian-dbw daxian-dbw added this to the 6.2.0 milestone Mar 11, 2019
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM

@iSazonov iSazonov changed the title Write-Output: Fix -NoEnumerate Behaviour Fix -NoEnumerate behaviour in Write-Output Mar 13, 2019
@iSazonov iSazonov merged commit 18d5037 into PowerShell:master Mar 13, 2019
TravisEz13 pushed a commit that referenced this pull request Mar 13, 2019
Fix is to preserve input collection type in output.
The regression was caused by #2038
@vexx32 vexx32 deleted the WriteOutputNoEnumerate branch June 13, 2020 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write-Output -NoEnumerate outputs PSObject[] rather than Object[] and generally doesn't respect the input collection type

6 participants