From 9a4c31ce2de78ff096821232b7e45b27ebf7778a Mon Sep 17 00:00:00 2001 From: Laurent Laville Date: Sun, 6 Mar 2022 17:37:35 +0000 Subject: [PATCH] fix issue #348 --- CHANGELOG-6.x.md | 1 + .../TrailingCommaSniff.php | 79 +++++++++++++++---- tests/Sniffs/TrailingCommaSniffTest.php | 6 ++ .../sniffs/functions/gh348_closure.php | 6 ++ .../sniffs/functions/gh348_function.php | 5 ++ 5 files changed, 83 insertions(+), 14 deletions(-) create mode 100644 tests/fixtures/sniffs/functions/gh348_closure.php create mode 100644 tests/fixtures/sniffs/functions/gh348_function.php diff --git a/CHANGELOG-6.x.md b/CHANGELOG-6.x.md index aacaff7b..213fb8f8 100644 --- a/CHANGELOG-6.x.md +++ b/CHANGELOG-6.x.md @@ -16,6 +16,7 @@ using the [Keep a CHANGELOG](http://keepachangelog.com) principles. - [#345](https://github.com/llaville/php-compatinfo/issues/345) : class `PhpParser\Node\Expr\PropertyFetch` could not be converted to string - [#346](https://github.com/llaville/php-compatinfo/issues/346) : Improves displaying Error in Datasource Analysis +- [#348](https://github.com/llaville/php-compatinfo/issues/348) : TrailingCommaSniff give wrong results ## [6.2.0] - 2022-02-06 diff --git a/src/Application/Sniffs/FunctionDeclarations/TrailingCommaSniff.php b/src/Application/Sniffs/FunctionDeclarations/TrailingCommaSniff.php index efbfe04f..af2cda42 100644 --- a/src/Application/Sniffs/FunctionDeclarations/TrailingCommaSniff.php +++ b/src/Application/Sniffs/FunctionDeclarations/TrailingCommaSniff.php @@ -62,23 +62,21 @@ public function enterSniff(): void public function enterNode(Node $node) { if ($node instanceof Node\Stmt\Function_) { - $argCount = count($node->params); - if (!$this->isTrailingComma($node, $argCount)) { - return null; - } + $trailingCommaFound = $this->checkParamsList($node); } elseif ($node instanceof Node\Expr\Closure) { - $argCount = count($node->params); // first attempt with closure parameters - if (!$this->isTrailingComma($node, $argCount)) { - $argCount = count($node->uses); + $trailingCommaFound = $this->checkParamsList($node); + if (!$trailingCommaFound) { // second attempt with closure use list - if (!$this->isTrailingComma($node, $argCount)) { - return null; - } + $trailingCommaFound = $this->checkUsesList($node); } } else { + $trailingCommaFound = false; + } + if (!$trailingCommaFound) { return null; } + if (!$parent = $node->getAttribute($this->attributeParentKeyStore)) { return null; } @@ -88,12 +86,65 @@ public function enterNode(Node $node) return null; } - private function isTrailingComma(Node $node, int $argCount): bool + private function checkParamsList(Node\FunctionLike $node): bool + { + $params = $node->getParams(); + $argCount = count($params); + if ($argCount === 0) { + // without parameters, no need to check + return false; + } + $returnType = $node->getReturnType(); + + $startTokenPos = $params[0]->getAttribute('startTokenPos'); + // CAUTION: depending on code context, this position is probably wrong + $endTokenPos = $params[$argCount - 1]->getAttribute('endTokenPos'); + // fallback strategy follows + if ($node instanceof Node\Expr\Closure && !empty($node->uses)) { + $endTokenPos = $node->uses[0]->getAttribute('startTokenPos'); + } elseif ($returnType !== null) { + // function or closure with return type + $endTokenPos = $returnType->getAttribute('startTokenPos'); + } elseif (!empty($node->stmts)) { + // function or closure with body + $endTokenPos = $node->stmts[0]->getAttribute('startTokenPos'); + } else { + // function or closure without body + $endTokenPos = $node->getAttribute('endTokenPos'); + } + + return $this->isTrailingComma($startTokenPos, $endTokenPos, $argCount); + } + + private function checkUsesList(Node\Expr\Closure $node): bool { - $i = $node->getAttribute('startTokenPos'); - $l = $node->getAttribute('endTokenPos'); + $argCount = count($node->uses); + if ($argCount === 0) { + // without use list, no need to check + return false; + } - $tokens = array_slice($this->tokens, $i, $l - $i); + $startTokenPos = $node->uses[0]->getAttribute('startTokenPos'); + // CAUTION: depending on code context, this position may be wrong + $endTokenPos = $node->uses[$argCount - 1]->getAttribute('endTokenPos'); + // fallback strategy follows + if ($node->returnType !== null) { + // function or closure with return type + $endTokenPos = $node->returnType->getAttribute('startTokenPos'); + } elseif (!empty($node->stmts)) { + // function or closure with body + $endTokenPos = $node->stmts[0]->getAttribute('startTokenPos'); + } else { + // function or closure without body + $endTokenPos = $node->getAttribute('endTokenPos'); + } + + return $this->isTrailingComma($startTokenPos, $endTokenPos, $argCount); + } + + private function isTrailingComma(int $startTokenPos, int $endTokenPos, int $argCount): bool + { + $tokens = array_slice($this->tokens, $startTokenPos, $endTokenPos - $startTokenPos); $commas = array_filter($tokens, function ($token) { return (is_string($token) && ',' == $token); diff --git a/tests/Sniffs/TrailingCommaSniffTest.php b/tests/Sniffs/TrailingCommaSniffTest.php index 7ff14407..89cfd77e 100644 --- a/tests/Sniffs/TrailingCommaSniffTest.php +++ b/tests/Sniffs/TrailingCommaSniffTest.php @@ -48,6 +48,12 @@ public function trailingCommaProvider(): iterable 'trailing_comma_closure_2.php' => [ 'php.min' => '8.0.0alpha1', ], + 'gh348_function.php' => [ // test to fix regression on trying to detect trailing comma + 'php.min' => '7.0.0alpha1', + ], + 'gh348_closure.php' => [ // test to fix regression on trying to detect trailing comma + 'php.min' => '5.3.0', + ], ]; foreach ($provides as $filename => $versions) { diff --git a/tests/fixtures/sniffs/functions/gh348_closure.php b/tests/fixtures/sniffs/functions/gh348_closure.php new file mode 100644 index 00000000..9fc57387 --- /dev/null +++ b/tests/fixtures/sniffs/functions/gh348_closure.php @@ -0,0 +1,6 @@ +