1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-25 08:12:40 +01:00

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
This commit is contained in:
epriestley 2020-02-26 08:48:17 -08:00
parent 31653f6b77
commit 66a6128239
6 changed files with 27 additions and 72 deletions

View file

@ -357,8 +357,6 @@ phutil_register_library_map(array(
'ArcanistPhutilXHPASTLinterStandard' => 'lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php', 'ArcanistPhutilXHPASTLinterStandard' => 'lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php',
'ArcanistPlusOperatorOnStringsXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPlusOperatorOnStringsXHPASTLinterRule.php', 'ArcanistPlusOperatorOnStringsXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPlusOperatorOnStringsXHPASTLinterRule.php',
'ArcanistPlusOperatorOnStringsXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPlusOperatorOnStringsXHPASTLinterRuleTestCase.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', 'ArcanistProjectConfigurationSource' => 'config/source/ArcanistProjectConfigurationSource.php',
'ArcanistPrompt' => 'toolset/ArcanistPrompt.php', 'ArcanistPrompt' => 'toolset/ArcanistPrompt.php',
'ArcanistPublicPropertyXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPublicPropertyXHPASTLinterRule.php', 'ArcanistPublicPropertyXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPublicPropertyXHPASTLinterRule.php',
@ -1299,8 +1297,6 @@ phutil_register_library_map(array(
'ArcanistPhutilXHPASTLinterStandard' => 'ArcanistLinterStandard', 'ArcanistPhutilXHPASTLinterStandard' => 'ArcanistLinterStandard',
'ArcanistPlusOperatorOnStringsXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistPlusOperatorOnStringsXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
'ArcanistPlusOperatorOnStringsXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistPlusOperatorOnStringsXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
'ArcanistPregQuoteMisuseXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
'ArcanistPregQuoteMisuseXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
'ArcanistProjectConfigurationSource' => 'ArcanistWorkingCopyConfigurationSource', 'ArcanistProjectConfigurationSource' => 'ArcanistWorkingCopyConfigurationSource',
'ArcanistPrompt' => 'Phobject', 'ArcanistPrompt' => 'Phobject',
'ArcanistPublicPropertyXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistPublicPropertyXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',

View file

@ -34,7 +34,20 @@ final class ArcanistClassNameLiteralXHPASTLinterRule
$replacement = '__CLASS__'; $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)) { if (!preg_match($regex, $contents)) {
continue; continue;
} }
@ -42,8 +55,7 @@ final class ArcanistClassNameLiteralXHPASTLinterRule
$this->raiseLintAtNode( $this->raiseLintAtNode(
$string, $string,
pht( pht(
"Don't hard-code class names, use `%s` instead.", 'Prefer "__CLASS__" over hard-coded class names.'),
'__CLASS__'),
$replacement); $replacement);
} }
} }

View file

@ -1,42 +0,0 @@
<?php
/**
* @{function:preg_quote} takes two arguments, but the second one is optional
* because it is possible to use `()`, `[]` or `{}` as regular expression
* delimiters. If you don't pass a second argument, you're probably going to
* get something wrong.
*/
final class ArcanistPregQuoteMisuseXHPASTLinterRule
extends ArcanistXHPASTLinterRule {
const ID = 14;
public function getLintName() {
return pht('Misuse of `%s`', 'preg_quote');
}
public function getLintSeverity() {
return ArcanistLintSeverity::SEVERITY_ADVICE;
}
public function process(XHPASTNode $root) {
$function_calls = $this->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'));
}
}
}
}

View file

@ -1,10 +0,0 @@
<?php
final class ArcanistPregQuoteMisuseXHPASTLinterRuleTestCase
extends ArcanistXHPASTLinterRuleTestCase {
public function testLinter() {
$this->executeTestsInDirectory(dirname(__FILE__).'/preg_quote-misuse/');
}
}

View file

@ -6,10 +6,15 @@ class MyClass {
} }
public function someOtherMethod() { public function someOtherMethod() {
$x = 'MyClass is awesome'; $x = 'MyClass';
$y = 'MyClassIsAwesome'; $y = 'MyClassIsAwesome';
$z = 'MyClass is awesome';
return __CLASS__; return __CLASS__;
} }
public function throwAnException() {
throw new Exception('MyClass is throwing an exception!');
}
} }
$c = new class { $c = new class {
@ -29,10 +34,15 @@ class MyClass {
} }
public function someOtherMethod() { public function someOtherMethod() {
$x = 'MyClass is awesome'; $x = __CLASS__;
$y = 'MyClassIsAwesome'; $y = 'MyClassIsAwesome';
$z = 'MyClass is awesome';
return __CLASS__; return __CLASS__;
} }
public function throwAnException() {
throw new Exception('MyClass is throwing an exception!');
}
} }
$c = new class { $c = new class {

View file

@ -1,11 +0,0 @@
<?php
function foo($bar) {
preg_quote($bar);
preg_quote($bar, '/');
preg_Quote('moo');
preg_quote('moo', '/');
}
~~~~~~~~~~
advice:4:3:XHP14:Misuse of `preg_quote`
advice:6:3:XHP14:Misuse of `preg_quote`