8000 S.M.A.PowerShell.HadErrors and $? return false positives when errors are suppressed · Issue #4613 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

S.M.A.PowerShell.HadErrors and $? return false positives when errors are suppressed #4613

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
KirkMunro opened this issue Aug 18, 2017 · 22 comments
Labels
Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif Resolution-No Activity Issue has had no activity for 6 months or more WG-Engine core PowerShell engine, interpreter, and runtime

Comments

@KirkMunro
Copy link
Contributor
KirkMunro commented Aug 18, 2017

Created from discussion started on #3768.

Description

If you invoke PowerShell using the System.Management.Automation.PowerShell class, and if you handle the errors in the command you invoke by indicating that they should be ignored, S.M.A.PowerShell will still set HadErrors to true. Below are some code samples illustrating the problem.

Sample 1: C#/.NET invocation of PowerShell commands/scripts

$ps = [PowerShell]::Create()
$ps.AddCommand('Get-Service').AddParameter('Name','Invalid').AddParameter('ErrorAction',[System.Management.Automation.ActionPreference]::Ignore) > $null
$ps.Invoke()
$ps.HadErrors # returns $true; expectation is that this would return false since the command was configured to ignore errors

Sample 1: Expected behavior

$ps.HadErrors should return $false

Sample 1: Actual behavior

$ps.HadErrors returns $true, even when $ps.Streams.Error does not contain any errors (because there were no errors!)

Sample 2: Direct command invocation in PowerShell

Get-Service -Name Invalid -ErrorAction Ignore
$? # returns $false; shouldn't this return $true?

Sample 2: Expected behavior

$? should return $true

Sample 2: Actual behavior

$? returns $false, even though there were no errors because the invoker of the command instructed PowerShell to ignore the error that would otherwise have been raised because it is a benign error for them (and therefore, not an error)

Additional Details

In both of these cases, -ErrorAction Ignore is being used to tell PowerShell that the error is actually not an error as far as this invocation is concerned, and therefore it can be completely ignored. Why then, do both $? and $ps.HadErrors indicate there was still error? Technically there was an error, by definition of the command being invoked, but it is being handled/treated like it is not an error in the command from which the command producing the error was invoked. In both of these cases $? and $ps.HadErrors should have values indicating that there wasn't an error. If you do not trust the command you are invoking to ignore benign errors and only notify you about errors that you actually need to care about then you should not be invoking that command.

Environment data

PS C:\> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      6.0.0-beta
PSEdition                      Core
GitCommitId                    v6.0.0-beta.5
OS                             Microsoft Windows 10.0.16251
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0
@SteveL-MSFT
Copy link
Member

The whole terminating vs non-terminating errors consistently is misunderstood. Even in some of our product code I see these mistakes and I make them myself. Perhaps an option here is to add HadTerminatingErrors, while HadErrors continues to capture Terminating/Nonterminating (even if handled/suppressed)?

@SteveL-MSFT SteveL-MSFT added WG-Engine core PowerShell engine, interpreter, and runtime Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif labels Aug 31, 2017
@KirkMunro
Copy link
Contributor Author

I agree discussion is required because error handling in PowerShell is a point of confusion. This particular issue has nothing to do with terminating vs non-terminating though. This has to do with handled vs non-handled, which is a completely different thing.

If I invoke a .NET method that internally receives some exception but handles that exception gracefully and then eventually returns back to me without error, should $? return $true or $false? The answer in that case is quite clear: it should return $true because there was no error as far as I, the method invoker, am concerned.

I believe PowerShell command invocations should behave no differently. In both examples I shared above, I am instructing PowerShell to suppress the error if one occurs. In other words, the error is handled in my command. Yet S.M.A.PowerShell.HadErrors returns $true and $? returns $false.

Aside -- This related behaviour also surprises me. Try executing these commands one at a time.

Get-Service -Name Invalid
$? # returns $false
Get-Service -Name Invalid -ErrorAction Stop
$? # returns $false
try {Get-Service -Name Invalid -ErrorAction Stop} catch {<# Ignore the error, it's benign #>}
$? # returns $false -- shouldn't this return $true?
& {try {Get-Service -Name Invalid -ErrorAction Stop} catch {<# Ignore the error, it's benign #>}}
$? # returns $true

@SteveL-MSFT SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Aug 31, 2017
@SteveL-MSFT
Copy link
Member

I see. I think that HadErrors and $? should be consistent in that they are only true if there are unhandled errors, but will defer to @PowerShell/powershell-committee to understand the history and original intent.

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision Breaking-Change breaking change that may affect users and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Sep 6, 2017
@SteveL-MSFT SteveL-MSFT added this to the 6.1.0 milestone Sep 6, 2017
@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee reviewed this

(get-service -name invalid); $?
True
get-service -name invalid; $?
False

the two above statements should be equivalent, however, it's not. The bug seems to be that in @KirkMunro 's samples above, #3 should return $true and the two equivalent statements here should both return $false.

@mklement0
Copy link
Contributor

@SteveL-MSFT:

The reason that (get-service -name invalid); $? returns true is the previously discussed issue that (...) turns a command into an expression and it is then (unexpectedly) the expression's success that determines the value of $?, and the mere act of enclosing a command in (...) is a "successful" expression - see Automatic $? variable is reset to $True when a command is enclosed in parentheses - make $? only reflect command status, not expression status

@KirkMunro:

Re try {Get-Service -Name Invalid -ErrorAction Stop} catch {<# Ignore the error, it's benign #>} returning $False:

To recap from #3768 (comment):

checking $? after try / catch is virtually pointless, because what $? is set to depends on whether the catch block happens to be empty ($False) or not (whatever statement happens to execute last in the catch block determines the value of $?).

Similarly (unexpectedly), anything that you invoke via & { ... } - i.e., & with a script block - currently makes $? indicate $True; try & { nosuchcommand }; $? and & { 1 / 0 }; $?

@SteveL-MSFT
Copy link
Member

Re-reviewing issues @PowerShell/powershell-committee had reviewed prior to GA to see if the decision would still stand post GA.

For this issue, based on @mklement0's explanation, it seems this is working as designed and I think just needs to be documented appropriately.

@SteveL-MSFT SteveL-MSFT removed this from the 6.1.0-Consider milestone Mar 16, 2018
@KirkMunro
Copy link
Contributor Author
KirkMunro commented Mar 16, 2018

@SteveL-MSFT I strongly disagree. Suggesting sample 1 from the original post is by design is wrong.

You tell PowerShell to outright ignore errors in part of an invocation you make from C#, and then PowerShell says oh, hey, there were errors, so you check, but there weren't any, PowerShell lied.

This completely breaks any trust that a developer can have in HadErrors, and forces them to wrap the invocation of PowerShell and build their own HadErrors because HadErrors is broken.

You can't document "broken" appropriately.

@SteveL-MSFT SteveL-MSFT added Review - Committee The PR/Issue needs a review from the PowerShell Committee and removed Committee-Reviewed PS-Committee has reviewed this and made a decision labels Mar 16, 2018
@SteveL-MSFT
Copy link
Member

@KirkMunro sorry, I focused only on the last few replies. Looking at your original sample, I would tend to agree that if you handle the error (or suppress it by choice), .HadErrors shouldn't indicate an error occurred. Remarked for @PowerShell/powershell-committee review to understand the history/intent.

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Mar 21, 2018
@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee reviewed this:

  • .HadErrors was originally designed for use with PSWorkflow. Recommendation is to remove this property or at least mark as deprecated, because PSWorkflow is not in PSCore6. If there is a need for .HadErrors equivalent, recommendation is to have a proposal for a new property to fill this need.
  • Regarding $?, the intent should be that if there are not any errors written to $error, then $? is $true, else it's $false.
  • These changes should be posted as an RFC.

@BrucePay
Copy link
Collaborator

Regarding $?, the intent should be that if there are not any errors written to $error, then $? is $true, else it's $false.

@SteveL-MSFT This statement is not quite right. I think you meant "written to the error stream" not "written to $error". That said, the following code

get-item nosuchitem 2> $null ; $?

MUST return false. The $? variable is the only way for a script to determine that a command failed and take action (without adding the complexity of exceptions). For example:

$content = get-content foo.txt 2> $null
if ($?) 
{ 
    "Do stuff with content" 
} 
else { 
    "Handle error" 
}

Like any shell, the value of $? is only valid right after the command or expression you are interested in has executed. If you need to use it after doing a bunch of other stuff, save it in another variable. If you are executing the command in another scope, don't expect that $? in your scope will be affected.

One important thing that is missing is a way for functions and scriptblocks to indicate to the caller that they failed. (Scripts can do it by calling exit 1).

@mklement0
Copy link
Contributor
mklement0 commented Mar 26, 2018

A few tangents:

I think you meant "written to the error stream" not "written to $error".

True, but as an aside: 2> $null still causes errors to be logged in $Error, perhaps surprisingly (as such it is the equivalent of -ErrorAction SilentlyContinue); only -ErrorAction Ignore prevents that.

The $? variable is the only way for a script to determine that a command failed and take action (without adding the complexity of exceptions).

That makes sense, but given how easy it is to wipe out the last command's $? status (e.g., statements such as (get-content nosuchfile) -match 'hi' and @(get-content nosuchfile), ... making $? return $True), $? is currently of limited value - see #3359 (comment)

One important thing that is missing is a way for functions and scriptblocks to indicate to the caller that they failed. (Scripts can do it by calling exit 1).

While an explicit way to do tha 8000 t would definitely be nice, making Write-Error calls set $? in the caller's scope would go a long way - see #3629

@mklement0
Copy link
Contributor

Someone just hit the $?-is-still-$false-with--ErrorAction Ignore issue indirectly on Stack Overflow, in the context of a Makefile:

A recipe such as powershell -c " Remove-Item -ErrorAction Ignore file.txt" unexpectedly fails if an error was ignored, because $? is mapped onto exit codes 0 and 1.

@vexx32
Copy link
Collaborator
vexx32 commented Apr 3, 2019

I think perhaps this might be resolved by splitting the functionality expected from $?

Currently it reports a mixed status, depending on the last executed command OR expression. And, as far as I can think this morning, the only way an expression would fail is if it's pretty much nonsensical and is either a parse or something like a math error, using operators in a context they don't allow, e.g., "hello" % 2

What if we split this in two, so that we have a variable like $LastCmdletFailed and a $LastExpressionFailed ?

@mklement0
Copy link
Contributor
mklement0 commented Apr 3, 2019

Agreed that $? shouldn't report virtually-always-successful expression status, @vexx32, and this has come up before in #3359, which asks that $?:

  • only ever reflect the last command's status
  • including whether the previous statement (whether command or expression) was aborted due to a statement-terminating error - which is what "hello" % 2 triggers (more simply 1 / 0).

In other words: if an expression (or command) was aborted due to a statement-terminating error, $? is sensibly $false, but it never makes sense for a "successful" (i.e., non-aborted) expression to set $? to $true, as is currently the case.

With that, I'd say there's no need for a separate $LastExpressionFailed.

@vexx32
Copy link
Collaborator
vexx32 commented Apr 3, 2019

That certainly seems to be pretty sensible to me. I can't really think of a case that doesn't effectively cover. 😄

@KirkMunro
Copy link
Contributor Author
KirkMunro commented Apr 10, 2019

only ever reflect the last command's status

I'd amend the quoted statement, as follows:

"only ever reflect the status of the last command that was executed in the current scope".

I've never, ever wanted it to work differently, and could care less about the status of the last command that was invoked any number of levels deep inside of something I invoked.

@vexx32
Copy link
Collaborator
vexx32 commented Apr 10, 2019

Definitely agreed on that. If my function handles an error state but doesn't itself emit an error, that error should not magically show up in $error or $?.

@mklement0
Copy link
Contributor

Agreed.

  • Fortunately, at least in PowerShell code it already seems to work that way:
 & { gci /nosuch }; $?  # $True - fortunately, the gci status did NOT leak
 & { Write-Error 'oh no' }; $?  # !! $True - despite the use of Write-Error

The workaround is to use $PSCmdlet.WriteError(), but that's (a) cumbersome and (b) only available in advanced function/scripts.

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

1 similar comment
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

@microsoft-github-policy-service microsoft-github-policy-service bot added Resolution-No Activity Issue has had no activity for 6 months or more labels Nov 16, 2023
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

Copy link
Contributor

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif Resolution-No Activity Issue has had no activity for 6 months or more WG-Engine core PowerShell engine, interpreter, and runtime
Projects
None yet
Development

No branches or pull requests

6 participants
0