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

Add a linter rule for use of is_a

Summary: `instanceof` should be used instead of `is_a`. I need to do a bit more research here to see if there are any edge cases.

Test Plan: Added unit tests.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14573
This commit is contained in:
Joshua Spence 2015-12-23 08:42:00 +11:00
parent 9ee14d2849
commit b3e68c9f17
5 changed files with 103 additions and 0 deletions

View file

@ -196,6 +196,8 @@ phutil_register_library_map(array(
'ArcanistInvalidModifiersXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistInvalidModifiersXHPASTLinterRuleTestCase.php',
'ArcanistInvalidOctalNumericScalarXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistInvalidOctalNumericScalarXHPASTLinterRule.php',
'ArcanistInvalidOctalNumericScalarXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistInvalidOctalNumericScalarXHPASTLinterRuleTestCase.php',
'ArcanistIsAShouldBeInstanceOfXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistIsAShouldBeInstanceOfXHPASTLinterRule.php',
'ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase.php',
'ArcanistJSHintLinter' => 'lint/linter/ArcanistJSHintLinter.php',
'ArcanistJSHintLinterTestCase' => 'lint/linter/__tests__/ArcanistJSHintLinterTestCase.php',
'ArcanistJSONLintLinter' => 'lint/linter/ArcanistJSONLintLinter.php',
@ -608,6 +610,8 @@ phutil_register_library_map(array(
'ArcanistInvalidModifiersXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
'ArcanistInvalidOctalNumericScalarXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
'ArcanistInvalidOctalNumericScalarXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
'ArcanistIsAShouldBeInstanceOfXHPASTLinterRule' => 'ArcanistXHPASTLinterRule',
'ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
'ArcanistJSHintLinter' => 'ArcanistExternalLinter',
'ArcanistJSHintLinterTestCase' => 'ArcanistExternalLinterTestCase',
'ArcanistJSONLintLinter' => 'ArcanistExternalLinter',

View file

@ -0,0 +1,69 @@
<?php
final class ArcanistIsAShouldBeInstanceOfXHPASTLinterRule
extends ArcanistXHPASTLinterRule {
const ID = 111;
public function getLintName() {
return pht('`%s` Should Be `%s`', 'is_a', 'instanceof');
}
public function getLintSeverity() {
return ArcanistLintSeverity::SEVERITY_ADVICE;
}
public function process(XHPASTNode $root) {
$calls = $this->getFunctionCalls($root, array('is_a'));
foreach ($calls as $call) {
$parameters = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST');
if (count($parameters->getChildren()) > 2) {
// If the `$allow_string` parameter is `true` then the `instanceof`
// operator cannot be used. Evaluating whether an expression is truthy
// or falsely is hard, and so we only check that the `$allow_string`
// parameter is either absent or literally `false`.
$allow_string = $parameters->getChildByIndex(2);
if (strtolower($allow_string->getConcreteString()) != 'false') {
continue;
}
}
$object = $parameters->getChildByIndex(0);
$class = $parameters->getChildByIndex(1);
switch ($class->getTypeName()) {
case 'n_STRING_SCALAR':
$replacement = stripslashes(
substr($class->getConcreteString(), 1, -1));
break;
case 'n_VARIABLE':
$replacement = $class->getConcreteString();
break;
default:
$replacement = null;
break;
}
$this->raiseLintAtNode(
$call,
pht(
'Use `%s` instead of `%s`. The former is a language '.
'construct whereas the latter is a function call, which '.
'has additional overhead.',
'instanceof',
'is_a'),
($replacement === null)
? null
: sprintf(
'%s instanceof %s',
$object->getConcreteString(),
$replacement));
}
}
}

View file

@ -0,0 +1,11 @@
<?php
final class ArcanistIsAShouldBeInstanceOfXHPASTLinterRuleTestCase
extends ArcanistXHPASTLinterRuleTestCase {
public function testLinter() {
$this->executeTestsInDirectory(
dirname(__FILE__).'/is_a-should-be-instanceof/');
}
}

View file

@ -0,0 +1,3 @@
<?php
is_a('SubClass', 'BaseClass', true);
~~~~~~~~~~

View file

@ -0,0 +1,16 @@
<?php
is_a($x, $y);
is_a($x, 'SomeClass');
is_a($x, '\\SomeClass');
is_a($x, '\\'.'Some'.'Class');
~~~~~~~~~~
advice:2:1
advice:3:1
advice:4:1
advice:5:1
~~~~~~~~~~
<?php
$x instanceof $y;
$x instanceof SomeClass;
$x instanceof \SomeClass;
is_a($x, '\\'.'Some'.'Class');