Stop setting $? to true for subpipelines and subexpressions by rjmholt · Pull Request #11040 · PowerShell/PowerShell

rjmholt · 2019-11-11T22:48:56Z

PR Summary

This PR alters the way we compile subpipelines (...), subexpressions $(...) and array expressions @() so that $? is not automatically true after them in the pipeline, but instead depends on the pipeline or statements they executed.

I've added tests for the various behaviours of this, plus some extra tests for pipeline chain operators, which this aids the UX of.

I've structured my changes by commit, so you'll probably find it easier to read this PR from commit to commit.

PR Context

In #9849, the && and || operators were introduced, but currently suffer from the implementation of $?. Specifically:

Write-Error "Bad" || Write-Output "Hi" # Outputs "Hi"

(Write-Error "Bad") || Write-Output "Hi" # Outputs only the error

This makes pipeline chain operators hard to use insofar as they can't be rearranged conveniently by means of parentheses.

In this PR, we change the implementation so that expression-only pipelines only have $? forcibly set when they are a pure value, expressions that actually execute their commands with $? set by the subpipeline.

Further considerations

This PR likely requires an RFC, which I can begin authoring immediately and will link back here
Calling Write-Error from within another function still has that function leaving $? as true. I looked into changing that behaviour briefly and it would be fairly complicated.
This PR doesn't try to expose new mechanisms for cmdlets to set $?

As a final note, looking into how to change this, it seems that while this behaviour may or may not have been originally intended, it occurs because we fix an assumption where all expressions in the pipeline are treated like pure expressions, which are optimised to be evaluated without the pipeline so need a $? = $true added after them. So theoretically this change also is a small (very small) performance improvement.

PR Checklist

SteveL-MSFT · 2019-11-12T00:07:05Z

@rjmholt don't start writing a RFC for this, I believe after @PowerShell/powershell-committee review, it'll just be an update to your existing RFC

test/powershell/Language/Operators/PipelineChainOperator.Tests.ps1

vexx32 · 2019-11-12T11:32:03Z

@rjmholt does this also affect the behaviour of @() array subexpressions? 🤔

rjmholt · 2019-11-12T16:27:48Z

@rjmholt does this also affect the behaviour of @() array subexpressions? 🤔

Not currently. I can add tests so we can establish what the behaviour is and then should be.

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

rjmholt · 2019-11-13T17:41:55Z

@vexx32 @KirkMunro are there any other expressions you can think of that could conceivably contain a statement?

KirkMunro · 2019-11-13T17:57:36Z

@vexx32 @KirkMunro are there any other expressions you can think of that could conceivably contain a statement?

Brackets, array enclosures, subexpressions, and anonymous functions (script blocks that are dot-sourced or invoked) are the only things that come to mind for me.

vexx32 · 2019-11-13T18:17:57Z

Yeah that's all I got, I think that about covers all the things we can do. Thanks for digging into this, Rob! 😊 💖

….ps1 Co-Authored-By: Ilya <darpa@yandex.ru>

SteveL-MSFT · 2019-11-14T00:42:40Z

@PowerShell/powershell-committee reviewed this and agree this is the correct behavior of $? although it is a breaking change but unlikely to impact existing users

iSazonov · 2019-11-14T04:14:17Z

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

+        /// <returns>True is the compiler should add the success setting, false otherwise.</returns>
+        private bool MustSetSuccessAfterEvaluating(StatementAst statementAst)
+        {
+            // Simple overload fan out


I'd remove the comment.

Wanted to explain why this method really exists here -- it doesn't do anything itself, it's just for type resolution

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

rjmholt · 2019-11-14T19:21:32Z

One case I just thought of: "$(Write-Error "Bad")"; $?. Not sure what the right behaviour there is.

Think for now we should leave it. Small (and strange) enough to address later.

vexx32 · 2019-11-14T19:31:34Z

I would be perfectly happy to just set $? to "undefined" at that point tbh Rob 😂

KirkMunro · 2019-11-14T19:32:05Z

One case I just thought of: "$(Write-Error "Bad")"; $?. Not sure what the right behaviour there is.

If you're embedding the results of a subexpression inside of a string, my initial reaction is that the outermost string-building should influence dollar-hook as it does now, because the string still gets built in that case, and that's what $? is checking.

That said, should a scalar or string value ever influence dollar-hook? There isn't much value in that.

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

rjmholt · 2019-11-15T00:29:08Z

If you're embedding the results of a subexpression inside of a string, my initial reaction is that the outermost string-building should influence dollar-hook as it does now, because the string still gets built in that case, and that's what $? is checking.

Given that the string value is returned, I very much agree. Have added tests for that.

daxian-dbw

LGTM

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

ghost · 2019-11-21T22:36:09Z

🎉v7.0.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

rjmholt added 2 commits November 11, 2019 12:30

Make $() and () not reset $?

8420b0f

Add tests

982d3f0

rjmholt requested a review from daxian-dbw as a code owner November 11, 2019 22:48

rjmholt added WG-Engine core PowerShell engine, interpreter, and runtime Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log labels Nov 11, 2019

rjmholt added 2 commits November 11, 2019 14:55

Fix iex pipeline behaviour

617c595

Add native $? tests

58a5652

rjmholt force-pushed the dollarhook-subpipelines branch from d6d8a55 to 58a5652 Compare November 11, 2019 22:55

rjmholt added 2 commits November 11, 2019 14:58

Add new pipeline chain tests

d489f49

Move error semantics cases down

5d12a0e

rjmholt requested review from BrucePay, JamesWTruher and SteveL-MSFT November 11, 2019 23:01

rjmholt added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Nov 11, 2019

SteveL-MSFT assigned daxian-dbw Nov 12, 2019

iSazonov reviewed Nov 12, 2019

View reviewed changes

test/powershell/Language/Operators/PipelineChainOperator.Tests.ps1 Outdated Show resolved Hide resolved

Add array and empty expression tests

8d5e158

PoshChan reviewed Nov 13, 2019

View reviewed changes

src/System.Management.Automation/engine/parser/Compiler.cs Show resolved Hide resolved

PoshChan reviewed Nov 13, 2019

View reviewed changes

src/System.Management.Automation/engine/parser/Compiler.cs Show resolved Hide resolved

rjmholt added 2 commits November 12, 2019 20:46

Add final fixes plus tests

18d3d41

Add comments to methods

772c690

Update test/powershell/Language/Operators/PipelineChainOperator.Tests…

19cd54b

….ps1 Co-Authored-By: Ilya <darpa@yandex.ru>

SteveL-MSFT removed the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Nov 14, 2019

SteveL-MSFT added the Committee-Reviewed PS-Committee has reviewed this and made a decision label Nov 14, 2019

iSazonov reviewed Nov 14, 2019

View reviewed changes

daxian-dbw reviewed Nov 14, 2019

View reviewed changes

src/System.Management.Automation/engine/parser/Compiler.cs Show resolved Hide resolved

Rob Holt added 2 commits November 14, 2019 16:26

Add comment to describe logic

f3ecc98

Add string expression tests

9b7127f

daxian-dbw approved these changes Nov 15, 2019

View reviewed changes

Add trap test for optimisation

5923439

iSazonov reviewed Nov 15, 2019

View reviewed changes

src/System.Management.Automation/engine/parser/Compiler.cs Show resolved Hide resolved

Add example comment

c420aa5

rjmholt mentioned this pull request Nov 15, 2019

Add documentation for $? changes MicrosoftDocs/PowerShell-Docs#5102

Merged

16 tasks

daxian-dbw merged commit ad12b14 into PowerShell:master Nov 15, 2019

iSazonov added this to the 7.0.0-preview.6 milestone Nov 16, 2019

rjmholt mentioned this pull request Jun 16, 2021

Automatic $? variable (success indicator) is reset to $true when a command is enclosed in parentheses - make $? only reflect command status, not expression status #3359

Closed

rjmholt deleted the dollarhook-subpipelines branch June 16, 2021 21:19

Conversation

rjmholt commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

Further considerations

PR Checklist

Uh oh!

SteveL-MSFT commented Nov 12, 2019

Uh oh!

Uh oh!

vexx32 commented Nov 12, 2019

Uh oh!

rjmholt commented Nov 12, 2019

Uh oh!

Uh oh!

Uh oh!

rjmholt commented Nov 13, 2019

Uh oh!

KirkMunro commented Nov 13, 2019

Uh oh!

vexx32 commented Nov 13, 2019

Uh oh!

SteveL-MSFT commented Nov 14, 2019

Uh oh!

iSazonov Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

rjmholt Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjmholt commented Nov 14, 2019

Uh oh!

vexx32 commented Nov 14, 2019

Uh oh!

KirkMunro commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rjmholt commented Nov 15, 2019

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Nov 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rjmholt commented Nov 11, 2019 •

edited

Loading

KirkMunro commented Nov 14, 2019 •

edited

Loading