New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix -NoEnumerate behaviour in Write-Output #9069
Conversation
⏪ Moving commit to another PR This reverts commit d30ffd6.
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. |
@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 Please open new issue in Docs repo if needed. |
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
@mklement0 Could you please look the PR before we merge? |
The parameter type of
and it behaves as expected:
So I wonder what is the actual change that breaks it on PowerShell Core ... |
Perhaps #2035 |
@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. |
Yes, the regression was caused by #2038. See another related issue: #5122
|
@daxian-dbw I shall do some digging to see where that occurs and what can be done about it. |
@vexx32 After #2038, the remaining argument are handled in a similar way as |
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. 🤔 |
Make alternative internal attribute? |
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. |
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? |
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. |
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. |
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
Fix is to preserve input collection type in output. The regression was caused by #2038
PR Summary
Write-Output's
InputObject
parameter was typed asPSObject[]
. This was forcing all collections to be enumerated during parameter binding, making-NoEnumerate
essentially useless; use of the switch would result in aPSObject[]
-typed output collection instead of the usualobject[]
, 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
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.[feature]
to your commit messages if the change is significant or affects feature tests