Statically detect "continue" inside "switch"

Summary: See 30 prior patches. This is a fatal in PHP7, let's just hunt these down.

Test Plan: Ran unit tests. See next diff for results.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19931

src/__phutil_library_map__.php

@@ -91,6 +91,8 @@ phutil_register_library_map(array(
     'ArcanistConsoleLintRendererTestCase' => 'lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php',
     'ArcanistConstructorParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php',
     'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistConstructorParenthesesXHPASTLinterRuleTestCase.php',
+    'ArcanistContinueInsideSwitchXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistContinueInsideSwitchXHPASTLinterRule.php',
+    'ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase.php',
     'ArcanistControlStatementSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistControlStatementSpacingXHPASTLinterRule.php',
     'ArcanistControlStatementSpacingXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistControlStatementSpacingXHPASTLinterRuleTestCase.php',
     'ArcanistCoverWorkflow' => 'workflow/ArcanistCoverWorkflow.php',
@@ -511,6 +513,8 @@ phutil_register_library_map(array(
     'ArcanistConsoleLintRendererTestCase' => 'PhutilTestCase',
     'ArcanistConstructorParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
+    'ArcanistContinueInsideSwitchXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
+    'ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistControlStatementSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistControlStatementSpacingXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistCoverWorkflow' => 'ArcanistWorkflow',

src/lint/linter/xhpast/rules/ArcanistContinueInsideSwitchXHPASTLinterRule.php

@@ -0,0 +1,53 @@
+<?php
+
+final class ArcanistContinueInsideSwitchXHPASTLinterRule
+  extends ArcanistXHPASTLinterRule {
+
+  const ID = 128;
+
+  public function getLintName() {
+    return pht('Continue Inside Switch');
+  }
+
+  public function process(XHPASTNode $root) {
+    $continues = $root->selectDescendantsOfType('n_CONTINUE');
+
+    $valid_containers = array(
+      'n_WHILE' => true,
+      'n_FOREACH' => true,
+      'n_FOR' => true,
+      'n_DO_WHILE' => true,
+    );
+
+    foreach ($continues as $continue) {
+      // If this is a "continue 2;" or similar, assume it's legitimate.
+      $label = $continue->getChildByIndex(0);
+      if ($label->getTypeName() !== 'n_EMPTY') {
+        continue;
+      }
+
+      $node = $continue->getParentNode();
+      while ($node) {
+        $node_type = $node->getTypeName();
+
+        // If we hit a valid loop which you can actually "continue;" inside,
+        // this is legitimate and we're done here.
+        if (isset($valid_containers[$node_type])) {
+          break;
+        }
+
+        if ($node_type === 'n_SWITCH') {
+          $this->raiseLintAtNode(
+            $continue,
+            pht(
+              'In a "switch" statement, "continue;" is equivalent to "break;" '.
+              'but causes compile errors beginning with PHP 7.0.0.'),
+            'break');
+        }
+
+        $node = $node->getParentNode();
+      }
+    }
+  }
+
+}

src/lint/linter/xhpast/rules/__tests__/ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase.php

@@ -0,0 +1,11 @@
+<?php
+
+final class ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase
+  extends ArcanistXHPASTLinterRuleTestCase {
+
+  public function testLinter() {
+    $this->executeTestsInDirectory(
+      dirname(__FILE__).'/continue-inside-switch/');
+  }
+
+}

src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-1-valid.lint-test

@@ -0,0 +1,21 @@
+<?php
+
+// These are valid loops which you can "continue;" inside.
+
+for ($x = 0; $x < 3; $x++) {
+  continue;
+}
+
+foreach ($x as $y) {
+  continue;
+}
+
+while ($x) {
+  continue;
+}
+
+do {
+  continue;
+} while ($x);
+~~~~~~~~~~
+~~~~~~~~~~

src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-2-n.lint-test

@@ -0,0 +1,24 @@
+<?php
+
+// It's okay to "continue N;" into a loop, although this is usually not the
+// cleanest way to write code and might be discouraged in the future.
+
+while ($x) {
+  switch ($y) {
+    case 1:
+      continue 2;
+  }
+}
+
+// This is invalid, but we don't detect it for now.
+
+switch ($x) {
+  case 1:
+    switch ($y) {
+      case 2:
+        continue 2;
+    }
+    break;
+}
+~~~~~~~~~~
+~~~~~~~~~~

src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-3-rewrite.lint-test

@@ -0,0 +1,26 @@
+<?php
+
+switch ($x) {
+  case 1:
+    continue;
+}
+
+switch ($x) {
+  case 1:
+    continue /* CRITICAL: Nuclear launch code is 1234. */ ;
+}
+~~~~~~~~~~
+error:5:5
+error:10:5
+~~~~~~~~~~
+<?php
+
+switch ($x) {
+  case 1:
+    break;
+}
+
+switch ($x) {
+  case 1:
+    break /* CRITICAL: Nuclear launch code is 1234. */ ;
+}