Skip to content

Commit e5ceaa1

Browse files
gsherwoodgsherwood
committed
Merge branch 'feature/psr2-switch-declaration-various-fixes' of https://github.com/jrfnl/PHP_CodeSniffer 
2 parents 10da08b + 5391139 commit e5ceaa1

File tree

4 files changed

+546
-21
lines changed

4 files changed

+546
-21
lines changed

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

Lines changed: 57 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -107,13 +107,13 @@ public function process(File $phpcsFile, $stackPtr)
107107
}
108108
}
109109

110-
$next = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), null, true);
111-
if ($tokens[$next]['line'] === $tokens[$opener]['line']
112-
&& ($tokens[$next]['code'] === T_COMMENT
113-
|| isset(Tokens::$phpcsCommentTokens[$tokens[$next]['code']]) === true)
114-
) {
115-
// Skip comments on the same line.
116-
$next = $phpcsFile->findNext(T_WHITESPACE, ($next + 1), null, true);
110+
for ($next = ($opener + 1); $next < $nextCloser; $next++) {
111+
if (isset(Tokens::$emptyTokens[$tokens[$next]['code']]) === false
112+
|| (isset(Tokens::$commentTokens[$tokens[$next]['code']]) === true
113+
&& $tokens[$next]['line'] !== $tokens[$opener]['line'])
114+
) {
115+
break;
116+
}
117117
}
118118

119119
if ($tokens[$next]['line'] !== ($tokens[$opener]['line'] + 1)) {
@@ -126,17 +126,21 @@ public function process(File $phpcsFile, $stackPtr)
126126
} else {
127127
$phpcsFile->fixer->beginChangeset();
128128
for ($i = ($opener + 1); $i < $next; $i++) {
129+
if ($tokens[$i]['line'] === $tokens[$opener]['line']) {
130+
// Ignore trailing comments.
131+
continue;
132+
}
133+
129134
if ($tokens[$i]['line'] === $tokens[$next]['line']) {
130135
break;
131136
}
132137

133138
$phpcsFile->fixer->replaceToken($i, '');
134139
}
135140

136-
$phpcsFile->fixer->addNewLineBefore($i);
137141
$phpcsFile->fixer->endChangeset();
138142
}
139-
}
143+
}//end if
140144
}//end if
141145

142146
if ($tokens[$nextCloser]['scope_condition'] === $nextCase) {
@@ -250,22 +254,25 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end)
250254

251255
if ($tokens[$lastToken]['code'] === T_CLOSE_CURLY_BRACKET) {
252256
// We found a closing curly bracket and want to check if its block
253-
// belongs to a SWITCH, IF, ELSEIF or ELSE clause. If yes, we
254-
// 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
255259
// block. Note that we have to make sure that every block of
256260
// 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.
257263
$currentCloser = $lastToken;
258264
$hasElseBlock = false;
265+
$hasCatchWithoutTerminator = false;
259266
do {
260267
$scopeOpener = $tokens[$currentCloser]['scope_opener'];
261268
$scopeCloser = $tokens[$currentCloser]['scope_closer'];
262269

263-
$prevToken = $phpcsFile->findPrevious(T_WHITESPACE, ($scopeOpener - 1), $stackPtr, true);
270+
$prevToken = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($scopeOpener - 1), $stackPtr, true);
264271
if ($prevToken === false) {
265272
return false;
266273
}
267274

268-
// 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.
269276
if ($tokens[$prevToken]['code'] === T_CLOSE_PARENTHESIS) {
270277
$prevToken = $tokens[$prevToken]['parenthesis_owner'];
271278
}
@@ -288,10 +295,39 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end)
288295
return false;
289296
}
290297

291-
$currentCloser = $phpcsFile->findPrevious(T_WHITESPACE, ($prevToken - 1), $stackPtr, true);
298+
$currentCloser = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($prevToken - 1), $stackPtr, true);
292299
if ($tokens[$prevToken]['code'] === T_ELSE) {
293300
$hasElseBlock = true;
294301
}
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);
295331
} else if ($tokens[$prevToken]['code'] === T_SWITCH) {
296332
$hasDefaultBlock = false;
297333
$endOfSwitch = $tokens[$prevToken]['scope_closer'];
@@ -305,7 +341,7 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end)
305341

306342
$opener = $tokens[$nextCase]['scope_opener'];
307343

308-
$nextCode = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), $endOfSwitch, true);
344+
$nextCode = $phpcsFile->findNext(Tokens::$emptyTokens, ($opener + 1), $endOfSwitch, true);
309345
if ($tokens[$nextCode]['code'] === T_CASE || $tokens[$nextCode]['code'] === T_DEFAULT) {
310346
// This case statement has no content, so skip it.
311347
continue;
@@ -339,15 +375,15 @@ private function findNestedTerminator($phpcsFile, $stackPtr, $end)
339375
// We found the last statement of the CASE. Now we want to
340376
// check whether it is a terminating one.
341377
$terminators = [
342-
T_RETURN,
343-
T_BREAK,
344-
T_CONTINUE,
345-
T_THROW,
346-
T_EXIT,
378+
T_RETURN => T_RETURN,
379+
T_BREAK => T_BREAK,
380+
T_CONTINUE => T_CONTINUE,
381+
T_THROW => T_THROW,
382+
T_EXIT => T_EXIT,
347383
];
348384

349385
$terminator = $phpcsFile->findStartOfStatement(($lastToken - 1));
350-
if (in_array($tokens[$terminator]['code'], $terminators, true) === true) {
386+
if (isset($terminators[$tokens[$terminator]['code']]) === true) {
351387
return $terminator;
352388
}
353389
}//end if

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

Lines changed: 244 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,3 +340,247 @@ switch ($foo) {
340340
case 3:
341341
return 2;
342342
}
343+
344+
// Issue 3352.
345+
switch ( $test ) {
346+
case 2: // comment followed by empty line
347+
348+
break;
349+
350+
case 3: /* phpcs:ignore Stnd.Cat.SniffName -- Verify correct handling of ignore comments. */
351+
352+
353+
354+
break;
355+
356+
case 4: /** inline docblock */
357+
358+
359+
360+
break;
361+
362+
case 5: /* checking how it handles */ /* two trailing comments */
363+
364+
break;
365+
366+
case 6:
367+
// Comment as first content of the body.
368+
369+
break;
370+
371+
case 7:
372+
/* phpcs:ignore Stnd.Cat.SniffName -- Verify correct handling of ignore comments at start of body. */
373+
374+
break;
375+
376+
case 8:
377+
/** inline docblock */
378+
379+
break;
380+
}
381+
382+
// Handle comments correctly.
383+
switch ($foo) {
384+
case 1:
385+
if ($bar > 0) {
386+
doSomething();
387+
}
388+
// Comment
389+
else {
390+
return 1;
391+
}
392+
case 2:
393+
return 2;
394+
}
395+
396+
switch ($foo) {
397+
case 1:
398+
if ($bar > 0) /*comment*/ {
399+
return doSomething();
400+
}
401+
else {
402+
return 1;
403+
}
404+
case 2:
405+
return 2;
406+
}
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)