Fix array expression to not return null or throw error by daxian-dbw · Pull Request #4296 · PowerShell/PowerShell

Dongbo Wang (daxian-dbw) · 2017-07-19T19:11:01Z

Summary

This PR focuses on 3 issues:

According to PowerShell Language Specification Version 3.0, as quoted: "The result is the (possibly empty) unconstrained 1-dimensional array", @(...) should only return object[] array.
@([object[]]$null).GetType() fails with error "You cannot call a method on a null-valued expression."
@([System.Collections.Generic.List[object]]$null) fails with error "Object reference not set to an instance of an object."

Marked with label Review-Committee to review the first issue.

Ilya (iSazonov) · 2017-07-20T03:33:17Z

+    It "@([object[]](1,2,3)) should return a 3-element array of object[]" {
+        $result = @([object[]](1,2,3))
+        $result.GetType().FullName | Should Be "System.Object[]"
+        $result.Length | Should Be 3


I believe we should check $result.Length before $result.GetType() in all test here. This is not important in these tests, but it is a bad pattern for other tests in which the $result are calculated. (I mean that somebody can copy-paste the bad pattern).

What do you mean by the $result are calculated? Can you please elaborate a bit and give an example?

From Get-Content.Tests.ps1:

It "should Get-Content with a variety of -Tail and -ReadCount values" {#[DRT] set-content -path $testPath "Hello,World","Hello2,World2","Hello3,World3","Hello4,World4" $result=get-content -path $testPath -readcount:-1 -tail 5 $result.Length | Should Be 4

Can we use Should BeOfType?

To use Should BeOfType I need to wrap $result in a new array because it will be unraveled otherwise. I feel it's clearer to just use GetType().FullName in this case.

As for the bad pattern, I'm still confused. Why do you think putting $result.Length | Should Be 3 after the type check would be a bad pattern? More technically speaking, the length check should happen after we know $result is an array, otherwise we might (actually impossible) end up checking the Length property of some random type :)

function foo {$null} $res = foo $res.GetType() You cannot call a method on a null-valued expression. At line:1 char:1 + $res.GetType() + ~~~~~~~~~~~~~~ + CategoryInfo : InvalidOperation: (:) [], RuntimeException + FullyQualifiedErrorId : InvokeMethodOnNull

Ah, I see your point. A Should Not BeNullOrEmpty check should be done in that case.

Ilya (iSazonov) · 2017-07-20T03:41:58Z

+    }
+
+    It "@([int[]](1,2,3)) should return a 3-element array of object[]" {
+        $result = @([int[]](1,2,3))


Should we test complex types? PSCustomObject?

For any array type other than [object[]], it will fall in the code path of PSToObjectArrayBinder. I just use [int[]] as a simple validation.

Sorry, I mean member types.

Oh, the member type doesn't matter for the scenario of this fix. It's OK as long as the conversion works.

Ilya (iSazonov) · 2017-07-20T17:48:31Z

Should we add tests for @($var) ? It seems absent.

Jason Shirk (lzybkr) · 2017-07-24T21:35:30Z

            values = values ?? CaptureAstResults(subExpr, CaptureAstContext.Enumerable);

-            if (values.Type.IsArray)
+            if (values.Type == typeof(object[]) || values.Type == typeof(List<object>))


The goal of the optimization was to cover @(1,2) - so maybe the original test was too general. This fix ends up adding unnecessary overhead to the optimization by cloning and adding an if test.

I think a better fix is to just constrain the optimization more - basically if (subExpr is ArrayLiteralAst).

I don't recall why I added the List<object> case.

The clone was added to address a comment raised by PetSerAl about $a = 1,2,3; @([object[]]$a) doesn't return a new array. But if we only consider if (pureExpr is ArrayLiteralAst) then the clone and the null check are both not necessary anymore.

The List<object> case seems like an optimization for CaptureAstResults, because a List<object> instance will be created to hold the objects written to pipe in CaptureAstResults.
Are you suggesting to not cover the cases of values.Type == typeof(object[]) and values.Type == typeof(List<object>) here?

Replace the cases of values.Type.IsArray and values.Type == typeof(List<object>) with if (pureExprAst is ArrayLiteralAst). So the former cases will be handled by the dynamic site.

Jason Shirk (lzybkr)

I think the change is fine.

I should add that the intent of @() is not to create a new array, but to ensure the result is an array, and not necessarily an object[].

In other words, I think we could be open to changing what the language specification says to allow for better performance.

Dongbo Wang (daxian-dbw) · 2017-07-25T03:18:35Z

the intent of @() is not to create a new array, but to ensure the result is an array

The last commit removed the [object]::ReferenceEquals check from the test per Jason's comment.
But the check for System.Object[] is kept as we haven't changed the language specification yet.

This change fixes 3 issues: - According to [PowerShell Language Specification Version 3.0](https://www.microsoft.com/en-us/download/details.aspx?id=36389), as quoted: "_The result is the (possibly empty) unconstrained 1-dimensional array_", `@(...)` should only return `object[]` array. - `@([object[]]$null).GetType()` fails with error `"You cannot call a method on a null-valued expression."` - `@([System.Collections.Generic.List[object]]$null)` fails with error `"Object reference not set to an instance of an object."`

Dongbo Wang (daxian-dbw) added 3 commits July 19, 2017 11:42

Fix array expression to not return null or throw error

e987ef1

Update tests

be0a266

Add '-Tags CI'

50b55cd

Dongbo Wang (daxian-dbw) added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jul 19, 2017

Dongbo Wang (daxian-dbw) self-assigned this Jul 19, 2017

Dongbo Wang (daxian-dbw) requested a review from Jason Shirk (lzybkr) July 19, 2017 19:11

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jul 19, 2017

Dongbo Wang (daxian-dbw) changed the title ~~Array~~ Fix array expression to not return null or throw error Jul 19, 2017

Update tests again

bdbe68d

Ilya (iSazonov) reviewed Jul 20, 2017

View reviewed changes

Jason Shirk (lzybkr) reviewed Jul 24, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) added 2 commits July 24, 2017 15:56

Address comment

33b5461

Add 2 more tests

88a68ab

Jason Shirk (lzybkr) approved these changes Jul 25, 2017

View reviewed changes

Change 2 tests

237c2bb

Add a test about @([System.Collections.Generic.List[object]]$null)

ce4d2d9

Dongbo Wang (daxian-dbw) merged commit 0d8eff6 into PowerShell:master Jul 25, 2017

Dongbo Wang (daxian-dbw) deleted the array branch July 25, 2017 04:52

Dongbo Wang (daxian-dbw) mentioned this pull request Aug 4, 2017

@() (array-subexpression operator) should not be optimized away when a cast to an array type is involved #4280

Closed

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

liushuang (shuangxue520) mentioned this pull request Jul 12, 2026

Array subexpression @($list) throws "Argument types do not match" for List[object] created by New-Object #27558

Open

Uh oh!

Conversation

Dongbo Wang (daxian-dbw) commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Jul 20, 2017

Uh oh!

Jason Shirk (lzybkr) Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jason Shirk (lzybkr) left a comment

Choose a reason for hiding this comment

Uh oh!

Dongbo Wang (daxian-dbw) commented Jul 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dongbo Wang (daxian-dbw) commented Jul 19, 2017 •

edited

Loading

Jason Shirk (lzybkr) Jul 24, 2017 •

edited

Loading

Dongbo Wang (daxian-dbw) Jul 24, 2017 •

edited

Loading