Skip to content

Commit 5391139

Browse files
jrfnljrfnl
committed
PSR2/SwitchDeclaration: bug fix - handle try/catch/finally correctly
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
1 parent b31b9b2 commit 5391139

File tree

4 files changed

+399
-3
lines changed

4 files changed

+399
-3
lines changed

‎src/Standards/PSR2/Sniffs/ControlStructures/SwitchDeclarationSniff.php

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -254,12 +254,15 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end)
254254

255255
if ($tokens[$lastToken]['code'] === T_CLOSE_CURLY_BRACKET) {
256256
// We found a closing curly bracket and want to check if its block
257-
// belongs to a SWITCH, IF, ELSEIF or ELSE clause. If yes, we
258-
// continue searching for a terminating statement within that
257+
// belongs to a SWITCH, IF, ELSEIF or ELSE, TRY, CATCH OR FINALLY clause.
258+
// If yes, we continue searching for a terminating statement within that
259259
// block. Note that we have to make sure that every block of
260260
// the entire if/else/switch statement has a terminating statement.
261+
// For a try/catch/finally statement, either the finally block has
262+
// to have a terminating statement or every try/catch block has to have one.
261263
$currentCloser = $lastToken;
262264
$hasElseBlock = false;
265+
$hasCatchWithoutTerminator = false;
263266
do {
264267
$scopeOpener = $tokens[$currentCloser]['scope_opener'];
265268
$scopeCloser = $tokens[$currentCloser]['scope_closer'];
@@ -269,7 +272,7 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end)
269272
return false;
270273
}
271274

272-
// SWITCH, IF and ELSEIF clauses possess a condition we have to account for.
275+
// SWITCH, IF, ELSEIF, CATCH clauses possess a condition we have to account for.
273276
if ($tokens[$prevToken]['code'] === T_CLOSE_PARENTHESIS) {
274277
$prevToken = $tokens[$prevToken]['parenthesis_owner'];
275278
}
@@ -296,6 +299,35 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end)
296299
if ($tokens[$prevToken]['code'] === T_ELSE) {
297300
$hasElseBlock = true;
298301
}
302+
} else if ($tokens[$prevToken]['code'] === T_FINALLY) {
303+
// If we find a terminating statement within this block,
304+
// the whole try/catch/finally statement is covered.
305+
$hasTerminator = $this->findNestedTerminator($phpcsFile, ($scopeOpener + 1), $scopeCloser);
306+
if ($hasTerminator !== false) {
307+
return $hasTerminator;
308+
}
309+
310+
// Otherwise, we continue with the previous TRY or CATCH clause.
311+
$currentCloser = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prevToken - 1), $stackPtr, true);
312+
} else if ($tokens[$prevToken]['code'] === T_TRY) {
313+
// If we've seen CATCH blocks without terminator statement and
314+
// have not seen a FINALLY *with* a terminator statement, we
315+
// don't even need to bother checking the TRY.
316+
if ($hasCatchWithoutTerminator === true) {
317+
return false;
318+
}
319+
320+
return $this->findNestedTerminator($phpcsFile, ($scopeOpener + 1), $scopeCloser);
321+
} else if ($tokens[$prevToken]['code'] === T_CATCH) {
322+
// Keep track of seen catch statements without terminating statement,
323+
// but don't bow out yet as there may still be a FINALLY clause
324+
// with a terminating statement before the CATCH.
325+
$hasTerminator = $this->findNestedTerminator($phpcsFile, ($scopeOpener + 1), $scopeCloser);
326+
if ($hasTerminator === false) {
327+
$hasCatchWithoutTerminator = true;
328+
}
329+
330+
$currentCloser = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prevToken - 1), $stackPtr, true);
299331
} else if ($tokens[$prevToken]['code'] === T_SWITCH) {
300332
$hasDefaultBlock = false;
301333
$endOfSwitch = $tokens[$prevToken]['scope_closer'];

‎src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,3 +404,183 @@ switch ($foo) {
404404
case 2:
405405
return 2;
406406
}
407+
408+
// Issue #3297.
409+
// Okay - finally will always be executed, so all branches are covered by the `return` in finally.
410+
switch ( $a ) {
411+
case 1:
412+
try {
413+
doSomething();
414+
} catch (Exception $e) {
415+
doSomething();
416+
} catch (AnotherException $e) {
417+
doSomething();
418+
} finally {
419+
return true;
420+
}
421+
default:
422+
$other = $code;
423+
break;
424+
}
425+
426+
// Okay - all - non-finally - branches have a terminating statement.
427+
switch ( $a ) {
428+
case 1:
429+
try {
430+
return false;
431+
} catch (Exception $e) /*comment*/ {
432+
return true;
433+
}
434+
// Comment
435+
catch (AnotherException $e) {
436+
return true;
437+
} finally {
438+
doSomething();
439+
}
440+
default:
441+
$other = $code;
442+
break;
443+
}
444+
445+
// Okay - finally will always be executed, so all branches are covered by the `return` in finally.
446+
// Non-standard structure order.
447+
switch ( $a ) {
448+
case 1:
449+
try {
450+
doSomething();
451+
} catch (Exception $e) {
452+
doSomething();
453+
} finally {
454+
return true;
455+
} catch (AnotherException $e) {
456+
doSomething();
457+
}
458+
default:
459+
$other = $code;
460+
break;
461+
}
462+
463+
// Okay - all - non-finally - branches have a terminating statement.
464+
// Non-standard structure order.
465+
switch ( $a ) {
466+
case 1:
467+
try {
468+
return false;
469+
} finally {
470+
doSomething();
471+
} catch (MyException $e) {
472+
return true;
473+
} catch (AnotherException $e) {
474+
return true;
475+
}
476+
default:
477+
$other = $code;
478+
break;
479+
}
480+
481+
// All okay, no finally. Any exception still uncaught will terminate the case anyhow, so we're good.
482+
switch ( $a ) {
483+
case 1:
484+
try {
485+
return false;
486+
} catch (MyException $e) {
487+
return true;
488+
} catch (AnotherException $e) {
489+
return true;
490+
}
491+
default:
492+
$other = $code;
493+
break;
494+
}
495+
496+
// All okay, no catch
497+
switch ( $a ) {
498+
case 1:
499+
try {
500+
return true;
501+
} finally {
502+
doSomething();
503+
}
504+
case 2:
505+
$other = $code;
506+
break;
507+
}
508+
509+
// All okay, try-catch nested in if.
510+
switch ( $a ) {
511+
case 1:
512+
if ($a) {
513+
try {
514+
return true;
515+
} catch (MyException $e) {
516+
throw new Exception($e->getMessage());
517+
}
518+
} else {
519+
return true;
520+
}
521+
case 2:
522+
$other = $code;
523+
break;
524+
}
525+
526+
// Missing fall-through comment.
527+
switch ( $a ) {
528+
case 1:
529+
try {
530+
doSomething();
531+
} finally {
532+
doSomething();
533+
}
534+
case 2:
535+
$other = $code;
536+
break;
537+
}
538+
539+
// Missing fall-through comment. One of the catches does not have a terminating statement.
540+
switch ( $a ) {
541+
case 1:
542+
try {
543+
return false;
544+
} catch (Exception $e) {
545+
doSomething();
546+
} catch (AnotherException $e) {
547+
return true;
548+
} finally {
549+
doSomething();
550+
}
551+
default:
552+
$other = $code;
553+
break;
554+
}
555+
556+
// Missing fall-through comment. Try does not have a terminating statement.
557+
switch ( $a ) {
558+
case 1:
559+
try {
560+
doSomething();
561+
} finally {
562+
doSomething();
563+
} catch (Exception $e) {
564+
return true;
565+
} catch (AnotherException $e) {
566+
return true;
567+
}
568+
default:
569+
$other = $code;
570+
break;
571+
}
572+
573+
// Missing fall-through comment. One of the catches does not have a terminating statement.
574+
switch ( $a ) {
575+
case 1:
576+
try {
577+
return false;
578+
} catch (Exception $e) {
579+
doSomething();
580+
} catch (AnotherException $e) {
581+
return true;
582+
}
583+
default:
584+
$other = $code;
585+
break;
586+
}

0 commit comments

Comments
 (0)