Detect and correct "final private" methods in lint

Summary:
Ref T13588. Marking a method "final private" has never been meaningful, and is an error in PHP8.

Add static analysis to detect (and correct) this issue.

Test Plan: Added unit tests, will lint "phabricator/".

Maniphest Tasks: T13588

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

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

@@ -22,68 +22,65 @@ final class ArcanistInvalidModifiersXHPASTLinterRule
       $is_final    = false;
       $is_static   = false;
       $visibility  = null;
+      $is_property = ($method->getTypeName() == 'n_CLASS_MEMBER_MODIFIER_LIST');
+      $is_method = !$is_property;
+
+      $final_modifier = null;
+      $visibility_modifier = null;
+      $abstract_modifier = null;
 
       foreach ($modifiers as $modifier) {
         switch ($modifier->getConcreteString()) {
           case 'abstract':
-            if ($method->getTypeName() == 'n_CLASS_MEMBER_MODIFIER_LIST') {
+            if ($is_property) {
               $this->raiseLintAtNode(
                 $modifier,
                 pht(
-                  'Properties cannot be declared `%s`.',
-                  'abstract'));
+                  'Properties cannot be declared "abstract".'));
             }
 
             if ($is_abstract) {
               $this->raiseLintAtNode(
                 $modifier,
                 pht(
-                  'Multiple `%s` modifiers are not allowed.',
-                  'abstract'));
-            }
-
-            if ($is_final) {
-              $this->raiseLintAtNode(
-                $modifier,
-                pht(
-                'Cannot use the `%s` modifier on an `%s` class member',
-                'final',
-                'abstract'));
+                  'Multiple "abstract" modifiers are not allowed.'));
             }
 
+            $abstract_modifier = $modifier;
             $is_abstract = true;
             break;
 
           case 'final':
-            if ($is_abstract) {
+            if ($is_property) {
               $this->raiseLintAtNode(
                 $modifier,
                 pht(
-                'Cannot use the `%s` modifier on an `%s` class member',
-                'final',
-                'abstract'));
+                  'Properties can not be declared "final".'));
             }
 
             if ($is_final) {
               $this->raiseLintAtNode(
                 $modifier,
                 pht(
-                  'Multiple `%s` modifiers are not allowed.',
-                  'final'));
+                  'Multiple "final" modifiers are not allowed.'));
             }
 
+            $final_modifier = $modifier;
             $is_final = true;
             break;
           case 'public':
           case 'protected':
           case 'private':
-            if ($visibility) {
+            if ($visibility !== null) {
               $this->raiseLintAtNode(
                 $modifier,
                 pht('Multiple access type modifiers are not allowed.'));
             }
 
+            $visibility_modifier = $modifier;
+
             $visibility = $modifier->getConcreteString();
+            $visibility = phutil_utf8_strtolower($visibility);
             break;
 
           case 'static':
@@ -91,12 +88,47 @@ final class ArcanistInvalidModifiersXHPASTLinterRule
               $this->raiseLintAtNode(
                 $modifier,
                 pht(
-                  'Multiple `%s` modifiers are not allowed.',
-                  'static'));
+                  'Multiple "static" modifiers are not allowed.'));
             }
             break;
         }
       }
+
+      $is_private = ($visibility === 'private');
+
+      if ($is_final && $is_abstract) {
+        if ($is_method) {
+          $this->raiseLintAtNode(
+            $final_modifier,
+            pht('Methods may not be both "abstract" and "final".'));
+        } else {
+          // Properties can't be "abstract" and "final" either, but they can't
+          // ever be "abstract" at all, and we've already raise a message about
+          // that earlier.
+        }
+      }
+
+      if ($is_private && $is_final) {
+        if ($is_method) {
+          $final_tokens = $final_modifier->getTokens();
+          $space_tokens = last($final_tokens)->getWhitespaceTokensAfter();
+
+          $final_offset = head($final_tokens)->getOffset();
+
+          $final_string = array_merge($final_tokens, $space_tokens);
+          $final_string = mpull($final_string, 'getValue');
+          $final_string = implode('', $final_string);
+
+          $this->raiseLintAtOffset(
+            $final_offset,
+            pht('Methods may not be both "private" and "final".'),
+            $final_string,
+            '');
+        } else {
+          // Properties can't be "final" at all, and we already raised a
+          // message about this.
+        }
+      }
     }
   }
 

src/lint/linter/xhpast/rules/__tests__/invalid-modifiers/invalid-modifiers.lint-test

@@ -5,14 +5,36 @@ class SomeClass {
   public public $b;
   public protected $c;
   private abstract $d;
+  private final $e;
 
   public function foo() {}
   public protected function bar() {}
   abstract final public function baz() {}
+  final private function wuff() {}
+  private final function fizz() {}
 }
 ~~~~~~~~~~
 error:5:10:XHP72:Invalid Modifiers
 error:6:10:XHP72:Invalid Modifiers
 error:7:11:XHP72:Invalid Modifiers
-error:10:10:XHP72:Invalid Modifiers
-error:11:12:XHP72:Invalid Modifiers
+error:8:11:XHP72:Invalid Modifiers
+error:11:10:XHP72:Invalid Modifiers
+error:12:12:XHP72:Invalid Modifiers
+error:13:3:XHP72:Invalid Modifiers
+error:14:11:XHP72:Invalid Modifiers
+~~~~~~~~~~
+<?php
+
+class SomeClass {
+  public $a;
+  public public $b;
+  public protected $c;
+  private abstract $d;
+  private final $e;
+
+  public function foo() {}
+  public protected function bar() {}
+  abstract final public function baz() {}
+  private function wuff() {}
+  private function fizz() {}
+}

src/parser/aast/api/AASTToken.php

@@ -84,6 +84,17 @@ abstract class AASTToken extends Phobject {
     return $result;
   }
 
+  public function getWhitespaceTokensAfter() {
+    $tokens = $this->tree->getRawTokenStream();
+    $result = array();
+    $ii = $this->id + 1;
+    while ($ii < count($tokens) && $tokens[$ii]->isAnyWhitespace()) {
+      $result[$ii] = $tokens[$ii];
+      ++$ii;
+    }
+    return $result;
+  }
+
   final public function getLineNumber() {
     return idx($this->tree->getOffsetToLineNumberMap(), $this->getOffset());
   }