Add an XHPAST lint rule for empty block statements

Summary: Adds an XHPAST linter rule for empty block statements. Basically, if a block statement is empty then it is much neater if the opening (`{`) and closing (`}`) braces are adjacent. Maybe this is just my own personal preference, in which case we could reduce the default severity to `ArcanistLintSeverity::SEVERITY_DISABLED`.

Test Plan: Wrote unit tests. I had to modify a bunch of existing unit tests accordingly.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

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

src/lint/linter/ArcanistXHPASTLinter.php

@@ -46,6 +46,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
   const LINT_CONCATENATION_OPERATOR    = 44;
   const LINT_PHP_COMPATIBILITY         = 45;
   const LINT_LANGUAGE_CONSTRUCT_PAREN  = 46;
+  const LINT_EMPTY_STATEMENT           = 47;
 
   private $naminghook;
   private $switchhook;
@@ -103,6 +104,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       self::LINT_CONCATENATION_OPERATOR    => 'Concatenation Spacing',
       self::LINT_PHP_COMPATIBILITY         => 'PHP Compatibility',
       self::LINT_LANGUAGE_CONSTRUCT_PAREN  => 'Language Construct Parentheses',
+      self::LINT_EMPTY_STATEMENT           => 'Empty Block Statement',
     );
   }
 
@@ -141,6 +143,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       self::LINT_SEMICOLON_SPACING         => $advice,
       self::LINT_CONCATENATION_OPERATOR    => $warning,
       self::LINT_LANGUAGE_CONSTRUCT_PAREN  => $warning,
+      self::LINT_EMPTY_STATEMENT           => $advice,
     );
   }
 
@@ -259,6 +262,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
         self::LINT_CONCATENATION_OPERATOR,
       'lintPHPCompatibility' => self::LINT_PHP_COMPATIBILITY,
       'lintLanguageConstructParentheses' => self::LINT_LANGUAGE_CONSTRUCT_PAREN,
+      'lintEmptyBlockStatements' => self::LINT_EMPTY_STATEMENT,
     );
 
     foreach ($method_codes as $method => $codes) {
@@ -2555,6 +2559,42 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
     }
   }
 
+  protected function lintEmptyBlockStatements(XHPASTNode $root) {
+    $nodes = $root->selectDescendantsOfType('n_STATEMENT_LIST');
+
+    foreach ($nodes as $node) {
+      $tokens = $node->getTokens();
+      $token = head($tokens);
+
+      if (count($tokens) <= 2) {
+        continue;
+      }
+
+      // Safety check... if the first token isn't an opening brace then
+      // there's nothing to do here.
+      if ($token->getTypeName() != '{') {
+        continue;
+      }
+
+      $only_whitespace = true;
+      for ($token = $token->getNextToken();
+           $token && $token->getTypeName() != '}';
+           $token = $token->getNextToken()) {
+        $only_whitespace = $only_whitespace && $token->isAnyWhitespace();
+      }
+
+      if (count($tokens) > 2 && $only_whitespace) {
+        $this->raiseLintAtNode(
+          $node,
+          self::LINT_EMPTY_STATEMENT,
+          pht(
+            "Braces for an empty block statement shouldn't ".
+            "contain only whitespace."),
+          '{}');
+      }
+    }
+  }
+
   public function getSuperGlobalNames() {
     return array(
       '$GLOBALS',

src/lint/linter/__tests__/xhpast/creative-brace-use.lint-test

@@ -28,7 +28,9 @@ catch (Exception $x)
 
 function h(){}
 ~~~~~~~~~~
+advice:3:14
 warning:7:13
+advice:8:1
 warning:12:7
 warning:15:20
 warning:18:10
@@ -39,13 +41,9 @@ warning:29:13
 ~~~~~~~~~~
 <?php
 
-function f() {
+function f() {}
 
-}
-
-function g() {
-
-}
+function g() {}
 
 if (1) {}
 

src/lint/linter/__tests__/xhpast/empty-block-statement.lint-test

@@ -0,0 +1,21 @@
+<?php
+function w() {}
+function x() {
+  // This is deliberately empty.
+}
+function y() { }
+function z() {
+
+
+}
+~~~~~~~~~~
+advice:6:14
+advice:7:14
+~~~~~~~~~~
+<?php
+function w() {}
+function x() {
+  // This is deliberately empty.
+}
+function y() {}
+function z() {}

src/lint/linter/__tests__/xhpast/keyword-casing.lint-test

@@ -10,9 +10,7 @@ nUlL;
 array();
 Array();
 
-function f(array $x, ArRaY $y) {
-
-}
+function f(array $x, ArRaY $y) {}
 
 new C();
 NeW C();
@@ -23,7 +21,7 @@ warning:8:1
 warning:9:1
 warning:11:1
 warning:13:22
-warning:18:1
+warning:16:1
 ~~~~~~~~~~
 <?php
 
@@ -37,9 +35,7 @@ null;
 array();
 array();
 
-function f(array $x, array $y) {
-
-}
+function f(array $x, array $y) {}
 
 new C();
 new C();

src/lint/linter/__tests__/xhpast/naming-conventions.lint-test

@@ -23,8 +23,7 @@ final class Quack {
 
 function () use ($this_is_a_closure) {};
 
-function f(&$YY) {
-}
+function f(&$YY) {}
 
 function g() {
   $lowerCamelCase = 0;
@@ -62,7 +61,7 @@ warning:8:11
 warning:12:10
 warning:12:13
 warning:26:13
+warning:29:3
 warning:30:3
 warning:31:3
-warning:32:3
-warning:34:3
+warning:33:3

src/lint/linter/__tests__/xhpast/parens-hug-contents.lint-test

@@ -14,11 +14,9 @@ for ( $ii = 0; $ii < 1; $ii++ ) { }
 foreach ( $x as $y ) {}
 function q( $x ) {}
 final class X {
-  public function f( $y ) {
-  }
-}
-foreach ( $z as $k => $v ) {
+  public function f( $y ) {}
 }
+foreach ( $z as $k => $v ) {}
 some_call( /* respect authorial intent */ $b,
   $a // if comments are present
   );
@@ -38,8 +36,8 @@ warning:15:15
 error:16:13 XHP19 Class-Filename Mismatch
 warning:17:21
 warning:17:24
-warning:20:10
-warning:20:25
+warning:19:10
+warning:19:25
 ~~~~~~~~~~
 <?php
 if ($x) {}
@@ -57,11 +55,9 @@ for ($ii = 0; $ii < 1; $ii++) { }
 foreach ($x as $y) {}
 function q($x) {}
 final class X {
-  public function f($y) {
-  }
-}
-foreach ($z as $k => $v) {
+  public function f($y) {}
 }
+foreach ($z as $k => $v) {}
 some_call( /* respect authorial intent */ $b,
   $a // if comments are present
   );

src/lint/linter/__tests__/xhpast/reused-local.lint-test

@@ -1,21 +1,15 @@
 <?php
 
 function f($stuff, $thing) {
-  foreach ($stuff as $ii) {
-
-  }
+  foreach ($stuff as $ii) {}
 
   // OK: Only reused for iteration.
 
-  foreach ($stuff as $ii) {
-
-  }
+  foreach ($stuff as $ii) {}
 }
 
 function g($stuff, $thing) {
-  foreach ($stuff as $thing) {
-
-  }
+  foreach ($stuff as $thing) {}
 
   // OK: Not reused later.
 }
@@ -24,9 +18,7 @@ function h($stuff, $thing) {
 
   // OK: Used afterwards but not before.
 
-  foreach ($stuff as $key => $val) {
-
-  }
+  foreach ($stuff as $key => $val) {}
 
   $key = 1;
   $thing = 1;
@@ -68,11 +60,9 @@ function k($stuff, $thing) {
   f($thing);
 
   $other = array();
-  foreach ($other as $item) {
-
-  }
+  foreach ($other as $item) {}
 }
 
 ~~~~~~~~~~
-error:51:22
-error:61:22
+error:43:22
+error:53:22

src/lint/linter/__tests__/xhpast/tautological-expressions.lint-test

@@ -1,16 +1,12 @@
 <?php
 
-if ($x == $x) {
-}
+if ($x == $x) {}
 
-if ($x->m(3) < $x->m(3)) {
-}
+if ($x->m(3) < $x->m(3)) {}
 
-if ($y[2] - $y[2]) {
-}
+if ($y[2] - $y[2]) {}
 
-if ($x == $y) {
-}
+if ($x == $y) {}
 
 // See xhpast 0.54 -> 0.55.
 return $a->sub->value < $b->sub->value;
@@ -22,7 +18,7 @@ $skip_cache = f();
 
 ~~~~~~~~~~
 error:3:5
-error:6:5
-error:9:5
-error:18:15
-error:20:15
+error:5:5
+error:7:5
+error:14:15
+error:16:15

src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test

@@ -9,9 +9,7 @@ function f($a, $b) {
   global $e, $f;
   $g = $h = x();
   list($i, list($j, $k)) = y();
-  foreach (q() as $l => $m) {
-
-  }
+  foreach (q() as $l => $m) {}
 
   $a++;
   $b++;
@@ -59,9 +57,7 @@ function superglobals() {
 }
 
 function ref_foreach($x) {
-  foreach ($x as &$z) {
-
-  }
+  foreach ($x as &$z) {}
   $z++;
 }
 
@@ -110,6 +106,7 @@ function instance_class() {
 
 function exception_vars() {
   try {
+    // This is intentionally left blank.
   } catch (Exception $y) {
     $y++;
   }
@@ -128,6 +125,7 @@ function twice() {
 
 function more_exceptions() {
   try {
+    // This is intentionally left blank.
 } catch (Exception $a) {
     $a++;
   } catch (Exception $b) {
@@ -175,21 +173,21 @@ function catchy() {
 }
 
 ~~~~~~~~~~
+error:28:3
 error:30:3
-error:32:3
-error:38:3
+error:36:3
+error:42:5
+error:43:7
 error:44:5
-error:45:7
-error:46:5
-error:47:5
-error:48:10
-error:53:10 worst ever
-warning:65:3
-error:91:3 This stuff is basically testing the lexer/parser for function decls.
-error:108:15 Variables in instance derefs should be checked, static should not.
-error:121:3 isset() and empty() should not trigger errors.
-error:125:3 Should only warn once in this function.
-error:146:8
-error:152:9
-error:166:9
-error:173:5
+error:45:5
+error:46:10
+error:51:10 worst ever
+warning:61:3
+error:87:3 This stuff is basically testing the lexer/parser for function decls.
+error:104:15 Variables in instance derefs should be checked, static should not.
+error:118:3 isset() and empty() should not trigger errors.
+error:122:3 Should only warn once in this function.
+error:144:8
+error:150:9
+error:164:9
+error:171:5