| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Rob Holt (@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 |
Sorry, something went wrong.
|
Rob Holt (@rjmholt) does this also affect the behaviour of @() array subexpressions? 🤔 |
Sorry, something went wrong.
Not currently. I can add tests so we can establish what the behaviour is and then should be. |
Sorry, something went wrong.
|
Rain Sallow (/u/ta11ow) (@vexx32) Kirk Munro (@KirkMunro) are there any other expressions you can think of that could conceivably contain a statement? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
Yeah that's all I got, I think that about covers all the things we can do. Thanks for digging into this, Rob! 😊 💖 |
Sorry, something went wrong.
….ps1 Co-Authored-By: Ilya <darpa@yandex.ru>
|
@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 |
Sorry, something went wrong.
| /// <returns>True is the compiler should add the success setting, false otherwise.</returns> | ||
| private bool MustSetSuccessAfterEvaluating(StatementAst statementAst) | ||
| { | ||
| // Simple overload fan out |
There was a problem hiding this comment.
I'd remove the comment.
Sorry, something went wrong.
There was a problem hiding this comment.
Wanted to explain why this method really exists here -- it doesn't do anything itself, it's just for type resolution
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I would be perfectly happy to just set $? to "undefined" at that point tbh Rob 😂 |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Given that the string value is returned, I very much agree. Have added tests for that. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
🎉v7.0.0-preview.6 has been released which incorporates this pull request.:tada: Handy links: |
Sorry, something went wrong.
…on` and `ArrayExpression` (PowerShell#11040)
| Back | FazBrowse Home | New Git URL |
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:
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
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