Add a linter rule to detect implict method visibility

Summary: I'm not sure whether this is in any coding standards, but it seems to be an unspoken rule that methods should have a visiblity modifier (`public`, `protected` or `private`) explicitly specified. According to the [[http://php.net/manual/en/language.oop5.visibility.php#language.oop5.visiblity-methods | PHP documentation]], methods declared without any explicit visibility keyword are defined as public.

Test Plan: Added a test case.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

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

src/lint/linter/ArcanistXHPASTLinter.php

@@ -50,7 +50,8 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
   const LINT_ARRAY_SEPARATOR           = 48;
   const LINT_CONSTRUCTOR_PARENTHESES   = 49;
   const LINT_DUPLICATE_SWITCH_CASE     = 50;
-  const LINT_BLACKLISTED_FUNCTION      = 50;
+  const LINT_BLACKLISTED_FUNCTION      = 51;
+  const LINT_IMPLICIT_VISIBILITY       = 52;
 
   private $blacklistedFunctions = array();
   private $naminghook;
@@ -114,6 +115,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       self::LINT_CONSTRUCTOR_PARENTHESES   => 'Constructor Parentheses',
       self::LINT_DUPLICATE_SWITCH_CASE     => 'Duplicate Case Statements',
       self::LINT_BLACKLISTED_FUNCTION      => 'Use of Blacklisted Function',
+      self::LINT_IMPLICIT_VISIBILITY       => 'Implicit Method Visibility',
     );
   }
 
@@ -155,6 +157,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       self::LINT_EMPTY_STATEMENT           => $advice,
       self::LINT_ARRAY_SEPARATOR           => $advice,
       self::LINT_CONSTRUCTOR_PARENTHESES   => $advice,
+      self::LINT_IMPLICIT_VISIBILITY       => $advice,
     );
   }
 
@@ -211,7 +214,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
   public function getVersion() {
     // The version number should be incremented whenever a new rule is added.
-    return '13';
+    return '14';
   }
 
   protected function resolveFuture($path, Future $future) {
@@ -285,6 +288,8 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       'lintConstructorParentheses' => self::LINT_CONSTRUCTOR_PARENTHESES,
       'lintSwitchStatements' => self::LINT_DUPLICATE_SWITCH_CASE,
       'lintBlacklistedFunction' => self::LINT_BLACKLISTED_FUNCTION,
+      'lintMethodModifier' => self::LINT_IMPLICIT_VISIBILITY,
+      'lintPropertyModifier' => self::LINT_IMPLICIT_VISIBILITY,
     );
 
     foreach ($method_codes as $method => $codes) {
@@ -2998,6 +3003,63 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
     }
   }
 
+  private function lintMethodModifier(XHPASTNode $root) {
+    $methods = $root->selectDescendantsOfType('n_METHOD_DECLARATION');
+
+    foreach ($methods as $method) {
+      $modifier_list = $method->getChildOfType(
+        0,
+        'n_METHOD_MODIFIER_LIST');
+
+      if (!$modifier_list->getChildren()) {
+        $this->raiseLintAtNode(
+          $method,
+          self::LINT_IMPLICIT_VISIBILITY,
+          pht('Methods should have their visibility declared explicitly.'),
+          'public '.$method->getConcreteString());
+      }
+    }
+  }
+
+  private function lintPropertyModifier(XHPASTNode $root) {
+    static $visibilities = array(
+      'public',
+      'protected',
+      'private',
+    );
+
+    $nodes = $root->selectDescendantsOfType('n_CLASS_MEMBER_MODIFIER_LIST');
+
+    foreach ($nodes as $node) {
+      $modifiers = $node->getChildren();
+
+      foreach ($modifiers as $modifier) {
+        if ($modifier->getConcreteString() == 'var') {
+          $this->raiseLintAtNode(
+            $modifier,
+            self::LINT_IMPLICIT_VISIBILITY,
+            pht(
+              'Use `%s` instead of `%s` to indicate public visibility.',
+              'public',
+              'var'),
+            'public');
+          continue 2;
+        }
+
+        if (in_array($modifier->getConcreteString(), $visibilities)) {
+          continue 2;
+        }
+      }
+
+      $this->raiseLintAtNode(
+        $node,
+        self::LINT_IMPLICIT_VISIBILITY,
+        pht('Properties should have their visibility declared explicitly.'),
+        'public '.$node->getConcreteString());
+    }
+  }
+
+
   public function getSuperGlobalNames() {
     return array(
       '$GLOBALS',

src/lint/linter/__tests__/xhpast/decl-parens-hug-closing.lint-test

@@ -8,8 +8,8 @@ function &i($x ) {}
 
 final class X {
 
-  function a($x) {}
+  public function a($x) {}
-  function b($x ) {}
+  public function b($x ) {}
 
   final public static function &c($x) {}
   final public static function &d($x ) {}
@@ -26,7 +26,7 @@ f(function($x ) use ($z) {});
 warning:4:14
 warning:7:15
 error:9:13
-warning:12:16
+warning:12:23
 warning:15:37
 warning:18:33
 warning:23:14
@@ -42,8 +42,8 @@ function &i($x) {}
 
 final class X {
 
-  function a($x) {}
+  public function a($x) {}
-  function b($x) {}
+  public function b($x) {}
 
   final public static function &c($x) {}
   final public static function &d($x) {}

src/lint/linter/__tests__/xhpast/implicit-visibility.lint-test

@@ -0,0 +1,24 @@
+<?php
+final class Foo {
+  public function bar() {}
+  function baz() {}
+
+  var $x;
+  static $y;
+  private $z;
+}
+~~~~~~~~~~
+error:2:13 XHP19
+advice:4:3
+advice:6:3
+advice:7:3
+~~~~~~~~~~
+<?php
+final class Foo {
+  public function bar() {}
+  public function baz() {}
+
+  public $x;
+  public static $y;
+  private $z;
+}