diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a0bedcaf..ddd71167 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -21,6 +21,7 @@ phutil_register_library_map(array( 'ArcanistBaseTestResultParser' => 'unit/engine/ArcanistBaseTestResultParser.php', 'ArcanistBaseUnitTestEngine' => 'unit/engine/ArcanistBaseUnitTestEngine.php', 'ArcanistBaseWorkflow' => 'workflow/ArcanistBaseWorkflow.php', + 'ArcanistBaseXHPASTLinter' => 'lint/linter/ArcanistBaseXHPASTLinter.php', 'ArcanistBranchWorkflow' => 'workflow/ArcanistBranchWorkflow.php', 'ArcanistBrowseWorkflow' => 'workflow/ArcanistBrowseWorkflow.php', 'ArcanistBundle' => 'parser/ArcanistBundle.php', @@ -106,6 +107,8 @@ phutil_register_library_map(array( 'ArcanistPhutilTestCaseTestCase' => 'unit/engine/phutil/testcase/ArcanistPhutilTestCaseTestCase.php', 'ArcanistPhutilTestSkippedException' => 'unit/engine/phutil/testcase/ArcanistPhutilTestSkippedException.php', 'ArcanistPhutilTestTerminatedException' => 'unit/engine/phutil/testcase/ArcanistPhutilTestTerminatedException.php', + 'ArcanistPhutilXHPASTLinter' => 'lint/linter/ArcanistPhutilXHPASTLinter.php', + 'ArcanistPhutilXHPASTLinterTestCase' => 'lint/linter/__tests__/ArcanistPhutilXHPASTLinterTestCase.php', 'ArcanistPyFlakesLinter' => 'lint/linter/ArcanistPyFlakesLinter.php', 'ArcanistPyLintLinter' => 'lint/linter/ArcanistPyLintLinter.php', 'ArcanistRepoUtilsTestCase' => 'repository/util/__tests__/ArcanistRepoUtilsTestCase.php', @@ -172,6 +175,7 @@ phutil_register_library_map(array( 'ArcanistArcanistLinterTestCase' => 'ArcanistLinterTestCase', 'ArcanistBaseCommitParserTestCase' => 'ArcanistTestCase', 'ArcanistBaseWorkflow' => 'Phobject', + 'ArcanistBaseXHPASTLinter' => 'ArcanistLinter', 'ArcanistBranchWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistBrowseWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistBundleTestCase' => 'ArcanistTestCase', @@ -234,6 +238,8 @@ phutil_register_library_map(array( 'ArcanistPhutilTestCaseTestCase' => 'ArcanistPhutilTestCase', 'ArcanistPhutilTestSkippedException' => 'Exception', 'ArcanistPhutilTestTerminatedException' => 'Exception', + 'ArcanistPhutilXHPASTLinter' => 'ArcanistBaseXHPASTLinter', + 'ArcanistPhutilXHPASTLinterTestCase' => 'ArcanistArcanistLinterTestCase', 'ArcanistPyFlakesLinter' => 'ArcanistLinter', 'ArcanistPyLintLinter' => 'ArcanistLinter', 'ArcanistRepoUtilsTestCase' => 'ArcanistTestCase', @@ -264,7 +270,7 @@ phutil_register_library_map(array( 'ArcanistUserAbortException' => 'ArcanistUsageException', 'ArcanistWhichWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistXHPASTLintNamingHookTestCase' => 'ArcanistTestCase', - 'ArcanistXHPASTLinter' => 'ArcanistLinter', + 'ArcanistXHPASTLinter' => 'ArcanistBaseXHPASTLinter', 'ArcanistXHPASTLinterTestCase' => 'ArcanistArcanistLinterTestCase', 'ComprehensiveLintEngine' => 'ArcanistLintEngine', 'ExampleLintEngine' => 'ArcanistLintEngine', diff --git a/src/lint/engine/PhutilLintEngine.php b/src/lint/engine/PhutilLintEngine.php index e7c897db..267dd5df 100644 --- a/src/lint/engine/PhutilLintEngine.php +++ b/src/lint/engine/PhutilLintEngine.php @@ -46,9 +46,16 @@ class PhutilLintEngine extends ArcanistLintEngine { $linters[] = id(new ArcanistTextLinter())->setPaths($text_paths); $linters[] = id(new ArcanistSpellingLinter())->setPaths($text_paths); - $linters[] = id(new ArcanistXHPASTLinter()) + $php_paths = preg_grep('/\.php$/', $paths); + + $xhpast_linter = id(new ArcanistXHPASTLinter()) ->setCustomSeverityMap($this->getXHPASTSeverityMap()) - ->setPaths(preg_grep('/\.php$/', $paths)); + ->setPaths($php_paths); + $linters[] = $xhpast_linter; + + $linters[] = id(new ArcanistPhutilXHPASTLinter()) + ->setXHPASTLinter($xhpast_linter) + ->setPaths($php_paths); return $linters; } @@ -61,11 +68,9 @@ class PhutilLintEngine extends ArcanistLintEngine { return array( ArcanistXHPASTLinter::LINT_PHP_53_FEATURES => $error, ArcanistXHPASTLinter::LINT_PHP_54_FEATURES => $error, - ArcanistXHPASTLinter::LINT_PHT_WITH_DYNAMIC_STRING => $error, ArcanistXHPASTLinter::LINT_COMMENT_SPACING => $error, ArcanistXHPASTLinter::LINT_RAGGED_CLASSTREE_EDGE => $warning, ArcanistXHPASTLinter::LINT_TODO_COMMENT => $advice, - ArcanistXHPASTLinter::LINT_ARRAY_COMBINE => $warning, ); } } diff --git a/src/lint/linter/ArcanistBaseXHPASTLinter.php b/src/lint/linter/ArcanistBaseXHPASTLinter.php new file mode 100644 index 00000000..1d36ffff --- /dev/null +++ b/src/lint/linter/ArcanistBaseXHPASTLinter.php @@ -0,0 +1,34 @@ +raiseLintAtOffset( + $token->getOffset(), + $code, + $desc, + $token->getValue(), + $replace); + } + + protected function raiseLintAtNode( + XHPASTNode $node, + $code, + $desc, + $replace = null) { + return $this->raiseLintAtOffset( + $node->getOffset(), + $code, + $desc, + $node->getConcreteString(), + $replace); + } + +} diff --git a/src/lint/linter/ArcanistPhutilXHPASTLinter.php b/src/lint/linter/ArcanistPhutilXHPASTLinter.php new file mode 100644 index 00000000..49cffbc1 --- /dev/null +++ b/src/lint/linter/ArcanistPhutilXHPASTLinter.php @@ -0,0 +1,149 @@ +xhpastLinter = $linter; + return $this; + } + + public function setEngine(ArcanistLintEngine $engine) { + if (!$this->xhpastLinter) { + throw new Exception( + 'Call setXHPASTLinter() before using ArcanistPhutilXHPASTLinter.'); + } + $this->xhpastLinter->setEngine($engine); + return parent::setEngine($engine); + } + + public function getLintNameMap() { + return array( + self::LINT_PHT_WITH_DYNAMIC_STRING => 'Use of pht() on Dynamic String', + self::LINT_ARRAY_COMBINE => 'array_combine() Unreliable', + ); + } + + public function getLintSeverityMap() { + return array( + self::LINT_ARRAY_COMBINE => ArcanistLintSeverity::SEVERITY_WARNING, + ); + } + + public function getLinterName() { + return 'PHLXHP'; + } + + public function willLintPaths(array $paths) { + $this->xhpastLinter->willLintPaths($paths); + } + + public function lintPath($path) { + $tree = $this->xhpastLinter->getXHPASTTreeForPath($path); + if (!$tree) { + return; + } + + $root = $tree->getRootNode(); + + $this->lintPHT($root); + $this->lintArrayCombine($root); + } + + + private function lintPHT($root) { + $calls = $root->selectDescendantsOfType('n_FUNCTION_CALL'); + foreach ($calls as $call) { + $name = $call->getChildByIndex(0)->getConcreteString(); + if (strcasecmp($name, 'pht') != 0) { + continue; + } + + $parameters = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST'); + if (!$parameters->getChildren()) { + continue; + } + + $identifier = $parameters->getChildByIndex(0); + if ($this->isConstantString($identifier)) { + continue; + } + + $this->raiseLintAtNode( + $call, + self::LINT_PHT_WITH_DYNAMIC_STRING, + "The first parameter of pht() can be only a scalar string, ". + "otherwise it can't be extracted."); + } + } + + private function isConstantString(XHPASTNode $node) { + $value = $node->getConcreteString(); + + switch ($node->getTypeName()) { + case 'n_HEREDOC': + if ($value[3] == "'") { // Nowdoc: <<<'EOT' + return true; + } + $value = preg_replace('/^.+\n|\n.*$/', '', $value); + break; + + case 'n_STRING_SCALAR': + if ($value[0] == "'") { + return true; + } + $value = substr($value, 1, -1); + break; + + case 'n_CONCATENATION_LIST': + foreach ($node->getChildren() as $child) { + if ($child->getTypeName() == 'n_OPERATOR') { + continue; + } + if (!$this->isConstantString($child)) { + return false; + } + } + return true; + + default: + return false; + } + + return preg_match('/^((?>[^$\\\\]*)|\\\\.)*$/s', $value); + } + + + private function lintArrayCombine($root) { + $function_calls = $root->selectDescendantsOfType('n_FUNCTION_CALL'); + foreach ($function_calls as $call) { + $name = $call->getChildByIndex(0)->getConcreteString(); + if (strcasecmp($name, 'array_combine') == 0) { + $parameter_list = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST'); + if (count($parameter_list->getChildren()) !== 2) { + // Wrong number of parameters, but raise that elsewhere if we want. + continue; + } + + $first = $parameter_list->getChildByIndex(0); + $second = $parameter_list->getChildByIndex(1); + + if ($first->getConcreteString() == $second->getConcreteString()) { + $this->raiseLintAtNode( + $call, + self::LINT_ARRAY_COMBINE, + 'Prior to PHP 5.4, array_combine() fails when given empty '. + 'arrays. Prefer to write array_combine(x, x) as array_fuse(x).'); + } + } + } + } + +} diff --git a/src/lint/linter/ArcanistXHPASTLinter.php b/src/lint/linter/ArcanistXHPASTLinter.php index 63b3c77d..05ed652c 100644 --- a/src/lint/linter/ArcanistXHPASTLinter.php +++ b/src/lint/linter/ArcanistXHPASTLinter.php @@ -5,7 +5,7 @@ * * @group linter */ -final class ArcanistXHPASTLinter extends ArcanistLinter { +final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter { protected $trees = array(); @@ -40,11 +40,9 @@ final class ArcanistXHPASTLinter extends ArcanistLinter { const LINT_IMPLICIT_FALLTHROUGH = 30; const LINT_PHP_53_FEATURES = 31; const LINT_REUSED_AS_ITERATOR = 32; - const LINT_PHT_WITH_DYNAMIC_STRING = 33; const LINT_COMMENT_SPACING = 34; const LINT_PHP_54_FEATURES = 35; const LINT_SLOWNESS = 36; - const LINT_ARRAY_COMBINE = 37; public function getLintNameMap() { @@ -81,10 +79,8 @@ final class ArcanistXHPASTLinter extends ArcanistLinter { self::LINT_PHP_53_FEATURES => 'Use Of PHP 5.3 Features', self::LINT_PHP_54_FEATURES => 'Use Of PHP 5.4 Features', self::LINT_REUSED_AS_ITERATOR => 'Variable Reused As Iterator', - self::LINT_PHT_WITH_DYNAMIC_STRING => 'Use of pht() on Dynamic String', self::LINT_COMMENT_SPACING => 'Comment Spaces', self::LINT_SLOWNESS => 'Slow Construct', - self::LINT_ARRAY_COMBINE => 'array_combine() Unreliable', ); } @@ -108,7 +104,6 @@ final class ArcanistXHPASTLinter extends ArcanistLinter { self::LINT_BINARY_EXPRESSION_SPACING => $warning, self::LINT_ARRAY_INDEX_SPACING => $warning, self::LINT_IMPLICIT_FALLTHROUGH => $warning, - self::LINT_PHT_WITH_DYNAMIC_STRING => $disabled, self::LINT_SLOWNESS => $warning, self::LINT_COMMENT_SPACING => $advice, @@ -120,24 +115,27 @@ final class ArcanistXHPASTLinter extends ArcanistLinter { // a specific minimum version. self::LINT_PHP_53_FEATURES => $disabled, self::LINT_PHP_54_FEATURES => $disabled, - - // This message specifically recommends array_fuse(), a libphutil - // function. - self::LINT_ARRAY_COMBINE => $disabled, ); } public function willLintPaths(array $paths) { $futures = array(); foreach ($paths as $path) { + if (array_key_exists($path, $this->trees)) { + continue; + } $futures[$path] = xhpast_get_parser_future($this->getData($path)); } foreach (Futures($futures)->limit(8) as $path => $future) { $this->willLintPath($path); + $this->trees[$path] = null; try { $this->trees[$path] = XHPASTTree::newFromDataAndResolvedExecFuture( $this->getData($path), $future->resolve()); + $root = $this->trees[$path]->getRootNode(); + $root->buildSelectCache(); + $root->buildTokenCache(); } catch (XHPASTSyntaxErrorException $ex) { $this->raiseLintAtLine( $ex->getErrorLine(), @@ -171,15 +169,12 @@ final class ArcanistXHPASTLinter extends ArcanistLinter { } public function lintPath($path) { - if (empty($this->trees[$path])) { + if (!$this->trees[$path]) { return; } $root = $this->trees[$path]->getRootNode(); - $root->buildSelectCache(); - $root->buildTokenCache(); - $this->lintUseOfThisInStaticMethods($root); $this->lintDynamicDefines($root); $this->lintSurpriseConstructors($root); @@ -206,10 +201,8 @@ final class ArcanistXHPASTLinter extends ArcanistLinter { $this->lintImplicitFallthrough($root); $this->lintPHP53Features($root); $this->lintPHP54Features($root); - $this->lintPHT($root); $this->lintStrposUsedForStart($root); $this->lintStrstrUsedForCheck($root); - $this->lintArrayCombine($root); } public function lintStrstrUsedForCheck($root) { @@ -297,68 +290,6 @@ final class ArcanistXHPASTLinter extends ArcanistLinter { } } - public function lintPHT($root) { - $calls = $root->selectDescendantsOfType('n_FUNCTION_CALL'); - foreach ($calls as $call) { - $name = strtolower($call->getChildByIndex(0)->getConcreteString()); - if ($name != 'pht') { - continue; - } - - $parameters = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST'); - if (!$parameters->getChildren()) { - continue; - } - - $identifier = $parameters->getChildByIndex(0); - if ($this->isConstantString($identifier)) { - continue; - } - - $this->raiseLintAtNode( - $call, - self::LINT_PHT_WITH_DYNAMIC_STRING, - "The first parameter of pht() can be only a scalar string, ". - "otherwise it can't be extracted."); - } - } - - private function isConstantString(XHPASTNode $node) { - $value = $node->getConcreteString(); - - switch ($node->getTypeName()) { - case 'n_HEREDOC': - if ($value[3] == "'") { // Nowdoc: <<<'EOT' - return true; - } - $value = preg_replace('/^.+\n|\n.*$/', '', $value); - break; - - case 'n_STRING_SCALAR': - if ($value[0] == "'") { - return true; - } - $value = substr($value, 1, -1); - break; - - case 'n_CONCATENATION_LIST': - foreach ($node->getChildren() as $child) { - if ($child->getTypeName() == 'n_OPERATOR') { - continue; - } - if (!$this->isConstantString($child)) { - return false; - } - } - return true; - - default: - return false; - } - - return preg_match('/^((?>[^$\\\\]*)|\\\\.)*$/s', $value); - } - public function lintPHP53Features($root) { $functions = $root->selectTokensOfType('T_FUNCTION'); @@ -1806,32 +1737,6 @@ final class ArcanistXHPASTLinter extends ArcanistLinter { } } - protected function lintArrayCombine($root) { - $function_calls = $root->selectDescendantsOfType('n_FUNCTION_CALL'); - foreach ($function_calls as $call) { - $name = $call->getChildByIndex(0)->getConcreteString(); - if (strtolower($name) === 'array_combine') { - $parameter_list = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST'); - if (count($parameter_list->getChildren()) !== 2) { - // Wrong number of parameters, but raise that elsewhere if we want. - continue; - } - - $first = $parameter_list->getChildByIndex(0); - $second = $parameter_list->getChildByIndex(1); - - if ($first->getConcreteString() == $second->getConcreteString()) { - $this->raiseLintAtNode( - $call, - self::LINT_ARRAY_COMBINE, - 'Prior to PHP 5.4, array_combine() fails when given empty '. - 'arrays. Prefer to write array_combine(x, x) as array_fuse(x).'); - } - } - } - } - - /** * Exit is parsed as an expression, but using it as such is almost always * wrong. That is, this is valid: @@ -2058,32 +1963,6 @@ final class ArcanistXHPASTLinter extends ArcanistLinter { } } - protected function raiseLintAtToken( - XHPASTToken $token, - $code, - $desc, - $replace = null) { - return $this->raiseLintAtOffset( - $token->getOffset(), - $code, - $desc, - $token->getValue(), - $replace); - } - - protected function raiseLintAtNode( - XHPASTNode $node, - $code, - $desc, - $replace = null) { - return $this->raiseLintAtOffset( - $node->getOffset(), - $code, - $desc, - $node->getConcreteString(), - $replace); - } - public function getSuperGlobalNames() { return array( '$GLOBALS', diff --git a/src/lint/linter/__tests__/ArcanistPhutilXHPASTLinterTestCase.php b/src/lint/linter/__tests__/ArcanistPhutilXHPASTLinterTestCase.php new file mode 100644 index 00000000..4f42f825 --- /dev/null +++ b/src/lint/linter/__tests__/ArcanistPhutilXHPASTLinterTestCase.php @@ -0,0 +1,20 @@ +setXHPASTLinter(new ArcanistXHPASTLinter()); + + $working_copy = ArcanistWorkingCopyIdentity::newFromPath(__FILE__); + return $this->executeTestsInDirectory( + dirname(__FILE__).'/phlxhp/', + $linter, + $working_copy); + } + +} diff --git a/src/lint/linter/__tests__/xhpast/array-combine.lint-test b/src/lint/linter/__tests__/phlxhp/array-combine.lint-test similarity index 83% rename from src/lint/linter/__tests__/xhpast/array-combine.lint-test rename to src/lint/linter/__tests__/phlxhp/array-combine.lint-test index abcaac70..b0670e39 100644 --- a/src/lint/linter/__tests__/xhpast/array-combine.lint-test +++ b/src/lint/linter/__tests__/phlxhp/array-combine.lint-test @@ -3,4 +3,4 @@ array_combine($x, $x); array_combine($x, $y); ~~~~~~~~~~ -disabled:3:1 +warning:3:1 diff --git a/src/lint/linter/__tests__/xhpast/pht.lint-test b/src/lint/linter/__tests__/phlxhp/pht.lint-test similarity index 71% rename from src/lint/linter/__tests__/xhpast/pht.lint-test rename to src/lint/linter/__tests__/phlxhp/pht.lint-test index 5b99f06f..31b1c45b 100644 --- a/src/lint/linter/__tests__/xhpast/pht.lint-test +++ b/src/lint/linter/__tests__/phlxhp/pht.lint-test @@ -20,8 +20,8 @@ $a EOT ); ~~~~~~~~~~ -disabled:5:1 -disabled:7:1 -disabled:8:1 -disabled:10:1 -disabled:18:1 +error:5:1 +error:7:1 +error:8:1 +error:10:1 +error:18:1