From 66a6128239e2df126858279feaa31f02ebfe1f6a Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 26 Feb 2020 08:48:17 -0800 Subject: [PATCH] Remove the "preg_quote()" lint rule and update the "__CLASS__" lint rule Summary: Depends on D21031. Ref T11968. This clears some lint with the word "Future", but is just a minor cleanup patch not really related to the main goal in that task. We currently warn on `preg_quote()` with no second parameter, but this is valid and correct: ``` preg_match('('.preg_quote($x).')', ...) ``` This is the modern recommended style for this sort of expression, so the warning is often a false positive; drop it. The "__CLASS__" warning looks for hard-coded class names in strings, but currently looks for them as a word anywhere in the string. This is often a false positive and sometimes makes error messages or exceptions untranslatable. For example, translators can't do anything with this: > Filesystem path "%s" does not exist. ...if it's written as: > %s path "%s" does not exist. ...just because it happens to appear in the class "Filesystem". Some other classes, including "Future", suffer from this. Even when the detection is correct, it's clunky and a mistake here (failing to update the class name in an error message) is unlikely to ever do any real damage. Test Plan: Ran unit tests and `arc lint`. Maniphest Tasks: T11968 Differential Revision: https://secure.phabricator.com/D21032 --- src/__phutil_library_map__.php | 4 -- ...canistClassNameLiteralXHPASTLinterRule.php | 18 ++++++-- ...rcanistPregQuoteMisuseXHPASTLinterRule.php | 42 ------------------- ...regQuoteMisuseXHPASTLinterRuleTestCase.php | 10 ----- .../class-name-literal.lint-test | 14 ++++++- .../preg_quote-misuse.lint-test | 11 ----- 6 files changed, 27 insertions(+), 72 deletions(-) delete mode 100644 src/lint/linter/xhpast/rules/ArcanistPregQuoteMisuseXHPASTLinterRule.php delete mode 100644 src/lint/linter/xhpast/rules/__tests__/ArcanistPregQuoteMisuseXHPASTLinterRuleTestCase.php delete mode 100644 src/lint/linter/xhpast/rules/__tests__/preg_quote-misuse/preg_quote-misuse.lint-test diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6b1bc7e8..b3a3e5e7 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -357,8 +357,6 @@ phutil_register_library_map(array( 'ArcanistPhutilXHPASTLinterStandard' => 'lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php', 'ArcanistPlusOperatorOnStringsXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPlusOperatorOnStringsXHPASTLinterRule.php', 'ArcanistPlusOperatorOnStringsXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPlusOperatorOnStringsXHPASTLinterRuleTestCase.php', - 'ArcanistPregQuoteMisuseXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPregQuoteMisuseXHPASTLinterRule.php', - 'ArcanistPregQuoteMisuseXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPregQuoteMisuseXHPASTLinterRuleTestCase.php', 'ArcanistProjectConfigurationSource' => 'config/source/ArcanistProjectConfigurationSource.php', 'ArcanistPrompt' => 'toolset/ArcanistPrompt.php', 'ArcanistPublicPropertyXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPublicPropertyXHPASTLinterRule.php', @@ -1299,8 +1297,6 @@ phutil_register_library_map(array( 'ArcanistPhutilXHPASTLinterStandard' => 'ArcanistLinterStandard', 'ArcanistPlusOperatorOnStringsXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistPlusOperatorOnStringsXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', - 'ArcanistPregQuoteMisuseXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', - 'ArcanistPregQuoteMisuseXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistProjectConfigurationSource' => 'ArcanistWorkingCopyConfigurationSource', 'ArcanistPrompt' => 'Phobject', 'ArcanistPublicPropertyXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', diff --git a/src/lint/linter/xhpast/rules/ArcanistClassNameLiteralXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistClassNameLiteralXHPASTLinterRule.php index 288fc05c..6d9da91b 100644 --- a/src/lint/linter/xhpast/rules/ArcanistClassNameLiteralXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistClassNameLiteralXHPASTLinterRule.php @@ -34,7 +34,20 @@ final class ArcanistClassNameLiteralXHPASTLinterRule $replacement = '__CLASS__'; } - $regex = '/\b'.preg_quote($class_name, '/').'\b/'; + // NOTE: We're only warning when the entire string content is the + // class name. It's okay to hard-code the class name as part of a + // longer string, like an error or exception message. + + // Sometimes the class name (like "Filesystem") is also a valid part + // of the message, which makes this warning a false positive. + + // Even when we're generating a true positive by detecting a class + // name in part of a longer string, the cost of an error message + // being out-of-date is generally very small (mild confusion, but + // no incorrect beahavior) and using "__CLASS__" in errors is often + // clunky. + + $regex = '(^'.preg_quote($class_name).'$)'; if (!preg_match($regex, $contents)) { continue; } @@ -42,8 +55,7 @@ final class ArcanistClassNameLiteralXHPASTLinterRule $this->raiseLintAtNode( $string, pht( - "Don't hard-code class names, use `%s` instead.", - '__CLASS__'), + 'Prefer "__CLASS__" over hard-coded class names.'), $replacement); } } diff --git a/src/lint/linter/xhpast/rules/ArcanistPregQuoteMisuseXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistPregQuoteMisuseXHPASTLinterRule.php deleted file mode 100644 index de5b522f..00000000 --- a/src/lint/linter/xhpast/rules/ArcanistPregQuoteMisuseXHPASTLinterRule.php +++ /dev/null @@ -1,42 +0,0 @@ -getFunctionCalls($root, array('preg_quote')); - - foreach ($function_calls as $call) { - $parameter_list = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST'); - if (count($parameter_list->getChildren()) !== 2) { - $this->raiseLintAtNode( - $call, - pht( - 'If you use pattern delimiters that require escaping '. - '(such as `%s`, but not `%s`) then you should pass two '. - 'arguments to %s, so that %s knows which delimiter to escape.', - '//', - '()', - 'preg_quote', - 'preg_quote')); - } - } - } - -} diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistPregQuoteMisuseXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistPregQuoteMisuseXHPASTLinterRuleTestCase.php deleted file mode 100644 index 47838fdd..00000000 --- a/src/lint/linter/xhpast/rules/__tests__/ArcanistPregQuoteMisuseXHPASTLinterRuleTestCase.php +++ /dev/null @@ -1,10 +0,0 @@ -executeTestsInDirectory(dirname(__FILE__).'/preg_quote-misuse/'); - } - -} 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 66bd140b..0182d151 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 @@ -6,10 +6,15 @@ class MyClass { } public function someOtherMethod() { - $x = 'MyClass is awesome'; + $x = 'MyClass'; $y = 'MyClassIsAwesome'; + $z = 'MyClass is awesome'; return __CLASS__; } + + public function throwAnException() { + throw new Exception('MyClass is throwing an exception!'); + } } $c = new class { @@ -29,10 +34,15 @@ class MyClass { } public function someOtherMethod() { - $x = 'MyClass is awesome'; + $x = __CLASS__; $y = 'MyClassIsAwesome'; + $z = 'MyClass is awesome'; return __CLASS__; } + + public function throwAnException() { + throw new Exception('MyClass is throwing an exception!'); + } } $c = new class { diff --git a/src/lint/linter/xhpast/rules/__tests__/preg_quote-misuse/preg_quote-misuse.lint-test b/src/lint/linter/xhpast/rules/__tests__/preg_quote-misuse/preg_quote-misuse.lint-test deleted file mode 100644 index 4df4f718..00000000 --- a/src/lint/linter/xhpast/rules/__tests__/preg_quote-misuse/preg_quote-misuse.lint-test +++ /dev/null @@ -1,11 +0,0 @@ -