Improve the "self member reference" linter rule

Summary:
Extends `ArcanistSelfMemberReferenceXHPASTLinterRule` to warn about the use of a hardcoded class name instead of `self` when instantiating a class. Arguably, we should maybe rename the linter rule from `ArcanistSelfMemberReferenceXHPASTLinterRule` to `ArcanistSelfUsageXHPASTLinterRule`, or even maybe split this rule into three separate rules:

  - `ArcanistSelfMemberReferenceXHPASTLinterRule`
  - `ArcanistSelfSpacingXHPASTLinterRule`
  - `ArcanistSelfClassReferenceXHPASTLinterRule`.

Test Plan: Added to existing test cases.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

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

src/__phutil_library_map__.php

@@ -272,6 +272,8 @@ phutil_register_library_map(array(
     'ArcanistPHPOpenTagXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPHPOpenTagXHPASTLinterRuleTestCase.php',
     'ArcanistPHPShortTagXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPHPShortTagXHPASTLinterRule.php',
     'ArcanistPHPShortTagXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPHPShortTagXHPASTLinterRuleTestCase.php',
+    'ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule.php',
+    'ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase.php',
     'ArcanistParentMemberReferenceXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistParentMemberReferenceXHPASTLinterRule.php',
     'ArcanistParentMemberReferenceXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistParentMemberReferenceXHPASTLinterRuleTestCase.php',
     'ArcanistParenthesesSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistParenthesesSpacingXHPASTLinterRule.php',
@@ -317,6 +319,8 @@ phutil_register_library_map(array(
     'ArcanistRubyLinter' => 'lint/linter/ArcanistRubyLinter.php',
     'ArcanistRubyLinterTestCase' => 'lint/linter/__tests__/ArcanistRubyLinterTestCase.php',
     'ArcanistScriptAndRegexLinter' => 'lint/linter/ArcanistScriptAndRegexLinter.php',
+    'ArcanistSelfClassReferenceXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php',
+    'ArcanistSelfClassReferenceXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistSelfClassReferenceXHPASTLinterRuleTestCase.php',
     'ArcanistSelfMemberReferenceXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistSelfMemberReferenceXHPASTLinterRule.php',
     'ArcanistSelfMemberReferenceXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistSelfMemberReferenceXHPASTLinterRuleTestCase.php',
     'ArcanistSemicolonSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistSemicolonSpacingXHPASTLinterRule.php',
@@ -680,6 +684,8 @@ phutil_register_library_map(array(
     'ArcanistPHPOpenTagXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistPHPShortTagXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistPHPShortTagXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
+    'ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
+    'ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistParentMemberReferenceXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistParentMemberReferenceXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistParenthesesSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
@@ -725,6 +731,8 @@ phutil_register_library_map(array(
     'ArcanistRubyLinter' => 'ArcanistExternalLinter',
     'ArcanistRubyLinterTestCase' => 'ArcanistExternalLinterTestCase',
     'ArcanistScriptAndRegexLinter' => 'ArcanistLinter',
+    'ArcanistSelfClassReferenceXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
+    'ArcanistSelfClassReferenceXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistSelfMemberReferenceXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
     'ArcanistSelfMemberReferenceXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistSemicolonSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',

src/lint/linter/xhpast/rules/ArcanistKeywordCasingXHPASTLinterRule.php

@@ -15,73 +15,98 @@ final class ArcanistKeywordCasingXHPASTLinterRule
 
   public function process(XHPASTNode $root) {
     $keywords = $root->selectTokensOfTypes(array(
-      'T_REQUIRE_ONCE',
-      'T_REQUIRE',
+      'T_ABSTRACT',
+      'T_ARRAY',
+      'T_AS',
+      'T_BREAK',
+      'T_CALLABLE',
+      'T_CASE',
+      'T_CATCH',
+      'T_CLASS',
+      'T_CLONE',
+      'T_CONST',
+      'T_CONTINUE',
+      'T_DECLARE',
+      'T_DEFAULT',
+      'T_DO',
+      'T_ECHO',
+      'T_ELSE',
+      'T_ELSEIF',
+      'T_EMPTY',
+      'T_ENDDECLARE',
+      'T_ENDFOR',
+      'T_ENDFOREACH',
+      'T_ENDIF',
+      'T_ENDSWITCH',
+      'T_ENDWHILE',
       'T_EVAL',
-      'T_INCLUDE_ONCE',
+      'T_EXIT',
+      'T_EXTENDS',
+      'T_FINAL',
+      'T_FINALLY',
+      'T_FOR',
+      'T_FOREACH',
+      'T_FUNCTION',
+      'T_GLOBAL',
+      'T_GOTO',
+      'T_HALT_COMPILER',
+      'T_IF',
+      'T_IMPLEMENTS',
       'T_INCLUDE',
+      'T_INCLUDE_ONCE',
+      'T_INSTANCEOF',
+      'T_INSTEADOF',
+      'T_INTERFACE',
+      'T_ISSET',
+      'T_LIST',
+      'T_LOGICAL_AND',
       'T_LOGICAL_OR',
       'T_LOGICAL_XOR',
-      'T_LOGICAL_AND',
-      'T_PRINT',
-      'T_INSTANCEOF',
-      'T_CLONE',
-      'T_NEW',
-      'T_EXIT',
-      'T_IF',
-      'T_ELSEIF',
-      'T_ELSE',
-      'T_ENDIF',
-      'T_ECHO',
-      'T_DO',
-      'T_WHILE',
-      'T_ENDWHILE',
-      'T_FOR',
-      'T_ENDFOR',
-      'T_FOREACH',
-      'T_ENDFOREACH',
-      'T_DECLARE',
-      'T_ENDDECLARE',
-      'T_AS',
-      'T_SWITCH',
-      'T_ENDSWITCH',
-      'T_CASE',
-      'T_DEFAULT',
-      'T_BREAK',
-      'T_CONTINUE',
-      'T_GOTO',
-      'T_FUNCTION',
-      'T_CONST',
-      'T_RETURN',
-      'T_TRY',
-      'T_CATCH',
-      'T_THROW',
-      'T_USE',
-      'T_GLOBAL',
-      'T_PUBLIC',
-      'T_PROTECTED',
-      'T_PRIVATE',
-      'T_FINAL',
-      'T_ABSTRACT',
-      'T_STATIC',
-      'T_VAR',
-      'T_UNSET',
-      'T_ISSET',
-      'T_EMPTY',
-      'T_HALT_COMPILER',
-      'T_CLASS',
-      'T_INTERFACE',
-      'T_EXTENDS',
-      'T_IMPLEMENTS',
-      'T_LIST',
-      'T_ARRAY',
       'T_NAMESPACE',
-      'T_INSTEADOF',
-      'T_CALLABLE',
+      'T_NEW',
+      'T_PRINT',
+      'T_PRIVATE',
+      'T_PROTECTED',
+      'T_PUBLIC',
+      'T_REQUIRE',
+      'T_REQUIRE_ONCE',
+      'T_RETURN',
+      'T_STATIC',
+      'T_SWITCH',
+      'T_THROW',
       'T_TRAIT',
+      'T_TRY',
+      'T_UNSET',
+      'T_USE',
+      'T_VAR',
+      'T_WHILE',
       'T_YIELD',
-      'T_FINALLY',
     ));
+
+    // Because there is no `T_SELF` or `T_PARENT` token.
+    $class_static_accesses = $root
+      ->selectDescendantsOfType('n_CLASS_DECLARATION')
+      ->selectDescendantsOfType('n_CLASS_STATIC_ACCESS');
+    foreach ($class_static_accesses as $class_static_access) {
+      $class_ref = $class_static_access->getChildByIndex(0);
+
+      switch (strtolower($class_ref->getConcreteString())) {
+        case 'parent':
+        case 'self':
+          $tokens = $class_ref->getTokens();
+
+          if (count($tokens) > 1) {
+            throw new Exception(
+              pht(
+                'Unexpected tokens whilst processing `%s`.',
+                __CLASS__));
+          }
+
+          $keywords[] = head($tokens);
+          break;
+      }
+    }
+
     foreach ($keywords as $keyword) {
       $value = $keyword->getValue();
 

src/lint/linter/xhpast/rules/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule.php

@@ -0,0 +1,40 @@
+<?php
+
+final class ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule
+  extends ArcanistXHPASTLinterRule {
+
+  const ID = 96;
+
+  public function getLintName() {
+    return pht('Paamayim Nekudotayim Spacing');
+  }
+
+  public function getLintSeverity() {
+    return ArcanistLintSeverity::SEVERITY_WARNING;
+  }
+
+  public function process(XHPASTNode $root) {
+    $double_colons = $root->selectTokensOfType('T_PAAMAYIM_NEKUDOTAYIM');
+
+    foreach ($double_colons as $double_colon) {
+      $tokens = $double_colon->getNonsemanticTokensBefore() +
+                $double_colon->getNonsemanticTokensAfter();
+
+      foreach ($tokens as $token) {
+        if ($token->isAnyWhitespace()) {
+          if (strpos($token->getValue(), "\n") !== false) {
+            continue;
+          }
+
+          $this->raiseLintAtToken(
+            $token,
+            pht(
+              'Unnecessary whitespace around paamayim nekudotayim '.
+              '(double colon) operator.'),
+            '');
+        }
+      }
+    }
+  }
+
+}

src/lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php

@@ -0,0 +1,45 @@
+<?php
+
+final class ArcanistSelfClassReferenceXHPASTLinterRule
+  extends ArcanistXHPASTLinterRule {
+
+  const ID = 95;
+
+  public function getLintName() {
+    return pht('Self Class Reference');
+  }
+
+  public function getLintSeverity() {
+    return ArcanistLintSeverity::SEVERITY_WARNING;
+  }
+
+  public function process(XHPASTNode $root) {
+    $class_declarations = $root->selectDescendantsOfType('n_CLASS_DECLARATION');
+
+    foreach ($class_declarations as $class_declaration) {
+      $class_name = $class_declaration
+        ->getChildOfType(1, 'n_CLASS_NAME')
+        ->getConcreteString();
+      $instantiations = $class_declaration
+        ->selectDescendantsOfType('n_NEW');
+
+      foreach ($instantiations as $instantiation) {
+        $type = $instantiation->getChildByIndex(0);
+
+        if ($type->getTypeName() != 'n_CLASS_NAME') {
+          continue;
+        }
+
+        if (strtolower($type->getConcreteString()) == strtolower($class_name)) {
+          $this->raiseLintAtNode(
+            $type,
+            pht(
+              'Use `%s` to instantiate the current class.',
+              'self'),
+            'self');
+        }
+      }
+    }
+  }
+
+}

src/lint/linter/xhpast/rules/ArcanistSelfMemberReferenceXHPASTLinterRule.php

@@ -10,7 +10,7 @@ final class ArcanistSelfMemberReferenceXHPASTLinterRule
   }
 
   public function getLintSeverity() {
-    return ArcanistLintSeverity::SEVERITY_ADVICE;
+    return ArcanistLintSeverity::SEVERITY_WARNING;
   }
 
   public function process(XHPASTNode $root) {
@@ -20,14 +20,11 @@ final class ArcanistSelfMemberReferenceXHPASTLinterRule
       $class_name = $class_declaration
         ->getChildOfType(1, 'n_CLASS_NAME')
         ->getConcreteString();
-
       $class_static_accesses = $class_declaration
         ->selectDescendantsOfType('n_CLASS_STATIC_ACCESS');
       $closures = $this->getAnonymousClosures($class_declaration);
 
       foreach ($class_static_accesses as $class_static_access) {
-        $double_colons = $class_static_access
-          ->selectTokensOfType('T_PAAMAYIM_NEKUDOTAYIM');
         $class_ref = $class_static_access->getChildByIndex(0);
 
         if ($class_ref->getTypeName() != 'n_CLASS_NAME') {
@@ -54,43 +51,6 @@ final class ArcanistSelfMemberReferenceXHPASTLinterRule
               'self');
           }
         }
-
-        static $self_refs = array(
-          'parent',
-          'self',
-          'static',
-        );
-
-        if (!in_array(strtolower($class_ref_name), $self_refs)) {
-          continue;
-        }
-
-        if ($class_ref_name != strtolower($class_ref_name)) {
-          $this->raiseLintAtNode(
-            $class_ref,
-            pht('PHP keywords should be lowercase.'),
-            strtolower($class_ref_name));
-        }
-      }
-    }
-
-    $double_colons = $root->selectTokensOfType('T_PAAMAYIM_NEKUDOTAYIM');
-
-    foreach ($double_colons as $double_colon) {
-      $tokens = $double_colon->getNonsemanticTokensBefore() +
-        $double_colon->getNonsemanticTokensAfter();
-
-      foreach ($tokens as $token) {
-        if ($token->isAnyWhitespace()) {
-          if (strpos($token->getValue(), "\n") !== false) {
-            continue;
-          }
-
-          $this->raiseLintAtToken(
-            $token,
-            pht('Unnecessary whitespace around double colon operator.'),
-            '');
-        }
       }
     }
   }

src/lint/linter/xhpast/rules/__tests__/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase.php

@@ -0,0 +1,11 @@
+<?php
+
+final class ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase
+  extends ArcanistXHPASTLinterRuleTestCase {
+
+  public function testLinter() {
+    $this->executeTestsInDirectory(
+      dirname(__FILE__).'/paamayim-nekudotayim-spacing/');
+  }
+
+}

src/lint/linter/xhpast/rules/__tests__/ArcanistSelfClassReferenceXHPASTLinterRuleTestCase.php

@@ -0,0 +1,10 @@
+<?php
+
+final class ArcanistSelfClassReferenceXHPASTLinterRuleTestCase
+  extends ArcanistXHPASTLinterRuleTestCase {
+
+  public function testLinter() {
+    $this->executeTestsInDirectory(dirname(__FILE__).'/self-class-reference/');
+  }
+
+}

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

@@ -0,0 +1,25 @@
+<?php
+
+class Foo extends Bar {
+  public static function doSomething() {
+    return PARENT::doSomething();
+  }
+
+  public static function doSomethingElse() {
+    return parent::doSomethingElse();
+  }
+}
+~~~~~~~~~~
+warning:5:12
+~~~~~~~~~~
+<?php
+
+class Foo extends Bar {
+  public static function doSomething() {
+    return parent::doSomething();
+  }
+
+  public static function doSomethingElse() {
+    return parent::doSomethingElse();
+  }
+}

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

@@ -0,0 +1,25 @@
+<?php
+
+class Foo extends Bar {
+  public static function doSomething() {
+    return SELF::doSomethingElse();
+  }
+
+  public static function doSomethingElse() {
+    return self::doSomething();
+  }
+}
+~~~~~~~~~~
+warning:5:12
+~~~~~~~~~~
+<?php
+
+class Foo extends Bar {
+  public static function doSomething() {
+    return self::doSomethingElse();
+  }
+
+  public static function doSomethingElse() {
+    return self::doSomething();
+  }
+}

src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/comment.lint-test

@@ -0,0 +1,5 @@
+<?php
+
+// This is quite odd, but seems somewhat reasonable.
+MyClass/* comment */::/* another comment */myMethod();
+~~~~~~~~~~

src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/newline.lint-test

@@ -0,0 +1,6 @@
+<?php
+SomeReallyLongClassName::
+  someMethod();
+SomeReallyLongClassName
+  ::someMethod();
+~~~~~~~~~~

src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/paamayim-nekudotayim-spacing.lint-test

@@ -0,0 +1,36 @@
+<?php
+
+class Foo extends Bar {
+  public function bar() {
+    echo self::FOOBAR;
+    echo self :: FOOBAR;
+  }
+}
+
+MyClass::myMethod();
+MyClass :: myMethod();
+
+MyClass::$myProperty;
+MyClass :: $myProperty;
+~~~~~~~~~~
+warning:6:14
+warning:6:17
+warning:11:8
+warning:11:11
+warning:14:8
+warning:14:11
+~~~~~~~~~~
+<?php
+
+class Foo extends Bar {
+  public function bar() {
+    echo self::FOOBAR;
+    echo self::FOOBAR;
+  }
+}
+
+MyClass::myMethod();
+MyClass::myMethod();
+
+MyClass::$myProperty;
+MyClass::$myProperty;

src/lint/linter/xhpast/rules/__tests__/self-class-reference/self-class-references.lint-test

@@ -0,0 +1,25 @@
+<?php
+
+class Foo extends Bar {
+  public static function newInstance() {
+    return new Foo();
+  }
+
+  public static function init() {
+    return new self();
+  }
+}
+~~~~~~~~~~
+warning:5:16
+~~~~~~~~~~
+<?php
+
+class Foo extends Bar {
+  public static function newInstance() {
+    return new self();
+  }
+
+  public static function init() {
+    return new self();
+  }
+}

src/lint/linter/xhpast/rules/__tests__/self-member-reference/php54.lint-test

@@ -4,12 +4,13 @@ final class SomeClass extends Phobject {
   public static function someMethod() {
     $closure = function () {
       SomeClass::someOtherMethod();
+      self::someOtherMethod();
     };
     $closure();
   }
 }
 ~~~~~~~~~~
-advice:6:7
+warning:6:7
 ~~~~~~~~~~
 <?php
 
@@ -17,6 +18,7 @@ final class SomeClass extends Phobject {
   public static function someMethod() {
     $closure = function () {
       self::someOtherMethod();
+      self::someOtherMethod();
     };
     $closure();
   }

src/lint/linter/xhpast/rules/__tests__/self-member-reference/self-member-reference.lint-test

@@ -3,59 +3,21 @@
 class Foo extends Bar {
   const FOOBAR = 'FOOBAR';
 
-  public function __construct() {
-    PARENT::__construct(null);
-  }
-
-  public function bar() {
-    echo self::FOOBAR;
-    echo Self :: FOOBAR;
-  }
-
-  public function baz(Foo $x) {
-    echo static::FOOBAR;
+  public function baz() {
     echo Foo::FOOBAR;
-
-    $x::bar();
+    echo self::FOOBAR;
   }
 }
-
-MyClass :: myMethod();
-
-SomeReallyLongClassName
-  ::someMethod();
 ~~~~~~~~~~
-advice:7:5
-advice:12:10
-advice:12:14
-advice:12:17
-advice:17:10
-advice:23:8
-advice:23:11
+warning:7:10
 ~~~~~~~~~~
 <?php
 
 class Foo extends Bar {
   const FOOBAR = 'FOOBAR';
 
-  public function __construct() {
-    parent::__construct(null);
-  }
-
-  public function bar() {
+  public function baz() {
     echo self::FOOBAR;
     echo self::FOOBAR;
   }
-
-  public function baz(Foo $x) {
-    echo static::FOOBAR;
-    echo self::FOOBAR;
-
-    $x::bar();
-  }
 }
-
-MyClass::myMethod();
-
-SomeReallyLongClassName
-  ::someMethod();