Fix try/catch to choose the more specific exception handler (#2429)

* Refactor 'FindMatchingHandler' to get rid of recursion
Search state can be saved and used throughout the searching process without the recursion.

* Rank the exception types based on how specific they are

* Refactor code to find handler based on the rank of given handlers
This commit is contained in:
Dongbo Wang
2016-12-09 12:37:08 -08:00
committed by Jason Shirk
parent c982f30d4e
commit bbf1766ac7
5 changed files with 849 additions and 57 deletions
@@ -4540,9 +4540,8 @@ namespace System.Management.Automation.Language
else
{
cases.Add(Expression.SwitchCase(catchBody,
Enumerable.Range(handlerTypeIndex,
handlerTypeIndex + c.CatchTypes.Count).Select(
ExpressionCache.Constant)));
Enumerable.Range(handlerTypeIndex, c.CatchTypes.Count).Select(
ExpressionCache.Constant)));
handlerTypeIndex += c.CatchTypes.Count;
}
}
@@ -1254,66 +1254,173 @@ namespace System.Management.Automation
{
internal class CatchAll { }
internal static int FindMatchingHandler(MutableTuple tuple, RuntimeException rte, Type[] types, ExecutionContext context)
/// <summary>
/// Represent a handler search result
/// </summary>
private class HandlerSearchResult
{
Exception exceptionToPass = rte;
Exception inner = rte.InnerException;
int handler = -1;
if (inner != null)
internal HandlerSearchResult()
{
handler = FindMatchingHandlerByType(inner.GetType(), types);
exceptionToPass = inner;
Handler = -1;
Rank = int.MaxValue;
ExceptionToPass = null;
ErrorRecordToPass = null;
}
// If no handler was found, or if the handler we found was the catch all handler,
// then look again, this time using the outer exception. If we found the catch all,
// there may be a handler that catches outer but not inner. Furthermore, rethrow
// should throw the original exception from a catchall, not the inner.
if (handler == -1 || types[handler].Equals(typeof(CatchAll)))
{
handler = FindMatchingHandlerByType(rte.GetType(), types);
exceptionToPass = rte;
}
// If we still didn't find a specific handler, we'll try unwrapping a few other of our exceptions:
// ActionPreferenceStopException - to cover -ea stop
// try { gci nosuchfile -ea stop } catch [System.Management.Automation.ItemNotFoundException] { 'caught' }
// CmdletInvocationException - to cover cmdlets like Invoke-Expression
//
if ((handler == -1 || types[handler].Equals(typeof(CatchAll))))
{
var apse = rte as ActionPreferenceStopException;
if (apse != null)
{
exceptionToPass = apse.ErrorRecord.Exception;
if (exceptionToPass is RuntimeException)
{
return FindMatchingHandler(tuple, (RuntimeException)exceptionToPass, types, context);
}
if (exceptionToPass != null)
{
handler = FindMatchingHandlerByType(exceptionToPass.GetType(), types);
}
}
else if (rte is CmdletInvocationException && inner != null)
{
exceptionToPass = inner.InnerException;
if (exceptionToPass != null)
{
handler = FindMatchingHandlerByType(exceptionToPass.GetType(), types);
}
}
}
if (handler != -1)
{
var errorRecord = new ErrorRecord(rte.ErrorRecord, exceptionToPass);
tuple.SetAutomaticVariable(AutomaticVariable.Underbar, errorRecord, context);
}
return handler;
internal int Handler;
internal int Rank;
internal Exception ExceptionToPass;
internal ErrorRecord ErrorRecordToPass;
}
/// <summary>
/// Rank the exception types based on how specific they are.
/// Smaller ranking number indicates more specific exception type.
/// </summary>
/// <remarks>
/// The ranking number for each type represent how many other
/// types from the array derive from it.
/// For example, 0 means no other types in the array derive from
/// the corresponding type, while 3 means there are 3 other types
/// in the array actually derive from the corresponding type.
/// 'CatchAll' is considered to be derived by all exception types.
/// </remarks>
private static int[] RankExceptionTypes(Type[] types)
{
int[] ranks = new int[types.Length];
int length = types.Length;
// If 'CatchAll' is specified, it must be the last catch block.
// Handle it specially. This can save a few iterations in the
// 'for' loop below, and also avoid some type comparisons.
if (types[length - 1].Equals(typeof(CatchAll)))
{
ranks[length - 1] = length - 1;
length = length - 1;
}
// For each type check if it's a sub-class of any types after it.
// The ordering of the type array guarantees the more specific type comes first.
for (int i = 0; i < length - 1; i++)
{
for (int j = i + 1; j < length; j++)
{
if (types[i].IsSubclassOf(types[j]))
ranks[j]++;
}
}
return ranks;
}
/// <summary>
/// Search for handler by the exception type and process the found result.
/// </summary>
private static void FindAndProcessHandler(Type[] types, int[] ranks,
HandlerSearchResult current,
Exception exception,
ErrorRecord errorRecord)
{
Diagnostics.Assert(current != null, "Caller makes sure 'current' is not null.");
int handler = FindMatchingHandlerByType(exception.GetType(), types);
// If no handler was found, return without changing the current result.
if (handler == -1) { return; }
// New handler was found.
// - 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.
int rank = ranks[handler];
if (rank < current.Rank ||
(rank == current.Rank && types[current.Handler].Equals(typeof(CatchAll)))
)
{
current.Handler = handler;
current.Rank = rank;
current.ExceptionToPass = exception;
current.ErrorRecordToPass = errorRecord;
}
}
/// <summary>
/// Find the matching handler for the caught exception
/// </summary>
internal static int FindMatchingHandler(MutableTuple tuple, RuntimeException rte, Type[] types, ExecutionContext context)
{
bool continueToSearch = false;
int[] ranks = RankExceptionTypes(types);
var current = new HandlerSearchResult();
do {
// Always assume no need to repeat the search for another interation
continueToSearch = false;
// The 'ErrorRecord' of the current RuntimeException would be passed to $_
ErrorRecord errorRecordToPass = rte.ErrorRecord;
Exception inner = rte.InnerException;
if (inner != null)
{
FindAndProcessHandler(types, ranks, current, inner, errorRecordToPass);
}
// If no handler was found (rank = int.MaxValue), or if the handler we found was not
// the most specific one, then look again, this time using the outer exception.
// If we found a handler, but not one of the most specific ones (rank != 0), there may
// be a more specific handler that catches outer but not inner exception.
if (current.Rank > 0)
{
FindAndProcessHandler(types, ranks, current, rte, errorRecordToPass);
}
// If we still didn't find one of the most specific handlers (rank != 0), we'll try unwrapping a few other of our exceptions:
// ActionPreferenceStopException - to cover '-ea stop'
// try { gci nosuchfile -ea stop } catch [System.Management.Automation.ItemNotFoundException] { 'caught' }
// CmdletInvocationException - to cover cmdlets like Invoke-Expression
if (current.Rank > 0)
{
var apse = rte as ActionPreferenceStopException;
if (apse != null)
{
var exceptionToPass = apse.ErrorRecord.Exception;
// If it's again a RuntimeException, we repeat the search using it
rte = exceptionToPass as RuntimeException;
if (rte != null)
{
continueToSearch = true;
}
else if (exceptionToPass != null)
{
FindAndProcessHandler(types, ranks, current, exceptionToPass, errorRecordToPass);
}
}
else if (rte is CmdletInvocationException && inner != null)
{
if (inner.InnerException != null)
{
FindAndProcessHandler(types, ranks, current, inner.InnerException, errorRecordToPass);
}
}
}
} while (continueToSearch);
if (current.Handler != -1)
{
var errorRecord = new ErrorRecord(current.ErrorRecordToPass, current.ExceptionToPass);
tuple.SetAutomaticVariable(AutomaticVariable.Underbar, errorRecord, context);
}
return current.Handler;
}
/// <summary>
/// Find the matching handler by the exception type
/// </summary>
private static int FindMatchingHandlerByType(Type exceptionType, Type[] types)
{
int i;
@@ -0,0 +1,28 @@
Describe "Test trap" -Tags "CI" {
Context "Trap with flow control" {
It "Line after exception should NOT be continued when it's from a nested script block" {
$a = . {trap {"trapped"; continue;}; . {"hello"; throw "exception"; "world"}}
$a.Length | Should Be 2
$a -join "," | Should Be "hello,trapped"
}
It "Line after exception should NOT be continued and both inner and outter traps should be triggered" {
$a = . {trap {"outer trap"; continue;}; . {trap {"inner trap"; break;}; "hello"; throw "exception"; "world"}}
$a.Length | Should Be 3
$a -join "," | Should Be "hello,inner trap,outer trap"
}
It "Line after exception should be invoked after continue" {
$a = . {trap {"outer trap"; continue;} "hello"; throw "exception"; "world"}
$a.Length | Should Be 3
$a -join "," | Should Be "hello,outer trap,world"
}
It "Line after exception should NOT be invoked and inner trap should not be triggered" {
$a = . {trap {"outer trap"; continue;}; . {trap [system.Argumentexception] {"inner trap"; continue;}; "hello"; throw "exception"; "world"}}
$a.Length | Should Be 2
$a -join "," | Should Be "hello,outer trap"
}
}
}
@@ -0,0 +1,656 @@
#############################################################
#
# Test simple parsing, ensure newlines allowed everywhere
#
#############################################################
Describe "Test try/catch" -Tags "CI" {
BeforeAll {
function AssertArraysEqual ($result, $expected)
{
$result.Count | Should Be $expected.Count
for ($i = 0; $i -lt $result.Count; $i++) {
$result[$i] | Should Be $expected[$i]
}
}
}
It "Test simple parsing, ensure newlines allowed everywhere" {
try
{
}
catch
{
}
try
{
}
catch
[int]
{
}
try
{
}
catch
[int]
,
[char]
{
}
try
{
}
finally
{
}
try
{
}
catch
{
}
finally
{
}
try
{
}
catch
[int]
{
}
finally
{
}
try
{
}
catch
[int]
,
[char]
{
}
finally
{
}
$true | Should Be $true # we only verify that there is no parsing error. This line contains a dummy Should to make pester happy.
}
Context "Basic exception handling" {
It "Simple throw and catch" {
$a = . { try { 1; throw "exception"; "test failed" } catch { 2 } }
AssertArraysEqual $a (1, 2)
}
It "Simple try finally" {
$a = . { try { 1 } finally { 2 } }
AssertArraysEqual $a (1,2)
}
It "Simple try, throw, catch, and finally" {
$a = . { try { 1; throw "exception"; "test failed" } catch { 2 } finally { 3 } }
AssertArraysEqual $a (1..3)
}
}
Context "Mix traps with try/catch" {
It "Trap shouldn't catch exception" {
$a = . { trap { "test failed" } try { 1; throw "exception"; "test failed" } catch { 2 } }
AssertArraysEqual $a (1,2)
}
It "Trap should catch exception" {
$a = . { try { 1; throw "exception"; trap { 2; return }; "test failed" } catch { "test failed" } }
AssertArraysEqual $a (1,2)
}
}
Context "Catch by type" {
It "Catch by type #1" {
$a = . { try { 1; $a = 0; 1/$a; "test failed" } catch [DivideByZeroException] { 2 } }
AssertArraysEqual $a (1,2)
}
It "Catch by type #2" {
$a = . { try { 1; $a = 0; 1/$a; "test failed" } catch [DivideByZeroException] { 2 } catch [Exception] { "test failed" } }
AssertArraysEqual $a (1,2)
}
It "Catch by type #3" {
$a = . { try { 1; $a = 0; 1/$a; "test failed" } catch [DivideByZeroException] { 2 } catch { "test failed" } }
AssertArraysEqual $a (1,2)
}
It "Catch by type #4" {
$a = . { try { 1; $a = 0; 1/$a; "test failed" } catch [DivideByZeroException],[ArgumentNullException] { 2 } }
AssertArraysEqual $a (1,2)
}
It "Catch by type #5" {
$a = . { try { 1; throw ([ArgumentNullException]::new("bad")) } catch [DivideByZeroException],[ArgumentNullException] { 2 } }
AssertArraysEqual $a (1,2)
}
}
Context "Control flow in try [exit not tested and throw tested elsewhere]" {
It "break in try" {
$a = . {
foreach ($i in (1..3)) {
try {
if ($i -eq 2) {
break
}
$i
} catch {
"test failed"
} finally {
"finally: $i"
}
}
}
AssertArraysEqual $a (1, "finally: 1", "finally: 2")
}
It "continue in try" {
$a = . {
foreach ($i in (1..3)) {
try {
if ($i -eq 2) {
continue
}
$i
} catch {
"test failed"
} finally {
"finally: $i"
}
}
}
AssertArraysEqual $a (1, "finally: 1", "finally: 2", 3, "finally: 3")
}
# Disabled - Compiled script has differing (but better) behavior
It "return in try" -Pending {
$a = . {
function foo($i) {
try {
if ($i -eq 2) {
return "return: $i"
}
$i
} catch {
"test failed"
} finally {
"finally: $i"
}
}
foo 1
foo 2
}
AssertArraysEqual $a (1, "finally: 1", "finally: 2", "return: 2")
}
It "continue in nested try within foreach loop" {
$a = . {
foreach ($i in (1..3)) {
try { #1
try { #2
if ($i -eq 2) {
continue
}
$i
} catch {
"test failed: catch#2"
} finally {
"finally#2: $i"
}
} catch {
"test failed: catch#1"
} finally {
"finally#1: $i"
}
}
}
AssertArraysEqual $a (1, "finally#2: 1", "finally#1: 1", "finally#2: 2", "finally#1: 2", 3, "finally#2: 3", "finally#1: 3")
}
It "break in nested try within foreach loop" {
$a = . {
foreach ($i in (1..3)) {
try { #1
try { #2
if ($i -eq 2) {
break
}
$i
} catch {
"test failed: catch#2"
} finally {
"finally#2: $i"
}
} catch {
"test failed: catch#1"
} finally {
"finally#1: $i"
}
}
}
AssertArraysEqual $a (1, "finally#2: 1", "finally#1: 1", "finally#2: 2", "finally#1: 2")
}
}
Context "Control flow in catch [exit not tested and throw tested elsewhere]" {
It "break in catch without loop" {
$a = . {
try {
throw 1
} catch {
foreach ($i in (1..3)) {
if ($i -eq 2) {
break
}
$i
}
} finally {
"finally"
}
}
AssertArraysEqual $a (1, "finally")
}
It "break in catch within foreach loop" {
$a = . {
foreach ($i in (1..3)) {
try {
throw 1
} catch {
if ($i -eq 2) {
break
}
$i
} finally {
"finally $i"
}
}
}
AssertArraysEqual $a (1, "finally 1", "finally 2")
}
It "continue in catch without loop" {
$a = . {
try {
throw 1
} catch {
foreach ($i in (1..3)) {
if ($i -eq 2) {
continue
}
$i
}
} finally {
"finally"
}
}
AssertArraysEqual $a (1, 3, "finally")
}
It "continue in catch within foreach loop" {
$a = . {
foreach ($i in (1..3)) {
try {
throw 1
} catch {
if ($i -eq 2) {
continue
}
$i
} finally {
"finally $i"
}
}
}
AssertArraysEqual $a (1, "finally 1", "finally 2", 3, "finally 3")
}
It "continue in nested catch within foreach loop" {
$a = . {
foreach ($i in (1..3)) {
try { #1
try { #2
throw 1
} catch {
if ($i -eq 2) {
continue
}
$i
} finally {
"finally#2: $i"
}
} catch {
"test failed: catch#1"
} finally {
"finally#1: $i"
}
}
}
AssertArraysEqual $a (1, "finally#2: 1", "finally#1: 1", "finally#2: 2", "finally#1: 2", 3, "finally#2: 3", "finally#1: 3")
}
It "break in nested catch within foreach loop" {
$a = . {
foreach ($i in (1..3)) {
try { #1
try { #2
throw 1
} catch {
if ($i -eq 2) {
break
}
$i
} finally {
"finally#2: $i"
}
} catch {
"test failed: catch#1"
} finally {
"finally#1: $i"
}
}
}
AssertArraysEqual $a (1, "finally#2: 1", "finally#1: 1", "finally#2: 2", "finally#1: 2")
}
# Disabled - Compiled script has differing (but better) behavior
It "return in catch without loop" -Pending {
$a = . {
function foo {
try {
throw 1
} catch {
foreach ($i in (1..3)) {
if ($i -eq 2) {
return "returned"
}
$i
}
} finally {
"finally"
}
}
foo
}
AssertArraysEqual $a (1, "finally", "returned") "return in catch"
}
# Disabled - Compiled script has differing (but better) behavior
It "return in catch within foreach loop" -Pending {
$a = . {
function foo {
foreach ($i in (1..3)) {
try {
throw 1
} catch {
if ($i -eq 2) {
return "returned"
}
$i
} finally {
"finally $i"
}
}
}
foo
}
AssertArraysEqual $a (1, "finally 1", "finally 2", "returned")
}
}
Context "Control flow in finally, normal execution" {
It "break in finally normal execution" {
$a = . {
try {
"try"
} catch {
} finally {
"finally"
foreach ($i in (1..3)) {
if ($i -eq 2) {
break
}
$i
}
}
}
AssertArraysEqual $a ("try", "finally", 1)
}
It "continue in finally normal execution" {
$a = . {
try {
"try"
} catch {
} finally {
"finally"
foreach ($i in (1..3)) {
if ($i -eq 2) {
continue
}
$i
}
}
}
AssertArraysEqual $a ("try", "finally", 1, 3)
}
}
Context "Control flow in finally, abnormal execution" {
It "break in finally normal execution" {
$a = . {
try {
"try"
throw 1
} catch {
"catch"
} finally {
"finally"
foreach ($i in (1..3)) {
if ($i -eq 2) {
break
}
$i
}
}
}
AssertArraysEqual $a ("try", "catch", "finally", 1)
}
It "continue in finally normal execution" {
$a = . {
try {
"try"
throw 1
} catch {
"catch"
} finally {
"finally"
foreach ($i in (1..3)) {
if ($i -eq 2) {
continue
}
$i
}
}
}
AssertArraysEqual $a ("try", "catch", "finally", 1, 3)
}
}
Context "Exception object" {
It "ErrorRecord object is set correctly" {
$a = . {
try {
throw 42
} catch {
$_
}
}
[int]$a.ToString() | Should Be 42
}
}
It "Nested try/catch" {
$a = . {
try {
"outer try"
try {
"inner try"
$a = 0
1 / $a
}
catch [OutOfMemoryException] {
"test failed"
}
finally {
"inner finally"
}
}
catch [DivideByZeroException] {
"caught"
}
finally {
"outer finally"
}
}
AssertArraysEqual $a ("outer try", "inner try", "inner finally", "caught", "outer finally")
}
Context "Rethrow" {
It "rethrow flow up" {
$a = . {
try {
try {
$a = 0
1 / $a
} catch {
"inner catch"
$ex_inner = $_
throw
}
} catch {
"outer catch"
$ex_outer = $_
}
}
AssertArraysEqual $a ("inner catch", "outer catch")
($ex_inner.Exception -eq $ex_outer.Exception) | Should Be $true
}
It "throw; outside catch threw wrong object" {
$a = . {
function foo {
trap [system.management.automation.runtimeexception] {
return "test passed"
}
trap {
return "test failed"
}
throw
}
try {
$a = 0
1 / $a
} catch {
foo
}
}
($a -eq "test passed") | Should Be $true
}
}
Context "Additional try/catch tests by exception types" {
It "Catch ActionPreferenceStopException" {
$exception = $null
$a = try {
Get-ChildItem TESTDRIVE:\NotExist -ErrorAction Stop
} catch [System.Management.Automation.ActionPreferenceStopException] {
$exception = $_.Exception.GetType().FullName
"ActionPreferenceStopException Caught"
}
$a | Should Be "ActionPreferenceStopException Caught"
## Many legacy scripts from PSv2 catch 'ActionPreferenceStopException' and then check '$_.Exception' to do the real handling
$exception | Should Be "System.Management.Automation.ItemNotFoundException"
}
It "Catch CmdletInvocationException" {
$exception = $null
$a = try {
Invoke-Expression "Get-Command -Name"
} catch [System.Management.Automation.CmdletInvocationException] {
$exception = $_.Exception.GetType().FullName
"CmdletInvocationException Caught"
}
$a | Should Be "CmdletInvocationException Caught"
$exception | Should Be "System.Management.Automation.ParameterBindingException"
}
It "Choose 'ItemNotFoundException' over 'Exception' when searching handler" {
$a = try {
Get-ChildItem TESTDRIVE:\NotExist -ErrorAction Stop
} catch [System.Management.Automation.ItemNotFoundException] {
"ItemNotFoundException caught"
} catch [System.Exception] {
"System.Exception caught"
}
$a | Should Be "ItemNotFoundException caught"
}
It "Choose 'ItemNotFoundException' over 'RuntimeException' when searching handler" {
$a = try {
Get-ChildItem TESTDRIVE:\NotExist -ErrorAction Stop
} catch [System.Management.Automation.ItemNotFoundException] {
"ItemNotFoundException caught"
} catch [System.Management.Automation.RuntimeException] {
"RuntimeException caught"
} catch [System.Exception] {
"System.Exception caught"
}
$a | Should Be "ItemNotFoundException caught"
}
It "Choose 'ItemNotFoundException' over 'RuntimeException' and 'Exception' when throw ItemNotFoundException directly" {
$a = try {
throw [System.Management.Automation.ItemNotFoundException]::new()
} catch [System.Management.Automation.ItemNotFoundException] {
"ItemNotFoundException caught"
} catch [System.Management.Automation.RuntimeException] {
"RuntimeException caught"
} catch [System.Exception] {
"System.Exception caught"
}
$a | Should Be "ItemNotFoundException caught"
}
}
}
+2
View File
@@ -61,4 +61,6 @@
"monad/tests/monad/DRT/utscripts/Language/I18n.Test_fallback.psd1":"Scripting/I18n.Test_fallback.psd1",
"monad/tests/monad/DRT/utscripts/Language/scriptHelp.ps1":"Scripting/ScriptHelp.Tests.ps1",
"monad/tests/monad/DRT/utscripts/Language/scriptHelp.xml":"Scripting/ScriptHelp.Tests.xml",
"monad/tests/monad/DRT/utscripts/Language/trycatch.ps1":"Scripting/TryCatch.Tests.ps1",
"monad/tests/monad/DRT/utscripts/Language/trap.ps1":"Scripting/Trap.Tests.ps1"
}