GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix -NoEnumerate behaviour in Write-Output by vexx32 · Pull Request #9069 · PowerShell/PowerShell

Conversation

@vexx32

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) commented Mar 6, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

  • 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: 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.
  • 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
      • Add [feature] to your commit messages if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

daxian-dbw and ExE-Boss reacted with thumbs up emoji ExE-Boss reacted with hooray emoji
All reactions
  • 👍 2 reactions
  • 🎉 1 reaction
Rain Sallow (/u/ta11ow) (vexx32) added 3 commits March 5, 2019 23:54
@vexx32
♻️ Quick style pass for WriteOutputCommand
a7a0e8d
@vexx32
♻️ Clarify method names to avoid confusion
d30ffd6
@vexx32
🔧 Fix parameter typing for Write-Output that was causing the bug
252f883
iSazonov
Ilya (iSazonov) reviewed Mar 6, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/MshCommandRuntime.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vexx32
⏪ Revert "♻️ Clarify method names to avoid confusion"
7f3b38d
⏪ Moving commit to another PR

This reverts commit d30ffd6.
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Mar 6, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vexx32
✅ Added test to ensure -NoEnumerate works as advertised
5640af6
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) marked this pull request as ready for review March 6, 2019 14:38
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) requested review from James Truher (JamesWTruher) and Paul Higinbotham (PaulHigin) as code owners March 6, 2019 14:38
@iSazonov Ilya (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

Steve Lee (SteveL-MSFT) commented Mar 6, 2019

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.

vexx32 reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@SteveL-MSFT Steve Lee (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

Ilya (iSazonov) commented Mar 7, 2019

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please open new issue in Docs repo if needed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Mar 7, 2019
Write-Output -NoEnumerate does not work as advertised in PS 6.0-6.2 MicrosoftDocs/PowerShell-Docs#3815
Closed
2 of 8 tasks
PaulHigin
Paul Higinbotham (PaulHigin) approved these changes Mar 7, 2019
View reviewed changes

@PaulHigin Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vexx32 reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@iSazonov Ilya (iSazonov) self-assigned this Mar 8, 2019
@iSazonov

Ilya (iSazonov) commented Mar 8, 2019

Copy link
Copy Markdown
Collaborator

Michael Klement (@mklement0) Could you please look the PR before we merge?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daxian-dbw

Dongbo Wang (daxian-dbw) commented Mar 8, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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 ...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Mar 8, 2019

Copy link
Copy Markdown
Collaborator Author

Dongbo Wang (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daxian-dbw

Dongbo Wang (daxian-dbw) commented Mar 8, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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 and mwallner reacted with confused emoji
All reactions
  • 😕 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Mar 8, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

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

daxian-dbw reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daxian-dbw

Dongbo Wang (daxian-dbw) commented Mar 8, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Rain Sallow (/u/ta11ow) (@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 :(

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vexx32

Rain Sallow (/u/ta11ow) (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. 🤔

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented Mar 9, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

Make alternative internal attribute?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@PrzemyslawKlys

Przemysław Kłys (PrzemyslawKlys) commented Mar 11, 2019

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Mar 11, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@PrzemyslawKlys

Przemysław Kłys (PrzemyslawKlys) commented Mar 11, 2019

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vexx32

Rain Sallow (/u/ta11ow) (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.

mklement0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daxian-dbw Dongbo Wang (daxian-dbw) added this to the 6.2.0 milestone Mar 11, 2019
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Mar 12, 2019
View reviewed changes

@daxian-dbw Dongbo Wang (daxian-dbw) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@iSazonov Ilya (iSazonov) changed the title Write-Output: Fix -NoEnumerate Behaviour Fix -NoEnumerate behaviour in Write-Output Mar 13, 2019
@iSazonov
Ilya (iSazonov) merged commit 18d5037 into PowerShell:master Mar 13, 2019
Travis Plunk (TravisEz13) pushed a commit that referenced this pull request Mar 13, 2019
@vexx32 @TravisEz13
Fix -NoEnumerate behaviour in Write-Output (#9069)
ea4bf50
Fix is to preserve input collection type in output.
The regression was caused by #2038
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) deleted the WriteOutputNoEnumerate branch June 13, 2020 22:09
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@vexx32 @iSazonov
Fix -NoEnumerate behaviour in Write-Output (PowerShell#9069)
81aecc7
Fix is to preserve input collection type in output.
The regression was caused by PowerShell#2038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
+1 more reviewer
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (iSazonov)

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

Milestone

6.2.0

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

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@vexx32 @SteveL-MSFT @iSazonov @daxian-dbw @PrzemyslawKlys @PaulHigin
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.