From 04de9311511de22a5bc0f03a6d561203532aad4b Mon Sep 17 00:00:00 2001
From: Joshua Spence <josh@joshuaspence.com>
Date: Tue, 9 Sep 2014 22:48:45 +1000
Subject: [PATCH] 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      | 40 +++++++++++++++++
 .../xhpast/creative-brace-use.lint-test       | 10 ++---
 .../xhpast/decl-parens-hug-closing.lint-test  | 28 ++++++------
 .../xhpast/empty-block-statement.lint-test    | 21 +++++++++
 .../__tests__/xhpast/keyword-casing.lint-test | 10 ++---
 .../xhpast/naming-conventions.lint-test       | 17 ++++---
 .../xhpast/parens-hug-contents.lint-test      | 40 ++++++++---------
 .../reused-iterator-reference.lint-test       | 38 ++++++++--------
 .../__tests__/xhpast/reused-local.lint-test   | 24 +++-------
 .../xhpast/space-around-operators.lint-test   | 16 +++----
 .../__tests__/xhpast/switches.lint-test       |  2 +-
 .../xhpast/tautological-expressions.lint-test | 20 ++++-----
 .../xhpast/undeclared-variables.lint-test     | 44 +++++++++----------
 13 files changed, 172 insertions(+), 138 deletions(-)
 create mode 100644 src/lint/linter/__tests__/xhpast/empty-block-statement.lint-test

diff --git a/src/lint/linter/ArcanistXHPASTLinter.php b/src/lint/linter/ArcanistXHPASTLinter.php
index 6f1439ae..06363b1c 100644
--- a/src/lint/linter/ArcanistXHPASTLinter.php
+++ b/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',
diff --git a/src/lint/linter/__tests__/xhpast/creative-brace-use.lint-test b/src/lint/linter/__tests__/xhpast/creative-brace-use.lint-test
index 3a5ef0ce..495f334a 100644
--- a/src/lint/linter/__tests__/xhpast/creative-brace-use.lint-test
+++ b/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) {}
 
diff --git a/src/lint/linter/__tests__/xhpast/decl-parens-hug-closing.lint-test b/src/lint/linter/__tests__/xhpast/decl-parens-hug-closing.lint-test
index 293dd4a5..43761dcd 100644
--- a/src/lint/linter/__tests__/xhpast/decl-parens-hug-closing.lint-test
+++ b/src/lint/linter/__tests__/xhpast/decl-parens-hug-closing.lint-test
@@ -8,20 +8,20 @@ function &i($x ) {}
 
 final class X {
 
-  function a($x) { }
-  function b($x ) { }
+  function a($x) {}
+  function b($x ) {}
 
-  final public static function &c($x) { }
-  final public static function &d($x ) { }
+  final public static function &c($x) {}
+  final public static function &d($x ) {}
 
   abstract private function e($x);
   abstract private function f($x );
 
 }
 
-f(function($x) { });
-f(function($x ) { });
-f(function($x ) use ($z) { });
+f(function($x) {});
+f(function($x ) {});
+f(function($x ) use ($z) {});
 ~~~~~~~~~~
 warning:4:14
 warning:7:15
@@ -42,17 +42,17 @@ function &i($x) {}
 
 final class X {
 
-  function a($x) { }
-  function b($x) { }
+  function a($x) {}
+  function b($x) {}
 
-  final public static function &c($x) { }
-  final public static function &d($x) { }
+  final public static function &c($x) {}
+  final public static function &d($x) {}
 
   abstract private function e($x);
   abstract private function f($x);
 
 }
 
-f(function($x) { });
-f(function($x) { });
-f(function($x) use ($z) { });
+f(function($x) {});
+f(function($x) {});
+f(function($x) use ($z) {});
diff --git a/src/lint/linter/__tests__/xhpast/empty-block-statement.lint-test b/src/lint/linter/__tests__/xhpast/empty-block-statement.lint-test
new file mode 100644
index 00000000..bf58f802
--- /dev/null
+++ b/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() {}
diff --git a/src/lint/linter/__tests__/xhpast/keyword-casing.lint-test b/src/lint/linter/__tests__/xhpast/keyword-casing.lint-test
index aa2d44dc..a3454b6e 100644
--- a/src/lint/linter/__tests__/xhpast/keyword-casing.lint-test
+++ b/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();
diff --git a/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test b/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test
index 78e16a51..892d6e36 100644
--- a/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test
+++ b/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test
@@ -2,29 +2,28 @@
 final class a {
   const b = 1, c = d;
   protected $E, $H;
-  public function F($G, $GG) { }
+  public function F($G, $GG) {}
 }
 
-interface i { }
+interface i {}
 
 
 
-function YY($ZZ) { }
+function YY($ZZ) {}
 
 final class Quack {
   const R = 1, S = 2;
   protected $tX, $uY;
-  public function vV($w_w) { }
+  public function vV($w_w) {}
 }
 
 
 
 
 
-function () use ($this_is_a_closure) { };
+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
diff --git a/src/lint/linter/__tests__/xhpast/parens-hug-contents.lint-test b/src/lint/linter/__tests__/xhpast/parens-hug-contents.lint-test
index 690f161f..5d2e0ada 100644
--- a/src/lint/linter/__tests__/xhpast/parens-hug-contents.lint-test
+++ b/src/lint/linter/__tests__/xhpast/parens-hug-contents.lint-test
@@ -1,24 +1,22 @@
 <?php
-if ( $x ) { }
+if ( $x ) {}
 f( );
 q(                      );
 g();
-if ($x) { }
-else if ( $y ) { }
+if ($x) {}
+else if ( $y ) {}
 
 $obj->m(
   $x,
   $y,
   $z);
-for ( $ii = 0; $ii < 1; $ii++ ) { }
-foreach ( $x as $y ) { }
-function q( $x ) { }
+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,30 +36,28 @@ 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) { }
+if ($x) {}
 f();
 q();
 g();
-if ($x) { }
-else if ($y) { }
+if ($x) {}
+else if ($y) {}
 
 $obj->m(
   $x,
   $y,
   $z);
-for ($ii = 0; $ii < 1; $ii++) { }
-foreach ($x as $y) { }
-function q($x) { }
+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
   );
diff --git a/src/lint/linter/__tests__/xhpast/reused-iterator-reference.lint-test b/src/lint/linter/__tests__/xhpast/reused-iterator-reference.lint-test
index 895af356..cbfa13b1 100644
--- a/src/lint/linter/__tests__/xhpast/reused-iterator-reference.lint-test
+++ b/src/lint/linter/__tests__/xhpast/reused-iterator-reference.lint-test
@@ -2,53 +2,53 @@
 
 function assign() {
   $ar = array();
-  foreach ($ar as &$a) { }
+  foreach ($ar as &$a) {}
   $a = 1; // Reuse of $a.
 }
 
 function expr() {
   $ar = array();
-  foreach ($ar as &$a) { }
+  foreach ($ar as &$a) {}
   $b = $a; // Reuse of $a.
 }
 
 function func_call() {
   $ar = array();
-  foreach ($ar as &$a) { }
+  foreach ($ar as &$a) {}
   $b = x($a); // Reuse of $a.
 }
 
-function x($b) { }
+function x($b) {}
 
 function iterator_reuse() {
   $ar1 = array();
   $ar2 = array();
-  foreach ($ar1 as &$a) { }
-  foreach ($ar2 as $a) { } // Reuse of $a
+  foreach ($ar1 as &$a) {}
+  foreach ($ar2 as $a) {} // Reuse of $a
 }
 
 function key_value() {
   $ar = array();
-  foreach ($ar as $k => &$v) { }
+  foreach ($ar as $k => &$v) {}
   $v++; // Reuse of $v
 }
 
 function key_value2() {
   $ar = array();
-  foreach ($ar as $k => $v) { }
+  foreach ($ar as $k => $v) {}
   $v++;
 }
 
 function unset() {
   $ar = array();
-  foreach ($ar as &$a) { }
+  foreach ($ar as &$a) {}
   unset($a);
   $a++;
 }
 
 function unset2() {
   $ar = array();
-  foreach ($ar as &$a) { }
+  foreach ($ar as &$a) {}
   $a++; // Reuse of $a
   unset($a);
 }
@@ -56,19 +56,19 @@ function unset2() {
 function twice_ref() {
   $ar1 = array();
   $ar2 = array();
-  foreach ($ar1 as &$b) { }
-  foreach ($ar2 as &$b) { }
+  foreach ($ar1 as &$b) {}
+  foreach ($ar2 as &$b) {}
 }
 
 function assign_ref(&$a) {
   $ar = array();
-  foreach ($ar as &$b) { }
+  foreach ($ar as &$b) {}
   $b = &$a;
 }
 
 function assign_ref2(&$a) {
   $ar = array();
-  foreach ($ar as &$b) { }
+  foreach ($ar as &$b) {}
   $b = &$a;
   $c = $b;
 }
@@ -82,14 +82,14 @@ function use_inside() {
 
 function variable_variable() {
   $ar = array();
-  foreach ($ar as &$$a) { }
+  foreach ($ar as &$$a) {}
   $a++;
   $$a++;
 }
 
 function closure1() {
   $ar = array();
-  foreach ($ar as &$a) { }
+  foreach ($ar as &$a) {}
   function($a) {
     $a++;
   };
@@ -98,7 +98,7 @@ function closure1() {
 function closure2() {
   function() {
     $ar = array();
-    foreach ($ar as &$a) { }
+    foreach ($ar as &$a) {}
   };
   $a++;
 }
@@ -106,14 +106,14 @@ function closure2() {
 function closure3() {
   function() {
     $ar = array();
-    foreach ($ar as &$a) { }
+    foreach ($ar as &$a) {}
     $a++; // Reuse of $a
   };
 }
 
 function closure4() {
   $ar = array();
-  foreach ($ar as &$a) { }
+  foreach ($ar as &$a) {}
   function($a) {
     unset($a);
   };
diff --git a/src/lint/linter/__tests__/xhpast/reused-local.lint-test b/src/lint/linter/__tests__/xhpast/reused-local.lint-test
index f9b4c3f6..ac2c64c4 100644
--- a/src/lint/linter/__tests__/xhpast/reused-local.lint-test
+++ b/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
diff --git a/src/lint/linter/__tests__/xhpast/space-around-operators.lint-test b/src/lint/linter/__tests__/xhpast/space-around-operators.lint-test
index acf96a3a..3b8b5542 100644
--- a/src/lint/linter/__tests__/xhpast/space-around-operators.lint-test
+++ b/src/lint/linter/__tests__/xhpast/space-around-operators.lint-test
@@ -10,8 +10,8 @@ $a -=$b;
 $a-= $b;
 $a===$b;
 $a.$b;
-function x($n=null) { }
-function x($n = null) { }
+function x($n=null) {}
+function x($n = null) {}
 
 $y /* ! */ += // ?
 $z;
@@ -19,8 +19,8 @@ $obj->method();
 Dog::bark();
 $prev = ($total == 1) ? $navids[0] : $navids[$total-1];
 $next = ($total == 1) ? $navids[0] : $navids[$current+1];
-if ($x instanceof z &&$w) { }
-if ($x instanceof z && $w) { }
+if ($x instanceof z &&$w) {}
+if ($x instanceof z && $w) {}
 f(1,2);
 ~~~~~~~~~~
 warning:3:3
@@ -48,8 +48,8 @@ $a -= $b;
 $a -= $b;
 $a === $b;
 $a.$b;
-function x($n=null) { }
-function x($n = null) { }
+function x($n=null) {}
+function x($n = null) {}
 
 $y /* ! */ += // ?
 $z;
@@ -57,6 +57,6 @@ $obj->method();
 Dog::bark();
 $prev = ($total == 1) ? $navids[0] : $navids[$total - 1];
 $next = ($total == 1) ? $navids[0] : $navids[$current + 1];
-if ($x instanceof z && $w) { }
-if ($x instanceof z && $w) { }
+if ($x instanceof z && $w) {}
+if ($x instanceof z && $w) {}
 f(1, 2);
diff --git a/src/lint/linter/__tests__/xhpast/switches.lint-test b/src/lint/linter/__tests__/xhpast/switches.lint-test
index 22501d08..1314395f 100644
--- a/src/lint/linter/__tests__/xhpast/switches.lint-test
+++ b/src/lint/linter/__tests__/xhpast/switches.lint-test
@@ -67,7 +67,7 @@ switch ($x) {
     function f() { throw new Exception(); }
     g(function() { return; });
     final class C { public function m() { return; } }
-    interface I { }
+    interface I {}
   case 5:
     do {
       break;
diff --git a/src/lint/linter/__tests__/xhpast/tautological-expressions.lint-test b/src/lint/linter/__tests__/xhpast/tautological-expressions.lint-test
index d053a351..4971f5a9 100644
--- a/src/lint/linter/__tests__/xhpast/tautological-expressions.lint-test
+++ b/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
diff --git a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test
index fb64fb00..a792b71e 100644
--- a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test
+++ b/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,7 +125,8 @@ function twice() {
 
 function more_exceptions() {
   try {
-  } catch (Exception $a) {
+    // This is intentionally left blank.
+} catch (Exception $a) {
     $a++;
   } catch (Exception $b) {
     $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