From 30b7835c37b56ac9ea1de6b2291b3fb71c9e6853 Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Thu, 6 Sep 2018 12:43:34 +1000 Subject: [PATCH 1/6] Allow `willLintPaths` and `didLintPaths` to be overridden Summary: I'm not sure if the upstream will be interested in this change, but we are writing a linter which works by running an external command on the entire repository. This can't be done with `ArcanistExternalLinter` at the moment, which meant that we ended up copy-pasting most of `ArcanistFutureLinter`. This would be a lot easier if we could override `willLintPaths` and `didLintPaths`, but these methods are currently marked as `final`. An alternative solution would be some sort of `ArcanistLinter::transformPath` method. Test Plan: N/A Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: faulconbridge, Korvin Differential Revision: https://secure.phabricator.com/D19630 --- src/lint/linter/ArcanistFutureLinter.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lint/linter/ArcanistFutureLinter.php b/src/lint/linter/ArcanistFutureLinter.php index d14fe49a..e9df8371 100644 --- a/src/lint/linter/ArcanistFutureLinter.php +++ b/src/lint/linter/ArcanistFutureLinter.php @@ -11,7 +11,7 @@ abstract class ArcanistFutureLinter extends ArcanistLinter { return 8; } - final public function willLintPaths(array $paths) { + public function willLintPaths(array $paths) { $limit = $this->getFuturesLimit(); $this->futures = id(new FutureIterator(array()))->limit($limit); foreach ($this->buildFutures($paths) as $path => $future) { @@ -23,7 +23,7 @@ abstract class ArcanistFutureLinter extends ArcanistLinter { return; } - final public function didLintPaths(array $paths) { + public function didLintPaths(array $paths) { if (!$this->futures) { return; } From 2650e8627a20e1bfe334a4a2b787f44ef5d6ebc5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Sep 2018 08:33:13 -0700 Subject: [PATCH 2/6] Make the Arcanist comment remover less aggressive about stripping instructional comments Summary: Ref T13098. See PHI858. If you write this at the end of a message in `arc diff`: ``` Subscribers: #projectname # NEW DIFFERENTIAL REVISION # Describe the changes in this new revision. # ... ``` ...we'll currently eat the `#projectname` as an instructional comment, even if it is followed by an empty line. Instead, stop eating stuff once we hit the first empty line. (We escape empty lines in comments already.) After T13098 I'll maybe adjust this to use a more explicit instruction escape, like `##`, since there's no reason we're bound to `#`. Test Plan: Added a unit test and made it pass. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13098 Differential Revision: https://secure.phabricator.com/D19639 --- src/parser/ArcanistCommentRemover.php | 18 +++++++++++------- .../ArcanistCommentRemoverTestCase.php | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/parser/ArcanistCommentRemover.php b/src/parser/ArcanistCommentRemover.php index dad113c3..17f19c69 100644 --- a/src/parser/ArcanistCommentRemover.php +++ b/src/parser/ArcanistCommentRemover.php @@ -8,21 +8,25 @@ final class ArcanistCommentRemover extends Phobject { * considered a comment. */ public static function removeComments($body) { - $lines = explode("\n", $body); + $body = rtrim($body); + + $lines = phutil_split_lines($body); $lines = array_reverse($lines); + foreach ($lines as $key => $line) { - if (!strlen($line)) { - unset($lines[$key]); - continue; - } - if ($line[0] == '#') { + if (preg_match('/^#/', $line)) { unset($lines[$key]); continue; } + break; } + $lines = array_reverse($lines); - return implode("\n", $lines)."\n"; + $lines = implode('', $lines); + $lines = rtrim($lines)."\n"; + + return $lines; } } diff --git a/src/parser/__tests__/ArcanistCommentRemoverTestCase.php b/src/parser/__tests__/ArcanistCommentRemoverTestCase.php index d89a3d08..7de70934 100644 --- a/src/parser/__tests__/ArcanistCommentRemoverTestCase.php +++ b/src/parser/__tests__/ArcanistCommentRemoverTestCase.php @@ -24,6 +24,21 @@ Here is a list: The end. +EOTEXT; + + $this->assertEqual($expect, ArcanistCommentRemover::removeComments($test)); + + $test = <<assertEqual($expect, ArcanistCommentRemover::removeComments($test)); From 83661809e532c3fe444a8bf7c7d6936e6377691b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Oct 2018 18:55:53 -0700 Subject: [PATCH 3/6] Work around a Windows escaping issue and security conecern in "hg cat --output ..." Summary: See PHI904. Ref T13210. Ref T13209. Currently, we have an `hg cat` construction which attempts to pass a literal `%p` to Mercurial. This fails because you can't pass `%` through `%s` outside of `wilds`. It also uses `%C` to pass a list of file paths. This is broadly unsafe and can cause command execution if you modify a file named, e.g., `; rm -rf xyz` or similar. I think it would be difficult to turn this into an attack but it's fairly bad. This dates from D5144 in 2013. Test Plan: With this patch, created D19757 which has valid binary data (see F5962134). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13210, T13209 Differential Revision: https://secure.phabricator.com/D19758 --- src/repository/api/ArcanistMercurialAPI.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php index d6d10a9e..7aaa04f7 100644 --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -460,12 +460,16 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { Filesystem::createDirectory(dirname($tmppath), 0755, true); } + // NOTE: The "%s%%p" construction passes a literal "%p" to Mercurial, + // which is a formatting directive for a repo-relative filepath. The + // particulars of the construction avoid Windows escaping issues. See + // PHI904. + list($err, $stdout) = $this->execManualLocal( - 'cat --rev %s --output %s -- %C', + 'cat --rev %s --output %s%%p -- %Ls', $revision, - // %p is the formatter for the repo-relative filepath - $tmpdir.'/%p', - implode(' ', $paths)); + $tmpdir.DIRECTORY_SEPARATOR, + $paths); $filedata = array(); foreach ($paths as $path) { From 3534d2baca4bf6dcdac46c49164bf5ba3a6660ad Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Wed, 7 Nov 2018 19:35:08 -0800 Subject: [PATCH 4/6] Small ReMarkup fix Summary: See https://discourse.phabricator-community.org/t/2086 Test Plan: paste new content in comment box, see it markedup Reviewers: #blessed_reviewers, epriestley, amckinley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Differential Revision: https://secure.phabricator.com/D19794 --- src/configuration/ArcanistConfiguration.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/configuration/ArcanistConfiguration.php b/src/configuration/ArcanistConfiguration.php index 80ebf6fa..aa7ea406 100644 --- a/src/configuration/ArcanistConfiguration.php +++ b/src/configuration/ArcanistConfiguration.php @@ -7,15 +7,15 @@ * pointing to your subclass in your project configuration. * * When specified as the **arcanist_configuration** class in your project's - * ##.arcconfig##, your subclass will be instantiated (instead of this class) + * `.arcconfig`, your subclass will be instantiated (instead of this class) * and be able to handle all the method calls. In particular, you can: * - * - create, replace, or disable workflows by overriding buildWorkflow() - * and buildAllWorkflows(); + * - create, replace, or disable workflows by overriding `buildWorkflow()` + * and `buildAllWorkflows()`; * - add additional steps before or after workflows run by overriding - * willRunWorkflow() or didRunWorkflow() or didAbortWorkflow(); and + * `willRunWorkflow()` or `didRunWorkflow()` or `didAbortWorkflow()`; and * - add new flags to existing workflows by overriding - * getCustomArgumentsForCommand(). + * `getCustomArgumentsForCommand()`. * * @concrete-extensible */ From eb732555a71a3a1b15596dd65b6dad3c2e2af90b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 12 Dec 2018 05:15:17 -0800 Subject: [PATCH 5/6] Fix a "continue;" inside a switch in ArcanistPHPCompatibilityXHPASTLinterRule Summary: See . Test Plan: Looked at the code carefully. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19868 --- .../xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php index d73d1fae..b2fa6770 100644 --- a/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php @@ -70,7 +70,7 @@ final class ArcanistPHPCompatibilityXHPASTLinterRule $symbol = $params->getChildByIndex(0); if (!$symbol->isStaticScalar()) { - continue; + break; } $symbol_name = $symbol->evalStatic(); From 25c2381959ac94d9249ae4023c5f9ea36436b81c Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 22 Dec 2018 05:30:54 -0800 Subject: [PATCH 6/6] Statically detect "continue" inside "switch" Summary: See 30 prior patches. This is a fatal in PHP7, let's just hunt these down. Test Plan: Ran unit tests. See next diff for results. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19931 --- src/__phutil_library_map__.php | 4 ++ ...stContinueInsideSwitchXHPASTLinterRule.php | 53 +++++++++++++++++++ ...ueInsideSwitchXHPASTLinterRuleTestCase.php | 11 ++++ .../continue-inside-switch-1-valid.lint-test | 21 ++++++++ .../continue-inside-switch-2-n.lint-test | 24 +++++++++ ...continue-inside-switch-3-rewrite.lint-test | 26 +++++++++ 6 files changed, 139 insertions(+) create mode 100644 src/lint/linter/xhpast/rules/ArcanistContinueInsideSwitchXHPASTLinterRule.php create mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase.php create mode 100644 src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-1-valid.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-2-n.lint-test create mode 100644 src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-3-rewrite.lint-test diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e88a8621..e22d342e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -91,6 +91,8 @@ phutil_register_library_map(array( 'ArcanistConsoleLintRendererTestCase' => 'lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php', 'ArcanistConstructorParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php', 'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistConstructorParenthesesXHPASTLinterRuleTestCase.php', + 'ArcanistContinueInsideSwitchXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistContinueInsideSwitchXHPASTLinterRule.php', + 'ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase.php', 'ArcanistControlStatementSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistControlStatementSpacingXHPASTLinterRule.php', 'ArcanistControlStatementSpacingXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistControlStatementSpacingXHPASTLinterRuleTestCase.php', 'ArcanistCoverWorkflow' => 'workflow/ArcanistCoverWorkflow.php', @@ -511,6 +513,8 @@ phutil_register_library_map(array( 'ArcanistConsoleLintRendererTestCase' => 'PhutilTestCase', 'ArcanistConstructorParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', + 'ArcanistContinueInsideSwitchXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', + 'ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistControlStatementSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistControlStatementSpacingXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistCoverWorkflow' => 'ArcanistWorkflow', diff --git a/src/lint/linter/xhpast/rules/ArcanistContinueInsideSwitchXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistContinueInsideSwitchXHPASTLinterRule.php new file mode 100644 index 00000000..be071fac --- /dev/null +++ b/src/lint/linter/xhpast/rules/ArcanistContinueInsideSwitchXHPASTLinterRule.php @@ -0,0 +1,53 @@ +selectDescendantsOfType('n_CONTINUE'); + + $valid_containers = array( + 'n_WHILE' => true, + 'n_FOREACH' => true, + 'n_FOR' => true, + 'n_DO_WHILE' => true, + ); + + foreach ($continues as $continue) { + // If this is a "continue 2;" or similar, assume it's legitimate. + $label = $continue->getChildByIndex(0); + if ($label->getTypeName() !== 'n_EMPTY') { + continue; + } + + $node = $continue->getParentNode(); + while ($node) { + $node_type = $node->getTypeName(); + + // If we hit a valid loop which you can actually "continue;" inside, + // this is legitimate and we're done here. + if (isset($valid_containers[$node_type])) { + break; + } + + if ($node_type === 'n_SWITCH') { + $this->raiseLintAtNode( + $continue, + pht( + 'In a "switch" statement, "continue;" is equivalent to "break;" '. + 'but causes compile errors beginning with PHP 7.0.0.'), + 'break'); + } + + $node = $node->getParentNode(); + } + } + } + +} diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase.php new file mode 100644 index 00000000..031e18f8 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistContinueInsideSwitchXHPASTLinterRuleTestCase.php @@ -0,0 +1,11 @@ +executeTestsInDirectory( + dirname(__FILE__).'/continue-inside-switch/'); + } + +} diff --git a/src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-1-valid.lint-test b/src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-1-valid.lint-test new file mode 100644 index 00000000..6a9f040f --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/continue-inside-switch/continue-inside-switch-1-valid.lint-test @@ -0,0 +1,21 @@ +