Skip to content

Set $? correctly for command expression with redirections#16046

Merged
rjmholt merged 1 commit intoPowerShell:masterfrom
daxian-dbw:redirect
Sep 3, 2021
Merged

Set $? correctly for command expression with redirections#16046
rjmholt merged 1 commit intoPowerShell:masterfrom
daxian-dbw:redirect

Conversation

@daxian-dbw
Copy link
Copy Markdown
Member

PR Summary

Fix #16033

This PR makes a small update to ShouldSetExecutionStatusToSuccess(PipelineAst) to not exclude single expression with redirections, so that $? can be set to true properly for single-element pipeline that only contains a command expression with redirections.

PR Checklist

@rjmholt rjmholt merged commit 66b5d50 into PowerShell:master Sep 3, 2021
@daxian-dbw daxian-dbw deleted the redirect branch September 3, 2021 22:48
@vexx32
Copy link
Copy Markdown
Collaborator

vexx32 commented Sep 4, 2021

Do we already have regression tests verifying that the (command) vs command still behaves as expected?

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 4, 2021
@iSazonov iSazonov added this to the 7.2.0-rc.1 milestone Sep 4, 2021
@daxian-dbw
Copy link
Copy Markdown
Member Author

@iSazonov Thanks for adding the needed labels!
@vexx32 Are you concerned about the paren expression specifically? Can yo please provide a couple examples to show what you think could be regressed?

@vexx32
Copy link
Copy Markdown
Collaborator

vexx32 commented Sep 7, 2021

The change that (seemingly) caused this one to crop up was the change for (Invoke-Cmd) where Invoke-Cmd would set $? to false if it fails, but in parentheses it would fail to set it correctly.

So I guess a test case would be:

function fail {
    [CmdletBinding()]
    param()
    try { throw } catch { $PSCmdlet.WriteError($_) }
}

fail
$? | Should -BeFalse
(fail)
$? | Should -BeFalse

@adityapatwardhan
Copy link
Copy Markdown
Member

Backport rejected as this does not meet the bar. This will cause a change in behavior for 7.0.x and 7.1.x.

@daxian-dbw
Copy link
Copy Markdown
Member Author

@vexx32 It still behaves as expected, but I don't know if we have such test already. I can add one if not.

image

@ghost
Copy link
Copy Markdown

ghost commented Dec 16, 2021

🎉v7.3.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backport-7.2.x-Done CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

$? of redirection >, >>, 2>, and 2>> is unexpected.

5 participants