Merge branch "master" into "experimental"

src/__phutil_library_map__.php

@@ -104,6 +104,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',
@@ -553,6 +555,8 @@ phutil_register_library_map(array(
     'ArcanistConsoleLintRendererTestCase' => 'PhutilTestCase',
     'ArcanistConstructorParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
+    'ArcanistContinueInsideSwitchXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
+    'ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistControlStatementSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistControlStatementSpacingXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistCoverWorkflow' => 'ArcanistWorkflow',

src/configuration/ArcanistConfiguration.php

@@ -7,15 +7,15 @@
  * pointing to your subclass in your project configuration.
  *
  * When specified as the **arcanist_configuration** class in your project's
- * ##.arcconfig##, your subclass will be instantiated (instead of this class)
+ * `.arcconfig`, your subclass will be instantiated (instead of this class)
  * and be able to handle all the method calls. In particular, you can:
  *
- *    - create, replace, or disable workflows by overriding buildWorkflow()
+ *    - create, replace, or disable workflows by overriding `buildWorkflow()`
- *      and buildAllWorkflows();
+ *      and `buildAllWorkflows()`;
  *    - add additional steps before or after workflows run by overriding
- *      willRunWorkflow() or didRunWorkflow() or didAbortWorkflow(); and
+ *      `willRunWorkflow()` or `didRunWorkflow()` or `didAbortWorkflow()`; and
  *    - add new flags to existing workflows by overriding
- *      getCustomArgumentsForCommand().
+ *      `getCustomArgumentsForCommand()`.
  *
  * @concrete-extensible
  */

src/lint/linter/ArcanistFutureLinter.php

@@ -11,7 +11,7 @@ abstract class ArcanistFutureLinter extends ArcanistLinter {
     return 8;
   }
 
-  final public function willLintPaths(array $paths) {
+  public function willLintPaths(array $paths) {
     $limit = $this->getFuturesLimit();
     $this->futures = id(new FutureIterator(array()))->limit($limit);
     foreach ($this->buildFutures($paths) as $path => $future) {
@@ -23,7 +23,7 @@ abstract class ArcanistFutureLinter extends ArcanistLinter {
     return;
   }
 
-  final public function didLintPaths(array $paths) {
+  public function didLintPaths(array $paths) {
     if (!$this->futures) {
       return;
     }

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/ArcanistPHPCompatibilityXHPASTLinterRule.php

@@ -70,7 +70,7 @@ final class ArcanistPHPCompatibilityXHPASTLinterRule
           $symbol = $params->getChildByIndex(0);
 
           if (!$symbol->isStaticScalar()) {
-            continue;
+            break;
           }
 
           $symbol_name = $symbol->evalStatic();

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. */ ;
+}

src/parser/ArcanistCommentRemover.php

@@ -8,21 +8,25 @@ final class ArcanistCommentRemover extends Phobject {
    * considered a comment.
    */
   public static function removeComments($body) {
-    $lines = explode("\n", $body);
+    $body = rtrim($body);
+
+    $lines = phutil_split_lines($body);
     $lines = array_reverse($lines);
+
     foreach ($lines as $key => $line) {
-      if (!strlen($line)) {
+      if (preg_match('/^#/', $line)) {
-        unset($lines[$key]);
-        continue;
-      }
-      if ($line[0] == '#') {
         unset($lines[$key]);
         continue;
       }
+
       break;
     }
+
     $lines = array_reverse($lines);
-    return implode("\n", $lines)."\n";
+    $lines = implode('', $lines);
+    $lines = rtrim($lines)."\n";
+
+    return $lines;
   }
 
 }

src/parser/__tests__/ArcanistCommentRemoverTestCase.php

@@ -24,6 +24,21 @@ Here is a list:
 
 The end.
 
+EOTEXT;
+
+    $this->assertEqual($expect, ArcanistCommentRemover::removeComments($test));
+
+    $test = <<<EOTEXT
+Subscribers:
+#projectname
+
+# Instructional comments.
+EOTEXT;
+
+    $expect = <<<EOTEXT
+Subscribers:
+#projectname
+
 EOTEXT;
 
     $this->assertEqual($expect, ArcanistCommentRemover::removeComments($test));