Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix -NoEnumerate behaviour in Write-Output#9069

Merged
iSazonov merged 5 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
vexx32:WriteOutputNoEnumeratevexx32/PowerShell:WriteOutputNoEnumerateCopy head branch name to clipboard
Mar 13, 2019
Merged

Fix -NoEnumerate behaviour in Write-Output#9069
iSazonov merged 5 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
vexx32:WriteOutputNoEnumeratevexx32/PowerShell:WriteOutputNoEnumerateCopy head branch name to clipboard

Conversation

@vexx32

@vexx32 vexx32 commented Mar 6, 2019

Copy link
Copy Markdown
Collaborator

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

vexx32 commented Mar 6, 2019

Copy link
Copy Markdown
Collaborator Author

Reverted the commit. I'll ignore the CodeFactor items for MshCommandRuntime.cs -- most of them look to be the usual Hungarian notation ones that we plan to ignore going forward anyway. 😄

Moved that change to #9074.

@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

iSazonov commented Mar 7, 2019

Copy link
Copy Markdown
Collaborator

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

@PaulHigin PaulHigin left a comment

Copy link
Copy Markdown
Contributor

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

iSazonov commented Mar 8, 2019

Copy link
Copy Markdown
Collaborator

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

@daxian-dbw

daxian-dbw commented Mar 8, 2019

Copy link
Copy Markdown
Member

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

iSazonov commented Mar 8, 2019

Copy link
Copy Markdown
Collaborator

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

Perhaps #2035

@vexx32

vexx32 commented Mar 8, 2019

Copy link
Copy Markdown
Collaborator Author

@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

daxian-dbw commented Mar 8, 2019

Copy link
Copy Markdown
Member

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

vexx32 commented Mar 8, 2019

Copy link
Copy Markdown
Collaborator Author

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

@daxian-dbw

daxian-dbw commented Mar 8, 2019

Copy link
Copy Markdown
Member

@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

vexx32 commented Mar 8, 2019

Copy link
Copy Markdown
Collaborator Author

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

iSazonov commented Mar 9, 2019

Copy link
Copy Markdown
Collaborator

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

vexx32 commented Mar 11, 2019

Copy link
Copy Markdown
Collaborator Author

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

vexx32 commented Mar 11, 2019

Copy link
Copy Markdown
Collaborator Author

Gotcha. Yeah, that might work on a surface level. Unfortunately it would probably mean enumerating the collection twice (once in the parameter binder -> PSObject[], and once before output -> OutputType) which is a bit much imo.

It would also not be retaining the original collection, so if it originally had NoteProperty or other ETS members, they would be stripped. In the few cases where -NoEnumerate are needed, this might be one motivation for using it.

@daxian-dbw daxian-dbw added this to the 6.2.0 milestone Mar 11, 2019

@daxian-dbw daxian-dbw left a comment

Copy link
Copy Markdown
Member

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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.