diff --git a/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php b/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php index 60785bdc97..e36b134247 100644 --- a/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php +++ b/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php @@ -254,12 +254,15 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end) if ($tokens[$lastToken]['code'] === T_CLOSE_CURLY_BRACKET) { // We found a closing curly bracket and want to check if its block - // belongs to a SWITCH, IF, ELSEIF or ELSE clause. If yes, we - // continue searching for a terminating statement within that + // belongs to a SWITCH, IF, ELSEIF or ELSE, TRY, CATCH OR FINALLY clause. + // If yes, we continue searching for a terminating statement within that // block. Note that we have to make sure that every block of // the entire if/else/switch statement has a terminating statement. + // For a try/catch/finally statement, either the finally block has + // to have a terminating statement or every try/catch block has to have one. $currentCloser = $lastToken; $hasElseBlock = false; + $hasCatchWithoutTerminator = false; do { $scopeOpener = $tokens[$currentCloser]['scope_opener']; $scopeCloser = $tokens[$currentCloser]['scope_closer']; @@ -269,7 +272,7 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end) return false; } - // SWITCH, IF and ELSEIF clauses possess a condition we have to account for. + // SWITCH, IF, ELSEIF, CATCH clauses possess a condition we have to account for. if ($tokens[$prevToken]['code'] === T_CLOSE_PARENTHESIS) { $prevToken = $tokens[$prevToken]['parenthesis_owner']; } @@ -296,6 +299,35 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end) if ($tokens[$prevToken]['code'] === T_ELSE) { $hasElseBlock = true; } + } else if ($tokens[$prevToken]['code'] === T_FINALLY) { + // If we find a terminating statement within this block, + // the whole try/catch/finally statement is covered. + $hasTerminator = $this->findNestedTerminator($phpcsFile, ($scopeOpener + 1), $scopeCloser); + if ($hasTerminator !== false) { + return $hasTerminator; + } + + // Otherwise, we continue with the previous TRY or CATCH clause. + $currentCloser = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prevToken - 1), $stackPtr, true); + } else if ($tokens[$prevToken]['code'] === T_TRY) { + // If we've seen CATCH blocks without terminator statement and + // have not seen a FINALLY *with* a terminator statement, we + // don't even need to bother checking the TRY. + if ($hasCatchWithoutTerminator === true) { + return false; + } + + return $this->findNestedTerminator($phpcsFile, ($scopeOpener + 1), $scopeCloser); + } else if ($tokens[$prevToken]['code'] === T_CATCH) { + // Keep track of seen catch statements without terminating statement, + // but don't bow out yet as there may still be a FINALLY clause + // with a terminating statement before the CATCH. + $hasTerminator = $this->findNestedTerminator($phpcsFile, ($scopeOpener + 1), $scopeCloser); + if ($hasTerminator === false) { + $hasCatchWithoutTerminator = true; + } + + $currentCloser = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prevToken - 1), $stackPtr, true); } else if ($tokens[$prevToken]['code'] === T_SWITCH) { $hasDefaultBlock = false; $endOfSwitch = $tokens[$prevToken]['scope_closer']; diff --git a/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc b/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc index 4dfa8fec9b..c425a1f20d 100644 --- a/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc +++ b/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc @@ -404,3 +404,183 @@ switch ($foo) { case 2: return 2; } + +// Issue #3297. +// Okay - finally will always be executed, so all branches are covered by the `return` in finally. +switch ( $a ) { + case 1: + try { + doSomething(); + } catch (Exception $e) { + doSomething(); + } catch (AnotherException $e) { + doSomething(); + } finally { + return true; + } + default: + $other = $code; + break; +} + +// Okay - all - non-finally - branches have a terminating statement. +switch ( $a ) { + case 1: + try { + return false; + } catch (Exception $e) /*comment*/ { + return true; + } + // Comment + catch (AnotherException $e) { + return true; + } finally { + doSomething(); + } + default: + $other = $code; + break; +} + +// Okay - finally will always be executed, so all branches are covered by the `return` in finally. +// Non-standard structure order. +switch ( $a ) { + case 1: + try { + doSomething(); + } catch (Exception $e) { + doSomething(); + } finally { + return true; + } catch (AnotherException $e) { + doSomething(); + } + default: + $other = $code; + break; +} + +// Okay - all - non-finally - branches have a terminating statement. +// Non-standard structure order. +switch ( $a ) { + case 1: + try { + return false; + } finally { + doSomething(); + } catch (MyException $e) { + return true; + } catch (AnotherException $e) { + return true; + } + default: + $other = $code; + break; +} + +// All okay, no finally. Any exception still uncaught will terminate the case anyhow, so we're good. +switch ( $a ) { + case 1: + try { + return false; + } catch (MyException $e) { + return true; + } catch (AnotherException $e) { + return true; + } + default: + $other = $code; + break; +} + +// All okay, no catch +switch ( $a ) { + case 1: + try { + return true; + } finally { + doSomething(); + } + case 2: + $other = $code; + break; +} + +// All okay, try-catch nested in if. +switch ( $a ) { + case 1: + if ($a) { + try { + return true; + } catch (MyException $e) { + throw new Exception($e->getMessage()); + } + } else { + return true; + } + case 2: + $other = $code; + break; +} + +// Missing fall-through comment. +switch ( $a ) { + case 1: + try { + doSomething(); + } finally { + doSomething(); + } + case 2: + $other = $code; + break; +} + +// Missing fall-through comment. One of the catches does not have a terminating statement. +switch ( $a ) { + case 1: + try { + return false; + } catch (Exception $e) { + doSomething(); + } catch (AnotherException $e) { + return true; + } finally { + doSomething(); + } + default: + $other = $code; + break; +} + +// Missing fall-through comment. Try does not have a terminating statement. +switch ( $a ) { + case 1: + try { + doSomething(); + } finally { + doSomething(); + } catch (Exception $e) { + return true; + } catch (AnotherException $e) { + return true; + } + default: + $other = $code; + break; +} + +// Missing fall-through comment. One of the catches does not have a terminating statement. +switch ( $a ) { + case 1: + try { + return false; + } catch (Exception $e) { + doSomething(); + } catch (AnotherException $e) { + return true; + } + default: + $other = $code; + break; +} diff --git a/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc.fixed b/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc.fixed index 404880e67b..47ae8d3de3 100644 --- a/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc.fixed +++ b/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc.fixed @@ -399,3 +399,183 @@ switch ($foo) { case 2: return 2; } + +// Issue #3297. +// Okay - finally will always be executed, so all branches are covered by the `return` in finally. +switch ( $a ) { + case 1: + try { + doSomething(); + } catch (Exception $e) { + doSomething(); + } catch (AnotherException $e) { + doSomething(); + } finally { + return true; + } + default: + $other = $code; + break; +} + +// Okay - all - non-finally - branches have a terminating statement. +switch ( $a ) { + case 1: + try { + return false; + } catch (Exception $e) /*comment*/ { + return true; + } + // Comment + catch (AnotherException $e) { + return true; + } finally { + doSomething(); + } + default: + $other = $code; + break; +} + +// Okay - finally will always be executed, so all branches are covered by the `return` in finally. +// Non-standard structure order. +switch ( $a ) { + case 1: + try { + doSomething(); + } catch (Exception $e) { + doSomething(); + } finally { + return true; + } catch (AnotherException $e) { + doSomething(); + } + default: + $other = $code; + break; +} + +// Okay - all - non-finally - branches have a terminating statement. +// Non-standard structure order. +switch ( $a ) { + case 1: + try { + return false; + } finally { + doSomething(); + } catch (MyException $e) { + return true; + } catch (AnotherException $e) { + return true; + } + default: + $other = $code; + break; +} + +// All okay, no finally. Any exception still uncaught will terminate the case anyhow, so we're good. +switch ( $a ) { + case 1: + try { + return false; + } catch (MyException $e) { + return true; + } catch (AnotherException $e) { + return true; + } + default: + $other = $code; + break; +} + +// All okay, no catch +switch ( $a ) { + case 1: + try { + return true; + } finally { + doSomething(); + } + case 2: + $other = $code; + break; +} + +// All okay, try-catch nested in if. +switch ( $a ) { + case 1: + if ($a) { + try { + return true; + } catch (MyException $e) { + throw new Exception($e->getMessage()); + } + } else { + return true; + } + case 2: + $other = $code; + break; +} + +// Missing fall-through comment. +switch ( $a ) { + case 1: + try { + doSomething(); + } finally { + doSomething(); + } + case 2: + $other = $code; + break; +} + +// Missing fall-through comment. One of the catches does not have a terminating statement. +switch ( $a ) { + case 1: + try { + return false; + } catch (Exception $e) { + doSomething(); + } catch (AnotherException $e) { + return true; + } finally { + doSomething(); + } + default: + $other = $code; + break; +} + +// Missing fall-through comment. Try does not have a terminating statement. +switch ( $a ) { + case 1: + try { + doSomething(); + } finally { + doSomething(); + } catch (Exception $e) { + return true; + } catch (AnotherException $e) { + return true; + } + default: + $other = $code; + break; +} + +// Missing fall-through comment. One of the catches does not have a terminating statement. +switch ( $a ) { + case 1: + try { + return false; + } catch (Exception $e) { + doSomething(); + } catch (AnotherException $e) { + return true; + } + default: + $other = $code; + break; +} diff --git a/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.php b/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.php index e099dd324a..0cd946d888 100644 --- a/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.php +++ b/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.php @@ -54,6 +54,10 @@ public function getErrorList() 356 => 1, 362 => 1, 384 => 1, + 528 => 1, + 541 => 1, + 558 => 1, + 575 => 1, ]; }//end getErrorList()