From 9b07d1c480f447fea5f0cd9adeb2e7fc838778cc Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Wed, 27 Apr 2016 13:48:46 -0700 Subject: [PATCH 1/5] Recognize error codes from Flake8 extensions Summary: Flake8 extensions are allowed to use their own letter-prefixed codes. For example, the flake8-debugger extension emits 'T002'-tagged messages. This change relaxes getLintCodeFromLinterConfigurationKey() to also recognize extension codes. Otherwise, attempting to configure message severities for e.g. 'T002' would result in an exception. Messages from extensions continue to default to ERROR severity, as they did before this change. Test Plan: Successfully reduced the severity of 'T002' to a warning via .arclint: "severity": { "T002": "warning" } Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D15810 --- src/lint/linter/ArcanistFlake8Linter.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lint/linter/ArcanistFlake8Linter.php b/src/lint/linter/ArcanistFlake8Linter.php index bd45082d..6f93ef8c 100644 --- a/src/lint/linter/ArcanistFlake8Linter.php +++ b/src/lint/linter/ArcanistFlake8Linter.php @@ -91,12 +91,13 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter { } else { // "E": PEP8 Error // "F": PyFlakes Error + // or: Flake8 Extension Message return ArcanistLintSeverity::SEVERITY_ERROR; } } protected function getLintCodeFromLinterConfigurationKey($code) { - if (!preg_match('/^(E|W|C|F)\d+$/', $code)) { + if (!preg_match('/^[A-Z]\d+$/', $code)) { throw new Exception( pht( 'Unrecognized lint message code "%s". Expected a valid flake8 '. From ea6fc7789295fc090ed2dd35dc19bfa464478024 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Fri, 29 Apr 2016 08:00:07 +0000 Subject: [PATCH 2/5] Fix incorrect variable for linter standards Summary: It is currently not possible to apply multiple linter standards because `$value` was used instead of `$standard`. Test Plan: Was able to apply multiple linter standards. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: eadler, thaiphv, Korvin Differential Revision: https://secure.phabricator.com/D15212 --- src/lint/linter/ArcanistLinter.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/lint/linter/ArcanistLinter.php b/src/lint/linter/ArcanistLinter.php index d2695148..6bb02e5d 100644 --- a/src/lint/linter/ArcanistLinter.php +++ b/src/lint/linter/ArcanistLinter.php @@ -574,8 +574,10 @@ abstract class ArcanistLinter extends Phobject { case 'standard': $standards = (array)$value; - foreach ($standards as $standard) { - $standard = ArcanistLinterStandard::getStandard($value, $this); + foreach ($standards as $standard_name) { + $standard = ArcanistLinterStandard::getStandard( + $standard_name, + $this); foreach ($standard->getLinterConfiguration() as $k => $v) { $this->setLinterConfigurationValue($k, $v); From 3ffed59bd7fb19e8d712aca9b86bf5ce55b7d8ca Mon Sep 17 00:00:00 2001 From: Richard van Velzen Date: Thu, 28 Apr 2016 16:59:50 +0200 Subject: [PATCH 3/5] Update xhpast linter rules for new function AST format Summary: See D15678. A node containing a return type hint is added right before the statement body node. This updates all relevant linter rules to now retrieve the body from the correct index. Test Plan: Ran `arc unit --everything`, inspected every single message to make sure all xhpast test cases succeeded. Also grepped for `getChildByIndex(5` and `getChildOfType(5`. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D15814 --- .../xhpast/rules/ArcanistAbstractMethodBodyXHPASTLinterRule.php | 2 +- .../rules/ArcanistInterfaceMethodBodyXHPASTLinterRule.php | 2 +- .../xhpast/rules/ArcanistReusedAsIteratorXHPASTLinterRule.php | 2 +- .../rules/ArcanistReusedIteratorReferenceXHPASTLinterRule.php | 2 +- .../linter/xhpast/rules/ArcanistStaticThisXHPASTLinterRule.php | 2 +- .../xhpast/rules/ArcanistToStringExceptionXHPASTLinterRule.php | 2 +- .../xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php | 2 +- .../rules/ArcanistUselessOverridingMethodXHPASTLinterRule.php | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/lint/linter/xhpast/rules/ArcanistAbstractMethodBodyXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistAbstractMethodBodyXHPASTLinterRule.php index 277e15e0..8c37ab43 100644 --- a/src/lint/linter/xhpast/rules/ArcanistAbstractMethodBodyXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistAbstractMethodBodyXHPASTLinterRule.php @@ -14,7 +14,7 @@ final class ArcanistAbstractMethodBodyXHPASTLinterRule foreach ($methods as $method) { $modifiers = $this->getModifiers($method); - $body = $method->getChildByIndex(5); + $body = $method->getChildByIndex(6); if (idx($modifiers, 'abstract') && $body->getTypeName() != 'n_EMPTY') { $this->raiseLintAtNode( diff --git a/src/lint/linter/xhpast/rules/ArcanistInterfaceMethodBodyXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistInterfaceMethodBodyXHPASTLinterRule.php index f4835d0b..bf230be0 100644 --- a/src/lint/linter/xhpast/rules/ArcanistInterfaceMethodBodyXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistInterfaceMethodBodyXHPASTLinterRule.php @@ -16,7 +16,7 @@ final class ArcanistInterfaceMethodBodyXHPASTLinterRule $methods = $interface->selectDescendantsOfType('n_METHOD_DECLARATION'); foreach ($methods as $method) { - $body = $method->getChildByIndex(5); + $body = $method->getChildByIndex(6); if ($body->getTypeName() != 'n_EMPTY') { $this->raiseLintAtNode( diff --git a/src/lint/linter/xhpast/rules/ArcanistReusedAsIteratorXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistReusedAsIteratorXHPASTLinterRule.php index 182edc69..a1a4cc52 100644 --- a/src/lint/linter/xhpast/rules/ArcanistReusedAsIteratorXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistReusedAsIteratorXHPASTLinterRule.php @@ -46,7 +46,7 @@ final class ArcanistReusedAsIteratorXHPASTLinterRule $vars[] = $var; } - $body = $def->getChildByIndex(5); + $body = $def->getChildByIndex(6); if ($body->getTypeName() === 'n_EMPTY') { // Abstract method declaration. continue; diff --git a/src/lint/linter/xhpast/rules/ArcanistReusedIteratorReferenceXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistReusedIteratorReferenceXHPASTLinterRule.php index 65851435..fc6b8a6b 100644 --- a/src/lint/linter/xhpast/rules/ArcanistReusedIteratorReferenceXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistReusedIteratorReferenceXHPASTLinterRule.php @@ -31,7 +31,7 @@ final class ArcanistReusedIteratorReferenceXHPASTLinterRule )); foreach ($defs as $def) { - $body = $def->getChildByIndex(5); + $body = $def->getChildByIndex(6); if ($body->getTypeName() === 'n_EMPTY') { // Abstract method declaration. continue; diff --git a/src/lint/linter/xhpast/rules/ArcanistStaticThisXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistStaticThisXHPASTLinterRule.php index 695d0858..e18c5135 100644 --- a/src/lint/linter/xhpast/rules/ArcanistStaticThisXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistStaticThisXHPASTLinterRule.php @@ -40,7 +40,7 @@ final class ArcanistStaticThisXHPASTLinterRule continue; } - $body = $method->getChildOfType(5, 'n_STATEMENT_LIST'); + $body = $method->getChildOfType(6, 'n_STATEMENT_LIST'); $variables = $body->selectDescendantsOfType('n_VARIABLE'); foreach ($variables as $variable) { diff --git a/src/lint/linter/xhpast/rules/ArcanistToStringExceptionXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistToStringExceptionXHPASTLinterRule.php index ab0eba56..315331ee 100644 --- a/src/lint/linter/xhpast/rules/ArcanistToStringExceptionXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistToStringExceptionXHPASTLinterRule.php @@ -21,7 +21,7 @@ final class ArcanistToStringExceptionXHPASTLinterRule continue; } - $statements = $method->getChildByIndex(5); + $statements = $method->getChildByIndex(6); if ($statements->getTypeName() != 'n_STATEMENT_LIST') { continue; diff --git a/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php index 2a389e34..b1f997e9 100644 --- a/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistUndeclaredVariableXHPASTLinterRule.php @@ -88,7 +88,7 @@ final class ArcanistUndeclaredVariableXHPASTLinterRule $vars[] = $var; } - $body = $def->getChildByIndex(5); + $body = $def->getChildByIndex(6); if ($body->getTypeName() === 'n_EMPTY') { // Abstract method declaration. continue; diff --git a/src/lint/linter/xhpast/rules/ArcanistUselessOverridingMethodXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistUselessOverridingMethodXHPASTLinterRule.php index 475ed25f..add72ca8 100644 --- a/src/lint/linter/xhpast/rules/ArcanistUselessOverridingMethodXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistUselessOverridingMethodXHPASTLinterRule.php @@ -40,7 +40,7 @@ final class ArcanistUselessOverridingMethodXHPASTLinterRule $parameters[] = $parameter->getConcreteString(); } - $statements = $method->getChildByIndex(5); + $statements = $method->getChildByIndex(6); if ($statements->getTypeName() != 'n_STATEMENT_LIST') { continue; From 768e1a56bc5f73c0e15610c74b7d0c023beee4e0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Apr 2016 05:59:24 -0700 Subject: [PATCH 4/5] When running XHPAST unit tests, include the "syntax error" lint rule Summary: See rARC3ffed59bd7. Currently, when a unit test includes a syntax error, it is raised in an unclear way ("error at line 10, char 1: XHP1 Unknown lint message!"). This is because each test case only activates rules it wants to test, so we lose the ID/name for the syntax message. However, we always want to test this and the lint engine can always raise it. To get a better error message, include it unconditionally. So a test for rule `X` really tests two rules: syntax, and `X`. Test Plan: Ran `arc unit` at HEAD, got a better test failure: ``` FAIL ArcanistCallTimePassByReferenceXHPASTLinterRuleTestCase::testLinter In 'call-time-pass-by-reference.lint-test', expected lint to raise error on line 10 at char 8, but no error was raised. Actually raised: error at line 10, char 1: XHP1 PHP Syntax Error! ``` NOTE: This doesn't pass tests yet, it just makes the test failure easier to understand. I'll see about fixing the test in the next change. Reviewers: chad, richardvanvelzen Reviewed By: richardvanvelzen Differential Revision: https://secure.phabricator.com/D15819 --- .../__tests__/ArcanistXHPASTLinterRuleTestCase.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistXHPASTLinterRuleTestCase.php index 342d059b..faf349bf 100644 --- a/src/lint/linter/xhpast/rules/__tests__/ArcanistXHPASTLinterRuleTestCase.php +++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistXHPASTLinterRuleTestCase.php @@ -8,8 +8,19 @@ abstract class ArcanistXHPASTLinterRuleTestCase extends ArcanistLinterTestCase { final protected function getLinter() { + // Always include this rule so we get good messages if a test includes + // a syntax error. No normal test should contain syntax errors. + $syntax_rule = new ArcanistSyntaxErrorXHPASTLinterRule(); + + $test_rule = $this->getLinterRule(); + + $rules = array( + $syntax_rule, + $test_rule, + ); + return id(new ArcanistXHPASTLinter()) - ->setRules(array($this->getLinterRule())); + ->setRules($rules); } /** From c58f1b9a2507488b2152473dd1f0bbc7e99c09c1 Mon Sep 17 00:00:00 2001 From: Eitan Adler Date: Fri, 29 Apr 2016 15:45:11 +0000 Subject: [PATCH 5/5] Prefer pip to easy_install Summary: Prefer pip to easy_install also fixes incorrect install instructions for closure linters Fixes T10892 Ref T10038 Test Plan: inspection Reviewers: avivey, #blessed_reviewers, epriestley, tjstum Reviewed By: avivey, #blessed_reviewers, epriestley, tjstum Subscribers: avivey, Korvin Maniphest Tasks: T10038, T10892 Differential Revision: https://secure.phabricator.com/D15818 --- src/lint/linter/ArcanistClosureLinter.php | 3 +-- src/lint/linter/ArcanistFlake8Linter.php | 2 +- src/lint/linter/ArcanistPEP8Linter.php | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/lint/linter/ArcanistClosureLinter.php b/src/lint/linter/ArcanistClosureLinter.php index 75eb1511..6a6bbc1d 100644 --- a/src/lint/linter/ArcanistClosureLinter.php +++ b/src/lint/linter/ArcanistClosureLinter.php @@ -33,8 +33,7 @@ final class ArcanistClosureLinter extends ArcanistExternalLinter { return pht( 'Install %s using `%s`.', 'gjslint', - 'sudo easy_install http://closure-linter.googlecode.com/'. - 'files/closure_linter-latest.tar.gz'); + 'pip install closure-linter'); } protected function parseLinterOutput($path, $err, $stdout, $stderr) { diff --git a/src/lint/linter/ArcanistFlake8Linter.php b/src/lint/linter/ArcanistFlake8Linter.php index 6f93ef8c..19d57432 100644 --- a/src/lint/linter/ArcanistFlake8Linter.php +++ b/src/lint/linter/ArcanistFlake8Linter.php @@ -45,7 +45,7 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter { } public function getInstallInstructions() { - return pht('Install flake8 using `%s`.', 'easy_install flake8'); + return pht('Install flake8 using `%s`.', 'pip install flake8'); } protected function parseLinterOutput($path, $err, $stdout, $stderr) { diff --git a/src/lint/linter/ArcanistPEP8Linter.php b/src/lint/linter/ArcanistPEP8Linter.php index 682ac6a8..6e47e395 100644 --- a/src/lint/linter/ArcanistPEP8Linter.php +++ b/src/lint/linter/ArcanistPEP8Linter.php @@ -43,7 +43,7 @@ final class ArcanistPEP8Linter extends ArcanistExternalLinter { } public function getInstallInstructions() { - return pht('Install PEP8 using `%s`.', 'easy_install pep8'); + return pht('Install PEP8 using `%s`.', 'pip install pep8'); } protected function parseLinterOutput($path, $err, $stdout, $stderr) {