From b61e890a0ca8e54845a39d2b53ee43316331a375 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 May 2019 10:18:50 -0700 Subject: [PATCH 1/6] Remove unnecessary "," from Pylint version regex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: See PHI1238. This "," in the regex can only make the lint binding fail if there is no "," in the version output string. In modern versions of Pylint, there is (apparently) no comma in the version string. Remove it. See also Test Plan: ``` $ pip install pylint Traceback (most recent call last): File "/Users/epriestley/Library/Python/2.7/bin/pip", line 6, in from pip._internal import main ImportError: No module named pip._internal ``` ¯\_(ツ)_/¯ Reviewers: amckinley, joshuaspence Reviewed By: joshuaspence Differential Revision: https://secure.phabricator.com/D20505 --- 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 44793a50..f8cc279a 100644 --- a/src/lint/linter/ArcanistPyLintLinter.php +++ b/src/lint/linter/ArcanistPyLintLinter.php @@ -38,7 +38,7 @@ final class ArcanistPyLintLinter extends ArcanistExternalLinter { list($stdout) = execx('%C --version', $this->getExecutableCommand()); $matches = array(); - $regex = '/^pylint (?P\d+\.\d+\.\d+),/'; + $regex = '/^pylint (?P\d+\.\d+\.\d+)/'; if (preg_match($regex, $stdout, $matches)) { return $matches['version']; } else { From 9ccbbee33675259f210e0d40b13be99b7ebd2571 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 3 May 2019 10:14:51 -0700 Subject: [PATCH 2/6] Fix a potential double-prompt in "arc land" when landing with ongoing builds Summary: The recently-added, build-plan-behavior-aware check here does all of its own prompting, so we should skip the other prompting if it doesn't throw. Test Plan: Will `arc land` something sketchy sooner or later. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20494 --- src/workflow/ArcanistLandWorkflow.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php index a3115872..7ca0028e 100644 --- a/src/workflow/ArcanistLandWorkflow.php +++ b/src/workflow/ArcanistLandWorkflow.php @@ -1376,6 +1376,7 @@ EOTEXT // if this one doesn't work out. try { $this->checkForBuildablesWithPlanBehaviors($diff_phid); + return; } catch (ArcanistUserAbortException $abort_ex) { throw $abort_ex; } catch (Exception $ex) { From 4f583dded366af8b2b316414bfc100063e580866 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Wed, 15 May 2019 09:00:29 +1000 Subject: [PATCH 3/6] Call `$linter->setEngine` in linter tests Summary: We aren't calling `$linter->setEngine($engine)`, even though we do have an `$engine`. This causes unit tests for any linters which require an engine to fail. Test Plan: Ran the unit tests for a [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php | third-party linter]]. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D20515 --- src/lint/linter/__tests__/ArcanistLinterTestCase.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lint/linter/__tests__/ArcanistLinterTestCase.php b/src/lint/linter/__tests__/ArcanistLinterTestCase.php index a6eba567..b86a43b2 100644 --- a/src/lint/linter/__tests__/ArcanistLinterTestCase.php +++ b/src/lint/linter/__tests__/ArcanistLinterTestCase.php @@ -114,6 +114,7 @@ abstract class ArcanistLinterTestCase extends PhutilTestCase { $path_name = idx($config, 'path', $path); $engine->setPaths(array($path_name)); + $linter->setEngine($engine); $linter->addPath($path_name); $linter->addData($path_name, $data); From fceac878f112c8301997c24d8b1ecf0c60db58e4 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Wed, 15 May 2019 09:03:02 +1000 Subject: [PATCH 4/6] Allow `setCustomSeverityRules` to be overridden in subclasses Summary: I am writing a proxy linter that can be used to wrap any `ArcanistExternalLinter` and execute all commands within a Docker container (see [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php |`ArcanistDockerContainerLinterProxy`]] from [[https://github.com/freelancer/flarc | `flarc`]]). In order for `ArcanistDockerContainerLinterProxy` to behave like the `ArcanistExternalLinter` that is being proxied, `final` needs to be removed from some methods. I figured this was reasonable to submit upstream as a similar change ({D19630}) was previously accepted. Test Plan: N/A Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D20514 --- src/lint/linter/ArcanistLinter.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/lint/linter/ArcanistLinter.php b/src/lint/linter/ArcanistLinter.php index 6bb02e5d..afb8cbe3 100644 --- a/src/lint/linter/ArcanistLinter.php +++ b/src/lint/linter/ArcanistLinter.php @@ -214,9 +214,6 @@ abstract class ArcanistLinter extends Phobject { return 1.0; } - /** - * TODO: This should be `final`. - */ public function setCustomSeverityMap(array $map) { $this->customSeverityMap = $map; return $this; @@ -227,7 +224,7 @@ abstract class ArcanistLinter extends Phobject { return $this; } - final public function setCustomSeverityRules(array $rules) { + public function setCustomSeverityRules(array $rules) { $this->customSeverityRules = $rules; return $this; } From 6a8e76db329e8bc4484644887c06d2b89d22304e Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Wed, 15 May 2019 09:09:46 +1000 Subject: [PATCH 5/6] Allow `buildFutures` and `resolveFutures` to be overridden Summary: I am writing a proxy linter that can be used to wrap any `ArcanistExternalLinter` and execute all commands within a Docker container (see [[https://github.com/freelancer/flarc/blob/master/src/lint/linter/ArcanistDockerContainerLinterProxy.php |`ArcanistDockerContainerLinterProxy`]] from [[https://github.com/freelancer/flarc | `flarc`]]). In order for `ArcanistDockerContainerLinterProxy` to behave like the `ArcanistExternalLinter` that is being proxied, `final` needs to be removed from some methods. I figured this was reasonable to submit upstream as a similar change ({D19630}) was previously accepted. Test Plan: N/A Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19730 --- src/lint/linter/ArcanistExternalLinter.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lint/linter/ArcanistExternalLinter.php b/src/lint/linter/ArcanistExternalLinter.php index d25c10a2..a394528f 100644 --- a/src/lint/linter/ArcanistExternalLinter.php +++ b/src/lint/linter/ArcanistExternalLinter.php @@ -410,7 +410,7 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { return csprintf('%s', $path); } - final protected function buildFutures(array $paths) { + protected function buildFutures(array $paths) { $executable = $this->getExecutableCommand(); $bin = csprintf('%C %Ls', $executable, $this->getCommandFlags()); @@ -428,7 +428,7 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { return $futures; } - final protected function resolveFuture($path, Future $future) { + protected function resolveFuture($path, Future $future) { list($err, $stdout, $stderr) = $future->resolve(); if ($err && !$this->shouldExpectCommandErrors()) { $future->resolvex(); From 82445bb6053451f3a6221c3d2c2f3bce50ea5871 Mon Sep 17 00:00:00 2001 From: Wenzheng Jiang Date: Wed, 15 May 2019 11:07:07 +1000 Subject: [PATCH 6/6] Let lint rules support anonymous classes Summary: Ref T4334. Depends on D19740. Improve some lint rules so they can handle anonymous classes. Test Plan: Ran updated tests Reviewers: joshuaspence, #blessed_reviewers, epriestley Reviewed By: joshuaspence, #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T4334 Differential Revision: https://secure.phabricator.com/D19741 --- .../ArcanistClassNameLiteralXHPASTLinterRule.php | 3 +++ ...rcanistConstructorParenthesesXHPASTLinterRule.php | 4 +++- .../ArcanistSelfClassReferenceXHPASTLinterRule.php | 3 +++ ...anistUnnecessaryFinalModifierXHPASTLinterRule.php | 3 +++ .../class-name-literal/class-name-literal.lint-test | 12 ++++++++++++ .../constructor-parentheses.lint-test | 2 ++ .../self-class-references.lint-test | 12 ++++++++++++ .../unnecessary-final-modifier.lint-test | 1 + 8 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/lint/linter/xhpast/rules/ArcanistClassNameLiteralXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistClassNameLiteralXHPASTLinterRule.php index a22a9859..288fc05c 100644 --- a/src/lint/linter/xhpast/rules/ArcanistClassNameLiteralXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistClassNameLiteralXHPASTLinterRule.php @@ -17,6 +17,9 @@ final class ArcanistClassNameLiteralXHPASTLinterRule $class_declarations = $root->selectDescendantsOfType('n_CLASS_DECLARATION'); foreach ($class_declarations as $class_declaration) { + if ($class_declaration->getChildByIndex(1)->getTypeName() == 'n_EMPTY') { + continue; + } $class_name = $class_declaration ->getChildOfType(1, 'n_CLASS_NAME') ->getConcreteString(); diff --git a/src/lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php index d18f0335..4179c899 100644 --- a/src/lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php @@ -20,7 +20,9 @@ final class ArcanistConstructorParenthesesXHPASTLinterRule $class = $node->getChildByIndex(0); $params = $node->getChildByIndex(1); - if ($params->getTypeName() == 'n_EMPTY') { + if ($class->getTypeName() != 'n_CLASS_DECLARATION' && + $params->getTypeName() == 'n_EMPTY') { + $this->raiseLintAtNode( $class, pht('Use parentheses when invoking a constructor.'), diff --git a/src/lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php index 7ad7c238..c87974d7 100644 --- a/src/lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistSelfClassReferenceXHPASTLinterRule.php @@ -17,6 +17,9 @@ final class ArcanistSelfClassReferenceXHPASTLinterRule $class_declarations = $root->selectDescendantsOfType('n_CLASS_DECLARATION'); foreach ($class_declarations as $class_declaration) { + if ($class_declaration->getChildByIndex(1)->getTypeName() == 'n_EMPTY') { + continue; + } $class_name = $class_declaration ->getChildOfType(1, 'n_CLASS_NAME') ->getConcreteString(); diff --git a/src/lint/linter/xhpast/rules/ArcanistUnnecessaryFinalModifierXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistUnnecessaryFinalModifierXHPASTLinterRule.php index 1abbaa01..5c197a60 100644 --- a/src/lint/linter/xhpast/rules/ArcanistUnnecessaryFinalModifierXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistUnnecessaryFinalModifierXHPASTLinterRule.php @@ -17,6 +17,9 @@ final class ArcanistUnnecessaryFinalModifierXHPASTLinterRule $classes = $root->selectDescendantsOfType('n_CLASS_DECLARATION'); foreach ($classes as $class) { + if ($class->getChildByIndex(0)->getTypeName() == 'n_EMPTY') { + continue; + } $attributes = $class->getChildOfType(0, 'n_CLASS_ATTRIBUTES'); $is_final = false; diff --git a/src/lint/linter/xhpast/rules/__tests__/class-name-literal/class-name-literal.lint-test b/src/lint/linter/xhpast/rules/__tests__/class-name-literal/class-name-literal.lint-test index f6050e6f..263fa6bc 100644 --- a/src/lint/linter/xhpast/rules/__tests__/class-name-literal/class-name-literal.lint-test +++ b/src/lint/linter/xhpast/rules/__tests__/class-name-literal/class-name-literal.lint-test @@ -11,6 +11,12 @@ class MyClass { return __CLASS__; } } + +$c = new class { + public function someMethod() { + return __CLASS__; + } +}; ~~~~~~~~~~ advice:5:12 advice:9:10 @@ -28,3 +34,9 @@ class MyClass { return __CLASS__; } } + +$c = new class { + public function someMethod() { + return __CLASS__; + } +}; diff --git a/src/lint/linter/xhpast/rules/__tests__/constructor-parentheses/constructor-parentheses.lint-test b/src/lint/linter/xhpast/rules/__tests__/constructor-parentheses/constructor-parentheses.lint-test index fa4ede7b..5bfa198d 100644 --- a/src/lint/linter/xhpast/rules/__tests__/constructor-parentheses/constructor-parentheses.lint-test +++ b/src/lint/linter/xhpast/rules/__tests__/constructor-parentheses/constructor-parentheses.lint-test @@ -3,6 +3,7 @@ new Foo; new Bar(); new Foo\Bar; +new class {}; ~~~~~~~~~~ advice:3:5 advice:5:5 @@ -12,3 +13,4 @@ advice:5:5 new Foo(); new Bar(); new Foo\Bar(); +new class {}; 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 index 2f2c1978..33c30f4d 100644 --- 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 @@ -9,6 +9,12 @@ class Foo extends Bar { return new self(); } } + +$c = new class { + public function newInstance() { + return new self(); + } +}; ~~~~~~~~~~ warning:5:16 ~~~~~~~~~~ @@ -23,3 +29,9 @@ class Foo extends Bar { return new self(); } } + +$c = new class { + public function newInstance() { + return new self(); + } +}; diff --git a/src/lint/linter/xhpast/rules/__tests__/unnecessary-final-modifier/unnecessary-final-modifier.lint-test b/src/lint/linter/xhpast/rules/__tests__/unnecessary-final-modifier/unnecessary-final-modifier.lint-test index 22185c01..0859d5ab 100644 --- a/src/lint/linter/xhpast/rules/__tests__/unnecessary-final-modifier/unnecessary-final-modifier.lint-test +++ b/src/lint/linter/xhpast/rules/__tests__/unnecessary-final-modifier/unnecessary-final-modifier.lint-test @@ -4,5 +4,6 @@ final class Foo { public function bar() {} final public function baz() {} } +$c = new class {}; ~~~~~~~~~~ advice:5:3