From e5946ed1a5fa9ea3cf0636c7a4af4342888a3d9b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Jul 2015 16:31:03 -0700 Subject: [PATCH 01/11] Also coerce "char" for lint messages Summary: Ref T8921. See similar change in D13695. This expands the scope to also coerce `setChar()`. Test Plan: `arc unit --everything` Reviewers: btrahan Subscribers: epriestley Maniphest Tasks: T8921 Differential Revision: https://secure.phabricator.com/D13737 --- src/lint/ArcanistLintMessage.php | 52 ++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php index 40502020..d4de43b8 100644 --- a/src/lint/ArcanistLintMessage.php +++ b/src/lint/ArcanistLintMessage.php @@ -72,24 +72,7 @@ final class ArcanistLintMessage extends Phobject { } public function setLine($line) { - if ($line === null) { - // This just means that we don't have any line information. - } else { - // For compatibility, accept digit strings since a lot of linters pass - // line numbers that they have parsed from command output or XML, which - // won't be properly typed. - if (is_string($line) && preg_match('/^\d+\z/', $line)) { - $line = (int)$line; - } - - if (!is_int($line)) { - throw new Exception( - pht( - 'Parameter passed to setLine() must be an integer.')); - } - } - - $this->line = $line; + $this->line = $this->validateInteger($line, 'setLine'); return $this; } @@ -98,7 +81,7 @@ final class ArcanistLintMessage extends Phobject { } public function setChar($char) { - $this->char = $char; + $this->char = $this->validateInteger($char, 'setChar'); return $this; } @@ -242,4 +225,35 @@ final class ArcanistLintMessage extends Phobject { return $this->bypassChangedLineFiltering; } + /** + * Validate an integer-like value, returning a strict integer. + * + * Further on, the pipeline is strict about types. We want to be a little + * less strict in linters themselves, since they often parse command line + * output or XML and will end up with string representations of numbers. + * + * @param mixed Integer or digit string. + * @return int Integer. + */ + private function validateInteger($value, $caller) { + if ($value === null) { + // This just means that we don't have any information. + return null; + } + + // Strings like "234" are fine, coerce them to integers. + if (is_string($value) && preg_match('/^\d+\z/', $value)) { + $value = (int)$value; + } + + if (!is_int($value)) { + throw new Exception( + pht( + 'Parameter passed to "%s" must be an integer.', + $caller.'()')); + } + + return $value; + } + } From d3b316ad3511be832df08cb994c33235db95e752 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Mon, 3 Aug 2015 06:47:20 +1000 Subject: [PATCH 02/11] Fix an undeclared property Summary: `NoseTestEngine::$parser` is undefined. Test Plan: N/A Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: epriestley, Korvin Differential Revision: https://secure.phabricator.com/D13760 --- src/unit/engine/NoseTestEngine.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/unit/engine/NoseTestEngine.php b/src/unit/engine/NoseTestEngine.php index 5e5870e1..e81bcb63 100644 --- a/src/unit/engine/NoseTestEngine.php +++ b/src/unit/engine/NoseTestEngine.php @@ -7,6 +7,8 @@ */ final class NoseTestEngine extends ArcanistUnitTestEngine { + private $parser; + public function run() { $paths = $this->getPaths(); From 0d6f3328a08fc2d22f97b9ec43e6b84924ddb62c Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Mon, 3 Aug 2015 06:47:47 +1000 Subject: [PATCH 03/11] Fix some failing unit tests Summary: I somehow missed these. Test Plan: `arc unit` Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: epriestley, Korvin Differential Revision: https://secure.phabricator.com/D13693 --- .../linter/__tests__/xhpast/decl-parens-hug-closing.lint-test | 2 +- src/lint/linter/__tests__/xhpast/naming-conventions.lint-test | 2 +- src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lint/linter/__tests__/xhpast/decl-parens-hug-closing.lint-test b/src/lint/linter/__tests__/xhpast/decl-parens-hug-closing.lint-test index a7ea5adf..c6accb15 100644 --- a/src/lint/linter/__tests__/xhpast/decl-parens-hug-closing.lint-test +++ b/src/lint/linter/__tests__/xhpast/decl-parens-hug-closing.lint-test @@ -28,7 +28,7 @@ warning:4:14 warning:5:11 warning:8:15 error:10:13 -warning:11:23 +warning:13:23 warning:16:31 warning:19:33 warning:24:14 diff --git a/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test b/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test index cc78e403..3bf1a71a 100644 --- a/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test +++ b/src/lint/linter/__tests__/xhpast/naming-conventions.lint-test @@ -21,7 +21,7 @@ final class Quack { -function () use ($this_is_a_closure) {}; +function() use ($this_is_a_closure) {}; function f(&$YY) {} diff --git a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test index 7c4b9c6d..3b07c19c 100644 --- a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test +++ b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test @@ -1,6 +1,6 @@ Date: Tue, 4 Aug 2015 13:05:38 -0700 Subject: [PATCH 04/11] Add more information about Harbormaster/Unit result codes to Arcanist Summary: Ref T7419. This makes it easier to render helpful documentation in the next diff without having to copy/paste things. Test Plan: In next diff: {F688894} Reviewers: chad Reviewed By: chad Maniphest Tasks: T7419 Differential Revision: https://secure.phabricator.com/D13788 --- src/unit/ArcanistUnitTestResult.php | 64 +++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/unit/ArcanistUnitTestResult.php b/src/unit/ArcanistUnitTestResult.php index f570c072..039393f8 100644 --- a/src/unit/ArcanistUnitTestResult.php +++ b/src/unit/ArcanistUnitTestResult.php @@ -152,4 +152,68 @@ final class ArcanistUnitTestResult extends Phobject { ); } + public static function getAllResultCodes() { + return array( + self::RESULT_PASS, + self::RESULT_FAIL, + self::RESULT_SKIP, + self::RESULT_BROKEN, + self::RESULT_UNSOUND, + ); + } + + public static function getResultCodeName($result_code) { + $spec = self::getResultCodeSpec($result_code); + if (!$spec) { + return null; + } + return idx($spec, 'name'); + } + + public static function getResultCodeDescription($result_code) { + $spec = self::getResultCodeSpec($result_code); + if (!$spec) { + return null; + } + return idx($spec, 'description'); + } + + private static function getResultCodeSpec($result_code) { + $specs = self::getResultCodeSpecs(); + return idx($specs, $result_code); + } + + private static function getResultCodeSpecs() { + return array( + self::RESULT_PASS => array( + 'name' => pht('Pass'), + 'description' => pht( + 'The test passed.'), + ), + self::RESULT_FAIL => array( + 'name' => pht('Fail'), + 'description' => pht( + 'The test failed.'), + ), + self::RESULT_SKIP => array( + 'name' => pht('Skip'), + 'description' => pht( + 'The test was not executed.'), + ), + self::RESULT_BROKEN => array( + 'name' => pht('Broken'), + 'description' => pht( + 'The test failed in an abnormal or severe way. For example, the '. + 'harness crashed instead of reporting a failure.'), + ), + self::RESULT_UNSOUND => array( + 'name' => pht('Unsound'), + 'description' => pht( + 'The test failed, but this change is probably not what broke it. '. + 'For example, it might have already been failing.'), + ), + ); + } + + } From 402899a9c32d6079a913546cadf0ea54f127e97b Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Thu, 6 Aug 2015 06:56:26 +1000 Subject: [PATCH 05/11] Allow closing tags in files containing HTML Summary: Ref T8742. PHP files which contain inline HTML should be allowed to use PHP closing tags (`?>`). This is in accordance with [[https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-2-coding-style-guide.md | PSR-2 guidelines]]. Test Plan: Updated unit tests. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T8742 Differential Revision: https://secure.phabricator.com/D13794 --- src/lint/linter/__tests__/xhpast/embedded-tags.lint-test | 1 - src/lint/linter/__tests__/xhpast/php-tags-bad.lint-test | 1 - .../xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php | 6 ++++++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/lint/linter/__tests__/xhpast/embedded-tags.lint-test b/src/lint/linter/__tests__/xhpast/embedded-tags.lint-test index b9f6be44..d15cefb5 100644 --- a/src/lint/linter/__tests__/xhpast/embedded-tags.lint-test +++ b/src/lint/linter/__tests__/xhpast/embedded-tags.lint-test @@ -2,4 +2,3 @@ This shouldn't fatal the parser. ~~~~~~~~~~ -error:1:10 diff --git a/src/lint/linter/__tests__/xhpast/php-tags-bad.lint-test b/src/lint/linter/__tests__/xhpast/php-tags-bad.lint-test index e749bfa6..7fb52e73 100644 --- a/src/lint/linter/__tests__/xhpast/php-tags-bad.lint-test +++ b/src/lint/linter/__tests__/xhpast/php-tags-bad.lint-test @@ -4,7 +4,6 @@ garbage garbage ?> ~~~~~~~~~~ error:1:1 -error:4:1 ~~~~~~~~~~ garbage garbage selectDescendantsOfType('n_INLINE_HTML'); + + if ($inline_html) { + return; + } + foreach ($root->selectTokensOfType('T_CLOSE_TAG') as $token) { $this->raiseLintAtToken( $token, From bf4e7d4ca89a9ab6eb17f5dba4d3f3b6d6dc4c08 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Thu, 6 Aug 2015 06:57:16 +1000 Subject: [PATCH 06/11] Improve undeclared variables linter rule Summary: Currently `ArcanistUndeclaredVariableXHPASTLinterRule` does not properly handle anonymous closures. Test Plan: Added test cases. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: johnny-bit, Korvin Differential Revision: https://secure.phabricator.com/D13795 --- .../__tests__/xhpast/undeclared-variables.lint-test | 8 ++++++++ .../ArcanistUndeclaredVariableXHPASTLinterRule.php | 11 +++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test index 3b07c19c..903e620a 100644 --- a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test +++ b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test @@ -172,6 +172,13 @@ function catchy() { } } +function some_func($x, $y) { + $func = function($z) use ($x) { + echo $x; + echo $y; + echo $z; + }; +} ~~~~~~~~~~ error:28:3 error:30:3 @@ -191,3 +198,4 @@ error:144:8 error:150:9 error:164:9 error:171:5 +error:178:10 diff --git a/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php index afb07968..c60b216c 100644 --- a/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php @@ -164,6 +164,17 @@ final class ArcanistUndeclaredVariableXHPASTLinterRule $scope_destroyed_at = min($scope_destroyed_at, $call->getOffset()); } + $func_decls = $body->selectDescendantsOfType('n_FUNCTION_DECLARATION'); + foreach ($func_decls as $func_decl) { + if ($func_decl->getChildByIndex(2)->getTypeName() != 'n_EMPTY') { + continue; + } + + foreach ($func_decl->selectDescendantsOfType('n_VARIABLE') as $var) { + $exclude_tokens[$var->getID()] = true; + } + } + // Now we have every declaration except foreach(), handled below. Build // two maps, one which just keeps track of which tokens are part of // declarations ($declaration_tokens) and one which has the first offset From 1e75302e77030b197ab8ac13e84458cf0a246635 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Thu, 6 Aug 2015 06:57:46 +1000 Subject: [PATCH 07/11] Add a linter rule for spacing before opening parenthesis in function calls Summary: Repurpose `ArcanistClosingCallParenthesesXHPASTLinterRule` (and rename it to `ArcanistCallParenthesesXHPASTLinterRule`) to ensure that there is no spacing before opening parenthesis in function calls. Test Plan: Added test case. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D13796 --- src/__phutil_library_map__.php | 4 ++-- .../xhpast/call-parens-hug-closing.lint-test | 3 +++ ...rcanistCallParenthesesXHPASTLinterRule.php} | 18 +++++++++++++++++- 3 files changed, 22 insertions(+), 3 deletions(-) rename src/lint/linter/xhpast/rules/{ArcanistClosingCallParenthesesXHPASTLinterRule.php => ArcanistCallParenthesesXHPASTLinterRule.php} (66%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 680febaf..49ccb0d8 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -32,6 +32,7 @@ phutil_register_library_map(array( 'ArcanistCSSLintLinterTestCase' => 'lint/linter/__tests__/ArcanistCSSLintLinterTestCase.php', 'ArcanistCSharpLinter' => 'lint/linter/ArcanistCSharpLinter.php', 'ArcanistCallConduitWorkflow' => 'workflow/ArcanistCallConduitWorkflow.php', + 'ArcanistCallParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistCallParenthesesXHPASTLinterRule.php', 'ArcanistCallTimePassByReferenceXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistCallTimePassByReferenceXHPASTLinterRule.php', 'ArcanistCapabilityNotSupportedException' => 'workflow/exception/ArcanistCapabilityNotSupportedException.php', 'ArcanistCastSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistCastSpacingXHPASTLinterRule.php', @@ -42,7 +43,6 @@ phutil_register_library_map(array( 'ArcanistClassNameLiteralXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistClassNameLiteralXHPASTLinterRule.php', 'ArcanistCloseRevisionWorkflow' => 'workflow/ArcanistCloseRevisionWorkflow.php', 'ArcanistCloseWorkflow' => 'workflow/ArcanistCloseWorkflow.php', - 'ArcanistClosingCallParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistClosingCallParenthesesXHPASTLinterRule.php', 'ArcanistClosureLinter' => 'lint/linter/ArcanistClosureLinter.php', 'ArcanistClosureLinterTestCase' => 'lint/linter/__tests__/ArcanistClosureLinterTestCase.php', 'ArcanistCoffeeLintLinter' => 'lint/linter/ArcanistCoffeeLintLinter.php', @@ -305,6 +305,7 @@ phutil_register_library_map(array( 'ArcanistCSSLintLinterTestCase' => 'ArcanistExternalLinterTestCase', 'ArcanistCSharpLinter' => 'ArcanistLinter', 'ArcanistCallConduitWorkflow' => 'ArcanistWorkflow', + 'ArcanistCallParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistCallTimePassByReferenceXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistCapabilityNotSupportedException' => 'Exception', 'ArcanistCastSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', @@ -315,7 +316,6 @@ phutil_register_library_map(array( 'ArcanistClassNameLiteralXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistCloseRevisionWorkflow' => 'ArcanistWorkflow', 'ArcanistCloseWorkflow' => 'ArcanistWorkflow', - 'ArcanistClosingCallParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistClosureLinter' => 'ArcanistExternalLinter', 'ArcanistClosureLinterTestCase' => 'ArcanistExternalLinterTestCase', 'ArcanistCoffeeLintLinter' => 'ArcanistExternalLinter', diff --git a/src/lint/linter/__tests__/xhpast/call-parens-hug-closing.lint-test b/src/lint/linter/__tests__/xhpast/call-parens-hug-closing.lint-test index 81ad7a13..3ec2eaa5 100644 --- a/src/lint/linter/__tests__/xhpast/call-parens-hug-closing.lint-test +++ b/src/lint/linter/__tests__/xhpast/call-parens-hug-closing.lint-test @@ -16,9 +16,11 @@ The duck says, "Quack!" The robot says, "Beep beep boop boop!" EODOC ); +f (1); ~~~~~~~~~~ warning:4:4 warning:9:4 +warning:19:2 ~~~~~~~~~~ getChildOfType(1, 'n_CALL_PARAMETER_LIST'); + $tokens = $params->getTokens(); + $first = head($tokens); + + $leading = $first->getNonsemanticTokensBefore(); + $leading_text = implode('', mpull($leading, 'getValue')); + if (preg_match('/^\s+$/', $leading_text)) { + $this->raiseLintAtOffset( + $first->getOffset() - strlen($leading_text), + pht('Convention: no spaces before opening parenthesis in calls.'), + $leading_text, + ''); + } + } + foreach ($calls as $call) { // If the last parameter of a call is a HEREDOC, don't apply this rule. $params = $call From c8f0deffab602bc3177c4e262d198f92c43818ff Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Thu, 6 Aug 2015 07:00:32 +1000 Subject: [PATCH 08/11] Add a linter rule for unary prefix expression spacing Summary: Add a linter rule to ensure that there is no space between the operator and the operand in a unary prefix expression. Test Plan: Added test cases. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D13797 --- src/__phutil_library_map__.php | 2 + .../xhpast/exit-expression.lint-test | 7 +++ .../unary-prefix-expression-spacing.lint-test | 12 ++++++ ...refixExpressionSpacingXHPASTLinterRule.php | 43 +++++++++++++++++++ 4 files changed, 64 insertions(+) create mode 100644 src/lint/linter/__tests__/xhpast/unary-prefix-expression-spacing.lint-test create mode 100644 src/lint/linter/xhpast/rules/ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 49ccb0d8..64dc1a44 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -237,6 +237,7 @@ phutil_register_library_map(array( 'ArcanistTodoWorkflow' => 'workflow/ArcanistTodoWorkflow.php', 'ArcanistUSEnglishTranslation' => 'internationalization/ArcanistUSEnglishTranslation.php', 'ArcanistUnableToParseXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnableToParseXHPASTLinterRule.php', + 'ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule.php', 'ArcanistUndeclaredVariableXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php', 'ArcanistUnitConsoleRenderer' => 'unit/renderer/ArcanistUnitConsoleRenderer.php', 'ArcanistUnitRenderer' => 'unit/renderer/ArcanistUnitRenderer.php', @@ -510,6 +511,7 @@ phutil_register_library_map(array( 'ArcanistTodoWorkflow' => 'ArcanistWorkflow', 'ArcanistUSEnglishTranslation' => 'PhutilTranslation', 'ArcanistUnableToParseXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', + 'ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistUndeclaredVariableXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistUnitConsoleRenderer' => 'ArcanistUnitRenderer', 'ArcanistUnitRenderer' => 'Phobject', diff --git a/src/lint/linter/__tests__/xhpast/exit-expression.lint-test b/src/lint/linter/__tests__/xhpast/exit-expression.lint-test index 96fbeb6f..c9724096 100644 --- a/src/lint/linter/__tests__/xhpast/exit-expression.lint-test +++ b/src/lint/linter/__tests__/xhpast/exit-expression.lint-test @@ -2,7 +2,14 @@ exit(-1); exit -1; strtoupper(33 * exit - 6); +echo ''; +print ''; + +$x = new stdClass(); +$y = clone $x; ~~~~~~~~~~ error:3:1 +warning:3:5 warning:3:6 error:4:17 +warning:4:21 diff --git a/src/lint/linter/__tests__/xhpast/unary-prefix-expression-spacing.lint-test b/src/lint/linter/__tests__/xhpast/unary-prefix-expression-spacing.lint-test new file mode 100644 index 00000000..87b65c64 --- /dev/null +++ b/src/lint/linter/__tests__/xhpast/unary-prefix-expression-spacing.lint-test @@ -0,0 +1,12 @@ +selectDescendantsOfType('n_UNARY_PREFIX_EXPRESSION'); + + foreach ($expressions as $expression) { + $operator = $expression->getChildOfType(0, 'n_OPERATOR'); + $operator_value = $operator->getConcreteString(); + list($before, $after) = $operator->getSurroundingNonsemanticTokens(); + + switch (strtolower($operator_value)) { + case 'clone': + case 'echo': + case 'print': + break; + + default: + if (!empty($after)) { + $this->raiseLintAtOffset( + $operator->getOffset() + strlen($operator->getConcreteString()), + pht('Unary operators should not be followed by whitespace.'), + implode('', mpull($after, 'getValue')), + ''); + } + break; + } + } + } + +} From 986f5d82d0739667d1436027691ceb1031a114cf Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Thu, 6 Aug 2015 07:14:38 +1000 Subject: [PATCH 09/11] Add a linter rule for object operator spacing Summary: Add a linter rule to check that there is no whitespace surrounding the object operator, `->`. Test Plan: Added test case. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D13798 --- src/__phutil_library_map__.php | 2 + .../xhpast/object-operating-spacing.lint-test | 12 +++++ ...tObjectOperatorSpacingXHPASTLinterRule.php | 47 +++++++++++++++++++ 3 files changed, 61 insertions(+) create mode 100644 src/lint/linter/__tests__/xhpast/object-operating-spacing.lint-test create mode 100644 src/lint/linter/xhpast/rules/ArcanistObjectOperatorSpacingXHPASTLinterRule.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 64dc1a44..9b12f49d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -171,6 +171,7 @@ phutil_register_library_map(array( 'ArcanistNoLintLinterTestCase' => 'lint/linter/__tests__/ArcanistNoLintLinterTestCase.php', 'ArcanistNoParentScopeXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistNoParentScopeXHPASTLinterRule.php', 'ArcanistNoneLintRenderer' => 'lint/renderer/ArcanistNoneLintRenderer.php', + 'ArcanistObjectOperatorSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistObjectOperatorSpacingXHPASTLinterRule.php', 'ArcanistPEP8Linter' => 'lint/linter/ArcanistPEP8Linter.php', 'ArcanistPEP8LinterTestCase' => 'lint/linter/__tests__/ArcanistPEP8LinterTestCase.php', 'ArcanistPHPCloseTagXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php', @@ -445,6 +446,7 @@ phutil_register_library_map(array( 'ArcanistNoLintLinterTestCase' => 'ArcanistLinterTestCase', 'ArcanistNoParentScopeXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistNoneLintRenderer' => 'ArcanistLintRenderer', + 'ArcanistObjectOperatorSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistPEP8Linter' => 'ArcanistExternalLinter', 'ArcanistPEP8LinterTestCase' => 'ArcanistExternalLinterTestCase', 'ArcanistPHPCloseTagXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', diff --git a/src/lint/linter/__tests__/xhpast/object-operating-spacing.lint-test b/src/lint/linter/__tests__/xhpast/object-operating-spacing.lint-test new file mode 100644 index 00000000..fd20b231 --- /dev/null +++ b/src/lint/linter/__tests__/xhpast/object-operating-spacing.lint-test @@ -0,0 +1,12 @@ + doSomething(); +id(new Something()) + ->doSomething(); +~~~~~~~~~~ +warning:2:3 +warning:2:6 +~~~~~~~~~~ +doSomething(); +id(new Something()) + ->doSomething(); diff --git a/src/lint/linter/xhpast/rules/ArcanistObjectOperatorSpacingXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistObjectOperatorSpacingXHPASTLinterRule.php new file mode 100644 index 00000000..8fb7f4a4 --- /dev/null +++ b/src/lint/linter/xhpast/rules/ArcanistObjectOperatorSpacingXHPASTLinterRule.php @@ -0,0 +1,47 @@ +selectTokensOfType('T_OBJECT_OPERATOR'); + + foreach ($operators as $operator) { + $before = $operator->getNonsemanticTokensBefore(); + $after = $operator->getNonsemanticTokensAfter(); + + if ($before) { + $value = implode('', mpull($before, 'getValue')); + + if (strpos($value, "\n") !== false) { + continue; + } + + $this->raiseLintAtOffset( + head($before)->getOffset(), + pht('There should be no whitespace before the object operator.'), + $value, + ''); + } + + if ($after) { + $this->raiseLintAtOffset( + head($after)->getOffset(), + pht('There should be no whitespace after the object operator.'), + implode('', mpull($before, 'getValue')), + ''); + } + } + } + +} From 968f4ae5d73e41ad79be70c748d88390ab9be8e7 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Thu, 6 Aug 2015 11:38:14 +1000 Subject: [PATCH 10/11] Add a linter rule for unary postfix expression spacing Summary: Unary postfix expressions should not have a space before the operator. Test Plan: Added test case. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D13805 --- src/__phutil_library_map__.php | 2 ++ ...unary-postfix-expression-spacing.lint-test | 9 +++++ ...stfixExpressionSpacingXHPASTLinterRule.php | 36 +++++++++++++++++++ ...refixExpressionSpacingXHPASTLinterRule.php | 5 +-- 4 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 src/lint/linter/__tests__/xhpast/unary-postfix-expression-spacing.lint-test create mode 100644 src/lint/linter/xhpast/rules/ArcanistUnaryPostfixExpressionSpacingXHPASTLinterRule.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9b12f49d..4783a8d6 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -238,6 +238,7 @@ phutil_register_library_map(array( 'ArcanistTodoWorkflow' => 'workflow/ArcanistTodoWorkflow.php', 'ArcanistUSEnglishTranslation' => 'internationalization/ArcanistUSEnglishTranslation.php', 'ArcanistUnableToParseXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnableToParseXHPASTLinterRule.php', + 'ArcanistUnaryPostfixExpressionSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnaryPostfixExpressionSpacingXHPASTLinterRule.php', 'ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule.php', 'ArcanistUndeclaredVariableXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php', 'ArcanistUnitConsoleRenderer' => 'unit/renderer/ArcanistUnitConsoleRenderer.php', @@ -513,6 +514,7 @@ phutil_register_library_map(array( 'ArcanistTodoWorkflow' => 'ArcanistWorkflow', 'ArcanistUSEnglishTranslation' => 'PhutilTranslation', 'ArcanistUnableToParseXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', + 'ArcanistUnaryPostfixExpressionSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistUndeclaredVariableXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistUnitConsoleRenderer' => 'ArcanistUnitRenderer', diff --git a/src/lint/linter/__tests__/xhpast/unary-postfix-expression-spacing.lint-test b/src/lint/linter/__tests__/xhpast/unary-postfix-expression-spacing.lint-test new file mode 100644 index 00000000..6e08218e --- /dev/null +++ b/src/lint/linter/__tests__/xhpast/unary-postfix-expression-spacing.lint-test @@ -0,0 +1,9 @@ +selectDescendantsOfType('n_UNARY_POSTFIX_EXPRESSION'); + + foreach ($expressions as $expression) { + $operator = $expression->getChildOfType(1, 'n_OPERATOR'); + $operator_value = $operator->getConcreteString(); + list($before, $after) = $operator->getSurroundingNonsemanticTokens(); + + if (!empty($before)) { + $leading_text = implode('', mpull($before, 'getValue')); + + $this->raiseLintAtOffset( + $operator->getOffset() - strlen($leading_text), + pht('Unary postfix operators should not be prefixed by whitespace.'), + $leading_text, + ''); + } + } + } + +} diff --git a/src/lint/linter/xhpast/rules/ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule.php index 2d398c4e..5fa91ffd 100644 --- a/src/lint/linter/xhpast/rules/ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule.php @@ -6,7 +6,7 @@ final class ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule const ID = 73; public function getLintName() { - return pht('Space After Unary Operator'); + return pht('Space After Unary Prefix Operator'); } public function getLintSeverity() { @@ -31,7 +31,8 @@ final class ArcanistUnaryPrefixExpressionSpacingXHPASTLinterRule if (!empty($after)) { $this->raiseLintAtOffset( $operator->getOffset() + strlen($operator->getConcreteString()), - pht('Unary operators should not be followed by whitespace.'), + pht( + 'Unary prefix operators should not be followed by whitespace.'), implode('', mpull($after, 'getValue')), ''); } From c304c4e04564cab8b11104e7814376223dc82468 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Sat, 8 Aug 2015 10:18:59 +1000 Subject: [PATCH 11/11] Minor linter fix Summary: Self-explanatory. Test Plan: Eyeball it. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D13807 --- src/workflow/ArcanistBackoutWorkflow.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/workflow/ArcanistBackoutWorkflow.php b/src/workflow/ArcanistBackoutWorkflow.php index 49ff007e..a9564dbb 100644 --- a/src/workflow/ArcanistBackoutWorkflow.php +++ b/src/workflow/ArcanistBackoutWorkflow.php @@ -161,8 +161,8 @@ EOTEXT $commit_hash = $commit['identifier']; // Convert commit hash from SVN to Git/HG (for FB case) if ($is_git_svn || $is_hg_svn) { - $commit_hash = $repository_api-> - getHashFromFromSVNRevisionNumber($commit_hash); + $commit_hash = $repository_api + ->getHashFromFromSVNRevisionNumber($commit_hash); } } else { // Assume input is a commit hash