From 097c894d083132c92c1e7234d3cd286971c4a74f Mon Sep 17 00:00:00 2001
From: Joshua Spence <josh@joshuaspence.com>
Date: Wed, 20 May 2015 09:43:37 +1000
Subject: [PATCH] Add linter rule for invalid modifiers

Summary: Add some linter rule to detect invalid modifiers for class methods/properties.

Test Plan: Added test cases.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D12922
---
 src/lint/linter/ArcanistXHPASTLinter.php      | 113 +++++++++++++++++-
 .../xhpast/invalid-modifiers.lint-test        |  18 +++
 2 files changed, 130 insertions(+), 1 deletion(-)
 create mode 100644 src/lint/linter/__tests__/xhpast/invalid-modifiers.lint-test

diff --git a/src/lint/linter/ArcanistXHPASTLinter.php b/src/lint/linter/ArcanistXHPASTLinter.php
index cd9744de..05f79a69 100644
--- a/src/lint/linter/ArcanistXHPASTLinter.php
+++ b/src/lint/linter/ArcanistXHPASTLinter.php
@@ -71,6 +71,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
   const LINT_INSTANCEOF_OPERATOR        = 69;
   const LINT_INVALID_DEFAULT_PARAMETER  = 70;
   const LINT_MODIFIER_ORDERING          = 71;
+  const LINT_INVALID_MODIFIERS          = 72;
 
   private $blacklistedFunctions = array();
   private $naminghook;
@@ -221,6 +222,8 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
         => pht('Invalid Default Parameter'),
       self::LINT_MODIFIER_ORDERING
         => pht('Modifier Ordering'),
+      self::LINT_INVALID_MODIFIERS
+        => pht('Invalid Modifiers'),
     );
   }
 
@@ -342,7 +345,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
   public function getVersion() {
     // The version number should be incremented whenever a new rule is added.
-    return '33';
+    return '34';
   }
 
   protected function resolveFuture($path, Future $future) {
@@ -441,6 +444,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       'lintInvalidDefaultParameters' => self::LINT_INVALID_DEFAULT_PARAMETER,
       'lintMethodModifierOrdering' => self::LINT_MODIFIER_ORDERING,
       'lintPropertyModifierOrdering' => self::LINT_MODIFIER_ORDERING,
+      'lintInvalidModifiers' => self::LINT_INVALID_MODIFIERS,
     );
 
     foreach ($method_codes as $method => $codes) {
@@ -4334,6 +4338,10 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
           $modifiers,
           $modifier_ordering));
 
+      if (count($modifier_ordering) != count($expected_modifier_ordering)) {
+        continue;
+      }
+
       if ($modifier_ordering != $expected_modifier_ordering) {
         $this->raiseLintAtNode(
           $method,
@@ -4363,6 +4371,10 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
           $modifiers,
           $modifier_ordering));
 
+      if (count($modifier_ordering) != count($expected_modifier_ordering)) {
+        continue;
+      }
+
       if ($modifier_ordering != $expected_modifier_ordering) {
         $this->raiseLintAtNode(
           $property,
@@ -4372,4 +4384,103 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       }
     }
   }
+
+  private function lintInvalidModifiers(XHPASTNode $root) {
+    $methods = $root->selectDescendantsOfTypes(array(
+      'n_CLASS_MEMBER_MODIFIER_LIST',
+      'n_METHOD_MODIFIER_LIST',
+    ));
+
+    foreach ($methods as $method) {
+      $modifiers = $method->getChildren();
+
+      $is_abstract = false;
+      $is_final    = false;
+      $is_static   = false;
+      $visibility  = null;
+
+      foreach ($modifiers as $modifier) {
+        switch ($modifier->getConcreteString()) {
+          case 'abstract':
+            if ($method->getTypeName() == 'n_CLASS_MEMBER_MODIFIER_LIST') {
+              $this->raiseLintAtNode(
+                $modifier,
+                self::LINT_INVALID_MODIFIERS,
+                pht(
+                  'Properties cannot be declared %s.',
+                  'abstract'));
+            }
+
+            if ($is_abstract) {
+              $this->raiseLintAtNode(
+                $modifier,
+                self::LINT_INVALID_MODIFIERS,
+                pht(
+                  'Multiple %s modifiers are not allowed.',
+                  'abstract'));
+            }
+
+            if ($is_final) {
+              $this->raiseLintAtNode(
+                $modifier,
+                self::LINT_INVALID_MODIFIERS,
+                pht(
+                'Cannot use the %s modifier on an %s class member',
+                'final',
+                'abstract'));
+            }
+
+            $is_abstract = true;
+            break;
+
+          case 'final':
+            if ($is_abstract) {
+              $this->raiseLintAtNode(
+                $modifier,
+                self::LINT_INVALID_MODIFIERS,
+                pht(
+                'Cannot use the %s modifier on an %s class member',
+                'final',
+                'abstract'));
+            }
+
+            if ($is_final) {
+              $this->raiseLintAtNode(
+                $modifier,
+                self::LINT_INVALID_MODIFIERS,
+                pht(
+                  'Multiple %s modifiers are not allowed.',
+                  'final'));
+            }
+
+            $is_final = true;
+            break;
+          case 'public':
+          case 'protected':
+          case 'private':
+            if ($visibility) {
+              $this->raiseLintAtNode(
+                $modifier,
+                self::LINT_INVALID_MODIFIERS,
+                pht('Multiple access type modifiers are not allowed.'));
+            }
+
+            $visibility = $modifier->getConcreteString();
+            break;
+
+          case 'static':
+            if ($is_static) {
+              $this->raiseLintAtNode(
+                $modifier,
+                self::LINT_INVALID_MODIFIERS,
+                pht(
+                  'Multiple %s modifiers are not allowed.',
+                  'static'));
+            }
+            break;
+        }
+      }
+    }
+  }
+
 }
diff --git a/src/lint/linter/__tests__/xhpast/invalid-modifiers.lint-test b/src/lint/linter/__tests__/xhpast/invalid-modifiers.lint-test
new file mode 100644
index 00000000..a84ef235
--- /dev/null
+++ b/src/lint/linter/__tests__/xhpast/invalid-modifiers.lint-test
@@ -0,0 +1,18 @@
+<?php
+class SomeClass {
+  public $a;
+  public public $b;
+  public protected $c;
+  private abstract $d;
+
+  public function foo() {}
+  public protected function bar() {}
+  abstract final public function baz() {}
+}
+~~~~~~~~~~
+error:2:7 XHP19
+error:4:10
+error:5:10
+error:6:11
+error:9:10
+error:10:12