From 25c2381959ac94d9249ae4023c5f9ea36436b81c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 22 Dec 2018 05:30:54 -0800
Subject: [PATCH] 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                |  4 ++
 ...stContinueInsideSwitchXHPASTLinterRule.php | 53 +++++++++++++++++++
 ...ueInsideSwitchXHPASTLinterRuleTestCase.php | 11 ++++
 .../continue-inside-switch-1-valid.lint-test  | 21 ++++++++
 .../continue-inside-switch-2-n.lint-test      | 24 +++++++++
 ...continue-inside-switch-3-rewrite.lint-test | 26 +++++++++
 6 files changed, 139 insertions(+)
 create mode 100644 src/lint/linter/xhpast/rules/ArcanistContinueInsideSwitchXHPASTLinterRule.php
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase.php
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-1-valid.lint-test
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-2-n.lint-test
 create mode 100644 src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-3-rewrite.lint-test

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index e88a8621..e22d342e 100644
--- a/src/__phutil_library_map__.php
+++ b/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',
diff --git a/src/lint/linter/xhpast/rules/ArcanistContinueInsideSwitchXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistContinueInsideSwitchXHPASTLinterRule.php
new file mode 100644
index 00000000..be071fac
--- /dev/null
+++ b/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();
+      }
+    }
+  }
+
+}
diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase.php
new file mode 100644
index 00000000..031e18f8
--- /dev/null
+++ b/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/');
+  }
+
+}
diff --git a/src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-1-valid.lint-test b/src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-1-valid.lint-test
new file mode 100644
index 00000000..6a9f040f
--- /dev/null
+++ b/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);
+~~~~~~~~~~
+~~~~~~~~~~
diff --git a/src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-2-n.lint-test b/src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-2-n.lint-test
new file mode 100644
index 00000000..30647fa6
--- /dev/null
+++ b/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;
+}
+~~~~~~~~~~
+~~~~~~~~~~
diff --git a/src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-3-rewrite.lint-test b/src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-3-rewrite.lint-test
new file mode 100644
index 00000000..1f86a8e2
--- /dev/null
+++ b/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. */ ;
+}