Fix try/catch to choose the more specific exception handler by daxian-dbw · Pull Request #2429 · PowerShell/PowerShell

daxian-dbw · Oct 5, 2016

Idea behind the Fix
After selecting a handler, for both try/catch and trap{}, we set $_ with new ErrorRecord(rte.ErrorRecord, exceptionToPass). The exceptionToPass will be adopted only if (rte.ErrorRecord.Exception is ParentContainsErrorRecordException) == true. So basically $_.Exception will always be rte.ErrorRecord.Exception unless it's a ParentContainsErrorRecordException instance. So we should just select the handler based on rte.ErrorRecord.Exception if it's not ParentContainsErrorRecordException. When it is ParentContainsErrorRecordException, then we go through the original code path to determine what exception to pass to user.

The old fix would cause unacceptable breaking changes (see the discussion below). The following is the new fix:

First rank the given exception types -- a smaller rank indicates a more specific exception type, for example, rank == 0 means the corresponding exception type is not derived by another other given exception types, while rank == 1 means the corresponding exception type is derived by another exception type in the given types. CatchAll is considered to be derived by any other exception type.

Then during the search, we try all existing searching methods in the same existing order, as long as we haven't found a handler whose exception type's rank is 0.
When a new handler is found, we use the following algorithm to decide whether to replace the current handler (the handler that was found before the new handler):

If new-rank is less than current-rank -- meaning the new handler is more specific, then we update the current result with it.
If new-rank is more than current-rank -- meaning the new handler is less specific, then we do NOT change the current result.
If new-rank is equal to current-rank, we do NOT change the current result UNLESS the current handler is catch-all. (This is to keep the original behavior -- prefer to use the later found exception as the exception-to-pass-in if all exceptions result in the catch-all handler.

With the Fix

PS D:\> try {
>>     Get-ChildItem c:\xyzzy -ea Stop
>> } catch [System.Management.Automation.ItemNotFoundException] {
>>     "ItemNotFoundException caught"
>> } catch [System.Exception] {
>>     "System.Exception caught"
>> }
ItemNotFoundException caught
PS D:\>
PS D:\> try {
>>     Get-ChildItem c:\xyzzy -ea Stop
>> } catch [System.Management.Automation.ItemNotFoundException] {
>>     "ItemNotFoundException caught"
>> } catch [System.Management.Automation.RuntimeException] {
>>     "RuntimeException caught"
>> }
ItemNotFoundException caught

Task

Refine the fix
Run our legacy tests to find potential regressions (ran the Github nightly build tests)
Add tests

msftclas · Oct 5, 2016

Hi @daxian-dbw, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Dongbo Wang). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

lzybkr · Oct 6, 2016

The potential breaking changes comes from trying to catch ParentContainsErrorRecordException?

If that's it, it seems safe. Most people just catch everything anyway.

I'd like some investigation to know for sure that ActionPreferenceStopException and CmdletInvocationException are always wrapped in a ParentContainsErrorRecordException.

daxian-dbw · Oct 6, 2016

I will investigate ActionPreferenceStopException and CmdletInvocationException further.

daxian-dbw · Nov 18, 2016

@lzybkr it turns out it causes quite a few regressions ☹️ for example:

Before the change

PS:20> try { dir c:\xyzz -ea Stop }
>>     catch [System.Management.Automation.ActionPreferenceStopException] {
>>        "ActionPreferenceStopException Caught" }
ActionPreferenceStopException Caught

PS:22> try { Invoke-Expression "gcm -Name" }
>>     catch [System.Management.Automation.CmdletInvocationException] {
>>        "CmdletInvocationException Caught" }
CmdletInvocationException Caught

After the change

PS C:\> try { dir c:\xyzz -ea Stop }
>>      catch [System.Management.Automation.ActionPreferenceStopException] {
>>         "ActionPreferenceStopException Caught" }
dir : Cannot find path 'C:\xyzz' because it does not exist.
At line:1 char:7
+ try { dir c:\xyzz -ea Stop }
+       ~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (C:\xyzz:String) [Get-ChildItem], ItemNotFoundException
    + FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.GetChildItemCommand

PS C:\> try { Invoke-Expression "gcm -Name" }
>>      catch [System.Management.Automation.CmdletInvocationException] {
>>         "CmdletInvocationException Caught" }
Invoke-Expression : Missing an argument for parameter 'Name'. Specify a parameter of type 'System.String[]' and try again.
At line:1 char:7
+ try { Invoke-Expression "gcm -Name" }
+       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidArgument: (:) [Invoke-Expression], ParameterBindingException
    + FullyQualifiedErrorId : MissingArgument,Microsoft.PowerShell.Commands.InvokeExpressionCommand

And the awkward part of the current behavior is that sometimes $_.Exception in the catch block is not the same kind of exception that the catch block is supposed to capture, take the same commands as examples:

PS:24> try { dir c:\xyzz -ea Stop }
>>     catch [System.Management.Automation.ActionPreferenceStopException] {
>>        $_.Exception.GetType().FullName }
System.Management.Automation.ItemNotFoundException

PS:25> try { Invoke-Expression "gcm -Name" }
>>     catch [System.Management.Automation.CmdletInvocationException] {
>>        $_.Exception.GetType().FullName }
System.Management.Automation.ParameterBindingException

You can see that $_.Exception caught by catch [ActionPreferenceStopException] and catch [CmdletInvocationException] are ItemNotFoundException and ParameterBindingException respectively.

daxian-dbw · Nov 18, 2016

The root problem is we don't have a specific exception to search for handler. Instead, we have multiple candidate exceptions from which we pick up to suit the given handlers.

The fix needs to be re-worked, and ~~I think we need to change new ErrorRecord(ErrorRecord, Exception) to allow forceful replacement of the exception.~~ it turns out we cannot just change ErrorRecord.Exception forcefully to be the one we pick for a handler, because people are depending on $_.Exception to get the real exception. Example: https://github.com/eccentricDBA/PowerShell/blob/02734d1b415deb74b46d38fae77b8d98cce5ede2/Demos/5_Test-Connection.ps1#L16.

Therefore, new ErrorRecord(ErrorRecord, Exception) should continue to behave the current way -- only replace the exception if ErrorRecord.Exception is ParentContainsErrorRecordException.

Search state can be saved and used throughout the searching process without the recursion.

JamesWTruher · Dec 1, 2016

+        }
+
+        It "Catch by type #4" {
+            $a = . { try { 1; $a = 0; 1/$a; "test failed" } catch [DivideByZeroException],[ArgumentNullException] { 2 } }


$a = . { try { 1; $a = 0; 1/$a; "test failed" } catch [DivideByZeroException],[ArgumentNullException] { 2 } } [](start = 12, length = 109)

should this be explicit?
$a = . { try { 1; throw ([ArgumentNullException]::new("bad")) } catch [DivideByZeroException],[ArgumentNullException] { 2 } }

This test case was moved from source depot. It originally was catch [DivideByZeroException],[StackOverflowException], and it looks to me it tries to verify the DivideByZeroException thrown by 1/0. StackOverflowException is not in CoreCLR so I replaced it with ArgumentNullException.

yah - since the test immediately prior is catching DivideByZeroException I wasn't sure what this test was really doing. Should we have two tests? One to catch DivideByZeroException and on to catch ArgumentNullException (e.g., we can catch both exception types) - if we can improve on our existing tests and make them better/clearer that's awesome

Sure, I will add a test for ArgumentNullException.

JamesWTruher · Dec 1, 2016

+        }
+
+        # Disabled - Compiled script has differing (but better) behavior
+        It "return in try" -Skip:$true {


should this be marked as pending rather than skip?

Sure, I will change it to -Pending.

daxian-dbw · Dec 1, 2016

@lzybkr I think this try/catch fix is ready. Can you please review this PR?

lzybkr · Dec 2, 2016

+            finally
+            {
+            }
+        } | Should Not Throw


Should Not Throw doesn't do anything useful here - parse only tests will fail to parse, so nothing would run.

Right, if parsing failed, | Should Not Throw will never run. The original test was to make sure those use of try/catch would not cause parsing error.
How about I just have a dummy $true | Should Be $true following the try/catch blocks? So if no parsing error, the test will pass.

lzybkr · Dec 2, 2016

+    Context "Line after exception should not be invoked" {
+        It "line after exception should not be invoked" {
+            $a = . {trap {"trapped"; continue;}; . {"hello"; throw "exception"; "world"}}
+            $a.Length | Should Be 2


I think the test should be more explicit in the expected results so it's easier to see the expected flow and to avoid a cascade of failures from helping the test succeed when it should fail.

My bad. I converted the Lite tests to Pester tests, but didn't look closely to their assert messages. Fixed.

lzybkr · Dec 2, 2016

+              foo 2
+            }
+
+            AssertArraysEqual $a (1, "finally: 1", "finally: 2", "return: 2")


I've started using -join ' ' on the array so that I can use Should Be ... which somehow feels more idiomatic to Pester even if it's a little weird. This is just a comment to discuss, I'm not asking for a change.

Yes, I will use -join ',' on array comparison in future. I used it in Trap.Tests.ps1.

msftclas added the cla-not-required label Oct 5, 2016

daxian-dbw added the Review - Needed label Oct 5, 2016

HemantMahawar added the WG-Language parser, language semantics label Oct 6, 2016

mirichmo added Review - Waiting on Author and removed Review - Needed labels Oct 13, 2016

PowerShellTeam added Review - Needed and removed Review - Waiting on Author labels Nov 10, 2016

daxian-dbw force-pushed the catch branch from 5fd6960 to 3ce1ae5 Compare November 18, 2016 06:06

daxian-dbw force-pushed the catch branch 2 times, most recently from c29372e to a7e0e7a Compare November 21, 2016 07:32

PowerShellTeam added the Review - Needed The PR is being reviewed label Nov 22, 2016

daxian-dbw added 3 commits November 28, 2016 14:06

Refactor 'FindMatchingHandler' to get rid of recursion

389b248

Search state can be saved and used throughout the searching process without the recursion.

Rank the exception types based on how specific they are

0aa53b1

Refactor code to find handler based on the rank of given handlers

6972b69

daxian-dbw force-pushed the catch branch from a7e0e7a to 6972b69 Compare November 28, 2016 22:07

PowerShellTeam and others added 3 commits November 28, 2016 16:28

Move 'trap' and 'try/catch' tests to Github

555269a

Refactor 'trap' and 'try/catch' tests to work in Pester

5e92665

Add additional try/catch tests to catch by exception types

f70652a

HemantMahawar removed the Review-Needed label Dec 1, 2016

JamesWTruher reviewed Dec 1, 2016

View reviewed changes

daxian-dbw assigned lzybkr Dec 1, 2016

lzybkr suggested changes Dec 2, 2016

View reviewed changes

Address code review comments in tests

a1f1cd8

Fix spelling

939360f

lzybkr approved these changes Dec 9, 2016

View reviewed changes

lzybkr merged commit bbf1766 into PowerShell:master Dec 9, 2016

daxian-dbw deleted the catch branch December 9, 2016 21:03

Search code, repositories, users, issues, pull requests...

Conversation

daxian-dbw commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Oct 5, 2016

Uh oh!

lzybkr commented Oct 6, 2016

Uh oh!

daxian-dbw commented Oct 6, 2016

Uh oh!

daxian-dbw commented Nov 18, 2016

Uh oh!

daxian-dbw commented Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

daxian-dbw commented Oct 5, 2016 •

edited

Loading

daxian-dbw commented Nov 18, 2016 •

edited

Loading