fix issue #348

llaville · Mar 6, 2022 · 9a4c31c · 9a4c31c
1 parent 18bc9d8
commit 9a4c31c
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 14 deletions.
diff --git 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
 <!-- MARKDOWN-RELEASE:END -->
 
 ## [6.2.0] - 2022-02-06

diff --git 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
@@ -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
@@ -0,0 +1,6 @@
+<?php
+$extension = 'json';
+
+$factories = function () use ($extension) {
+    return $extension;
+};
diff --git a/tests/fixtures/sniffs/functions/gh348_function.php b/tests/fixtures/sniffs/functions/gh348_function.php
@@ -0,0 +1,5 @@
+<?php
+function foo($a, $b, $c = []): int
+{
+    return 1;
+}