PowerShell · daxian-dbw · Nov 15, 2019 · Nov 11, 2019 · Nov 11, 2019 · Nov 11, 2019
@@ -3217,37 +3217,133 @@ private void CompileTrappableExpression(List<Expression> exprList, StatementAst
             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)
-                    {
-                        right = assignAst.Right;
-                        assignAst = right as AssignmentStatementAst;
-                    }
+                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);
 
-                    pipeAst = right as PipelineAst;
-                    if (right is CommandExpressionAst ||
-                        (pipeAst != null && pipeAst.PipelineElements.Count == 1 &&
-                         pipeAst.PipelineElements[0] is CommandExpressionAst))
+                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)
                     {
-                        // If the RHS of the assign was an expression,
-                        exprList.Add(s_setDollarQuestionToTrue);
+                        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;
                     }
-                }
+
+                default:
+                    return true;
             }
         }
 
@@ -3558,9 +3654,8 @@ public object VisitPipelineChain(PipelineChainAst pipelineChainAst)
         /// <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);
             }
 

@@ -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'