Improve Arcanist symbol name linter

Summary:
  - Move name helper functions to ArcanistXHPASTLintNamingHook to make it easier
to write custom linters.
  - Add test coverage for name functions.
  - Add 'variable' and 'global' naming convention tests.
  - Expand test cases.
  - Improve lint message error when an unexpected message is raised during a
test.
  - Remove a defunct XHP lint message.

Test Plan:
  - Ran unit tests.
  - Ran "arc lint --lintall" on arcanist/.

Reviewers: btrahan, nh, jungejason

Reviewed By: btrahan

CC: johnduhart, aran, epriestley, arudolph

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

src/__phutil_library_map__.php

@@ -99,6 +99,7 @@ phutil_register_library_map(array(
     'ArcanistWhichWorkflow' => 'workflow/which',
     'ArcanistWorkingCopyIdentity' => 'workingcopyidentity',
     'ArcanistXHPASTLintNamingHook' => 'lint/linter/xhpast/naminghook',
+    'ArcanistXHPASTLintNamingHookTestCase' => 'lint/linter/xhpast/naminghook/__tests__',
     'ArcanistXHPASTLinter' => 'lint/linter/xhpast',
     'ArcanistXHPASTLinterTestCase' => 'lint/linter/xhpast/__tests__',
     'BranchInfo' => 'branch',
@@ -170,6 +171,7 @@ phutil_register_library_map(array(
     'ArcanistUploadWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistUserAbortException' => 'ArcanistUsageException',
     'ArcanistWhichWorkflow' => 'ArcanistBaseWorkflow',
+    'ArcanistXHPASTLintNamingHookTestCase' => 'ArcanistPhutilTestCase',
     'ArcanistXHPASTLinter' => 'ArcanistLinter',
     'ArcanistXHPASTLinterTestCase' => 'ArcanistLinterTestCase',
     'ComprehensiveLintEngine' => 'ArcanistLintEngine',

src/lint/linter/base/test/ArcanistLinterTestCase.php

@@ -118,13 +118,16 @@ abstract class ArcanistLinterTestCase extends ArcanistPhutilTestCase {
   private function compareLint($file, $expect, $result) {
     $seen = array();
     $raised = array();
+    $message_map = array();
     foreach ($result->getMessages() as $message) {
       $sev = $message->getSeverity();
       $line = $message->getLine();
       $char = $message->getChar();
       $code = $message->getCode();
       $name = $message->getName();
-      $seen[] = $sev.":".$line.":".$char;
+      $message_key = $sev.":".$line.":".$char;
+      $message_map[$message_key] = $message;
+      $seen[] = $message_key;
       $raised[] = "  {$sev} at line {$line}, char {$char}: {$code} {$name}";
     }
     $expect = trim($expect);
@@ -154,11 +157,15 @@ abstract class ArcanistLinterTestCase extends ArcanistPhutilTestCase {
     }
 
     foreach (array_diff_key($seen, $expect) as $surprising => $ignored) {
+
+      $message = $message_map[$surprising];
+      $message_info = $message->getDescription();
+
       list($sev, $line, $char) = explode(':', $surprising);
       $this->assertFailure(
         "In '{$file}', ".
         "lint raised {$sev} on line {$line} at char {$char}, ".
-        "but nothing was expected. {$raised}");
+        "but nothing was expected:\n\n{$message_info}\n\n{$raised}");
     }
   }
 

src/lint/linter/xhpast/ArcanistXHPASTLinter.php

@@ -455,16 +455,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
 
       $declarations = array(
         '$this'     => 0,
-        '$GLOBALS'  => 0,
+      ) + array_fill_keys($this->getSuperGlobalNames(), 0);
-        '$_SERVER'  => 0,
-        '$_GET'     => 0,
-        '$_POST'    => 0,
-        '$_FILES'   => 0,
-        '$_COOKIE'  => 0,
-        '$_SESSION' => 0,
-        '$_REQUEST' => 0,
-        '$_ENV'     => 0,
-      );
       $declaration_tokens = array();
       $exclude_tokens = array();
       $vars = array();
@@ -711,29 +702,17 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
     foreach ($classes as $class) {
       $name_token = $class->getChildByIndex(1);
       $name_string = $name_token->getConcreteString();
-      $is_xhp = ($name_string[0] == ':');
+
-      if ($is_xhp) {
-        $names[] = array(
-          'xhp-class',
-          $name_string,
-          $name_token,
-          $this->isLowerCaseWithXHP($name_string)
-            ? null
-            : 'Follow naming conventions: XHP elements should be named using '.
-              'lower case.',
-        );
-      } else {
       $names[] = array(
         'class',
         $name_string,
         $name_token,
-          $this->isUpperCamelCase($name_string)
+        ArcanistXHPASTLintNamingHook::isUpperCamelCase($name_string)
           ? null
           : 'Follow naming conventions: classes should be named using '.
             'UpperCamelCase.',
       );
     }
-    }
 
     $ifaces = $root->selectDescendantsOfType('n_INTERFACE_DECLARATION');
     foreach ($ifaces as $iface) {
@@ -743,7 +722,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
         'interface',
         $name_string,
         $name_token,
-        $this->isUpperCamelCase($name_string)
+        ArcanistXHPASTLintNamingHook::isUpperCamelCase($name_string)
           ? null
           : 'Follow naming conventions: interfaces should be named using '.
             'UpperCamelCase.',
@@ -763,7 +742,8 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
         'function',
         $name_string,
         $name_token,
-        $this->isLowercaseWithUnderscores($name_string)
+        ArcanistXHPASTLintNamingHook::isLowercaseWithUnderscores(
+          ArcanistXHPASTLintNamingHook::stripPHPFunction($name_string))
           ? null
           : 'Follow naming conventions: functions should be named using '.
             'lowercase_with_underscores.',
@@ -779,24 +759,32 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
         'method',
         $name_string,
         $name_token,
-        $this->isLowerCamelCase($name_string)
+        ArcanistXHPASTLintNamingHook::isLowerCamelCase(
+          ArcanistXHPASTLintNamingHook::stripPHPFunction($name_string))
           ? null
           : 'Follow naming conventions: methods should be named using '.
             'lowerCamelCase.',
       );
     }
 
+    $param_tokens = array();
 
     $params = $root->selectDescendantsOfType('n_DECLARATION_PARAMETER_LIST');
     foreach ($params as $param_list) {
       foreach ($param_list->getChildren() as $param) {
         $name_token = $param->getChildByIndex(1);
+        if ($name_token->getTypeName() == 'n_VARIABLE_REFERENCE') {
+          $name_token = $name_token->getChildOfType(0, 'n_VARIABLE');
+        }
+        $param_tokens[$name_token->getID()] = true;
         $name_string = $name_token->getConcreteString();
+
         $names[] = array(
           'parameter',
           $name_string,
           $name_token,
-          $this->isLowercaseWithUnderscores($name_string)
+          ArcanistXHPASTLintNamingHook::isLowercaseWithUnderscores(
+            ArcanistXHPASTLintNamingHook::stripPHPVariable($name_string))
             ? null
             : 'Follow naming conventions: parameters should be named using '.
               'lowercase_with_underscores.',
@@ -815,7 +803,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
           'constant',
           $name_string,
           $name_token,
-          $this->isUppercaseWithUnderscores($name_string)
+          ArcanistXHPASTLintNamingHook::isUppercaseWithUnderscores($name_string)
             ? null
             : 'Follow naming conventions: class constants should be named '.
               'using UPPERCASE_WITH_UNDERSCORES.',
@@ -823,19 +811,25 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
       }
     }
 
+    $member_tokens = array();
+
     $props = $root->selectDescendantsOfType('n_CLASS_MEMBER_DECLARATION_LIST');
     foreach ($props as $prop_list) {
-      foreach ($prop_list->getChildren() as $prop) {
+      foreach ($prop_list->getChildren() as $token_id => $prop) {
         if ($prop->getTypeName() == 'n_CLASS_MEMBER_MODIFIER_LIST') {
           continue;
         }
+
         $name_token = $prop->getChildByIndex(0);
+        $member_tokens[$name_token->getID()] = true;
+
         $name_string = $name_token->getConcreteString();
         $names[] = array(
           'member',
           $name_string,
           $name_token,
-          $this->isLowerCamelCase($name_string)
+          ArcanistXHPASTLintNamingHook::isLowerCamelCase(
+            ArcanistXHPASTLintNamingHook::stripPHPVariable($name_string))
             ? null
             : 'Follow naming conventions: class properties should be named '.
               'using lowerCamelCase.',
@@ -843,6 +837,84 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
       }
     }
 
+    $superglobal_map = array_fill_keys(
+      $this->getSuperGlobalNames(),
+      true);
+
+
+    $fdefs = $root->selectDescendantsOfType('n_FUNCTION_DECLARATION');
+    $mdefs = $root->selectDescendantsOfType('n_METHOD_DECLARATION');
+    $defs = $fdefs->add($mdefs);
+
+    foreach ($defs as $def) {
+      $globals = $def->selectDescendantsOfType('n_GLOBAL_DECLARATION_LIST');
+      $globals = $globals->selectDescendantsOfType('n_VARIABLE');
+
+      $globals_map = array();
+      foreach ($globals as $global) {
+        $global_string = $global->getConcreteString();
+        $globals_map[$global_string] = true;
+        $names[] = array(
+          'global',
+          $global_string,
+          $global,
+
+          // No advice for globals, but hooks have an option to provide some.
+          null);
+      }
+
+      // Exclude access of static properties, since lint will be raised at
+      // their declaration if they're invalid and they may not conform to
+      // variable rules. This is slightly overbroad (includes the entire
+      // rhs of a "Class::..." token) to cover cases like "Class:$x[0]". These
+      // varaibles are simply made exempt from naming conventions.
+      $exclude_tokens = array();
+      $statics = $def->selectDescendantsOfType('n_CLASS_STATIC_ACCESS');
+      foreach ($statics as $static) {
+        $rhs = $static->getChildByIndex(1);
+        $rhs_vars = $def->selectDescendantsOfType('n_VARIABLE');
+        foreach ($rhs_vars as $var) {
+          $exclude_tokens[$var->getID()] = true;
+        }
+      }
+
+      $vars = $def->selectDescendantsOfType('n_VARIABLE');
+      foreach ($vars as $token_id => $var) {
+        if (isset($member_tokens[$token_id])) {
+          continue;
+        }
+        if (isset($param_tokens[$token_id])) {
+          continue;
+        }
+        if (isset($exclude_tokens[$token_id])) {
+          continue;
+        }
+
+        $var_string = $var->getConcreteString();
+
+        // Awkward artifact of "$o->{$x}".
+        $var_string = trim($var_string, '{}');
+
+        if (isset($superglobal_map[$var_string])) {
+          continue;
+        }
+        if (isset($globals_map[$var_string])) {
+          continue;
+        }
+
+        $names[] = array(
+          'variable',
+          $var_string,
+          $var,
+          ArcanistXHPASTLintNamingHook::isLowercaseWithUnderscores(
+            ArcanistXHPASTLintNamingHook::stripPHPVariable($var_string))
+              ? null
+              : 'Follow naming conventions: variables should be named using '.
+                'lowercase_with_underscores.',
+        );
+      }
+    }
+
     $engine = $this->getEngine();
     $working_copy = $engine->getWorkingCopy();
 
@@ -872,28 +944,6 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
     }
   }
 
-  protected function isUpperCamelCase($str) {
-    return preg_match('/^[A-Z][A-Za-z0-9]*$/', $str);
-  }
-
-  protected function isLowerCamelCase($str) {
-    //  Allow initial "__" for magic methods like __construct; we could also
-    //  enumerate these explicitly.
-    return preg_match('/^\$?(?:__)?[a-z][A-Za-z0-9]*$/', $str);
-  }
-
-  protected function isUppercaseWithUnderscores($str) {
-    return preg_match('/^[A-Z0-9_]+$/', $str);
-  }
-
-  protected function isLowercaseWithUnderscores($str) {
-    return preg_match('/^[&]?\$?[a-z0-9_]+$/', $str);
-  }
-
-  protected function isLowercaseWithXHP($str) {
-    return preg_match('/^:[a-z0-9_:-]+$/', $str);
-  }
-
   protected function lintSurpriseConstructors($root) {
     $classes = $root->selectDescendantsOfType('n_CLASS_DECLARATION');
     foreach ($classes as $class) {
@@ -1352,4 +1402,18 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
       $replace);
   }
 
+  public function getSuperGlobalNames() {
+    return array(
+      '$GLOBALS',
+      '$_SERVER',
+      '$_GET',
+      '$_POST',
+      '$_FILES',
+      '$_COOKIE',
+      '$_SESSION',
+      '$_REQUEST',
+      '$_ENV',
+    );
+  }
+
 }

src/lint/linter/xhpast/__init__.php

@@ -7,10 +7,11 @@
 
 
 phutil_require_module('arcanist', 'lint/linter/base');
+phutil_require_module('arcanist', 'lint/linter/xhpast/naminghook');
 phutil_require_module('arcanist', 'lint/severity');
+
 phutil_require_module('phutil', 'parser/xhpast/api/tree');
 phutil_require_module('phutil', 'parser/xhpast/bin');
-
 phutil_require_module('phutil', 'utils');
 
 

src/lint/linter/xhpast/__tests__/data/duplicate-key-in-array.lint-test

@@ -23,8 +23,8 @@ $d = array(
 $e = array(
   self::B => 'bee',
   self::B => 'bea',
-  self::B() => 'ehh',
+  self::b() => 'ehh',
-  self::$B => 'weh',
+  self::$b => 'weh',
   B => 'b',
   C::B => 'cb',
 );

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

@@ -25,6 +25,30 @@ function () use ($this_is_a_closure) { };
 
 function f(&$YY) {
 }
+
+function g() {
+  $lowerCamelCase = 0;
+  $UpperCamelCase = 0;
+  $UPPERCASE_WITH_UNDERSCORES = 0;
+  $lowercase_with_underscores = 0;
+  $mIxEd_CaSe = 0;
+}
+
+function h() {
+  global $mIxEd_CaSe;
+  $mIxEd_CaSe = 2;
+  $GLOBALS[0] = 2;
+  $_ENV[0] = 1;
+}
+
+function i() {
+  self::$X_x;
+  Other::$Y_y;
+  parent::$Z_z;
+  self::$X_x[0];
+  Other::$Y_y[0];
+  parent::$Z_z[0];
+}
 ~~~~~~~~~~
 warning:2:7
 warning:3:9
@@ -37,4 +61,8 @@ warning:5:25
 warning:8:11
 warning:12:10
 warning:12:13
-warning:26:12
+warning:26:13
+warning:30:3
+warning:31:3
+warning:32:3
+warning:34:3

src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,18 +17,37 @@
  */
 
 /**
- * You can extend this class and set "lint.xhpast.naminghook" in your
+ * You can extend this class and set ##"lint.xhpast.naminghook"## in your
- * .arcconfig to have an opportunity to override lint results for symbol names.
+ * ##.arcconfig## to have an opportunity to override lint results for symbol
+ * names.
  *
- * @task override Overriding Symbol Name Lint
+ * @task override   Overriding Symbol Name Lint Messages
+ * @task util       Name Utilities
+ * @task internal   Internals
  * @group lint
+ * @stable
  */
 abstract class ArcanistXHPASTLintNamingHook {
 
+
+/* -(  Internals  )---------------------------------------------------------- */
+
+
+  /**
+   * The constructor is final because @{class:ArcanistXHPASTLinter} is
+   * responsible for hook instantiation.
+   *
+   * @return this
+   * @task internals
+   */
   final public function __construct() {
     // <empty>
   }
 
+
+/* -(  Overriding Symbol Name Lint Messages  )------------------------------- */
+
+
   /**
    * Callback invoked for each symbol, which can override the default
    * determination of name validity or accept it by returning $default. The
@@ -57,4 +76,85 @@ abstract class ArcanistXHPASTLintNamingHook {
    */
   abstract public function lintSymbolName($type, $name, $default);
 
+
+/* -(  Name Utilities  )----------------------------------------------------- */
+
+
+  /**
+   * Returns true if a symbol name is UpperCamelCase.
+   *
+   * @param string Symbol name.
+   * @return bool True if the symbol is UpperCamelCase.
+   * @task util
+   */
+  public static function isUpperCamelCase($symbol) {
+    return preg_match('/^[A-Z][A-Za-z0-9]*$/', $symbol);
+  }
+
+
+  /**
+   * Returns true if a symbol name is lowerCamelCase.
+   *
+   * @param string Symbol name.
+   * @return bool True if the symbol is lowerCamelCase.
+   * @task util
+   */
+  public static function isLowerCamelCase($symbol) {
+    return preg_match('/^[a-z][A-Za-z0-9]*$/', $symbol);
+  }
+
+
+  /**
+   * Returns true if a symbol name is UPPERCASE_WITH_UNDERSCORES.
+   *
+   * @param string Symbol name.
+   * @return bool True if the symbol is UPPERCASE_WITH_UNDERSCORES.
+   * @task util
+   */
+  public static function isUppercaseWithUnderscores($symbol) {
+    return preg_match('/^[A-Z0-9_]+$/', $symbol);
+  }
+
+
+  /**
+   * Returns true if a symbol name is lowercase_with_underscores.
+   *
+   * @param string Symbol name.
+   * @return bool True if the symbol is lowercase_with_underscores.
+   * @task util
+   */
+  public static function isLowercaseWithUnderscores($symbol) {
+    return preg_match('/^[a-z0-9_]+$/', $symbol);
+  }
+
+
+  /**
+   * Strip non-name components from PHP function symbols. Notably, this discards
+   * the "__" magic-method signifier, to make a symbol appropriate for testing
+   * with methods like @{method:isLowerCamelCase}.
+   *
+   * @param   string Symbol name.
+   * @return  string Stripped symbol.
+   * @task util
+   */
+  public static function stripPHPFunction($symbol) {
+    //  Allow initial "__" for magic methods like __construct; we could also
+    //  enumerate these explicitly.
+    return preg_replace('/^__/', '', $symbol);
+  }
+
+
+  /**
+   * Strip non-name components from PHP variable symbols. Notably, this discards
+   * the "$", to make a symbol appropriate for testing with methods like
+   * @{method:isLowercaseWithUnderscores}.
+   *
+   * @param string Symbol name.
+   * @return bool True if the symbol is UpperCamelCase.
+   * @task util
+   */
+  public static function stripPHPVariable($symbol) {
+    return preg_replace('/^\$/', '', $symbol);
+  }
+
 }

src/lint/linter/xhpast/naminghook/__tests__/ArcanistXHPASTLintNamingHookTestCase.php

@@ -0,0 +1,84 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Test cases for @{class:ArcanistXHPASTLintNamingHook}.
+ *
+ * @group testcase
+ */
+final class ArcanistXHPASTLintNamingHookTestCase
+  extends ArcanistPhutilTestCase {
+
+  public function testCaseUtilities() {
+    $tests = array(
+      'UpperCamelCase'                    => array(1, 0, 0, 0),
+      'UpperCamelCaseROFL'                => array(1, 0, 0, 0),
+
+      'lowerCamelCase'                    => array(0, 1, 0, 0),
+      'lowerCamelCaseROFL'                => array(0, 1, 0, 0),
+
+      'UPPERCASE_WITH_UNDERSCORES'        => array(0, 0, 1, 0),
+      '_UPPERCASE_WITH_UNDERSCORES_'      => array(0, 0, 1, 0),
+      '__UPPERCASE__WITH__UNDERSCORES__'  => array(0, 0, 1, 0),
+
+      'lowercase_with_underscores'        => array(0, 0, 0, 1),
+      '_lowercase_with_underscores_'      => array(0, 0, 0, 1),
+      '__lowercase__with__underscores__'  => array(0, 0, 0, 1),
+
+      'mixedCASE_NoNsEnSe'                => array(0, 0, 0, 0),
+    );
+
+    foreach ($tests as $test => $expect) {
+      $this->assertEqual(
+        $expect[0],
+        ArcanistXHPASTLintNamingHook::isUpperCamelCase($test),
+        "UpperCamelCase: '{$test}'");
+      $this->assertEqual(
+        $expect[1],
+        ArcanistXHPASTLintNamingHook::isLowerCamelCase($test),
+        "lowerCamelCase: '{$test}'");
+      $this->assertEqual(
+        $expect[2],
+        ArcanistXHPASTLintNamingHook::isUppercaseWithUnderscores($test),
+        "UPPERCASE_WITH_UNDERSCORES: '{$test}'");
+      $this->assertEqual(
+        $expect[3],
+        ArcanistXHPASTLintNamingHook::isLowercaseWithUnderscores($test),
+        "lowercase_with_underscores: '{$test}'");
+    }
+  }
+
+  public function testStripUtilities() {
+    // Variable stripping.
+    $this->assertEqual(
+      'stuff',
+      ArcanistXHPASTLintNamingHook::stripPHPVariable('stuff'));
+    $this->assertEqual(
+      'stuff',
+      ArcanistXHPASTLintNamingHook::stripPHPVariable('$stuff'));
+
+    // Function/method stripping.
+    $this->assertEqual(
+      'construct',
+      ArcanistXHPASTLintNamingHook::stripPHPFunction('construct'));
+    $this->assertEqual(
+      'construct',
+      ArcanistXHPASTLintNamingHook::stripPHPFunction('__construct'));
+  }
+
+}

src/lint/linter/xhpast/naminghook/__tests__/__init__.php

@@ -0,0 +1,13 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('arcanist', 'lint/linter/xhpast/naminghook');
+phutil_require_module('arcanist', 'unit/engine/phutil/testcase');
+
+
+phutil_require_source('ArcanistXHPASTLintNamingHookTestCase.php');