Skip to content

Fix array expression to not return null or throw error #4296

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.

Sign up for GitHub

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

Merged
daxian-dbw merged 8 commits into from
Jul 25, 2017
Merged

Fix array expression to not return null or throw error #4296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e987ef1
Fix array expression to not return null or throw error
daxian-dbw Jul 19, 2017
be0a266
Update tests
daxian-dbw Jul 19, 2017
50b55cd
Add '-Tags CI'
daxian-dbw Jul 19, 2017
bdbe68d
Update tests again
daxian-dbw Jul 19, 2017
33b5461
Address comment
daxian-dbw Jul 24, 2017
88a68ab
Add 2 more tests
daxian-dbw Jul 24, 2017
237c2bb
Change 2 tests
daxian-dbw Jul 25, 2017
ce4d2d9
Add a test about @([System.Collections.Generic.List[object]]$null)
daxian-dbw Jul 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions src/System.Management.Automation/engine/parser/Compiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5477,6 +5477,8 @@ public object VisitInvokeMemberExpression(InvokeMemberExpressionAst invokeMember
public object VisitArrayExpression(ArrayExpressionAst arrayExpressionAst)
{
Expression values = null;
ExpressionAst pureExprAst = null;

var subExpr = arrayExpressionAst.SubExpression;
if (subExpr.Traps == null)
{
Expand All @@ -5485,10 +5487,10 @@ public object VisitArrayExpression(ArrayExpressionAst arrayExpressionAst)
var pipelineBase = subExpr.Statements[0] as PipelineBaseAst;
if (pipelineBase != null)
{
var exprAst = pipelineBase.GetPureExpression();
if (exprAst != null)
pureExprAst = pipelineBase.GetPureExpression();
if (pureExprAst != null)
{
values = Compile(exprAst);
values = Compile(pureExprAst);
}
}
}
Expand All @@ -5500,16 +5502,12 @@ public object VisitArrayExpression(ArrayExpressionAst arrayExpressionAst)
}
values = values ?? CaptureAstResults(subExpr, CaptureAstContext.Enumerable);

if (values.Type.IsArray)
if (pureExprAst is ArrayLiteralAst)
{
// If the result is already an array, don't wrap the array.
// If the pure expression is ArrayLiteralAst, just return the result.
return values;
}
if (values.Type == typeof(List<object>))
{
return Expression.Call(values, CachedReflectionInfo.ObjectList_ToArray);
}
if (values.Type.GetTypeInfo().IsPrimitive || values.Type == typeof(string))
if (values.Type.IsPrimitive || values.Type == typeof(string))
{
// Slight optimization - no need for a dynamic site. We could special case other
// types as well, but it's probably not worth it.
Expand Down
75 changes: 75 additions & 0 deletions test/powershell/Language/Scripting/Array.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
Describe "ArrayExpression Tests" -Tags "CI" {
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
Copy link
Collaborator

@iSazonov iSazonov Jul 20, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@daxian-dbw daxian-dbw Jul 20, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@iSazonov iSazonov Jul 20, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@iSazonov iSazonov Jul 20, 2017

Choose a reason for hiding this comment

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

Can we use Should BeOfType?

Copy link
Member Author

@daxian-dbw daxian-dbw Jul 20, 2017

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator

@iSazonov iSazonov Jul 20, 2017

Choose a reason for hiding this comment

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

 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

Copy link
Member Author

@daxian-dbw daxian-dbw Jul 20, 2017

Choose a reason for hiding this comment

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

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

}

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

@iSazonov iSazonov Jul 20, 2017

Choose a reason for hiding this comment

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

Should we test complex types? PSCustomObject?

Copy link
Member Author

@daxian-dbw daxian-dbw Jul 20, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@iSazonov iSazonov Jul 20, 2017

Choose a reason for hiding this comment

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

Sorry, I mean member types.

Copy link
Member Author

@daxian-dbw daxian-dbw Jul 20, 2017

Choose a reason for hiding this comment

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

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

$result.GetType().FullName | Should Be "System.Object[]"
$result.Length | Should Be 3
}

It "@([object[]]`$null) should return a 1-element(`$null) array of object[]" {
$result = @([object[]]$null)
$result.GetType().FullName | Should Be "System.Object[]"
$result.Length | Should Be 1
$result[0] | Should Be $null
}

It "@([int[]]`$null) should return a 1-element(`$null) array of object[]" {
$result = @([int[]]$null)
$result.GetType().FullName | Should Be "System.Object[]"
$result.Length | Should Be 1
$result[0] | Should Be $null
}

It "@([object[]][System.Management.Automation.Internal.AutomationNull]::Value) should return a 1-element(`$null) array of object[]" {
$result = @([object[]][System.Management.Automation.Internal.AutomationNull]::Value)
$result.GetType().FullName | Should Be "System.Object[]"
$result.Length | Should Be 1
$result[0] | Should Be $null
}

It "@([int[]][System.Management.Automation.Internal.AutomationNull]::Value) should return a 1-element(`$null) array of object[]" {
$result = @([int[]][System.Management.Automation.Internal.AutomationNull]::Value)
$result.GetType().FullName | Should Be "System.Object[]"
$result.Length | Should Be 1
$result[0] | Should Be $null
}

It "@(`$null) should return a 1-element(`$null) array of object[]" {
$result = @($null)
$result.GetType().FullName | Should Be "System.Object[]"
$result.Length | Should Be 1
$result[0] | Should Be $null
}

It "@([System.Management.Automation.Internal.AutomationNull]::Value) should return an empty array of object[]" {
$result = @([System.Management.Automation.Internal.AutomationNull]::Value)
$result.GetType().FullName | Should Be "System.Object[]"
$result.Length | Should Be 0
}

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

It "@([int[]]`$a) should return a new array" {
$a = 1,2,3
$result = @([int[]]$a)
$result.GetType().FullName | Should Be "System.Object[]"
$result.Length | Should Be 3
}

It "@([System.Collections.Generic.List[object]]`$null) should return a 1-element(`$null) array of object[]" {
$result = @([System.Collections.Generic.List[object]]$null)
$result.GetType().FullName | Should Be "System.Object[]"
$result.Length | Should Be 1
$result[0] | Should Be $null
}
}