From b52e9dc702c2b47e6c1604b8d7d7c0940bd5579a Mon Sep 17 00:00:00 2001 From: Joshua Spence <josh@freelancer.com> Date: Mon, 7 Dec 2015 21:35:34 +1100 Subject: [PATCH 01/14] Linter fixes Summary: Minor linter fixes Test Plan: N/A Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14633 --- ...tClassFilenameMismatchXHPASTLinterRule.php | 2 +- ...istImplicitFallthroughXHPASTLinterRule.php | 2 +- src/parser/ArcanistBaseCommitParser.php | 1 - src/repository/api/ArcanistMercurialAPI.php | 2 +- src/repository/api/ArcanistSubversionAPI.php | 6 ++-- src/unit/engine/NoseTestEngine.php | 6 ++-- src/unit/engine/PytestTestEngine.php | 6 ++-- src/workflow/ArcanistFlagWorkflow.php | 32 ++++++++++++++----- 8 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/lint/linter/xhpast/rules/ArcanistClassFilenameMismatchXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistClassFilenameMismatchXHPASTLinterRule.php index 6c8c2f41..44eb9b6e 100644 --- a/src/lint/linter/xhpast/rules/ArcanistClassFilenameMismatchXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistClassFilenameMismatchXHPASTLinterRule.php @@ -46,7 +46,7 @@ final class ArcanistClassFilenameMismatchXHPASTLinterRule $this->raiseLintAtNode( $decl_name, pht( - "The name of this file differs from the name of the ". + 'The name of this file differs from the name of the '. 'class or interface it declares. Rename the file to `%s`.', $rename)); } diff --git a/src/lint/linter/xhpast/rules/ArcanistImplicitFallthroughXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistImplicitFallthroughXHPASTLinterRule.php index 1fe7a2a7..7a86f2a1 100644 --- a/src/lint/linter/xhpast/rules/ArcanistImplicitFallthroughXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistImplicitFallthroughXHPASTLinterRule.php @@ -193,7 +193,7 @@ final class ArcanistImplicitFallthroughXHPASTLinterRule 'This `%s` or `%s` has a nonempty block which does not end '. 'with `%s`, `%s`, `%s`, `%s` or `%s`. Did you forget to add '. 'one of those? If you intend to fall through, add a `%s` '. - "comment to silence this warning.", + 'comment to silence this warning.', 'case', 'default', 'break', diff --git a/src/parser/ArcanistBaseCommitParser.php b/src/parser/ArcanistBaseCommitParser.php index af2a71ff..5ed6ce4a 100644 --- a/src/parser/ArcanistBaseCommitParser.php +++ b/src/parser/ArcanistBaseCommitParser.php @@ -8,7 +8,6 @@ final class ArcanistBaseCommitParser extends Phobject { public function __construct(ArcanistRepositoryAPI $api) { $this->api = $api; - return $this; } private function tokenizeBaseCommitSpecification($raw_spec) { diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php index 33105c20..6fa6c48a 100644 --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -360,7 +360,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { $working_status = ArcanistMercurialParser::parseMercurialStatus($stdout); foreach ($working_status as $path => $mask) { - if (!($mask & ArcanistRepositoryAPI::FLAG_UNTRACKED)) { + if (!($mask & parent::FLAG_UNTRACKED)) { // Mark tracked files as uncommitted. $mask |= self::FLAG_UNCOMMITTED; } diff --git a/src/repository/api/ArcanistSubversionAPI.php b/src/repository/api/ArcanistSubversionAPI.php index d8420a1b..f5768b14 100644 --- a/src/repository/api/ArcanistSubversionAPI.php +++ b/src/repository/api/ArcanistSubversionAPI.php @@ -139,7 +139,7 @@ final class ArcanistSubversionAPI extends ArcanistRepositoryAPI { $status = $this->svnStatus; if (!$with_externals) { foreach ($status as $path => $mask) { - if ($mask & ArcanistRepositoryAPI::FLAG_EXTERNALS) { + if ($mask & parent::FLAG_EXTERNALS) { unset($status[$path]); } } @@ -384,7 +384,7 @@ final class ArcanistSubversionAPI extends ArcanistRepositoryAPI { $status = $status[$path]; // Build meaningful diff text for "svn copy" operations. - if ($status & ArcanistRepositoryAPI::FLAG_ADDED) { + if ($status & parent::FLAG_ADDED) { $info = $this->getSVNInfo($path); if (!empty($info['Copied From URL'])) { return $this->buildSyntheticAdditionDiff( @@ -403,7 +403,7 @@ final class ArcanistSubversionAPI extends ArcanistRepositoryAPI { if (preg_match('/\.(gif|png|jpe?g|swf|pdf|ico)$/i', $path, $matches)) { // Check if the file is deleted first; SVN will complain if we try to // get properties of a deleted file. - if ($status & ArcanistRepositoryAPI::FLAG_DELETED) { + if ($status & parent::FLAG_DELETED) { return <<<EODIFF Index: {$path} =================================================================== diff --git a/src/unit/engine/NoseTestEngine.php b/src/unit/engine/NoseTestEngine.php index e81bcb63..039daf5e 100644 --- a/src/unit/engine/NoseTestEngine.php +++ b/src/unit/engine/NoseTestEngine.php @@ -139,14 +139,14 @@ final class NoseTestEngine extends ArcanistUnitTestEngine { for ($ii = 0; $ii < $lines->length; $ii++) { $line = $lines->item($ii); - $next_line = intval($line->getAttribute('number')); + $next_line = (int)$line->getAttribute('number'); for ($start_line; $start_line < $next_line; $start_line++) { $coverage .= 'N'; } - if (intval($line->getAttribute('hits')) == 0) { + if ((int)$line->getAttribute('hits') == 0) { $coverage .= 'U'; - } else if (intval($line->getAttribute('hits')) > 0) { + } else if ((int)$line->getAttribute('hits') > 0) { $coverage .= 'C'; } diff --git a/src/unit/engine/PytestTestEngine.php b/src/unit/engine/PytestTestEngine.php index 49ef3c48..1ebf98e6 100644 --- a/src/unit/engine/PytestTestEngine.php +++ b/src/unit/engine/PytestTestEngine.php @@ -116,14 +116,14 @@ final class PytestTestEngine extends ArcanistUnitTestEngine { for ($ii = 0; $ii < $lines->length; $ii++) { $line = $lines->item($ii); - $next_line = intval($line->getAttribute('number')); + $next_line = (int)$line->getAttribute('number'); for ($start_line; $start_line < $next_line; $start_line++) { $coverage .= 'N'; } - if (intval($line->getAttribute('hits')) == 0) { + if ((int)$line->getAttribute('hits') == 0) { $coverage .= 'U'; - } else if (intval($line->getAttribute('hits')) > 0) { + } else if ((int)$line->getAttribute('hits') > 0) { $coverage .= 'C'; } diff --git a/src/workflow/ArcanistFlagWorkflow.php b/src/workflow/ArcanistFlagWorkflow.php index d8420728..9f1367b9 100644 --- a/src/workflow/ArcanistFlagWorkflow.php +++ b/src/workflow/ArcanistFlagWorkflow.php @@ -14,14 +14,30 @@ final class ArcanistFlagWorkflow extends ArcanistWorkflow { ); private static $colorSpec = array( - 'red' => 0, 'r' => 0, 0 => 0, - 'orange' => 1, 'o' => 1, 1 => 1, - 'yellow' => 2, 'y' => 2, 2 => 2, - 'green' => 3, 'g' => 3, 3 => 3, - 'blue' => 4, 'b' => 4, 4 => 4, - 'pink' => 5, 'p' => 5, 5 => 5, - 'purple' => 6, 'v' => 6, 6 => 6, - 'checkered' => 7, 'c' => 7, 7 => 7, + 'red' => 0, + 'r' => 0, + 0 => 0, + 'orange' => 1, + 'o' => 1, + 1 => 1, + 'yellow' => 2, + 'y' => 2, + 2 => 2, + 'green' => 3, + 'g' => 3, + 3 => 3, + 'blue' => 4, + 'b' => 4, + 4 => 4, + 'pink' => 5, + 'p' => 5, + 5 => 5, + 'purple' => 6, + 'v' => 6, + 6 => 6, + 'checkered' => 7, + 'c' => 7, + 7 => 7, ); public function getWorkflowName() { From a6e81daad1bbd470c1907a7ea705b2e084fc07dd Mon Sep 17 00:00:00 2001 From: Joshua Spence <josh@freelancer.com> Date: Mon, 7 Dec 2015 20:20:57 +0000 Subject: [PATCH 02/14] Improve brace formatting linter rule Summary: Allow the brace formatting linter rule to complain about `class X{}`. Test Plan: Updated unit tests. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14690 --- ...rcanistBraceFormattingXHPASTLinterRule.php | 8 -- .../brace-formatting.lint-test | 87 ------------------- .../brace-formatting/class.lint-test | 14 +++ .../control-statement.lint-test | 53 +++++++++++ .../brace-formatting/declare.lint-test | 4 + .../brace-formatting/function.lint-test | 27 ++++++ .../brace-formatting/try-catch.lint-test | 14 +++ 7 files changed, 112 insertions(+), 95 deletions(-) delete mode 100644 src/lint/linter/xhpast/rules/__tests__/brace-formatting/brace-formatting.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/brace-formatting/class.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/brace-formatting/control-statement.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/brace-formatting/declare.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/brace-formatting/function.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/brace-formatting/try-catch.lint-test diff --git a/src/lint/linter/xhpast/rules/ArcanistBraceFormattingXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistBraceFormattingXHPASTLinterRule.php index 0a8c33b7..775c6f18 100644 --- a/src/lint/linter/xhpast/rules/ArcanistBraceFormattingXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistBraceFormattingXHPASTLinterRule.php @@ -24,14 +24,6 @@ final class ArcanistBraceFormattingXHPASTLinterRule if (!$before) { $first = head($tokens); - // Only insert the space if we're after a closing parenthesis. If - // we're in a construct like "else{}", other rules will insert space - // after the 'else' correctly. - $prev = $first->getPrevToken(); - if (!$prev || $prev->getValue() !== ')') { - continue; - } - $this->raiseLintAtToken( $first, pht( diff --git a/src/lint/linter/xhpast/rules/__tests__/brace-formatting/brace-formatting.lint-test b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/brace-formatting.lint-test deleted file mode 100644 index 725773f3..00000000 --- a/src/lint/linter/xhpast/rules/__tests__/brace-formatting/brace-formatting.lint-test +++ /dev/null @@ -1,87 +0,0 @@ -<?php - -function f() { - -} - -function g() -{ - -} - -if (1) -{} - -foreach (x() as $y) -{} - -while (1) -{} - -switch (1) -{} - -try -{} -catch (Exception $x) -{} - -function h(){} -if ($x) foo(); -else bar(); -do baz(); while ($x); - -if ($x) {} -else if ($y) {} -else {} - -if ($x) {}else{} - -declare(ticks = 1); -~~~~~~~~~~ -warning:7:13 -warning:12:7 -warning:15:20 -warning:18:10 -warning:21:11 -warning:24:4 -warning:26:21 -warning:29:13 -warning:30:9 -warning:31:6 -warning:32:4 -warning:34:11 -warning:35:16 -warning:38:11 -~~~~~~~~~~ -<?php - -function f() { - -} - -function g() { - -} - -if (1) {} - -foreach (x() as $y) {} - -while (1) {} - -switch (1) {} - -try {} -catch (Exception $x) {} - -function h() {} -if ($x) foo(); -else bar(); -do baz(); while ($x); - -if ($x) {} else if ($y) {} else {} - -if ($x) {} else{} - -declare(ticks = 1); diff --git a/src/lint/linter/xhpast/rules/__tests__/brace-formatting/class.lint-test b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/class.lint-test new file mode 100644 index 00000000..37c94ec9 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/class.lint-test @@ -0,0 +1,14 @@ +<?php + +class SomeClass {} +class SomeOtherClass{} +class YetAnotherClass extends SomeClass{} +~~~~~~~~~~ +warning:4:21 +warning:5:40 +~~~~~~~~~~ +<?php + +class SomeClass {} +class SomeOtherClass {} +class YetAnotherClass extends SomeClass {} diff --git a/src/lint/linter/xhpast/rules/__tests__/brace-formatting/control-statement.lint-test b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/control-statement.lint-test new file mode 100644 index 00000000..2454529e --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/control-statement.lint-test @@ -0,0 +1,53 @@ +<?php + +if (1) +{} + +foreach (x() as $y) +{} + +while (1) +{} + +switch (1) +{} + +if ($x) foo(); +else bar(); +do baz(); while ($x); + +if ($x) {} +else if ($y) {} +else {} + +if ($x) {}else{} +~~~~~~~~~~ +warning:3:7 +warning:6:20 +warning:9:10 +warning:12:11 +warning:15:9 +warning:16:6 +warning:17:4 +warning:19:11 +warning:20:16 +warning:23:11 +warning:23:15 +~~~~~~~~~~ +<?php + +if (1) {} + +foreach (x() as $y) {} + +while (1) {} + +switch (1) {} + +if ($x) foo(); +else bar(); +do baz(); while ($x); + +if ($x) {} else if ($y) {} else {} + +if ($x) {} else {} diff --git a/src/lint/linter/xhpast/rules/__tests__/brace-formatting/declare.lint-test b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/declare.lint-test new file mode 100644 index 00000000..da2694db --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/declare.lint-test @@ -0,0 +1,4 @@ +<?php + +declare(ticks = 1); +~~~~~~~~~~ diff --git a/src/lint/linter/xhpast/rules/__tests__/brace-formatting/function.lint-test b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/function.lint-test new file mode 100644 index 00000000..46ee8076 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/function.lint-test @@ -0,0 +1,27 @@ +<?php + +function f() { + +} + +function g() +{ + +} + +function h(){} +~~~~~~~~~~ +warning:7:13 +warning:12:13 +~~~~~~~~~~ +<?php + +function f() { + +} + +function g() { + +} + +function h() {} diff --git a/src/lint/linter/xhpast/rules/__tests__/brace-formatting/try-catch.lint-test b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/try-catch.lint-test new file mode 100644 index 00000000..7b3f1494 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/brace-formatting/try-catch.lint-test @@ -0,0 +1,14 @@ +<?php + +try +{} +catch (Exception $x) +{} +~~~~~~~~~~ +warning:3:4 +warning:5:21 +~~~~~~~~~~ +<?php + +try {} +catch (Exception $x) {} From d2e7785497279e31c635d1457c863b49c3a492ed Mon Sep 17 00:00:00 2001 From: Joshua Spence <josh@freelancer.com> Date: Wed, 9 Dec 2015 06:56:48 +1100 Subject: [PATCH 03/14] Add a linter rule for curly brace array indexes Summary: In PHP, both `$x['key']` and `$x{'key'}` can be used to access an array (see http://stackoverflow.com/questions/8092248/php-curly-braces-in-array-notation), but the former should be preferred. Test Plan: Added test cases. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14603 --- src/__phutil_library_map__.php | 4 ++ ...stCurlyBraceArrayIndexXHPASTLinterRule.php | 52 +++++++++++++++++++ ...raceArrayIndexXHPASTLinterRuleTestCase.php | 11 ++++ .../curly-brace-array-index/array.lint-test | 9 ++++ .../curly-brace-array-index/nested.lint-test | 7 +++ .../whitespace.lint-test | 7 +++ 6 files changed, 90 insertions(+) create mode 100644 src/lint/linter/xhpast/rules/ArcanistCurlyBraceArrayIndexXHPASTLinterRule.php create mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistCurlyBraceArrayIndexXHPASTLinterRuleTestCase.php create mode 100644 src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/array.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/nested.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/whitespace.lint-test diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4d1a81c6..2fbc0334 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -96,6 +96,8 @@ phutil_register_library_map(array( 'ArcanistCppcheckLinterTestCase' => 'lint/linter/__tests__/ArcanistCppcheckLinterTestCase.php', 'ArcanistCpplintLinter' => 'lint/linter/ArcanistCpplintLinter.php', 'ArcanistCpplintLinterTestCase' => 'lint/linter/__tests__/ArcanistCpplintLinterTestCase.php', + 'ArcanistCurlyBraceArrayIndexXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistCurlyBraceArrayIndexXHPASTLinterRule.php', + 'ArcanistCurlyBraceArrayIndexXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistCurlyBraceArrayIndexXHPASTLinterRuleTestCase.php', 'ArcanistDeclarationParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistDeclarationParenthesesXHPASTLinterRule.php', 'ArcanistDeclarationParenthesesXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistDeclarationParenthesesXHPASTLinterRuleTestCase.php', 'ArcanistDefaultParametersXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistDefaultParametersXHPASTLinterRule.php', @@ -502,6 +504,8 @@ phutil_register_library_map(array( 'ArcanistCppcheckLinterTestCase' => 'ArcanistExternalLinterTestCase', 'ArcanistCpplintLinter' => 'ArcanistExternalLinter', 'ArcanistCpplintLinterTestCase' => 'ArcanistExternalLinterTestCase', + 'ArcanistCurlyBraceArrayIndexXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', + 'ArcanistCurlyBraceArrayIndexXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistDeclarationParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistDeclarationParenthesesXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistDefaultParametersXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', diff --git a/src/lint/linter/xhpast/rules/ArcanistCurlyBraceArrayIndexXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistCurlyBraceArrayIndexXHPASTLinterRule.php new file mode 100644 index 00000000..2d2fa171 --- /dev/null +++ b/src/lint/linter/xhpast/rules/ArcanistCurlyBraceArrayIndexXHPASTLinterRule.php @@ -0,0 +1,52 @@ +<?php + +final class ArcanistCurlyBraceArrayIndexXHPASTLinterRule + extends ArcanistXHPASTLinterRule { + + const ID = 119; + + public function getLintName() { + return pht('Curly Brace Array Index'); + } + + public function getLintSeverity() { + return ArcanistLintSeverity::SEVERITY_WARNING; + } + + public function process(XHPASTNode $root) { + $index_accesses = $root->selectDescendantsOfType('n_INDEX_ACCESS'); + + foreach ($index_accesses as $index_access) { + $tokens = $index_access->getChildByIndex(1)->getTokens(); + + $left_brace = head($tokens)->getPrevToken(); + while (!$left_brace->isSemantic()) { + $left_brace = $left_brace->getPrevToken(); + } + + $right_brace = last($tokens)->getNextToken(); + while (!$right_brace->isSemantic()) { + $right_brace = $right_brace->getNextToken(); + } + + if ($left_brace->getValue() == '{' || $right_brace->getValue() == '}') { + $replacement = null; + foreach ($index_access->getTokens() as $token) { + if ($token === $left_brace) { + $replacement .= '['; + } else if ($token === $right_brace) { + $replacement .= ']'; + } else { + $replacement .= $token->getValue(); + } + } + + $this->raiseLintAtNode( + $index_access, + pht('Use `%s` instead of `%s`.', "\$x['key']", "\$x{'key'}"), + $replacement); + } + } + } + +} diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistCurlyBraceArrayIndexXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistCurlyBraceArrayIndexXHPASTLinterRuleTestCase.php new file mode 100644 index 00000000..1426e555 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistCurlyBraceArrayIndexXHPASTLinterRuleTestCase.php @@ -0,0 +1,11 @@ +<?php + +final class ArcanistCurlyBraceArrayIndexXHPASTLinterRuleTestCase + extends ArcanistXHPASTLinterRuleTestCase { + + public function testLinter() { + $this->executeTestsInDirectory( + dirname(__FILE__).'/curly-brace-array-index/'); + } + +} diff --git a/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/array.lint-test b/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/array.lint-test new file mode 100644 index 00000000..0d2ec94d --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/array.lint-test @@ -0,0 +1,9 @@ +<?php +$x['key']; +$y{'key'}; +~~~~~~~~~~ +warning:3:1 +~~~~~~~~~~ +<?php +$x['key']; +$y['key']; diff --git a/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/nested.lint-test b/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/nested.lint-test new file mode 100644 index 00000000..7f56597f --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/nested.lint-test @@ -0,0 +1,7 @@ +<?php +$x[$y{'key'}]; +~~~~~~~~~~ +warning:2:4 +~~~~~~~~~~ +<?php +$x[$y['key']]; diff --git a/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/whitespace.lint-test b/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/whitespace.lint-test new file mode 100644 index 00000000..a078e314 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/whitespace.lint-test @@ -0,0 +1,7 @@ +<?php +$x { 'key' /* comment */ }; +~~~~~~~~~~ +warning:2:1 +~~~~~~~~~~ +<?php +$x [ 'key' /* comment */ ]; From 0c8124a2729d70874fcacbccffdbf5b9bfc6997d Mon Sep 17 00:00:00 2001 From: Joshua Spence <josh@freelancer.com> Date: Wed, 9 Dec 2015 08:09:02 +1100 Subject: [PATCH 04/14] Fix handling of empty array index Fix handling of empty array index by `ArcanistCurlyBraceArrayIndexXHPASTLinterRule`. Auditors: epriestley --- .../rules/ArcanistCurlyBraceArrayIndexXHPASTLinterRule.php | 4 ++++ .../__tests__/curly-brace-array-index/array_push.lint-test | 3 +++ 2 files changed, 7 insertions(+) create mode 100644 src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/array_push.lint-test diff --git a/src/lint/linter/xhpast/rules/ArcanistCurlyBraceArrayIndexXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistCurlyBraceArrayIndexXHPASTLinterRule.php index 2d2fa171..99b3ab94 100644 --- a/src/lint/linter/xhpast/rules/ArcanistCurlyBraceArrayIndexXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistCurlyBraceArrayIndexXHPASTLinterRule.php @@ -19,6 +19,10 @@ final class ArcanistCurlyBraceArrayIndexXHPASTLinterRule foreach ($index_accesses as $index_access) { $tokens = $index_access->getChildByIndex(1)->getTokens(); + if (!$tokens) { + continue; + } + $left_brace = head($tokens)->getPrevToken(); while (!$left_brace->isSemantic()) { $left_brace = $left_brace->getPrevToken(); diff --git a/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/array_push.lint-test b/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/array_push.lint-test new file mode 100644 index 00000000..266b2844 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/curly-brace-array-index/array_push.lint-test @@ -0,0 +1,3 @@ +<?php +$x[] = 'value'; +~~~~~~~~~~ From d0e73bb656105e82582cf72c415a8c310b30915d Mon Sep 17 00:00:00 2001 From: epriestley <git@epriestley.com> Date: Thu, 10 Dec 2015 14:10:02 -0800 Subject: [PATCH 05/14] Don't use "-c" flag in "git:branch-unique()" revision range rule Summary: Fixes T9953. - "-c" was introduced in 1.7.2. - "--no-color" has existed forever as far as I can tell. - "--no-column" was introducd in 1.7.11, but there was nothing that needed to be disabled before that (hopefully). Test Plan: - Ran `arc which --trace` and observed a reasonable `git branch` command with correct output. - Ran `arc which --trace` with a faked older Git version, observed command omit `--no-column`. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9953 Differential Revision: https://secure.phabricator.com/D14735 --- src/repository/api/ArcanistGitAPI.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 8d8181e3..8600674c 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -1216,8 +1216,20 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { // Ideally, we would use something like "for-each-ref --contains" // to get a filtered list of branches ready for script consumption. // Instead, try to get predictable output from "branch --contains". + + $flags = array(); + $flags[] = '--no-color'; + + // NOTE: The "--no-column" flag was introduced in Git 1.7.11, so + // don't pass it if we're running an older version. See T9953. + $version = $this->getGitVersion(); + if (version_compare($version, '1.7.11', '>=')) { + $flags[] = '--no-column'; + } + list($branches) = $this->execxLocal( - '-c column.ui=never -c color.ui=never branch --contains %s', + 'branch %Ls --contains %s', + $flags, $commit); $branches = array_filter(explode("\n", $branches)); From dae2f0073f019221839924b4b1431c6cba5dcdda Mon Sep 17 00:00:00 2001 From: epriestley <git@epriestley.com> Date: Thu, 10 Dec 2015 14:33:33 -0800 Subject: [PATCH 06/14] In `arc diff`, try to guess where a change should land Summary: Ref T9952. Ref T3462. My primary goal is to improve prefilling of the "Onto Branch:" field in the "Land Revision" dialog. When uploading a diff with `arc diff`, add a property with some information about which branch to target. In particular: - If the local branch tracks an upstream branch (or tracks something which tracks something which tracks the upstream), target that. - If not, but "arc.land.onto.default" is set, target that. This doesn't try to guess in other cases, since they're more involved. I'll add some context about this in T3462. I don't //love// using "diff properties" for this, but it doesn't make cleaning them up any harder since we already use it for other stuff which isn't going away (lint/unit excuses). Test Plan: - Added some `var_dump()` and used `arc diff --only` to generate diffs. - Saw upstream tracking and config-based rules generate reasonable values and submit them. Reviewers: chad Reviewed By: chad Maniphest Tasks: T3462, T9952 Differential Revision: https://secure.phabricator.com/D14736 --- src/workflow/ArcanistDiffWorkflow.php | 49 +++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php index aeae0526..b75de9cd 100644 --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/src/workflow/ArcanistDiffWorkflow.php @@ -531,6 +531,7 @@ EOTEXT $this->updateLintDiffProperty(); $this->updateUnitDiffProperty(); $this->updateLocalDiffProperty(); + $this->updateOntoDiffProperty(); $this->resolveDiffPropertyUpdates(); $output_json = $this->getArgument('json'); @@ -2406,6 +2407,54 @@ EOTEXT $this->updateDiffProperty('local:commits', json_encode($local_info)); } + private function updateOntoDiffProperty() { + $onto = $this->getDiffOntoTargets(); + + if (!$onto) { + return; + } + + $this->updateDiffProperty('arc:onto', json_encode($onto)); + } + + private function getDiffOntoTargets() { + $api = $this->getRepositoryAPI(); + + if (!($api instanceof ArcanistGitAPI)) { + return null; + } + + // If we track an upstream branch either directly or indirectly, use that. + $branch = $api->getBranchName(); + if (strlen($branch)) { + $upstream_path = $api->getPathToUpstream($branch); + $remote_branch = $upstream_path->getRemoteBranchName(); + if (strlen($remote_branch)) { + return array( + array( + 'type' => 'branch', + 'name' => $remote_branch, + 'kind' => 'upstream', + ), + ); + } + } + + // If "arc.land.onto.default" is configured, use that. + $config_key = 'arc.land.onto.default'; + $onto = $this->getConfigFromAnySource($config_key); + if (strlen($onto)) { + return array( + array( + 'type' => 'branch', + 'name' => $onto, + 'kind' => 'arc.land.onto.default', + ), + ); + } + + return null; + } /** * Update an arbitrary diff property. From 74c7495b1a92433d6d8d577831b73b1ecffcaaf4 Mon Sep 17 00:00:00 2001 From: epriestley <git@epriestley.com> Date: Sat, 12 Dec 2015 09:38:32 -0800 Subject: [PATCH 07/14] Clarify that "arc land" means it is merging changes, not branch refences Summary: Ref T9973. Make this language unambiguously clear about the underlying operations. Test Plan: Ran `arc help land`. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9973 Differential Revision: https://secure.phabricator.com/D14754 --- src/workflow/ArcanistLandWorkflow.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php index 23b0b7b4..884f3a54 100644 --- a/src/workflow/ArcanistLandWorkflow.php +++ b/src/workflow/ArcanistLandWorkflow.php @@ -87,19 +87,23 @@ EOTEXT With **--preview**, execution stops here, before the change is merged. - The change is merged into the target branch, following these rules: + The change is merged with the changes in the target branch, + following these rules: - In mutable repositories or with **--squash**, this will perform a - squash merge (the entire branch will be represented as one commit on - the target branch). + In repositories with mutable history or with **--squash**, this will + perform a squash merge (the entire branch will be represented as one + commit after the merge). - In immutable repositories or with **--merge**, this will perform a - strict merge (a merge commit will always be created, and local - commits will be preserved). + In repositories with immutable history or with **--merge**, this will + perform a strict merge (a merge commit will always be created, and + local commits will be preserved). The resulting commit will be given an up-to-date commit message describing the final state of the revision in Differential. + In Git, the merge occurs in a detached HEAD. The local branch + reference (if one exists) is not updated yet. + With **--hold**, execution stops here, before the change is pushed. The change is pushed into the remote. From 97056a3b85bbee72d6ede1b9478b03dee1db2d4e Mon Sep 17 00:00:00 2001 From: Dimitry Andric <dimitry@andric.com> Date: Sun, 13 Dec 2015 02:21:43 -0800 Subject: [PATCH 08/14] Preserve the exit code of the diff command in binary_safe_diff.sh Summary: Some versions of Subversion (1.9 in any case, maybe others) will duplicate diff headers, if the diff command run through --diff-cmd returns 0. This lead to T9970, where the addition of a new file with properties only shows the properties themselves in the review, not the content of the new file. Test Plan: This is a trivial change, is a test needed at all? Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: stevenh, Korvin, eadler Differential Revision: https://secure.phabricator.com/D14755 --- scripts/repository/binary_safe_diff.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/repository/binary_safe_diff.sh b/scripts/repository/binary_safe_diff.sh index 1473e804..b51d31a6 100755 --- a/scripts/repository/binary_safe_diff.sh +++ b/scripts/repository/binary_safe_diff.sh @@ -1,6 +1,8 @@ #!/bin/sh diff "$@" -if [ "$?" = "2" ]; then +RES="$?" +if [ "$RES" = "2" ]; then exit 1 fi +exit "$RES" From 7d25fcdbdb51f28a1e6465edd819aa5d99bc0f71 Mon Sep 17 00:00:00 2001 From: Dimitry Andric <dimitry@andric.com> Date: Sun, 13 Dec 2015 02:35:07 -0800 Subject: [PATCH 09/14] Simplify diff exit code handling. Summary: This fixes T9970 in an alternate manner, with the same effect: the binary_safe_diff.sh script returns 0 if the diff succeeds, 1 in all other cases. Test Plan: Tested locally with a fixed binary_safe_diff.sh, resulting in this correct review: https://reviews.freebsd.org/D4542 Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: eadler, Korvin, stevenh Maniphest Tasks: T9970 Differential Revision: https://secure.phabricator.com/D14759 --- scripts/repository/binary_safe_diff.sh | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/scripts/repository/binary_safe_diff.sh b/scripts/repository/binary_safe_diff.sh index b51d31a6..00b02680 100755 --- a/scripts/repository/binary_safe_diff.sh +++ b/scripts/repository/binary_safe_diff.sh @@ -1,8 +1,3 @@ #!/bin/sh -diff "$@" -RES="$?" -if [ "$RES" = "2" ]; then - exit 1 -fi -exit "$RES" +diff "$@" || exit 1 From 9a373c88d74793a21857b07735059212d5ad7bba Mon Sep 17 00:00:00 2001 From: epriestley <git@epriestley.com> Date: Sun, 13 Dec 2015 03:08:06 -0800 Subject: [PATCH 10/14] Explain how to move forward or backward from `arc land --hold` Summary: Fixes T9973. When users run `arc land --hold`, give them the commands to move forward (push) or backward (checkout the branch/tag/commit they were on) and be explicit that branches have not changed. Test Plan: Ran `arc land --hold`, got lots of explanatory text. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9973 Differential Revision: https://secure.phabricator.com/D14762 --- src/land/ArcanistGitLandEngine.php | 41 +++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/src/land/ArcanistGitLandEngine.php b/src/land/ArcanistGitLandEngine.php index 7256b050..068124d0 100644 --- a/src/land/ArcanistGitLandEngine.php +++ b/src/land/ArcanistGitLandEngine.php @@ -29,9 +29,7 @@ final class ArcanistGitLandEngine $this->updateWorkingCopy(); if ($this->getShouldHold()) { - $this->writeInfo( - pht('HOLD'), - pht('Holding change locally, it has not been pushed.')); + $this->didHoldChanges(); } else { $this->pushChange(); $this->reconcileLocalState(); @@ -542,4 +540,41 @@ final class ArcanistGitLandEngine ); } + private function didHoldChanges() { + $this->writeInfo( + pht('HOLD'), + pht( + 'Holding change locally, it has not been pushed.')); + + $push_command = csprintf( + '$ git push -- %R %R:%R', + $this->getTargetRemote(), + $this->mergedRef, + $this->getTargetOnto()); + + $restore_command = csprintf( + '$ git checkout %R --', + $this->localRef); + + echo tsprintf( + "\n%s\n\n". + "%s\n\n". + " %s\n\n". + "%s\n\n". + " %s\n\n". + "%s\n", + pht( + 'This local working copy now contains the merged changes in a '. + 'detached state.'), + pht('You can push the changes manually with this command:'), + $push_command, + pht( + 'You can go back to how things were before you ran `arc land` with '. + 'this command:'), + $restore_command, + pht( + 'Local branches have not been changed, and are still in exactly the '. + 'same state as before.')); + } + } From a183c2c4ba5836628a6bf1285a7e9109d2866fde Mon Sep 17 00:00:00 2001 From: epriestley <git@epriestley.com> Date: Wed, 16 Dec 2015 06:00:50 -0800 Subject: [PATCH 11/14] When `arc` is run with `--trace`, print out argv explicitly Summary: Ref T9993. Users may have shell aliases or wrapper scripts that they forget about. Print out the arguments we received to make it obvious that something went through an indirection layer. Test Plan: Ran `arc version --help`. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9993 Differential Revision: https://secure.phabricator.com/D14797 --- scripts/arcanist.php | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/scripts/arcanist.php b/scripts/arcanist.php index f29311f6..c888fae9 100755 --- a/scripts/arcanist.php +++ b/scripts/arcanist.php @@ -74,17 +74,27 @@ $config = null; $workflow = null; try { + if ($config_trace_mode) { + echo tsprintf( + "**<bg:magenta> %s </bg>** %s\n", + pht('ARGV'), + csprintf('%Ls', $original_argv)); - $console->writeLog( - "%s\n", - pht( - "libphutil loaded from '%s'.", - phutil_get_library_root('phutil'))); - $console->writeLog( - "%s\n", - pht( - "arcanist loaded from '%s'.", - phutil_get_library_root('arcanist'))); + $libraries = array( + 'phutil', + 'arcanist', + ); + + foreach ($libraries as $library_name) { + echo tsprintf( + "**<bg:magenta> %s </bg>** %s\n", + pht('LOAD'), + pht( + 'Loaded "%s" from "%s".', + $library_name, + phutil_get_library_root($library_name))); + } + } if (!$args) { if ($help) { From 8762e3f36715875605a205326bee7bae1f7eb481 Mon Sep 17 00:00:00 2001 From: epriestley <git@epriestley.com> Date: Thu, 17 Dec 2015 16:16:05 -0800 Subject: [PATCH 12/14] Use "--whitespace nowarn" in `arc patch` to respect trailing whitespace Summary: Fixes T10008. Git tries to fix some issues by default (apparently? empirically; not consistent with documentation, I think?), but patches from `arc patch` are "always" accurate (disregarding other bugs we might have -- basically, they haven't been emailed or copy/pasted or anything like that) so we can just tell it to apply the patch exactly as-is. Test Plan: {F1029182} Reviewers: chad, joshuaspence Reviewed By: chad, joshuaspence Maniphest Tasks: T10008 Differential Revision: https://secure.phabricator.com/D14816 --- src/workflow/ArcanistPatchWorkflow.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/workflow/ArcanistPatchWorkflow.php b/src/workflow/ArcanistPatchWorkflow.php index 7dd62cf2..e7c6760d 100644 --- a/src/workflow/ArcanistPatchWorkflow.php +++ b/src/workflow/ArcanistPatchWorkflow.php @@ -691,7 +691,7 @@ EOTEXT Filesystem::writeFile($patchfile, $bundle->toGitPatch()); $passthru = new PhutilExecPassthru( - 'git apply --index --reject -- %s', + 'git apply --whitespace nowarn --index --reject -- %s', $patchfile); $passthru->setCWD($repository_api->getPath()); $err = $passthru->execute(); From 9ee14d28490c32151b98f21c2371454a3aae77a2 Mon Sep 17 00:00:00 2001 From: Joshua Spence <josh@freelancer.com> Date: Wed, 23 Dec 2015 08:39:44 +1100 Subject: [PATCH 13/14] Improve the "self member reference" linter rule Summary: Extends `ArcanistSelfMemberReferenceXHPASTLinterRule` to warn about the use of a hardcoded class name instead of `self` when instantiating a class. Arguably, we should maybe rename the linter rule from `ArcanistSelfMemberReferenceXHPASTLinterRule` to `ArcanistSelfUsageXHPASTLinterRule`, or even maybe split this rule into three separate rules: - `ArcanistSelfMemberReferenceXHPASTLinterRule` - `ArcanistSelfSpacingXHPASTLinterRule` - `ArcanistSelfClassReferenceXHPASTLinterRule`. Test Plan: Added to existing test cases. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D13935 --- src/__phutil_library_map__.php | 8 + .../ArcanistKeywordCasingXHPASTLinterRule.php | 143 ++++++++++-------- ...ayimNekudotayimSpacingXHPASTLinterRule.php | 40 +++++ ...nistSelfClassReferenceXHPASTLinterRule.php | 45 ++++++ ...istSelfMemberReferenceXHPASTLinterRule.php | 42 +---- ...dotayimSpacingXHPASTLinterRuleTestCase.php | 11 ++ ...ClassReferenceXHPASTLinterRuleTestCase.php | 10 ++ .../__tests__/keyword-casing/parent.lint-test | 25 +++ .../__tests__/keyword-casing/self.lint-test | 25 +++ .../comment.lint-test | 5 + .../newline.lint-test | 6 + .../paamayim-nekudotayim-spacing.lint-test | 36 +++++ .../self-class-references.lint-test | 25 +++ .../self-member-reference/php54.lint-test | 4 +- .../self-member-reference.lint-test | 46 +----- 15 files changed, 328 insertions(+), 143 deletions(-) create mode 100644 src/lint/linter/xhpast/rules/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule.php create mode 100644 src/lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php create mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase.php create mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistSelfClassReferenceXHPASTLinterRuleTestCase.php create mode 100644 src/lint/linter/xhpast/rules/__tests__/keyword-casing/parent.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/keyword-casing/self.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/comment.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/newline.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/paamayim-nekudotayim-spacing.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/self-class-reference/self-class-references.lint-test diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2fbc0334..13ef8642 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -272,6 +272,8 @@ phutil_register_library_map(array( 'ArcanistPHPOpenTagXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPHPOpenTagXHPASTLinterRuleTestCase.php', 'ArcanistPHPShortTagXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPHPShortTagXHPASTLinterRule.php', 'ArcanistPHPShortTagXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPHPShortTagXHPASTLinterRuleTestCase.php', + 'ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule.php', + 'ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase.php', 'ArcanistParentMemberReferenceXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistParentMemberReferenceXHPASTLinterRule.php', 'ArcanistParentMemberReferenceXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistParentMemberReferenceXHPASTLinterRuleTestCase.php', 'ArcanistParenthesesSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistParenthesesSpacingXHPASTLinterRule.php', @@ -317,6 +319,8 @@ phutil_register_library_map(array( 'ArcanistRubyLinter' => 'lint/linter/ArcanistRubyLinter.php', 'ArcanistRubyLinterTestCase' => 'lint/linter/__tests__/ArcanistRubyLinterTestCase.php', 'ArcanistScriptAndRegexLinter' => 'lint/linter/ArcanistScriptAndRegexLinter.php', + 'ArcanistSelfClassReferenceXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php', + 'ArcanistSelfClassReferenceXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistSelfClassReferenceXHPASTLinterRuleTestCase.php', 'ArcanistSelfMemberReferenceXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistSelfMemberReferenceXHPASTLinterRule.php', 'ArcanistSelfMemberReferenceXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistSelfMemberReferenceXHPASTLinterRuleTestCase.php', 'ArcanistSemicolonSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistSemicolonSpacingXHPASTLinterRule.php', @@ -680,6 +684,8 @@ phutil_register_library_map(array( 'ArcanistPHPOpenTagXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistPHPShortTagXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistPHPShortTagXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', + 'ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', + 'ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistParentMemberReferenceXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistParentMemberReferenceXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistParenthesesSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', @@ -725,6 +731,8 @@ phutil_register_library_map(array( 'ArcanistRubyLinter' => 'ArcanistExternalLinter', 'ArcanistRubyLinterTestCase' => 'ArcanistExternalLinterTestCase', 'ArcanistScriptAndRegexLinter' => 'ArcanistLinter', + 'ArcanistSelfClassReferenceXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', + 'ArcanistSelfClassReferenceXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistSelfMemberReferenceXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistSelfMemberReferenceXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistSemicolonSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', diff --git a/src/lint/linter/xhpast/rules/ArcanistKeywordCasingXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistKeywordCasingXHPASTLinterRule.php index af0b5903..8af5a7c8 100644 --- a/src/lint/linter/xhpast/rules/ArcanistKeywordCasingXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistKeywordCasingXHPASTLinterRule.php @@ -15,73 +15,98 @@ final class ArcanistKeywordCasingXHPASTLinterRule public function process(XHPASTNode $root) { $keywords = $root->selectTokensOfTypes(array( - 'T_REQUIRE_ONCE', - 'T_REQUIRE', + 'T_ABSTRACT', + 'T_ARRAY', + 'T_AS', + 'T_BREAK', + 'T_CALLABLE', + 'T_CASE', + 'T_CATCH', + 'T_CLASS', + 'T_CLONE', + 'T_CONST', + 'T_CONTINUE', + 'T_DECLARE', + 'T_DEFAULT', + 'T_DO', + 'T_ECHO', + 'T_ELSE', + 'T_ELSEIF', + 'T_EMPTY', + 'T_ENDDECLARE', + 'T_ENDFOR', + 'T_ENDFOREACH', + 'T_ENDIF', + 'T_ENDSWITCH', + 'T_ENDWHILE', 'T_EVAL', - 'T_INCLUDE_ONCE', + 'T_EXIT', + 'T_EXTENDS', + 'T_FINAL', + 'T_FINALLY', + 'T_FOR', + 'T_FOREACH', + 'T_FUNCTION', + 'T_GLOBAL', + 'T_GOTO', + 'T_HALT_COMPILER', + 'T_IF', + 'T_IMPLEMENTS', 'T_INCLUDE', + 'T_INCLUDE_ONCE', + 'T_INSTANCEOF', + 'T_INSTEADOF', + 'T_INTERFACE', + 'T_ISSET', + 'T_LIST', + 'T_LOGICAL_AND', 'T_LOGICAL_OR', 'T_LOGICAL_XOR', - 'T_LOGICAL_AND', - 'T_PRINT', - 'T_INSTANCEOF', - 'T_CLONE', - 'T_NEW', - 'T_EXIT', - 'T_IF', - 'T_ELSEIF', - 'T_ELSE', - 'T_ENDIF', - 'T_ECHO', - 'T_DO', - 'T_WHILE', - 'T_ENDWHILE', - 'T_FOR', - 'T_ENDFOR', - 'T_FOREACH', - 'T_ENDFOREACH', - 'T_DECLARE', - 'T_ENDDECLARE', - 'T_AS', - 'T_SWITCH', - 'T_ENDSWITCH', - 'T_CASE', - 'T_DEFAULT', - 'T_BREAK', - 'T_CONTINUE', - 'T_GOTO', - 'T_FUNCTION', - 'T_CONST', - 'T_RETURN', - 'T_TRY', - 'T_CATCH', - 'T_THROW', - 'T_USE', - 'T_GLOBAL', - 'T_PUBLIC', - 'T_PROTECTED', - 'T_PRIVATE', - 'T_FINAL', - 'T_ABSTRACT', - 'T_STATIC', - 'T_VAR', - 'T_UNSET', - 'T_ISSET', - 'T_EMPTY', - 'T_HALT_COMPILER', - 'T_CLASS', - 'T_INTERFACE', - 'T_EXTENDS', - 'T_IMPLEMENTS', - 'T_LIST', - 'T_ARRAY', 'T_NAMESPACE', - 'T_INSTEADOF', - 'T_CALLABLE', + 'T_NEW', + 'T_PRINT', + 'T_PRIVATE', + 'T_PROTECTED', + 'T_PUBLIC', + 'T_REQUIRE', + 'T_REQUIRE_ONCE', + 'T_RETURN', + 'T_STATIC', + 'T_SWITCH', + 'T_THROW', 'T_TRAIT', + 'T_TRY', + 'T_UNSET', + 'T_USE', + 'T_VAR', + 'T_WHILE', 'T_YIELD', - 'T_FINALLY', )); + + // Because there is no `T_SELF` or `T_PARENT` token. + $class_static_accesses = $root + ->selectDescendantsOfType('n_CLASS_DECLARATION') + ->selectDescendantsOfType('n_CLASS_STATIC_ACCESS'); + foreach ($class_static_accesses as $class_static_access) { + $class_ref = $class_static_access->getChildByIndex(0); + + switch (strtolower($class_ref->getConcreteString())) { + case 'parent': + case 'self': + $tokens = $class_ref->getTokens(); + + if (count($tokens) > 1) { + throw new Exception( + pht( + 'Unexpected tokens whilst processing `%s`.', + __CLASS__)); + } + + $keywords[] = head($tokens); + break; + } + } + foreach ($keywords as $keyword) { $value = $keyword->getValue(); diff --git a/src/lint/linter/xhpast/rules/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule.php new file mode 100644 index 00000000..4be549ba --- /dev/null +++ b/src/lint/linter/xhpast/rules/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule.php @@ -0,0 +1,40 @@ +<?php + +final class ArcanistPaamayimNekudotayimSpacingXHPASTLinterRule + extends ArcanistXHPASTLinterRule { + + const ID = 96; + + public function getLintName() { + return pht('Paamayim Nekudotayim Spacing'); + } + + public function getLintSeverity() { + return ArcanistLintSeverity::SEVERITY_WARNING; + } + + public function process(XHPASTNode $root) { + $double_colons = $root->selectTokensOfType('T_PAAMAYIM_NEKUDOTAYIM'); + + foreach ($double_colons as $double_colon) { + $tokens = $double_colon->getNonsemanticTokensBefore() + + $double_colon->getNonsemanticTokensAfter(); + + foreach ($tokens as $token) { + if ($token->isAnyWhitespace()) { + if (strpos($token->getValue(), "\n") !== false) { + continue; + } + + $this->raiseLintAtToken( + $token, + pht( + 'Unnecessary whitespace around paamayim nekudotayim '. + '(double colon) operator.'), + ''); + } + } + } + } + +} diff --git a/src/lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php new file mode 100644 index 00000000..7ad7c238 --- /dev/null +++ b/src/lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php @@ -0,0 +1,45 @@ +<?php + +final class ArcanistSelfClassReferenceXHPASTLinterRule + extends ArcanistXHPASTLinterRule { + + const ID = 95; + + public function getLintName() { + return pht('Self Class Reference'); + } + + public function getLintSeverity() { + return ArcanistLintSeverity::SEVERITY_WARNING; + } + + public function process(XHPASTNode $root) { + $class_declarations = $root->selectDescendantsOfType('n_CLASS_DECLARATION'); + + foreach ($class_declarations as $class_declaration) { + $class_name = $class_declaration + ->getChildOfType(1, 'n_CLASS_NAME') + ->getConcreteString(); + $instantiations = $class_declaration + ->selectDescendantsOfType('n_NEW'); + + foreach ($instantiations as $instantiation) { + $type = $instantiation->getChildByIndex(0); + + if ($type->getTypeName() != 'n_CLASS_NAME') { + continue; + } + + if (strtolower($type->getConcreteString()) == strtolower($class_name)) { + $this->raiseLintAtNode( + $type, + pht( + 'Use `%s` to instantiate the current class.', + 'self'), + 'self'); + } + } + } + } + +} diff --git a/src/lint/linter/xhpast/rules/ArcanistSelfMemberReferenceXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistSelfMemberReferenceXHPASTLinterRule.php index bfec780e..8db13ca7 100644 --- a/src/lint/linter/xhpast/rules/ArcanistSelfMemberReferenceXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistSelfMemberReferenceXHPASTLinterRule.php @@ -10,7 +10,7 @@ final class ArcanistSelfMemberReferenceXHPASTLinterRule } public function getLintSeverity() { - return ArcanistLintSeverity::SEVERITY_ADVICE; + return ArcanistLintSeverity::SEVERITY_WARNING; } public function process(XHPASTNode $root) { @@ -20,14 +20,11 @@ final class ArcanistSelfMemberReferenceXHPASTLinterRule $class_name = $class_declaration ->getChildOfType(1, 'n_CLASS_NAME') ->getConcreteString(); - $class_static_accesses = $class_declaration ->selectDescendantsOfType('n_CLASS_STATIC_ACCESS'); $closures = $this->getAnonymousClosures($class_declaration); foreach ($class_static_accesses as $class_static_access) { - $double_colons = $class_static_access - ->selectTokensOfType('T_PAAMAYIM_NEKUDOTAYIM'); $class_ref = $class_static_access->getChildByIndex(0); if ($class_ref->getTypeName() != 'n_CLASS_NAME') { @@ -54,43 +51,6 @@ final class ArcanistSelfMemberReferenceXHPASTLinterRule 'self'); } } - - static $self_refs = array( - 'parent', - 'self', - 'static', - ); - - if (!in_array(strtolower($class_ref_name), $self_refs)) { - continue; - } - - if ($class_ref_name != strtolower($class_ref_name)) { - $this->raiseLintAtNode( - $class_ref, - pht('PHP keywords should be lowercase.'), - strtolower($class_ref_name)); - } - } - } - - $double_colons = $root->selectTokensOfType('T_PAAMAYIM_NEKUDOTAYIM'); - - foreach ($double_colons as $double_colon) { - $tokens = $double_colon->getNonsemanticTokensBefore() + - $double_colon->getNonsemanticTokensAfter(); - - foreach ($tokens as $token) { - if ($token->isAnyWhitespace()) { - if (strpos($token->getValue(), "\n") !== false) { - continue; - } - - $this->raiseLintAtToken( - $token, - pht('Unnecessary whitespace around double colon operator.'), - ''); - } } } } diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase.php new file mode 100644 index 00000000..d847cdf1 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase.php @@ -0,0 +1,11 @@ +<?php + +final class ArcanistPaamayimNekudotayimSpacingXHPASTLinterRuleTestCase + extends ArcanistXHPASTLinterRuleTestCase { + + public function testLinter() { + $this->executeTestsInDirectory( + dirname(__FILE__).'/paamayim-nekudotayim-spacing/'); + } + +} diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistSelfClassReferenceXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistSelfClassReferenceXHPASTLinterRuleTestCase.php new file mode 100644 index 00000000..873abfd6 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistSelfClassReferenceXHPASTLinterRuleTestCase.php @@ -0,0 +1,10 @@ +<?php + +final class ArcanistSelfClassReferenceXHPASTLinterRuleTestCase + extends ArcanistXHPASTLinterRuleTestCase { + + public function testLinter() { + $this->executeTestsInDirectory(dirname(__FILE__).'/self-class-reference/'); + } + +} diff --git a/src/lint/linter/xhpast/rules/__tests__/keyword-casing/parent.lint-test b/src/lint/linter/xhpast/rules/__tests__/keyword-casing/parent.lint-test new file mode 100644 index 00000000..281f4148 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/keyword-casing/parent.lint-test @@ -0,0 +1,25 @@ +<?php + +class Foo extends Bar { + public static function doSomething() { + return PARENT::doSomething(); + } + + public static function doSomethingElse() { + return parent::doSomethingElse(); + } +} +~~~~~~~~~~ +warning:5:12 +~~~~~~~~~~ +<?php + +class Foo extends Bar { + public static function doSomething() { + return parent::doSomething(); + } + + public static function doSomethingElse() { + return parent::doSomethingElse(); + } +} diff --git a/src/lint/linter/xhpast/rules/__tests__/keyword-casing/self.lint-test b/src/lint/linter/xhpast/rules/__tests__/keyword-casing/self.lint-test new file mode 100644 index 00000000..59692bd3 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/keyword-casing/self.lint-test @@ -0,0 +1,25 @@ +<?php + +class Foo extends Bar { + public static function doSomething() { + return SELF::doSomethingElse(); + } + + public static function doSomethingElse() { + return self::doSomething(); + } +} +~~~~~~~~~~ +warning:5:12 +~~~~~~~~~~ +<?php + +class Foo extends Bar { + public static function doSomething() { + return self::doSomethingElse(); + } + + public static function doSomethingElse() { + return self::doSomething(); + } +} diff --git a/src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/comment.lint-test b/src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/comment.lint-test new file mode 100644 index 00000000..37920a78 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/comment.lint-test @@ -0,0 +1,5 @@ +<?php + +// This is quite odd, but seems somewhat reasonable. +MyClass/* comment */::/* another comment */myMethod(); +~~~~~~~~~~ diff --git a/src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/newline.lint-test b/src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/newline.lint-test new file mode 100644 index 00000000..01ef589c --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/newline.lint-test @@ -0,0 +1,6 @@ +<?php +SomeReallyLongClassName:: + someMethod(); +SomeReallyLongClassName + ::someMethod(); +~~~~~~~~~~ diff --git a/src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/paamayim-nekudotayim-spacing.lint-test b/src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/paamayim-nekudotayim-spacing.lint-test new file mode 100644 index 00000000..7ee65d7b --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/paamayim-nekudotayim-spacing/paamayim-nekudotayim-spacing.lint-test @@ -0,0 +1,36 @@ +<?php + +class Foo extends Bar { + public function bar() { + echo self::FOOBAR; + echo self :: FOOBAR; + } +} + +MyClass::myMethod(); +MyClass :: myMethod(); + +MyClass::$myProperty; +MyClass :: $myProperty; +~~~~~~~~~~ +warning:6:14 +warning:6:17 +warning:11:8 +warning:11:11 +warning:14:8 +warning:14:11 +~~~~~~~~~~ +<?php + +class Foo extends Bar { + public function bar() { + echo self::FOOBAR; + echo self::FOOBAR; + } +} + +MyClass::myMethod(); +MyClass::myMethod(); + +MyClass::$myProperty; +MyClass::$myProperty; diff --git a/src/lint/linter/xhpast/rules/__tests__/self-class-reference/self-class-references.lint-test b/src/lint/linter/xhpast/rules/__tests__/self-class-reference/self-class-references.lint-test new file mode 100644 index 00000000..2f2c1978 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/self-class-reference/self-class-references.lint-test @@ -0,0 +1,25 @@ +<?php + +class Foo extends Bar { + public static function newInstance() { + return new Foo(); + } + + public static function init() { + return new self(); + } +} +~~~~~~~~~~ +warning:5:16 +~~~~~~~~~~ +<?php + +class Foo extends Bar { + public static function newInstance() { + return new self(); + } + + public static function init() { + return new self(); + } +} diff --git a/src/lint/linter/xhpast/rules/__tests__/self-member-reference/php54.lint-test b/src/lint/linter/xhpast/rules/__tests__/self-member-reference/php54.lint-test index 44c94cf2..f0b46c17 100644 --- a/src/lint/linter/xhpast/rules/__tests__/self-member-reference/php54.lint-test +++ b/src/lint/linter/xhpast/rules/__tests__/self-member-reference/php54.lint-test @@ -4,12 +4,13 @@ final class SomeClass extends Phobject { public static function someMethod() { $closure = function () { SomeClass::someOtherMethod(); + self::someOtherMethod(); }; $closure(); } } ~~~~~~~~~~ -advice:6:7 +warning:6:7 ~~~~~~~~~~ <?php @@ -17,6 +18,7 @@ final class SomeClass extends Phobject { public static function someMethod() { $closure = function () { self::someOtherMethod(); + self::someOtherMethod(); }; $closure(); } diff --git a/src/lint/linter/xhpast/rules/__tests__/self-member-reference/self-member-reference.lint-test b/src/lint/linter/xhpast/rules/__tests__/self-member-reference/self-member-reference.lint-test index 7ba51103..5aef8974 100644 --- a/src/lint/linter/xhpast/rules/__tests__/self-member-reference/self-member-reference.lint-test +++ b/src/lint/linter/xhpast/rules/__tests__/self-member-reference/self-member-reference.lint-test @@ -3,59 +3,21 @@ class Foo extends Bar { const FOOBAR = 'FOOBAR'; - public function __construct() { - PARENT::__construct(null); - } - - public function bar() { - echo self::FOOBAR; - echo Self :: FOOBAR; - } - - public function baz(Foo $x) { - echo static::FOOBAR; + public function baz() { echo Foo::FOOBAR; - - $x::bar(); + echo self::FOOBAR; } } - -MyClass :: myMethod(); - -SomeReallyLongClassName - ::someMethod(); ~~~~~~~~~~ -advice:7:5 -advice:12:10 -advice:12:14 -advice:12:17 -advice:17:10 -advice:23:8 -advice:23:11 +warning:7:10 ~~~~~~~~~~ <?php class Foo extends Bar { const FOOBAR = 'FOOBAR'; - public function __construct() { - parent::__construct(null); - } - - public function bar() { + public function baz() { echo self::FOOBAR; echo self::FOOBAR; } - - public function baz(Foo $x) { - echo static::FOOBAR; - echo self::FOOBAR; - - $x::bar(); - } } - -MyClass::myMethod(); - -SomeReallyLongClassName - ::someMethod(); From b3e68c9f179318d65b6a2512efa6830e0362de8e Mon Sep 17 00:00:00 2001 From: Joshua Spence <josh@freelancer.com> Date: Wed, 23 Dec 2015 08:42:00 +1100 Subject: [PATCH 14/14] Add a linter rule for use of `is_a` Summary: `instanceof` should be used instead of `is_a`. I need to do a bit more research here to see if there are any edge cases. Test Plan: Added unit tests. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14573 --- src/__phutil_library_map__.php | 4 ++ ...tIsAShouldBeInstanceOfXHPASTLinterRule.php | 69 +++++++++++++++++++ ...ldBeInstanceOfXHPASTLinterRuleTestCase.php | 11 +++ .../allow_string.lint-test | 3 + .../is_a-should-be-instanceof/is_a.lint-test | 16 +++++ 5 files changed, 103 insertions(+) create mode 100644 src/lint/linter/xhpast/rules/ArcanistIsAShouldBeInstanceOfXHPASTLinterRule.php create mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase.php create mode 100644 src/lint/linter/xhpast/rules/__tests__/is_a-should-be-instanceof/allow_string.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/is_a-should-be-instanceof/is_a.lint-test diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 13ef8642..6012daf7 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -196,6 +196,8 @@ phutil_register_library_map(array( 'ArcanistInvalidModifiersXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistInvalidModifiersXHPASTLinterRuleTestCase.php', 'ArcanistInvalidOctalNumericScalarXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistInvalidOctalNumericScalarXHPASTLinterRule.php', 'ArcanistInvalidOctalNumericScalarXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistInvalidOctalNumericScalarXHPASTLinterRuleTestCase.php', + 'ArcanistIsAShouldBeInstanceOfXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistIsAShouldBeInstanceOfXHPASTLinterRule.php', + 'ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase.php', 'ArcanistJSHintLinter' => 'lint/linter/ArcanistJSHintLinter.php', 'ArcanistJSHintLinterTestCase' => 'lint/linter/__tests__/ArcanistJSHintLinterTestCase.php', 'ArcanistJSONLintLinter' => 'lint/linter/ArcanistJSONLintLinter.php', @@ -608,6 +610,8 @@ phutil_register_library_map(array( 'ArcanistInvalidModifiersXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistInvalidOctalNumericScalarXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistInvalidOctalNumericScalarXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', + 'ArcanistIsAShouldBeInstanceOfXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', + 'ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistJSHintLinter' => 'ArcanistExternalLinter', 'ArcanistJSHintLinterTestCase' => 'ArcanistExternalLinterTestCase', 'ArcanistJSONLintLinter' => 'ArcanistExternalLinter', diff --git a/src/lint/linter/xhpast/rules/ArcanistIsAShouldBeInstanceOfXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistIsAShouldBeInstanceOfXHPASTLinterRule.php new file mode 100644 index 00000000..70561e41 --- /dev/null +++ b/src/lint/linter/xhpast/rules/ArcanistIsAShouldBeInstanceOfXHPASTLinterRule.php @@ -0,0 +1,69 @@ +<?php + +final class ArcanistIsAShouldBeInstanceOfXHPASTLinterRule + extends ArcanistXHPASTLinterRule { + + const ID = 111; + + public function getLintName() { + return pht('`%s` Should Be `%s`', 'is_a', 'instanceof'); + } + + public function getLintSeverity() { + return ArcanistLintSeverity::SEVERITY_ADVICE; + } + + public function process(XHPASTNode $root) { + $calls = $this->getFunctionCalls($root, array('is_a')); + + foreach ($calls as $call) { + $parameters = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST'); + + if (count($parameters->getChildren()) > 2) { + // If the `$allow_string` parameter is `true` then the `instanceof` + // operator cannot be used. Evaluating whether an expression is truthy + // or falsely is hard, and so we only check that the `$allow_string` + // parameter is either absent or literally `false`. + $allow_string = $parameters->getChildByIndex(2); + + if (strtolower($allow_string->getConcreteString()) != 'false') { + continue; + } + } + + $object = $parameters->getChildByIndex(0); + $class = $parameters->getChildByIndex(1); + + switch ($class->getTypeName()) { + case 'n_STRING_SCALAR': + $replacement = stripslashes( + substr($class->getConcreteString(), 1, -1)); + break; + + case 'n_VARIABLE': + $replacement = $class->getConcreteString(); + break; + + default: + $replacement = null; + break; + } + + $this->raiseLintAtNode( + $call, + pht( + 'Use `%s` instead of `%s`. The former is a language '. + 'construct whereas the latter is a function call, which '. + 'has additional overhead.', + 'instanceof', + 'is_a'), + ($replacement === null) + ? null + : sprintf( + '%s instanceof %s', + $object->getConcreteString(), + $replacement)); + } + } + +} diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase.php new file mode 100644 index 00000000..7a463bf6 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase.php @@ -0,0 +1,11 @@ +<?php + +final class ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase + extends ArcanistXHPASTLinterRuleTestCase { + + public function testLinter() { + $this->executeTestsInDirectory( + dirname(__FILE__).'/is_a-should-be-instanceof/'); + } + +} diff --git a/src/lint/linter/xhpast/rules/__tests__/is_a-should-be-instanceof/allow_string.lint-test b/src/lint/linter/xhpast/rules/__tests__/is_a-should-be-instanceof/allow_string.lint-test new file mode 100644 index 00000000..9d184141 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/is_a-should-be-instanceof/allow_string.lint-test @@ -0,0 +1,3 @@ +<?php +is_a('SubClass', 'BaseClass', true); +~~~~~~~~~~ diff --git a/src/lint/linter/xhpast/rules/__tests__/is_a-should-be-instanceof/is_a.lint-test b/src/lint/linter/xhpast/rules/__tests__/is_a-should-be-instanceof/is_a.lint-test new file mode 100644 index 00000000..3362c41c --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/is_a-should-be-instanceof/is_a.lint-test @@ -0,0 +1,16 @@ +<?php +is_a($x, $y); +is_a($x, 'SomeClass'); +is_a($x, '\\SomeClass'); +is_a($x, '\\'.'Some'.'Class'); +~~~~~~~~~~ +advice:2:1 +advice:3:1 +advice:4:1 +advice:5:1 +~~~~~~~~~~ +<?php +$x instanceof $y; +$x instanceof SomeClass; +$x instanceof \SomeClass; +is_a($x, '\\'.'Some'.'Class');