Skip to content

Commit

Permalink
PSR2/SwitchDeclaration: bug fix - handle try/catch/finally correctly
Browse files Browse the repository at this point in the history 
Add code to handle `try-catch-finally` statements correctly when determining whether a `case` needs a terminating comment.

As per #3297 (comment):

> * If there is a `finally` statement and the body of that contains a "terminating statement" (`return`, `break` etc), we don't need to check the body of the `try` or any of the `catch` statements as, no matter what, all execution branches are then covered by that one terminating statement.
> * If there is **_no_** `finally` or if there is, but it does not contain a terminating statement, then all `try` and `catch` structures should each contain a terminating statement in the body.

Includes plenty of unit tests.

Fixes 3297
  • Loading branch information
@jrfnl @gsherwood
jrfnl authored and gsherwood committed May 27, 2021
1 parent 263d8e9 commit 422172d
Show file tree
Hide file tree
Showing 4 changed files with 399 additions and 3 deletions.
38 changes: 35 additions & 3 deletions src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php
View file
Expand Up @@ -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'];
Expand All @@ -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'];
}
Expand All @@ -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'];
Expand Down
180 changes: 180 additions & 0 deletions src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc
View file
Expand Up @@ -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;
}

0 comments on commit 422172d

Please sign in to comment.