admin/PowerShell-PowerShell
1
0
Fork 0
mirror of https://github.com/PowerShell/PowerShell synced 2026-06-08 12:12:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

Stop blindly setting $? to true for ParenExpression, SubExpression and ArrayExpression (#11040)

Browse Source
This commit is contained in:
Robert Holt
2019-11-15 11:57:45 -08:00
committed by Dongbo Wang
parent fe40fbf6e1
commit ad12b14517
3 changed files with 302 additions and 112 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/System.Management.Automation/engine/parser/Compiler.cs
+122 -27
View File
@@ -3217,37 +3217,133 @@ namespace System.Management.Automation.Language
var expr = Compile(stmt);
exprList.Add(expr);
var pipeAst = stmt as PipelineAst;
if (pipeAst != null)
if (ShouldSetExecutionStatusToSuccess(stmt))
{
if (pipeAst.PipelineElements.Count == 1 && pipeAst.PipelineElements[0] is CommandExpressionAst)
{
// A single expression - must set $? after the expression.
exprList.Add(s_setDollarQuestionToTrue);
}
exprList.Add(s_setDollarQuestionToTrue);
}
else
}
/// <summary>
/// Determines whether a statement must have an explicit setting
/// for $? = $true after it by the compiler.
/// </summary>
/// <param name="statementAst">The statement to examine.</param>
/// <returns>True is the compiler should add the success setting, false otherwise.</returns>
private bool ShouldSetExecutionStatusToSuccess(StatementAst statementAst)
{
// Simple overload fan out
switch (statementAst)
{
var assignmentStatementAst = stmt as AssignmentStatementAst;
if (assignmentStatementAst != null)
{
Ast right = null;
var assignAst = assignmentStatementAst;
while (assignAst != null)
case PipelineAst pipelineAst:
return ShouldSetExecutionStatusToSuccess(pipelineAst);
case AssignmentStatementAst assignmentStatementAst:
return ShouldSetExecutionStatusToSuccess(assignmentStatementAst);
default:
return false;
}
}
/// <summary>
/// Determines whether a pipeline must have an explicit setting
/// for $? = $true after it by the compiler.
/// </summary>
/// <param name="pipelineAst">The pipeline to examine.</param>
/// <returns>True is the compiler should add the success setting, false otherwise.</returns>
private bool ShouldSetExecutionStatusToSuccess(PipelineAst pipelineAst)
{
ExpressionAst expressionAst = pipelineAst.GetPureExpression();
// If the pipeline is not a simple expression, it will set $?
if (expressionAst == null)
{
return false;
}
// Expressions may still set $? themselves, so dig deeper
return ShouldSetExecutionStatusToSuccess(expressionAst);
}
/// <summary>
/// Determines whether an assignment statement must have an explicit setting
/// for $? = $true after it by the compiler.
/// </summary>
/// <param name="assignmentStatementAst">The assignment statement to examine.</param>
/// <returns>True is the compiler should add the success setting, false otherwise.</returns>
private bool ShouldSetExecutionStatusToSuccess(AssignmentStatementAst assignmentStatementAst)
{
// Get right-most RHS in cases like $x = $y = <expr>
StatementAst innerRhsStatementAst = assignmentStatementAst.Right;
while (innerRhsStatementAst is AssignmentStatementAst rhsAssignmentAst)
{
innerRhsStatementAst = rhsAssignmentAst.Right;
}
// Simple assignments to pure expressions may need $? set, so examine the RHS statement for pure expressions
switch (innerRhsStatementAst)
{
case CommandExpressionAst commandExpression:
return ShouldSetExecutionStatusToSuccess(commandExpression.Expression);
case PipelineAst rhsPipelineAst:
return ShouldSetExecutionStatusToSuccess(rhsPipelineAst);
default:
return false;
}
}
/// <summary>
/// Determines whether an expression in a statement must have an explicit setting
/// for $? = $true after it by the compiler.
/// </summary>
/// <param name="expressionAst">The expression to examine.</param>
/// <returns>True is the compiler should add the success setting, false otherwise.</returns>
private bool ShouldSetExecutionStatusToSuccess(ExpressionAst expressionAst)
{
switch (expressionAst)
{
case ParenExpressionAst parenExpression:
// Pipelines in paren expressions that are just pure expressions will need $? set
// e.g. ("Hi"), vs (Test-Path ./here.txt)
return ShouldSetExecutionStatusToSuccess(parenExpression.Pipeline);
case SubExpressionAst subExpressionAst:
// Subexpressions generally set $? since they encapsulate a statement block
// But $() requires an explicit setting
return subExpressionAst.SubExpression.Statements.Count == 0;
case ArrayExpressionAst arrayExpressionAst:
// ArrayExpressionAsts and SubExpressionAsts must be treated differently,
// since they are optimised for a single expression differently.
// A SubExpressionAst with a single expression in it has the $? = $true added,
// but the optimisation drills deeper for ArrayExpressionAsts,
// meaning we must inspect the expression itself in these cases
switch (arrayExpressionAst.SubExpression.Statements.Count)
{
right = assignAst.Right;
assignAst = right as AssignmentStatementAst;
case 0:
// @() needs $? set
return true;
case 1:
// Single expressions with a trap are handled as statements
// For example: @(trap { continue } "Value")
if (arrayExpressionAst.SubExpression.Traps != null)
{
return false;
}
// Pure, single statement expressions need $? set
// For example @("One") and @("One", "Two")
return ShouldSetExecutionStatusToSuccess(arrayExpressionAst.SubExpression.Statements[0]);
default:
// Arrays with multiple statements in them will have $? set
return false;
}
pipeAst = right as PipelineAst;
if (right is CommandExpressionAst ||
(pipeAst != null && pipeAst.PipelineElements.Count == 1 &&
pipeAst.PipelineElements[0] is CommandExpressionAst))
{
// If the RHS of the assign was an expression,
exprList.Add(s_setDollarQuestionToTrue);
}
}
default:
return true;
}
}
@@ -3558,9 +3654,8 @@ namespace System.Management.Automation.Language
/// <returns>The compiled expression to execute the pipeline.</returns>
private Expression CompilePipelineChainElement(PipelineAst pipelineAst)
{
if (pipelineAst.PipelineElements.Count == 1 && pipelineAst.PipelineElements[0] is CommandExpressionAst)
if (ShouldSetExecutionStatusToSuccess(pipelineAst))
{
// A single expression - must set $? after the expression.
return Expression.Block(Compile(pipelineAst), s_setDollarQuestionToTrue);
}
test/powershell/Language/Operators/PipelineChainOperator.Tests.ps1
+92 -85
View File
@@ -89,6 +89,13 @@ Describe "Experimental Feature: && and || operators - Feature-Enabled" -Tag CI {
@{ Statement = '0,1 | % { testexe -returncode $_ } && testexe -returncode 0'; Output = @('0','1','0') }
@{ Statement = '1,2 | % { testexe -returncode $_ } && testexe -returncode 0'; Output = @('1','2','0') }
# Subpipeline and subexpression cases
@{ Statement = '(testexe -returncode 0 && testexe -returncode 1) && "Hi"'; Output = @('0','1') }
@{ Statement = '$(testexe -returncode 0 && testexe -returncode 1) && "Hi"'; Output = @('0','1') }
@{ Statement = '(testexe -returncode 0 && testexe -returncode 1) || "Bad"'; Output = @('0','1','Bad') }
@{ Statement = '$(testexe -returncode 0 && testexe -returncode 1) && "Bad"'; Output = @('0','1') }
@{ Statement = '(testexe -returncode 1 || testexe -returncode 1) && "Hi"'; Output = @('1','1') }
# Control flow statements
@{ Statement = 'foreach ($v in 0,1,2) { testexe -returncode $v || $(break) }'; Output = @('0', '1') }
@{ Statement = 'foreach ($v in 0,1,2) { testexe -returncode $v || $(continue); $v + 1 }'; Output = @('0', 1, '1', '2') }
@@ -134,6 +141,91 @@ Describe "Experimental Feature: && and || operators - Feature-Enabled" -Tag CI {
}
}
It "Gets the correct output with statement '<Statement>'" -TestCases $simpleTestCases {
param($Statement, $Output)
$result = Invoke-Expression -Command $Statement 2>$null
$result | Should -Be $Output
}
It "Sets the variable correctly with statement '<Statement>'" -TestCases $variableTestCases {
param($Statement, $Variables)
Invoke-Expression -Command $Statement
foreach ($variableName in $Variables.get_Keys())
{
(Get-Variable -Name $variableName -ErrorAction Ignore).Value | Should -Be $Variables[$variableName] -Because "variable is '`$$variableName'"
}
}
It "Runs the statement chain '<Statement>' as a job" -TestCases $jobTestCases {
param($Statement, $Output, $Variable)
$resultJob = Invoke-Expression -Command $Statement
if ($Variable)
{
$resultJob = (Get-Variable $Variable).Value
}
$resultJob | Wait-Job | Receive-Job | Should -Be $Output
}
It "Rejects invalid syntax usage in '<Statement>'" -TestCases $invalidSyntaxCases {
param([string]$Statement, [string]$ErrorID, [bool]$IncompleteInput)
$tokens = $errors = $null
[System.Management.Automation.Language.Parser]::ParseInput($Statement, [ref]$tokens, [ref]$errors)
$errors.Count | Should -BeExactly 1
$errors[0].ErrorId | Should -BeExactly $ErrorID
$errors[0].IncompleteInput | Should -Be $IncompleteInput
}
Context "File redirection with && and ||" {
BeforeAll {
$redirectionTestCases = @(
@{ Statement = "testexe -returncode 0 > '$TestDrive/1.txt' && testexe -returncode 1 > '$TestDrive/2.txt'"; Files = @{ "$TestDrive/1.txt" = '0'; "$TestDrive/2.txt" = '1' } }
@{ Statement = "testexe -returncode 1 > '$TestDrive/1.txt' && testexe -returncode 1 > '$TestDrive/2.txt'"; Files = @{ "$TestDrive/1.txt" = '1'; "$TestDrive/2.txt" = $null } }
@{ Statement = "testexe -returncode 1 > '$TestDrive/1.txt' || testexe -returncode 1 > '$TestDrive/2.txt'"; Files = @{ "$TestDrive/1.txt" = '1'; "$TestDrive/2.txt" = '1' } }
@{ Statement = "testexe -returncode 0 > '$TestDrive/1.txt' || testexe -returncode 1 > '$TestDrive/2.txt'"; Files = @{ "$TestDrive/1.txt" = '0'; "$TestDrive/2.txt" = $null } }
@{ Statement = "(testexe -returncode 0 && testexe -returncode 1) > '$TestDrive/3.txt'"; Files = @{ "$TestDrive/3.txt" = "0$([System.Environment]::NewLine)1$([System.Environment]::NewLine)" } }
@{ Statement = "(testexe -returncode 0 && testexe -returncode 1 > '$TestDrive/2.txt') > '$TestDrive/3.txt'"; Files = @{ "$TestDrive/2.txt" = '1'; "$TestDrive/3.txt" = '0' } }
@{ Statement = "(testexe -returncode 0 > '$TestDrive/1.txt' && testexe -returncode 1 > '$TestDrive/2.txt') > '$TestDrive/3.txt'"; Files = @{ "$TestDrive/1.txt" = '0'; "$TestDrive/2.txt" = '1'; "$TestDrive/3.txt" = '' } }
)
}
BeforeEach {
Remove-Item -Path $TestDrive/*
}
It "Handles redirection correctly with statement '<Statement>'" -TestCases $redirectionTestCases {
param($Statement, $Files)
Invoke-Expression -Command $Statement
foreach ($file in $Files.get_Keys())
{
$expectedValue = $Files[$file]
if ($null -eq $expectedValue)
{
$file | Should -Not -Exist
continue
}
# Special case for empty file
if ($expectedValue -eq '')
{
(Get-Item $file).Length | Should -Be 0
continue
}
$file | Should -FileContentMatchMultiline $expectedValue
}
}
}
Context "Pipeline chain error semantics" {
BeforeAll {
$pwsh = [powershell]::Create()
@@ -261,91 +353,6 @@ function Test-FullyTerminatingError
}
}
It "Gets the correct output with statement '<Statement>'" -TestCases $simpleTestCases {
param($Statement, $Output)
Invoke-Expression -Command $Statement 2>$null | Should -Be $Output
}
It "Sets the variable correctly with statement '<Statement>'" -TestCases $variableTestCases {
param($Statement, $Variables)
Invoke-Expression -Command $Statement
foreach ($variableName in $Variables.get_Keys())
{
(Get-Variable -Name $variableName -ErrorAction Ignore).Value | Should -Be $Variables[$variableName] -Because "variable is '`$$variableName'"
}
}
It "Runs the statement chain '<Statement>' as a job" -TestCases $jobTestCases {
param($Statement, $Output, $Variable)
$resultJob = Invoke-Expression -Command $Statement
if ($Variable)
{
$resultJob = (Get-Variable $Variable).Value
}
$resultJob | Wait-Job | Receive-Job | Should -Be $Output
}
It "Rejects invalid syntax usage in '<Statement>'" -TestCases $invalidSyntaxCases {
param([string]$Statement, [string]$ErrorID, [bool]$IncompleteInput)
$tokens = $errors = $null
[System.Management.Automation.Language.Parser]::ParseInput($Statement, [ref]$tokens, [ref]$errors)
$errors.Count | Should -BeExactly 1
$errors[0].ErrorId | Should -BeExactly $ErrorID
$errors[0].IncompleteInput | Should -Be $IncompleteInput
}
Context "File redirection with && and ||" {
BeforeAll {
$redirectionTestCases = @(
@{ Statement = "testexe -returncode 0 > '$TestDrive/1.txt' && testexe -returncode 1 > '$TestDrive/2.txt'"; Files = @{ "$TestDrive/1.txt" = '0'; "$TestDrive/2.txt" = '1' } }
@{ Statement = "testexe -returncode 1 > '$TestDrive/1.txt' && testexe -returncode 1 > '$TestDrive/2.txt'"; Files = @{ "$TestDrive/1.txt" = '1'; "$TestDrive/2.txt" = $null } }
@{ Statement = "testexe -returncode 1 > '$TestDrive/1.txt' || testexe -returncode 1 > '$TestDrive/2.txt'"; Files = @{ "$TestDrive/1.txt" = '1'; "$TestDrive/2.txt" = '1' } }
@{ Statement = "testexe -returncode 0 > '$TestDrive/1.txt' || testexe -returncode 1 > '$TestDrive/2.txt'"; Files = @{ "$TestDrive/1.txt" = '0'; "$TestDrive/2.txt" = $null } }
@{ Statement = "(testexe -returncode 0 && testexe -returncode 1) > '$TestDrive/3.txt'"; Files = @{ "$TestDrive/3.txt" = "0$([System.Environment]::NewLine)1$([System.Environment]::NewLine)" } }
@{ Statement = "(testexe -returncode 0 && testexe -returncode 1 > '$TestDrive/2.txt') > '$TestDrive/3.txt'"; Files = @{ "$TestDrive/2.txt" = '1'; "$TestDrive/3.txt" = '0' } }
@{ Statement = "(testexe -returncode 0 > '$TestDrive/1.txt' && testexe -returncode 1 > '$TestDrive/2.txt') > '$TestDrive/3.txt'"; Files = @{ "$TestDrive/1.txt" = '0'; "$TestDrive/2.txt" = '1'; "$TestDrive/3.txt" = '' } }
)
}
BeforeEach {
Remove-Item -Path $TestDrive/*
}
It "Handles redirection correctly with statement '<Statement>'" -TestCases $redirectionTestCases {
param($Statement, $Files)
Invoke-Expression -Command $Statement
foreach ($file in $Files.get_Keys())
{
$expectedValue = $Files[$file]
if ($null -eq $expectedValue)
{
$file | Should -Not -Exist
continue
}
# Special case for empty file
if ($expectedValue -eq '')
{
(Get-Item $file).Length | Should -Be 0
continue
}
$file | Should -FileContentMatchMultiline $expectedValue
}
}
}
It "Recognises invalid assignment" {
{
Invoke-Expression -Command '$x = $x, $y += $z = testexe -returncode 0 && testexe -returncode 1'
test/powershell/Language/Scripting/DollarHook.Tests.ps1
+88
View File
@@ -0,0 +1,88 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.
Describe 'Tests for setting $? for execution success' -Tag 'CI' {
BeforeAll {
$script:hookValues = [System.Collections.Generic.List[bool]]::new()
function Hook
{
param([Parameter(Position = 0)][bool]$Value)
$script:hookValues.Add($Value)
}
}
AfterEach {
$script:hookValues.Clear()
}
It 'Sets $? for case ''<Script>''' -TestCases @(
# Cmdlets
@{ Script = 'Write-Output "Hi"; Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; Hook $?'; Results = @($false) }
@{ Script = 'Write-Error "Bad"; Hook $?; Hook $?'; Results = @($false, $true) }
# Native executables
@{ Script = 'testexe -returncode 0; Hook $?'; Results = @($true) }
@{ Script = 'testexe -returncode 1; Hook $?; Hook $?'; Results = @($false, $true) }
# Pure values
@{ Script = '"Hi"; Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; "Hi"; Hook $?'; Results = @($true) }
# Paren expressions (subpipelines)
@{ Script = '(Write-Output "Hi"); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; ("Hi"); Hook $?'; Results = @($true) }
@{ Script = '(Write-Error "Bad"); Hook $?; Hook $?'; Results = @($false, $true) }
@{ Script = '(testexe -returncode 0); Hook $?'; Results = @($true) }
@{ Script = '(testexe -returncode 1); Hook $?; Hook $?'; Results = @($false, $true) }
# Subexpressions
@{ Script = '$("Hi"); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; $("Hi"); Hook $?'; Results = @($true) }
@{ Script = '$(); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; $(); Hook $?'; Results = @($true) }
@{ Script = '$(Write-Error "Bad"); Hook $?; Hook $?'; Results = @($false, $true) }
@{ Script = '$(testexe -returncode 1); Hook $?; Hook $?'; Results = @($false, $true) }
@{ Script = 'Write-Error "Bad"; $(trap { continue }); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; $(trap { continue } "Hi"); Hook $?'; Results = @($true) }
# Array expressions
@{ Script = '@("Hi"); Hook $?'; Results = @($true) }
@{ Script = '@(); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; @("Hi"); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; @(); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; @("Hi", "There"); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; @("Hi"; "There"); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; @(trap { continue }); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; @(trap { continue } "Hi"); Hook $?'; Results = @($true) }
@{ Script = 'Write-Error "Bad"; @(trap { continue } "Hi", "There"); Hook $?'; Results = @($true) }
@{ Script = '@(Write-Error "Bad"); Hook $?; Hook $?'; Results = @($false, $true) }
@{ Script = '@(Write-Error "Bad"; "Hi"); Hook $?; Hook $?'; Results = @($true, $true) }
@{ Script = '@("Hi"; Write-Error "Bad"); Hook $?; Hook $?'; Results = @($false, $true) }
@{ Script = '@(Write-Error "Bad"; Write-Output "Hi"); Hook $?; Hook $?'; Results = @($true, $true) }
@{ Script = '@(Write-Output "Hi"; Write-Error "Bad"); Hook $?; Hook $?'; Results = @($false, $true) }
) {
param([string]$Script, [object[]]$Results)
Invoke-Expression $Script 2>&1 > $null
$script:hookValues | Should -Be $Results
}
It 'Sets $? correctly for string expression ''<Expression>''' -TestCases @(
@{ Expression = '"Hi"; Hook $?'; HookResults = $($true); PipelineResults = @('Hi') }
@{ Expression = '"Hi $("Good")"; Hook $?'; HookResults = $($true); PipelineResults = @('Hi Good') }
@{ Expression = '"Hi $(Write-Error "Bad")"; Hook $?'; HookResults = $($true); PipelineResults = @('Hi ') }
@{ Expression = '"Hi $(Write-Output "Good")"; Hook $?'; HookResults = $($true); PipelineResults = @('Hi Good') }
) {
param([string]$Expression, [object[]]$HookResults, [object[]]$PipelineResults)
$output = Invoke-Expression $Expression 2>$null
$script:hookValues | Should -Be $HookResults
$output | Should -Be $PipelineResults
}
}