Raise a lint warning for classes not marked "abstract", "final" or

"@concrete-extensible"

Summary:
See T795. I think ~all classes in the classtree should be "abstract" or "final",
but I provided "@concrete-extensible" if you really have "Rectangle extends
Square extends Shape" or something.

I'm not totally sure this should be enabled globally by default, maybe I should
default it to DISABLED and then enable it for libphutil/arcanist/Phabricator? It
feels like it might be a little overbearing to push on everyone by defualt.

Test Plan: See unit tests.

Reviewers: btrahan, aran, nh, arudolph, jungejason

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T795

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

src/lint/engine/phutil/PhutilLintEngine.php

@@ -88,6 +88,11 @@ class PhutilLintEngine extends ArcanistLintEngine {
     }
 
     $xhpast_linter = new ArcanistXHPASTLinter();
+    $xhpast_linter->setCustomSeverityMap(
+      array(
+        ArcanistXHPASTLinter::LINT_RAGGED_CLASSTREE_EDGE
+          => ArcanistLintSeverity::SEVERITY_WARNING,
+      ));
     $license_linter = new ArcanistApacheLicenseLinter();
     $linters[] = $xhpast_linter;
     $linters[] = $license_linter;

src/lint/engine/phutil/__init__.php

@@ -15,6 +15,7 @@ phutil_require_module('arcanist', 'lint/linter/phutilmodule');
 phutil_require_module('arcanist', 'lint/linter/spelling');
 phutil_require_module('arcanist', 'lint/linter/text');
 phutil_require_module('arcanist', 'lint/linter/xhpast');
+phutil_require_module('arcanist', 'lint/severity');
 
 
 phutil_require_source('PhutilLintEngine.php');

src/lint/linter/xhpast/ArcanistXHPASTLinter.php

@@ -51,7 +51,8 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
   const LINT_PARENTHESES_SPACING       = 25;
   const LINT_CONTROL_STATEMENT_SPACING = 26;
   const LINT_BINARY_EXPRESSION_SPACING = 27;
-  const LINT_ARRAY_INDEX_SPACING       = 27;
+  const LINT_ARRAY_INDEX_SPACING       = 28;
+  const LINT_RAGGED_CLASSTREE_EDGE     = 29;
 
 
   public function getLintNameMap() {
@@ -83,6 +84,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
       self::LINT_CONTROL_STATEMENT_SPACING => 'Space After Control Statement',
       self::LINT_BINARY_EXPRESSION_SPACING => 'Space Around Binary Operator',
       self::LINT_ARRAY_INDEX_SPACING       => 'Spacing Before Array Index',
+      self::LINT_RAGGED_CLASSTREE_EDGE     => 'Class Not abstract Or final',
     );
   }
 
@@ -110,6 +112,10 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
       self::LINT_ARRAY_INDEX_SPACING
         => ArcanistLintSeverity::SEVERITY_WARNING,
 
+      // This is disabled by default because it implies a very strict policy
+      // which isn't necessary in the general case.
+      self::LINT_RAGGED_CLASSTREE_EDGE
+        => ArcanistLintSeverity::SEVERITY_DISABLED,
     );
   }
 
@@ -175,6 +181,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
     $this->lintDuplicateKeysInArray($root);
     $this->lintReusedIterators($root);
     $this->lintBraceFormatting($root);
+    $this->lintRaggedClasstreeEdges($root);
   }
 
   private function lintBraceFormatting($root) {
@@ -1376,6 +1383,42 @@ class ArcanistXHPASTLinter extends ArcanistLinter {
     }
   }
 
+  private function lintRaggedClasstreeEdges($root) {
+    $parser = new PhutilDocblockParser();
+
+    $classes = $root->selectDescendantsOfType('n_CLASS_DECLARATION');
+    foreach ($classes as $class) {
+
+      $is_final = false;
+      $is_abstract = false;
+      $is_concrete_extensible = false;
+
+      $attributes = $class->getChildOfType(0, 'n_CLASS_ATTRIBUTES');
+      foreach ($attributes->getChildren() as $child) {
+        if ($child->getConcreteString() == 'final') {
+          $is_final = true;
+        }
+        if ($child->getConcreteString() == 'abstract') {
+          $is_abstract = true;
+        }
+      }
+
+      $docblock = $class->getDocblockToken();
+      if ($docblock) {
+        list($text, $specials) = $parser->parse($docblock->getValue());
+        $is_concrete_extensible = idx($specials, 'concrete-extensible');
+      }
+
+      if (!$is_final && !$is_abstract && !$is_concrete_extensible) {
+        $this->raiseLintAtNode(
+          $class->getChildOfType(1, 'n_CLASS_NAME'),
+          self::LINT_RAGGED_CLASSTREE_EDGE,
+          "This class is neither 'final' nor 'abstract', and does not have ".
+          "a docblock marking it '@concrete-extensible'.");
+      }
+    }
+  }
+
   protected function raiseLintAtToken(
     XHPASTToken $token,
     $code,

src/lint/linter/xhpast/__init__.php

@@ -10,6 +10,7 @@ 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/docblock');
 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__/ArcanistXHPASTLinterTestCase.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.
@@ -25,6 +25,13 @@ class ArcanistXHPASTLinterTestCase extends ArcanistLinterTestCase {
 
   public function testXHPASTLint() {
     $linter = new ArcanistXHPASTLinter();
+
+    $linter->setCustomSeverityMap(
+      array(
+        ArcanistXHPASTLinter::LINT_RAGGED_CLASSTREE_EDGE
+          => ArcanistLintSeverity::SEVERITY_WARNING,
+      ));
+
     $working_copy = ArcanistWorkingCopyIdentity::newFromPath(__FILE__);
     return $this->executeTestsInDirectory(
       dirname(__FILE__).'/data/',

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

@@ -8,6 +8,7 @@
 
 phutil_require_module('arcanist', 'lint/linter/base/test');
 phutil_require_module('arcanist', 'lint/linter/xhpast');
+phutil_require_module('arcanist', 'lint/severity');
 phutil_require_module('arcanist', 'workingcopyidentity');
 
 

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

@@ -1,5 +1,5 @@
 <?php
-class a {
+final class a {
   const b = 1, c = d;
   protected $E, $H;
   public function F($G, $GG) { }
@@ -11,7 +11,7 @@ interface i { }
 
 function YY($ZZ) { }
 
-class Quack {
+final class Quack {
   const R = 1, S = 2;
   protected $tX, $uY;
   public function vV($w_w) { }
@@ -50,7 +50,7 @@ function i() {
   parent::$Z_z[0];
 }
 ~~~~~~~~~~
-warning:2:7
+warning:2:13
 warning:3:9
 warning:3:16
 warning:4:13

src/lint/linter/xhpast/__tests__/data/parens-hug-contents.lint-test

@@ -13,7 +13,7 @@ $obj->m(
 for ( $ii = 0; $ii < 1; $ii++ ) { }
 foreach ( $x as $y ) { }
 function q( $x ) { }
-class X {
+final class X {
   public function f( $y ) {
   }
 }
@@ -35,7 +35,7 @@ warning:14:10
 warning:14:19
 warning:15:12
 warning:15:15
-error:16:7 XHP19 Class-Filename Mismatch
+error:16:13 XHP19 Class-Filename Mismatch
 warning:17:21
 warning:17:24
 warning:20:10
@@ -56,7 +56,7 @@ $obj->m(
 for ($ii = 0; $ii < 1; $ii++) { }
 foreach ($x as $y) { }
 function q($x) { }
-class X {
+final class X {
   public function f($y) {
   }
 }

src/lint/linter/xhpast/__tests__/data/ragged-classtree-edges.lint-test

@@ -0,0 +1,12 @@
+<?php
+
+class A { }
+abstract class B { }
+final class C { }
+
+/**
+ * @concrete-extensible
+ */
+class D { }
+~~~~~~~~~~
+warning:3:7

src/lint/linter/xhpast/__tests__/data/surprising-constructors.lint-test

@@ -1,9 +1,9 @@
 <?php
-class Platypus {
+final class Platypus {
   public function platypus() {
     // This method must be renamed to __construct().
   }
 }
 ~~~~~~~~~~
-error:2:7 XHP19 Class-Filename Mismatch
+error:2:13 XHP19 Class-Filename Mismatch
 error:3:19

src/lint/linter/xhpast/__tests__/data/undeclared-variables.lint-test

@@ -39,7 +39,7 @@ function g($q) {
   $r = y();
 }
 
-class C {
+final class C {
   public function m() {
     $a++;
     x($b);
@@ -135,7 +135,7 @@ function more_exceptions() {
   }
 }
 
-class P {
+abstract class P {
   abstract public function q();
 }
 
@@ -146,7 +146,7 @@ function x() {
   f(((($lub))));
 }
 
-class A {
+final class A {
   public function foo($a) {
     $im_service = foo($a);
     if ($im_servce === false) {

src/lint/linter/xhpast/__tests__/data/use-of-this-in-static-method.lint-test

@@ -1,6 +1,6 @@
 <?php
 
-class A {
+final class A {
   public function u() {
     $this->f();
   }
@@ -10,5 +10,5 @@ class A {
 }
 
 ~~~~~~~~~~
-error:3:7 XHP19 Class-Filename Mismatch
+error:3:13 XHP19 Class-Filename Mismatch
 error:8:5 Use of $this in a static method.