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

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.

iSazonov · 2017-07-20T03:33:17Z

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

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

iSazonov · 2017-07-20T03:41:58Z

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

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

iSazonov · 2017-07-20T17:48:31Z

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

lzybkr · 2017-07-24T21:35:30Z

src/System.Management.Automation/engine/parser/Compiler.cs

            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.

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.

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.

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

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

daxian-dbw self-assigned this Jul 19, 2017

daxian-dbw requested a review from lzybkr July 19, 2017 19:11

msftclas added the cla-not-required label Jul 19, 2017

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

Update tests again

bdbe68d

iSazonov reviewed Jul 20, 2017

View reviewed changes

lzybkr reviewed Jul 24, 2017

View reviewed changes

daxian-dbw added 2 commits July 24, 2017 15:56

Address comment

33b5461

Add 2 more tests

88a68ab

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

daxian-dbw merged commit 0d8eff6 into PowerShell:master Jul 25, 2017

daxian-dbw deleted the array branch July 25, 2017 04:52

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

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

Conversation

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!

iSazonov commented Jul 20, 2017

Uh oh!

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!

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!

lzybkr left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Jul 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

daxian-dbw commented Jul 19, 2017 •

edited

Loading

lzybkr Jul 24, 2017 •

edited

Loading

daxian-dbw Jul 24, 2017 •

edited

Loading