Stop stream variables (e.g. OutVariable) becoming ArrayLists by rjmholt · Pull Request #6409 · PowerShell/PowerShell

rjmholt · 2018-03-16T03:07:29Z

PR Summary

Currently, when a value is passed out of a command using -OutVariable, we use an ArrayList to collect the value to give back to the user, but we leak that detail so that the variable comes back as an ArrayList and not object[] or as itself.

This PR fixes this, so that implementation differences between

$null = Write-Output -OutVariable x $value; $x

and

Write-Output $value

are transparent.

To summarize:

Input	Non-OutVar Result Type	Old Result Type	New Result Type
`'Hello'`	System.String	System.Collections.ArrayList	System.String
`@(1, 2)`	object[]	System.Collections.ArrayList	object[]
`[System.Collections.ArrayList]::new(@(1,2))`	object[]	System.Collections.ArrayList	object[]
`@(,[System.Collections.ArrayList]::new(@(1,2))`	System.Collections.ArrayList	System.Collections.ArrayList	System.Collections.ArrayList
`$null`	⊥	System.Collections.ArrayList	⊥

This is technically a breaking change. The PowerShell commitee reviewed it in #3154. A suggested classification was as a class-3 unlikely grey area change, where the new behaviour is more likely to be expected.

PR Checklist

iSazonov · 2018-03-16T03:46:33Z

src/System.Management.Automation/engine/MshCommandRuntime.cs

Could you please use more informative variable name then v?

iSazonov · 2018-03-16T03:50:01Z

src/System.Management.Automation/engine/MshCommandRuntime.cs

Seems return is not correct term in comments here and below.

Good point!

iSazonov · 2018-03-16T03:51:48Z

src/System.Management.Automation/engine/MshCommandRuntime.cs

Do we really need this assign? It seems is default.

If you take a case like

$x = Write-Output -OutVariable ov @()

Then $x is $null but without the assignment, $ov is an empty object[]. If our intent is for them to be the same, then I think we need the assignment to null here.

iSazonov · 2018-03-16T04:00:38Z

test/powershell/Language/Scripting/OutErrorVariable.Tests.ps1

Indentation. I believe it should be in 4th position.

iSazonov · 2018-03-16T04:01:35Z

test/powershell/Language/Scripting/OutErrorVariable.Tests.ps1

It is used once - please move directly in -TestCases.

Happy to do this. Is there an example of the code style you can link where $TestCases is inlined in the call?

iSazonov · 2018-03-16T04:02:22Z

test/powershell/Language/Scripting/OutErrorVariable.Tests.ps1

Indentation. I believe it should be 8th position.

rjmholt · 2018-03-16T16:33:52Z

Quick question - the [feature] checkbox - is that supposed to just be in the commit message and not in the title of the PR?

Jaykul · 2018-03-16T17:33:33Z

This would be a pretty major breaking change...

markekraus · 2018-03-16T17:41:54Z

Umm... I disagree that this is a Bucket 3 breaking change and request it be classified as Bucket 1. Especially if you are going to return a string as a String instead of an ArrayList.

I have been preaching for years as well as using the -OutVariable specifically because it always returns an ArrayList. Since it is always a predictable type you can always use the same methods and actions on it.

If Strings are suddenly returned... this becomes problematic:

$null = Get-MaybeOneOrMoreStrings -OutVariable Results
$FirstString = $Results[0]

This is a simplified example of something I make great use of in many places.

@SteveL-MSFT

markekraus · 2018-03-16T17:45:49Z

To clarify, I would be ok with these returning any collection type (changed in a major version bump). In order of preference List<Object>, ArrayList, ~~Object[]~~ (an object array can't be used, see my other comment.). But I am wholeheartedly against this returning single values and not a collection.

markekraus · 2018-03-16T18:06:33Z

This PR breaks the following usage:

$Results = [System.Collections.ArrayList]::new()
$Results.Add('before')
Write-Output 'a' -OutVariable '+Results'
$Results.Add('After')

$Results.Count
$Results[0]
$Results[1]
$Results[2]

Exception calling "Add" with "1" argument(s): "Collection was of a fixed size."
At line:1 char:1
+ $Results.Add('After')
+ ~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
+ FullyQualifiedErrorId : NotSupportedException

markekraus

Please have PowerShell Committee review this again. I believe this is a Bucket 1 breaking change and should go through RFC.

rjmholt · 2018-03-16T22:28:20Z

Apologies -- after reading through #3154, the responses there seemed to encourage the change.

Since I've already created a PR, I've gone ahead an created an RFC, so feel free to comment there.

I may also submit a PR to change the docs to emphasise and strengthen the language on RFCs for breaking changes. After going through the PR checklist, I didn't catch the reference to possibly submitting an RFC.

I will mothball this PR for now.

markekraus · 2018-03-16T23:11:26Z

@rjmholt No worries! if I had seen those other issues I would have raised a stink about it needing an RFC before the PR! 😆

iSazonov · 2018-03-19T04:40:47Z

Quick question - the [feature] checkbox - is that supposed to just be in the commit message and not in the title of the PR?

In last commit header only.

rjmholt requested review from BrucePay and daxian-dbw as code owners March 16, 2018 03:07

iSazonov added Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision labels Mar 16, 2018

iSazonov reviewed Mar 16, 2018

View reviewed changes

rjmholt changed the title ~~[Feature] Stop stream variables (e.g. OutVariable) becoming ArrayLists~~ Stop stream variables (e.g. OutVariable) becoming ArrayLists Mar 16, 2018

markekraus suggested changes Mar 16, 2018

View reviewed changes

SteveL-MSFT added Review - Committee The PR/Issue needs a review from the PowerShell Committee and removed Committee-Reviewed PS-Committee has reviewed this and made a decision labels Mar 16, 2018

[feature] Stop stream variables becoming ArrayLists

2ff078f

rjmholt force-pushed the out-variable-array-3154 branch from c534435 to 2ff078f Compare March 16, 2018 21:31

rjmholt mentioned this pull request Mar 16, 2018

OutVariable Transparency PowerShell/PowerShell-RFC#120

Closed

rjmholt closed this Mar 16, 2018

SteveL-MSFT removed the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Aug 16, 2018

Conversation

rjmholt commented Mar 16, 2018

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt commented Mar 16, 2018

Uh oh!

Jaykul commented Mar 16, 2018

Uh oh!

markekraus commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markekraus commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markekraus commented Mar 16, 2018

Uh oh!

markekraus left a comment

Choose a reason for hiding this comment

Uh oh!

rjmholt commented Mar 16, 2018

Uh oh!

markekraus commented Mar 16, 2018

Uh oh!

iSazonov commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

markekraus commented Mar 16, 2018 •

edited

Loading

markekraus commented Mar 16, 2018 •

edited

Loading

iSazonov commented Mar 19, 2018 •

edited

Loading