Fixed bug squizlabs#834: PSR2.ControlStructures.SwitchDeclaration doe…

…s not handle if branches with returns

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

@@ -179,21 +179,19 @@ public function process(File $phpcsFile, $stackPtr)
  continue;
  }
 
- $nextCode = $phpcsFile->findNext(
- T_WHITESPACE,
- ($tokens[$nextCase]['scope_opener'] + 1),
- $nextCloser,
- true
- );
+ $nextCode = $phpcsFile->findNext(T_WHITESPACE, ($opener + 1), $nextCloser, true);
 
  if ($tokens[$nextCode]['code'] !== T_CASE && $tokens[$nextCode]['code'] !== T_DEFAULT) {
  // This case statement has content. If the next case or default comes
- // before the closer, it means we dont have a terminating statement
- // and instead need a comment.
- $nextCode = $this->findNextCase($phpcsFile, ($tokens[$nextCase]['scope_opener'] + 1), $nextCloser);
+ // before the closer, it means we don't have an obvious terminating
+ // statement and need to make some more effort to find one. If we
+ // don't, we do need a comment.
+ $nextCode = $this->findNextCase($phpcsFile, ($opener + 1), $nextCloser);
  if ($nextCode !== false) {
  $prevCode = $phpcsFile->findPrevious(T_WHITESPACE, ($nextCode - 1), $nextCase, true);
- if ($tokens[$prevCode]['code'] !== T_COMMENT) {
+ if ($tokens[$prevCode]['code'] !== T_COMMENT
+ && $this->findNestedTerminator($phpcsFile, ($opener + 1), $nextCode) === false
+ ) {
  $error = 'There must be a comment when fall-through is intentional in a non-empty case body';
  $phpcsFile->addError($error, $nextCase, 'TerminatingComment');
  }
@@ -233,4 +231,91 @@ private function findNextCase($phpcsFile, $stackPtr, $end)
  }//end findNextCase()
 
 
+ /**
+ * Returns true if a nested terminating statement is found.
+ *
+ * @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
+ * @param int $stackPtr The position to start looking at.
+ * @param int $end The position to stop looking at.
+ *
+ * @return bool
+ */
+ private function findNestedTerminator($phpcsFile, $stackPtr, $end)
+ {
+ $tokens = $phpcsFile->getTokens();
+ $terminators = array(
+ T_RETURN,
+ T_BREAK,
+ T_CONTINUE,
+ T_THROW,
+ T_EXIT,
+ );
+
+ $lastToken = $phpcsFile->findPrevious(T_WHITESPACE, ($end - 1), $stackPtr, true);
+ if ($lastToken !== false) {
+ if ($tokens[$lastToken]['code'] === T_CLOSE_CURLY_BRACKET) {
+ // We found a closing curly bracket and want to check if its
+ // block belongs to an IF, ELSEIF or ELSE 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 statement has a terminating statement.
+ $currentCloser = $lastToken;
+ $hasElseBlock = false;
+ do {
+ $scopeOpener = $tokens[$currentCloser]['scope_opener'];
+ $scopeCloser = $tokens[$currentCloser]['scope_closer'];
+
+ $prevToken = $phpcsFile->findPrevious(T_WHITESPACE, ($scopeOpener - 1), $stackPtr, true);
+ if ($prevToken === false) {
+ return false;
+ }
+
+ // IF and ELSEIF clauses possess a condition we have to account for.
+ if ($tokens[$prevToken]['code'] === T_CLOSE_PARENTHESIS) {
+ $prevToken = $tokens[$prevToken]['parenthesis_owner'];
+ }
+
+ if ($tokens[$prevToken]['code'] === T_IF) {
+ // If we have not encountered an ELSE clause by now, we cannot
+ // be sure that the whole statement terminates in every case.
+ if ($hasElseBlock === false) {
+ return false;
+ }
+
+ return $this->findNestedTerminator($phpcsFile, ($scopeOpener + 1), $scopeCloser);
+ } else if ($tokens[$prevToken]['code'] === T_ELSEIF
+ || $tokens[$prevToken]['code'] === T_ELSE
+ ) {
+ // If we find a terminating statement within this block,
+ // we continue with the previous ELSEIF or IF clause.
+ $hasTerminator = $this->findNestedTerminator($phpcsFile, ($scopeOpener + 1), $scopeCloser);
+ if ($hasTerminator === false) {
+ return false;
+ }
+
+ $currentCloser = $phpcsFile->findPrevious(T_WHITESPACE, ($prevToken - 1), $stackPtr, true);
+ if ($tokens[$prevToken]['code'] === T_ELSE) {
+ $hasElseBlock = true;
+ }
+ } else {
+ return false;
+ }//end if
+ } while ($currentCloser !== false && $tokens[$currentCloser]['code'] === T_CLOSE_CURLY_BRACKET);
+
+ return true;
+ } else if ($tokens[$lastToken]['code'] === T_SEMICOLON) {
+ // We found the last statement of the CASE. Now we want to
+ // check whether it is a terminating one.
+ $terminator = $phpcsFile->findStartOfStatement(($lastToken - 1));
+ if (in_array($tokens[$terminator]['code'], $terminators, true) === true) {
+ return $terminator;
+ }
+ }//end if
+ }//end if
+
+ return false;
+
+ }//end findNestedTerminator()
+
+
 }//end class

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

@@ -154,3 +154,95 @@ switch ($foo) {
  case Foo::ARRAY:
  return self::VALUE;
 }
+
+// OK: Every clause terminates
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ return 0;
+ } else {
+ return 1;
+ }
+ case 2:
+ return 2;
+}
+
+// ERROR: No else clause
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ return 0;
+ } elseif ($bar < 0) {
+ return 1;
+ }
+ case 2:
+ return 2;
+}
+
+// OK: No fall-through present
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ return 0;
+ } elseif ($bar < 0) {
+ return 1;
+ }
+}
+
+// ERROR: No else clause (nested)
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ return 0;
+ } else {
+ if ($foo > $bar) {
+ continue;
+ }
+ }
+ case 2:
+ return 2;
+}
+
+// OK: Every clause terminates
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ return 0;
+ } else {
+ if ($foo > $bar) {
+ continue;
+ } else {
+ break;
+ }
+ }
+ case 2:
+ return 2;
+}
+
+// ERROR: Non-termination IF clause
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ $offset = 0;
+ } else {
+ break;
+ }
+ case 2:
+ return 2;
+}
+
+// ERROR: Non-termination IF clause (nested)
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ continue;
+ } else {
+ if ($foo > $bar) {
+ $offset = 0;
+ } else {
+ break;
+ }
+ }
+ case 2:
+ return 2;
+}

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

@@ -157,3 +157,95 @@ switch ($foo) {
  case Foo::ARRAY:
  return self::VALUE;
 }
+
+// OK: Every clause terminates
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ return 0;
+ } else {
+ return 1;
+ }
+ case 2:
+ return 2;
+}
+
+// ERROR: No else clause
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ return 0;
+ } elseif ($bar < 0) {
+ return 1;
+ }
+ case 2:
+ return 2;
+}
+
+// OK: No fall-through present
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ return 0;
+ } elseif ($bar < 0) {
+ return 1;
+ }
+}
+
+// ERROR: No else clause (nested)
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ return 0;
+ } else {
+ if ($foo > $bar) {
+ continue;
+ }
+ }
+ case 2:
+ return 2;
+}
+
+// OK: Every clause terminates
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ return 0;
+ } else {
+ if ($foo > $bar) {
+ continue;
+ } else {
+ break;
+ }
+ }
+ case 2:
+ return 2;
+}
+
+// ERROR: Non-termination IF clause
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ $offset = 0;
+ } else {
+ break;
+ }
+ case 2:
+ return 2;
+}
+
+// ERROR: Non-termination IF clause (nested)
+switch ($foo) {
+ case 1:
+ if ($bar > 0) {
+ continue;
+ } else {
+ if ($foo > $bar) {
+ $offset = 0;
+ } else {
+ break;
+ }
+ }
+ case 2:
+ return 2;
+}

src/Standards/PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.php

@@ -42,6 +42,10 @@ public function getErrorList()
  114 => 1,
  128 => 1,
  141 => 1,
+ 172 => 1,
+ 194 => 1,
+ 224 => 1,
+ 236 => 1,
  );
 
  }//end getErrorList()