diff --git a/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php b/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php index 11f6a3f49e..e36b134247 100644 --- a/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php +++ b/src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php @@ -107,13 +107,13 @@ public function process(File $phpcsFile, $stackPtr) } } - $next = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), null, true); - if ($tokens[$next]['line'] === $tokens[$opener]['line'] - && ($tokens[$next]['code'] === T_COMMENT - || isset(Tokens::$phpcsCommentTokens[$tokens[$next]['code']]) === true) - ) { - // Skip comments on the same line. - $next = $phpcsFile->findNext(T_WHITESPACE, ($next + 1), null, true); + for ($next = ($opener + 1); $next < $nextCloser; $next++) { + if (isset(Tokens::$emptyTokens[$tokens[$next]['code']]) === false + || (isset(Tokens::$commentTokens[$tokens[$next]['code']]) === true + && $tokens[$next]['line'] !== $tokens[$opener]['line']) + ) { + break; + } } if ($tokens[$next]['line'] !== ($tokens[$opener]['line'] + 1)) { @@ -126,6 +126,11 @@ public function process(File $phpcsFile, $stackPtr) } else { $phpcsFile->fixer->beginChangeset(); for ($i = ($opener + 1); $i < $next; $i++) { + if ($tokens[$i]['line'] === $tokens[$opener]['line']) { + // Ignore trailing comments. + continue; + } + if ($tokens[$i]['line'] === $tokens[$next]['line']) { break; } @@ -133,10 +138,9 @@ public function process(File $phpcsFile, $stackPtr) $phpcsFile->fixer->replaceToken($i, ''); } - $phpcsFile->fixer->addNewLineBefore($i); $phpcsFile->fixer->endChangeset(); } - } + }//end if }//end if if ($tokens[$nextCloser]['scope_condition'] === $nextCase) { @@ -250,22 +254,25 @@ 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']; - $prevToken = $phpcsFile->findPrevious(T_WHITESPACE, ($scopeOpener - 1), $stackPtr, true); + $prevToken = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($scopeOpener - 1), $stackPtr, true); if ($prevToken === false) { 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']; } @@ -288,10 +295,39 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end) return false; } - $currentCloser = $phpcsFile->findPrevious(T_WHITESPACE, ($prevToken - 1), $stackPtr, true); + $currentCloser = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prevToken - 1), $stackPtr, true); 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']; @@ -305,7 +341,7 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end) $opener = $tokens[$nextCase]['scope_opener']; - $nextCode = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), $endOfSwitch, true); + $nextCode = $phpcsFile->findNext(Tokens::$emptyTokens, ($opener + 1), $endOfSwitch, true); if ($tokens[$nextCode]['code'] === T_CASE || $tokens[$nextCode]['code'] === T_DEFAULT) { // This case statement has no content, so skip it. continue; @@ -339,15 +375,15 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end) // We found the last statement of the CASE. Now we want to // check whether it is a terminating one. $terminators = [ - T_RETURN, - T_BREAK, - T_CONTINUE, - T_THROW, - T_EXIT, + T_RETURN => T_RETURN, + T_BREAK => T_BREAK, + T_CONTINUE => T_CONTINUE, + T_THROW => T_THROW, + T_EXIT => T_EXIT, ]; $terminator = $phpcsFile->findStartOfStatement(($lastToken - 1)); - if (in_array($tokens[$terminator]['code'], $terminators, true) === true) { + if (isset($terminators[$tokens[$terminator]['code']]) === true) { return $terminator; } }//end if diff --git a/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc b/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc index 9bf0f6055c..c425a1f20d 100644 --- a/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc +++ b/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc @@ -340,3 +340,247 @@ switch ($foo) { case 3: return 2; } + +// Issue 3352. +switch ( $test ) { + case 2: // comment followed by empty line + + break; + + case 3: /* phpcs:ignore Stnd.Cat.SniffName -- Verify correct handling of ignore comments. */ + + + + break; + + case 4: /** inline docblock */ + + + + break; + + case 5: /* checking how it handles */ /* two trailing comments */ + + break; + + case 6: + // Comment as first content of the body. + + break; + + case 7: + /* phpcs:ignore Stnd.Cat.SniffName -- Verify correct handling of ignore comments at start of body. */ + + break; + + case 8: + /** inline docblock */ + + break; +} + +// Handle comments correctly. +switch ($foo) { + case 1: + if ($bar > 0) { + doSomething(); + } + // Comment + else { + return 1; + } + case 2: + return 2; +} + +switch ($foo) { + case 1: + if ($bar > 0) /*comment*/ { + return doSomething(); + } + else { + return 1; + } + 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 85617836db..47ae8d3de3 100644 --- a/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc.fixed +++ b/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc.fixed @@ -343,3 +343,239 @@ switch ($foo) { case 3: return 2; } + +// Issue 3352. +switch ( $test ) { + case 2: // comment followed by empty line + break; + + case 3: /* phpcs:ignore Stnd.Cat.SniffName -- Verify correct handling of ignore comments. */ + break; + + case 4: /** inline docblock */ + break; + + case 5: /* checking how it handles */ /* two trailing comments */ + break; + + case 6: + // Comment as first content of the body. + + break; + + case 7: + /* phpcs:ignore Stnd.Cat.SniffName -- Verify correct handling of ignore comments at start of body. */ + + break; + + case 8: + /** inline docblock */ + + break; +} + +// Handle comments correctly. +switch ($foo) { + case 1: + if ($bar > 0) { + doSomething(); + } + // Comment + else { + return 1; + } + case 2: + return 2; +} + +switch ($foo) { + case 1: + if ($bar > 0) /*comment*/ { + return doSomething(); + } + else { + return 1; + } + 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 97a68704f5..0cd946d888 100644 --- a/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.php +++ b/src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.php @@ -49,6 +49,15 @@ public function getErrorList() 260 => 1, 300 => 1, 311 => 1, + 346 => 1, + 350 => 1, + 356 => 1, + 362 => 1, + 384 => 1, + 528 => 1, + 541 => 1, + 558 => 1, + 575 => 1, ]; }//end getErrorList()