From 4404e66735c470cddd7f1cdb8aef05d9f322ad66 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Wed, 19 Aug 2015 15:34:43 +1000 Subject: [PATCH 1/7] Improve the "inline HTML" linter rule Summary: Improve `ArcanistInlineHTMLXHPASTLinterRule` such that it doesn't raise duplicate warnings. Also be a bit more lax with whitespace. Test Plan: Unit tests now pass. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D13896 --- src/lint/linter/ArcanistInlineHTMLXHPASTLinterRule.php | 5 +++++ src/lint/linter/__tests__/xhpast/embedded-tags.lint-test | 1 + src/lint/linter/__tests__/xhpast/php-tags-bad.lint-test | 1 + 3 files changed, 7 insertions(+) diff --git a/src/lint/linter/ArcanistInlineHTMLXHPASTLinterRule.php b/src/lint/linter/ArcanistInlineHTMLXHPASTLinterRule.php index 3cb758ad..701b7261 100644 --- a/src/lint/linter/ArcanistInlineHTMLXHPASTLinterRule.php +++ b/src/lint/linter/ArcanistInlineHTMLXHPASTLinterRule.php @@ -22,9 +22,14 @@ final class ArcanistInlineHTMLXHPASTLinterRule continue; } + if (preg_match('/^\s*$/', $html->getValue())) { + continue; + } + $this->raiseLintAtToken( $html, pht('PHP files must only contain PHP code.')); + break; } } diff --git a/src/lint/linter/__tests__/xhpast/embedded-tags.lint-test b/src/lint/linter/__tests__/xhpast/embedded-tags.lint-test index d15cefb5..67018229 100644 --- a/src/lint/linter/__tests__/xhpast/embedded-tags.lint-test +++ b/src/lint/linter/__tests__/xhpast/embedded-tags.lint-test @@ -2,3 +2,4 @@ This shouldn't fatal the parser. ~~~~~~~~~~ +disabled:2:1 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 7fb52e73..dbe6aa14 100644 --- a/src/lint/linter/__tests__/xhpast/php-tags-bad.lint-test +++ b/src/lint/linter/__tests__/xhpast/php-tags-bad.lint-test @@ -3,6 +3,7 @@ garbage garbage ?> ~~~~~~~~~~ +disabled:1:1 error:1:1 ~~~~~~~~~~ garbage garbage From 27ec3a2e34aab131e596ffa4560daef0c0fc8534 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Wed, 19 Aug 2015 15:35:16 +1000 Subject: [PATCH 2/7] Improve linter handling of short array syntax Summary: Currently, linting PHP short array syntax (i.e. `[...]`) throws an exception ("Expected open parentheses"). This diff relaxes some restrictions which prevent short array syntax from linting with `ArcanistParenthesesSpacingXHPASTLinterRule`. Test Plan: Modified test cases. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: agenticarus, Korvin Differential Revision: https://secure.phabricator.com/D13895 --- src/lint/linter/__tests__/xhpast/array-formatting.lint-test | 4 ++++ .../rules/ArcanistCallParenthesesXHPASTLinterRule.php | 6 ++++++ .../rules/ArcanistParenthesesSpacingXHPASTLinterRule.php | 6 ------ 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/lint/linter/__tests__/xhpast/array-formatting.lint-test b/src/lint/linter/__tests__/xhpast/array-formatting.lint-test index 3dda317d..7ae23f5d 100644 --- a/src/lint/linter/__tests__/xhpast/array-formatting.lint-test +++ b/src/lint/linter/__tests__/xhpast/array-formatting.lint-test @@ -2,6 +2,7 @@ array ( 1, 2, 3 ); list ( $x, $y ) = array(); +[ 1, 2 , 3 ]; ~~~~~~~~~~ warning:3:6 warning:3:8 @@ -9,8 +10,11 @@ warning:3:16 warning:4:5 warning:4:7 warning:4:14 +warning:5:2 +warning:5:11 ~~~~~~~~~~ getTypeName()) { case 'n_ARRAY_LITERAL': + if (head($node->getTokens())->getTypeName() == '[') { + // Short array syntax. + continue 2; + } + $params = $node->getChildOfType(0, 'n_ARRAY_VALUE_LIST'); break; @@ -44,6 +49,7 @@ final class ArcanistCallParenthesesXHPASTLinterRule $tokens = $params->getTokens(); $first = head($tokens); + $leading = $first->getNonsemanticTokensBefore(); $leading_text = implode('', mpull($leading, 'getValue')); if (preg_match('/^\s+$/', $leading_text)) { diff --git a/src/lint/linter/xhpast/rules/ArcanistParenthesesSpacingXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistParenthesesSpacingXHPASTLinterRule.php index 7ad16a44..7d7f8c4d 100644 --- a/src/lint/linter/xhpast/rules/ArcanistParenthesesSpacingXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistParenthesesSpacingXHPASTLinterRule.php @@ -29,12 +29,6 @@ final class ArcanistParenthesesSpacingXHPASTLinterRule $token_o = array_shift($tokens); $token_c = array_pop($tokens); - if ($token_o->getTypeName() !== '(') { - throw new Exception(pht('Expected open parentheses.')); - } - if ($token_c->getTypeName() !== ')') { - throw new Exception(pht('Expected close parentheses.')); - } $nonsem_o = $token_o->getNonsemanticTokensAfter(); $nonsem_c = $token_c->getNonsemanticTokensBefore(); From f9bd6b058f8af4bcbcfd35246367bed0f7a61af5 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Wed, 19 Aug 2015 21:39:11 +1000 Subject: [PATCH 3/7] Add a Composer linter Summary: Adds a basic linter for ensuring that `composer.lock` files are up-to-date. Test Plan: We have been using this in a private project for around a month. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: johnny-bit, Korvin Differential Revision: https://secure.phabricator.com/D13883 --- src/__phutil_library_map__.php | 2 + src/lint/linter/ArcanistComposerLinter.php | 55 ++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 src/lint/linter/ArcanistComposerLinter.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ca5061e4..722bea0a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -54,6 +54,7 @@ phutil_register_library_map(array( 'ArcanistCommentStyleXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistCommentStyleXHPASTLinterRule.php', 'ArcanistCommitWorkflow' => 'workflow/ArcanistCommitWorkflow.php', 'ArcanistCompilerLintRenderer' => 'lint/renderer/ArcanistCompilerLintRenderer.php', + 'ArcanistComposerLinter' => 'lint/linter/ArcanistComposerLinter.php', 'ArcanistComprehensiveLintEngine' => 'lint/engine/ArcanistComprehensiveLintEngine.php', 'ArcanistConcatenationOperatorXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistConcatenationOperatorXHPASTLinterRule.php', 'ArcanistConfiguration' => 'configuration/ArcanistConfiguration.php', @@ -337,6 +338,7 @@ phutil_register_library_map(array( 'ArcanistCommentStyleXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistCommitWorkflow' => 'ArcanistWorkflow', 'ArcanistCompilerLintRenderer' => 'ArcanistLintRenderer', + 'ArcanistComposerLinter' => 'ArcanistLinter', 'ArcanistComprehensiveLintEngine' => 'ArcanistLintEngine', 'ArcanistConcatenationOperatorXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistConfiguration' => 'Phobject', diff --git a/src/lint/linter/ArcanistComposerLinter.php b/src/lint/linter/ArcanistComposerLinter.php new file mode 100644 index 00000000..02b7c673 --- /dev/null +++ b/src/lint/linter/ArcanistComposerLinter.php @@ -0,0 +1,55 @@ + pht('Lock file out-of-date'), + ); + } + + public function lintPath($path) { + switch (basename($path)) { + case 'composer.json': + $this->lintComposerJson($path); + break; + case 'composer.lock': + break; + } + } + + private function lintComposerJson($path) { + $composer_hash = md5(Filesystem::readFile(dirname($path).'/composer.json')); + $composer_lock = phutil_json_decode( + Filesystem::readFile(dirname($path).'/composer.lock')); + + if ($composer_hash !== $composer_lock['hash']) { + $this->raiseLintAtPath( + self::LINT_OUT_OF_DATE, + pht( + "The '%s' file seems to be out-of-date. ". + "You probably need to run `%s`.", + 'composer.lock', + 'composer update')); + } + } + +} From 05aaa1a5a3b8fa42c46bde079bc861f956e8c875 Mon Sep 17 00:00:00 2001 From: ealbright Date: Wed, 19 Aug 2015 05:45:46 -0700 Subject: [PATCH 4/7] ArcanistPyLintLinter fix to getMandatoryFlags msg-template Summary: Removed excess quotations on the `--msg-template` option as it was interfering with the string-int coercion Test Plan: Unsure Reviewers: joshuaspence, epriestley, #blessed_reviewers Reviewed By: joshuaspence, epriestley, #blessed_reviewers Subscribers: joshuaspence, e-m-albright, Korvin Maniphest Tasks: T9214 Differential Revision: https://secure.phabricator.com/D13931 --- src/lint/linter/ArcanistPyLintLinter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lint/linter/ArcanistPyLintLinter.php b/src/lint/linter/ArcanistPyLintLinter.php index 111cc27e..4884ab9f 100644 --- a/src/lint/linter/ArcanistPyLintLinter.php +++ b/src/lint/linter/ArcanistPyLintLinter.php @@ -82,7 +82,7 @@ final class ArcanistPyLintLinter extends ArcanistExternalLinter { $options = array(); $options[] = '--reports=no'; - $options[] = '--msg-template="{line}|{column}|{msg_id}|{symbol}|{msg}"'; + $options[] = '--msg-template={line}|{column}|{msg_id}|{symbol}|{msg}'; // Specify an `--rcfile`, either absolute or relative to the project root. // Stupidly, the command line args above are overridden by rcfile, so be From f47d15387b2871cec7654e755af15f368fa59e97 Mon Sep 17 00:00:00 2001 From: Javier Arteaga Date: Thu, 20 Aug 2015 04:19:22 -0700 Subject: [PATCH 5/7] Fix wrong plural of an `arc land` message Summary: "Branch" was pluralized as "branchs". Fixes T9225. Test Plan: * Created test repo with two revisions on a feature branch. * Saw old message, frowned a little. * Applied patch. * No longer frowning. Reviewers: chad, epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: avivey, Korvin Maniphest Tasks: T9225 Differential Revision: https://secure.phabricator.com/D13944 --- src/workflow/ArcanistLandWorkflow.php | 39 +++++++++++++++++++-------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php index 98c804b6..8f4356ee 100644 --- a/src/workflow/ArcanistLandWorkflow.php +++ b/src/workflow/ArcanistLandWorkflow.php @@ -492,17 +492,34 @@ EOTEXT 'arc amend', '--revision ')); } else if (count($revisions) > 1) { - $message = pht( - "There are multiple revisions on feature %s '%s' which are not ". - "present on '%s':\n\n". - "%s\n". - "Separate these revisions onto different %s, or use --revision ' ". - "to use the commit message from and land them all.", - $this->branchType, - $this->branch, - $this->onto, - $this->renderRevisionList($revisions), - $this->branchType.'s'); + switch ($this->branchType) { + case self::REFTYPE_BOOKMARK: + $message = pht( + "There are multiple revisions on feature bookmark '%s' which are ". + "not present on '%s':\n\n". + "%s\n". + 'Separate these revisions onto different bookmarks, or use '. + '--revision to use the commit message from '. + 'and land them all.', + $this->branch, + $this->onto, + $this->renderRevisionList($revisions)); + break; + case self::REFTYPE_BRANCH: + default: + $message = pht( + "There are multiple revisions on feature branch '%s' which are ". + "not present on '%s':\n\n". + "%s\n". + 'Separate these revisions onto different branches, or use '. + '--revision to use the commit message from '. + 'and land them all.', + $this->branch, + $this->onto, + $this->renderRevisionList($revisions)); + break; + } + throw new ArcanistUsageException($message); } From e56f326cf72e26a05fc164cbcac7855912d954a2 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Thu, 20 Aug 2015 22:44:52 +1000 Subject: [PATCH 6/7] Add a call to `assert_instances_of` Summary: This was taken from D13569. Test Plan: `arc lint` still works. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D13943 --- src/lint/engine/ArcanistLintEngine.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lint/engine/ArcanistLintEngine.php b/src/lint/engine/ArcanistLintEngine.php index 720af8ef..8add0a88 100644 --- a/src/lint/engine/ArcanistLintEngine.php +++ b/src/lint/engine/ArcanistLintEngine.php @@ -471,6 +471,8 @@ abstract class ArcanistLintEngine extends Phobject { } private function executeLinters(array $runnable) { + assert_instances_of($runnable, 'ArcanistLinter'); + $all_paths = $this->getPaths(); $path_chunks = array_chunk($all_paths, 32, $preserve_keys = true); From 9b8c9d280ea47e7256dd1ac0ff17fe4aa363477e Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Fri, 21 Aug 2015 07:26:52 +1000 Subject: [PATCH 7/7] Exclude variables used in strings inside closures when checking for undeclared variables Summary: Improves upon D13795 to correctly handle variables within strings. Specifically, the following code currently (incorrectly) warns about `$x` being undeclared: ```lang=php function some_func() { return function ($x) { echo "$x"; }; } ``` It's worth noting that the situation would be improved if XHPAST properly parsed strings (see T8049). Test Plan: Added test case. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D13938 --- .../xhpast/undeclared-variables.lint-test | 7 +++++++ ...ArcanistUndeclaredVariableXHPASTLinterRule.php | 15 +++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test index 1bedf0fa..ad03c44c 100644 --- a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test +++ b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test @@ -179,6 +179,12 @@ function some_func($x, $y) { echo $z; }; } + +function some_func($x, $y) { + $func = function ($z) use ($x) { + echo "$x/$y/$z"; + }; +} ~~~~~~~~~~ warning:9:3 error:28:3 @@ -201,3 +207,4 @@ error:150:9 error:164:9 error:171:5 error:178:10 +error:185:14 diff --git a/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php index 05464d41..2a389e34 100644 --- a/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php @@ -68,6 +68,7 @@ final class ArcanistUndeclaredVariableXHPASTLinterRule ) + array_fill_keys($this->getSuperGlobalNames(), 0); $declaration_tokens = array(); $exclude_tokens = array(); + $exclude_strings = array(); $vars = array(); // First up, find all the different kinds of declarations, as explained @@ -175,6 +176,16 @@ final class ArcanistUndeclaredVariableXHPASTLinterRule foreach ($func_decl->selectDescendantsOfType('n_VARIABLE') as $var) { $exclude_tokens[$var->getID()] = true; } + + foreach (array('n_STRING_SCALAR', 'n_HEREDOC') as $type) { + foreach ($func_decl->selectDescendantsOfType($type) as $string) { + $exclude_strings[$string->getID()] = array(); + + foreach ($string->getStringVariables() as $offset => $var) { + $exclude_strings[$string->getID()][$var] = true; + } + } + } } // Now we have every declaration except foreach(), handled below. Build @@ -316,6 +327,10 @@ final class ArcanistUndeclaredVariableXHPASTLinterRule foreach (array('n_STRING_SCALAR', 'n_HEREDOC') as $type) { foreach ($body->selectDescendantsOfType($type) as $string) { foreach ($string->getStringVariables() as $offset => $var) { + if (isset($exclude_strings[$string->getID()][$var])) { + continue; + } + $all[$string->getOffset() + $offset - 1] = '$'.$var; } }