Skip to content

Commit 3d02356

Browse files
kukulichkukulich
committed
UselessVariableSniff: Fixed false positives
1 parent c7fb254 commit 3d02356

File tree

5 files changed

+59
-112
lines changed

5 files changed

+59
-112
lines changed

‎SlevomatCodingStandard/Sniffs/Variables/UselessVariableSniff.php‎

Lines changed: 39 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
use PHP_CodeSniffer\Sniffs\Sniff;
77
use SlevomatCodingStandard\Helpers\TokenHelper;
88
use const T_AND_EQUAL;
9-
use const T_CLOSE_CURLY_BRACKET;
109
use const T_CONCAT_EQUAL;
1110
use const T_DIV_EQUAL;
1211
use const T_DOC_COMMENT_CLOSE_TAG;
@@ -25,7 +24,6 @@
2524
use const T_VARIABLE;
2625
use const T_WHITESPACE;
2726
use const T_XOR_EQUAL;
28-
use function array_key_exists;
2927
use function array_reverse;
3028
use function count;
3129
use function in_array;
@@ -91,66 +89,11 @@ public function process(File $phpcsFile, $returnPointer): void
9189
return;
9290
}
9391

94-
for ($i = $variablePointer + 1; $i < count($tokens); $i++) {
95-
if (
96-
in_array($tokens[$i]['code'], TokenHelper::$functionTokenCodes, true)
97-
&& $this->isInSameScope($phpcsFile, $variablePointer, $i)
98-
) {
99-
$i = $tokens[$i]['scope_closer'];
100-
continue;
101-
}
102-
103-
if ($tokens[$i]['code'] !== T_VARIABLE) {
104-
continue;
105-
}
106-
107-
if ($tokens[$i]['content'] !== $variableName) {
108-
continue;
109-
}
110-
111-
if ($this->isInSameScope($phpcsFile, $variablePointer, $i)) {
112-
return;
113-
}
114-
}
115-
116-
$errorParameters = [
117-
sprintf('Useless variable %s.', $variableName),
118-
$previousVariablePointer,
119-
self::CODE_USELESS_VARIABLE,
120-
];
121-
122-
$pointerAfterReturnSemicolon = TokenHelper::findNextEffective($phpcsFile, $returnSemicolonPointer + 1);
123-
124-
if (
125-
$tokens[$pointerAfterReturnSemicolon]['code'] !== T_CLOSE_CURLY_BRACKET
126-
|| !array_key_exists('scope_condition', $tokens[$pointerAfterReturnSemicolon])
127-
|| !in_array($tokens[$tokens[$pointerAfterReturnSemicolon]['scope_condition']]['code'], TokenHelper::$functionTokenCodes, true)
128-
) {
129-
$phpcsFile->addError(...$errorParameters);
130-
return;
131-
}
132-
133-
$previousVariableSemicolonPointer = null;
134-
for ($i = $previousVariablePointer + 1; $i < $returnPointer - 1; $i++) {
135-
if ($tokens[$i]['code'] !== T_SEMICOLON) {
136-
continue;
137-
}
138-
139-
if (!$this->isInSameScope($phpcsFile, $previousVariablePointer, $i)) {
140-
continue;
141-
}
142-
143-
$previousVariableSemicolonPointer = $i;
144-
break;
145-
}
146-
$pointerAfterPreviousVariableSemicolon = TokenHelper::findNextEffective($phpcsFile, $previousVariableSemicolonPointer + 1);
147-
148-
if ($returnPointer !== $pointerAfterPreviousVariableSemicolon) {
149-
$phpcsFile->addError(...$errorParameters);
92+
if (!$this->areBothPointersNearby($phpcsFile, $previousVariablePointer, $returnPointer)) {
15093
return;
15194
}
15295

153-
$fix = $phpcsFile->addFixableError(...$errorParameters);
96+
$fix = $phpcsFile->addFixableError(sprintf('Useless variable %s.', $variableName), $previousVariablePointer, self::CODE_USELESS_VARIABLE);
15497

15598
if (!$fix) {
15699
return;
@@ -174,6 +117,8 @@ public function process(File $phpcsFile, $returnPointer): void
174117
T_CONCAT_EQUAL => '.',
175118
];
176119

120+
$previousVariableSemicolonPointer = $this->findSemicolon($phpcsFile, $previousVariablePointer);
121+
177122
$phpcsFile->fixer->beginChangeset();
178123

179124
if ($tokens[$assigmentPointer]['code'] === T_EQUAL) {
@@ -198,15 +143,17 @@ private function isInSameScope(File $phpcsFile, int $firstPointer, int $secondPo
198143
$tokens = $phpcsFile->getTokens();
199144

200145
$getScopeLevel = function (int $pointer) use ($tokens): int {
146+
$level = $tokens[$pointer]['level'];
201147
foreach (array_reverse($tokens[$pointer]['conditions'], true) as $conditionPointer => $conditionTokenCode) {
202148
if (!in_array($conditionTokenCode, TokenHelper::$functionTokenCodes, true)) {
203149
continue;
204150
}
205151

206-
return $tokens[$conditionPointer]['level'];
152+
$level = $tokens[$conditionPointer]['level'];
153+
break;
207154
}
208155

209-
return $tokens[$pointer]['level'];
156+
return $level;
210157
};
211158

212159
return $getScopeLevel($firstPointer) === $getScopeLevel($secondPointer);
@@ -292,10 +239,38 @@ private function hasAnotherAssigmentBefore(File $phpcsFile, int $variablePointer
292239
return false;
293240
}
294241

295-
$previousVariableSemicolonPointer = TokenHelper::findNext($phpcsFile, T_SEMICOLON, $previousVariablePointer + 1);
296-
$pointerAfterPreviousVariableSemicolon = TokenHelper::findNextEffective($phpcsFile, $previousVariableSemicolonPointer + 1);
242+
return $this->areBothPointersNearby($phpcsFile, $previousVariablePointer, $variablePointer);
243+
}
244+
245+
private function areBothPointersNearby(File $phpcsFile, int $firstPointer, int $secondPointer): bool
246+
{
247+
$firstVariableSemicolonPointer = $this->findSemicolon($phpcsFile, $firstPointer);
248+
$pointerAfterFirstVariableSemicolon = TokenHelper::findNextEffective($phpcsFile, $firstVariableSemicolonPointer + 1);
249+
250+
return $pointerAfterFirstVariableSemicolon === $secondPointer;
251+
}
252+
253+
private function findSemicolon(File $phpcsFile, int $pointer): int
254+
{
255+
$tokens = $phpcsFile->getTokens();
256+
257+
$semicolonPointer = null;
258+
for ($i = $pointer + 1; $i < count($tokens) - 1; $i++) {
259+
if ($tokens[$i]['code'] !== T_SEMICOLON) {
260+
continue;
261+
}
262+
263+
if (!$this->isInSameScope($phpcsFile, $pointer, $i)) {
264+
continue;
265+
}
266+
267+
$semicolonPointer = $i;
268+
break;
269+
}
297270

298-
return $pointerAfterPreviousVariableSemicolon === $variablePointer;
271+
/** @var int $semicolonPointer */
272+
$semicolonPointer = $semicolonPointer;
273+
return $semicolonPointer;
299274
}
300275

301276
}

‎tests/Sniffs/Variables/UselessVariableSniffTest.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ public function testErrors(): void
1717
{
1818
$report = self::checkFile(__DIR__ . '/data/uselessVariableErrors.php');
1919

20-
// self::assertSame(16, $report->getErrorCount());
20+
self::assertSame(15, $report->getErrorCount());
2121

2222
self::assertSniffError($report, 4, UselessVariableSniff::CODE_USELESS_VARIABLE, 'Useless variable $a.');
2323
self::assertSniffError($report, 9, UselessVariableSniff::CODE_USELESS_VARIABLE, 'Useless variable $b.');
@@ -33,9 +33,7 @@ public function testErrors(): void
3333
self::assertSniffError($report, 59, UselessVariableSniff::CODE_USELESS_VARIABLE, 'Useless variable $l.');
3434
self::assertSniffError($report, 64, UselessVariableSniff::CODE_USELESS_VARIABLE, 'Useless variable $m.');
3535
self::assertSniffError($report, 69, UselessVariableSniff::CODE_USELESS_VARIABLE, 'Useless variable $n.');
36-
self::assertSniffError($report, 77, UselessVariableSniff::CODE_USELESS_VARIABLE, 'Useless variable $o.');
37-
self::assertSniffError($report, 90, UselessVariableSniff::CODE_USELESS_VARIABLE, 'Useless variable $result.');
38-
self::assertSniffError($report, 98, UselessVariableSniff::CODE_USELESS_VARIABLE, 'Useless variable $result.');
36+
self::assertSniffError($report, 78, UselessVariableSniff::CODE_USELESS_VARIABLE, 'Useless variable $o.');
3937

4038
self::assertAllFixedInFile($report);
4139
}

‎tests/Sniffs/Variables/data/uselessVariableErrors.fixed.php‎

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -58,27 +58,7 @@ function sameVariableInDifferentScope() {
5858
}, []);
5959
}
6060

61-
function differentVariableAfterReturn() {
62-
$o = 0;
63-
64-
if (true) {
65-
return $o;
66-
}
67-
68-
$p = 1;
69-
}
70-
71-
function moreVariables() {
72-
try {
73-
$result = true;
74-
} catch (Throwable $e) {
75-
$result = false;
76-
}
77-
78-
return $result;
79-
}
80-
8161
function moreVariableOneWithoutAssigment() {
82-
$result++;
62+
$o++;
8363
return 10;
8464
}

‎tests/Sniffs/Variables/data/uselessVariableErrors.php‎

Lines changed: 3 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -73,29 +73,9 @@ function sameVariableInDifferentScope() {
7373
return $n;
7474
}
7575

76-
function differentVariableAfterReturn() {
77-
$o = 0;
78-
79-
if (true) {
80-
return $o;
81-
}
82-
83-
$p = 1;
84-
}
85-
86-
function moreVariables() {
87-
try {
88-
$result = true;
89-
} catch (Throwable $e) {
90-
$result = false;
91-
}
92-
93-
return $result;
94-
}
95-
9676
function moreVariableOneWithoutAssigment() {
97-
$result++;
98-
$result = 10;
77+
$o++;
78+
$o = 10;
9979

100-
return $result;
80+
return $o;
10181
}

‎tests/Sniffs/Variables/data/uselessVariableNoErrors.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,3 +53,17 @@ function moreAssigments() {
5353
$i .= 'ii';
5454
return $i;
5555
}
56+
57+
function somethingBetweenAssigmentAndReturn() {
58+
$j = 'j';
59+
doSomething();
60+
return $j;
61+
}
62+
63+
function differentScope() {
64+
$k = 'k';
65+
66+
if (true) {
67+
return $k;
68+
}
69+
}

0 commit comments

Comments
 (0)