diff --git a/src/lint/engine/phutil/PhutilLintEngine.php b/src/lint/engine/phutil/PhutilLintEngine.php index e1db7b4b..ac4ecdd8 100644 --- a/src/lint/engine/phutil/PhutilLintEngine.php +++ b/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; diff --git a/src/lint/engine/phutil/__init__.php b/src/lint/engine/phutil/__init__.php index 2a59785f..9c270ee5 100644 --- a/src/lint/engine/phutil/__init__.php +++ b/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'); diff --git a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php index 89840212..97598bfd 100644 --- a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php +++ b/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, diff --git a/src/lint/linter/xhpast/__init__.php b/src/lint/linter/xhpast/__init__.php index b98d8edb..4b8d2cfe 100644 --- a/src/lint/linter/xhpast/__init__.php +++ b/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'); diff --git a/src/lint/linter/xhpast/__tests__/ArcanistXHPASTLinterTestCase.php b/src/lint/linter/xhpast/__tests__/ArcanistXHPASTLinterTestCase.php index fa0e41ee..b18aafd4 100644 --- a/src/lint/linter/xhpast/__tests__/ArcanistXHPASTLinterTestCase.php +++ b/src/lint/linter/xhpast/__tests__/ArcanistXHPASTLinterTestCase.php @@ -1,7 +1,7 @@ setCustomSeverityMap( + array( + ArcanistXHPASTLinter::LINT_RAGGED_CLASSTREE_EDGE + => ArcanistLintSeverity::SEVERITY_WARNING, + )); + $working_copy = ArcanistWorkingCopyIdentity::newFromPath(__FILE__); return $this->executeTestsInDirectory( dirname(__FILE__).'/data/', diff --git a/src/lint/linter/xhpast/__tests__/__init__.php b/src/lint/linter/xhpast/__tests__/__init__.php index d960ac84..98800a54 100644 --- a/src/lint/linter/xhpast/__tests__/__init__.php +++ b/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'); diff --git a/src/lint/linter/xhpast/__tests__/data/naming-conventions.lint-test b/src/lint/linter/xhpast/__tests__/data/naming-conventions.lint-test index 96403246..78e16a51 100644 --- a/src/lint/linter/xhpast/__tests__/data/naming-conventions.lint-test +++ b/src/lint/linter/xhpast/__tests__/data/naming-conventions.lint-test @@ -1,5 +1,5 @@ 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) { } } diff --git a/src/lint/linter/xhpast/__tests__/data/ragged-classtree-edges.lint-test b/src/lint/linter/xhpast/__tests__/data/ragged-classtree-edges.lint-test new file mode 100644 index 00000000..9e18089c --- /dev/null +++ b/src/lint/linter/xhpast/__tests__/data/ragged-classtree-edges.lint-test @@ -0,0 +1,12 @@ +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.