skip non-closure sets in NoServiceSameNameSetClassRule (#216)

Author: TomasVotruba

src/Helper/NamingHelper.php

@@ -5,13 +5,18 @@
 namespace Symplify\PHPStanRules\Helper;
 
 use PhpParser\Node;
+use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Identifier;
 use PhpParser\Node\Name;
 
 final class NamingHelper
 {
     public static function getName(Node $node): ?string
     {
+        if ($node instanceof Variable && is_string($node->name)) {
+            return $node->name;
+        }
+
         if ($node instanceof Identifier || $node instanceof Name) {
             return $node->toString();
         }
@@ -21,11 +26,7 @@ public static function getName(Node $node): ?string
 
     public static function isName(Node $node, string $name): bool
     {
-        if ($node instanceof Identifier || $node instanceof Name) {
-            return $node->toString() === $name;
-        }
-
-        return false;
+        return self::getName($node) === $name;
     }
 
     /**

src/Rules/Symfony/ConfigClosure/NoServiceSameNameSetClassRule.php

@@ -7,82 +7,125 @@
 use Nette\Utils\Strings;
 use PhpParser\Node;
 use PhpParser\Node\Expr\ClassConstFetch;
+use PhpParser\Node\Expr\Closure;
 use PhpParser\Node\Expr\MethodCall;
+use PhpParser\NodeFinder;
 use PHPStan\Analyser\Scope;
 use PHPStan\Rules\Rule;
 use PHPStan\Rules\RuleError;
 use PHPStan\Rules\RuleErrorBuilder;
 use Symplify\PHPStanRules\Enum\RuleIdentifier\SymfonyRuleIdentifier;
 use Symplify\PHPStanRules\Helper\NamingHelper;
+use Symplify\PHPStanRules\Symfony\NodeAnalyzer\SymfonyClosureDetector;
 
 /**
- * @implements Rule<MethodCall>
+ * @implements Rule<Closure>
  *
  * @see \Symplify\PHPStanRules\Tests\Rules\Symfony\ConfigClosure\NoServiceSameNameSetClassRule\NoServiceSameNameSetClassRuleTest
  */
-final class NoServiceSameNameSetClassRule implements Rule
+final readonly class NoServiceSameNameSetClassRule implements Rule
 {
     /**
      * @var string
      */
     public const ERROR_MESSAGE = 'No need to duplicate service class and name. Use only "$services->set(%s::class)" instead';
 
+    private NodeFinder $nodeFinder;
+
+    public function __construct()
+    {
+        $this->nodeFinder = new NodeFinder();
+    }
+
     public function getNodeType(): string
     {
-        return MethodCall::class;
+        return Closure::class;
     }
 
     /**
-     * @param MethodCall $node
+     * @param Closure $node
      * @return RuleError[]
      */
     public function processNode(Node $node, Scope $scope): array
     {
-        if ($node->isFirstClassCallable()) {
+        if (! SymfonyClosureDetector::detect($node)) {
             return [];
         }
 
-        if (! NamingHelper::isName($node->name, 'set')) {
-            return [];
+        /** @var MethodCall[] $methodCalls */
+        $methodCalls = $this->nodeFinder->findInstanceOf($node, MethodCall::class);
+
+        $ruleErrors = [];
+
+        foreach ($methodCalls as $methodCall) {
+            if ($methodCall->isFirstClassCallable()) {
+                continue;
+            }
+
+            if (! NamingHelper::isName($methodCall->var, 'services')) {
+                continue;
+            }
+
+            if (! NamingHelper::isName($methodCall->name, 'set')) {
+                continue;
+            }
+
+            $serviceNameValue = $this->matchTwoArgsOfSameClassConstName($methodCall);
+            if (! is_string($serviceNameValue)) {
+                continue;
+            }
+
+            if (str_contains($serviceNameValue, '\\')) {
+                $serviceNameValue = Strings::after($serviceNameValue, '\\', -1);
+            }
+
+            $identifierRuleError = RuleErrorBuilder::message(sprintf(self::ERROR_MESSAGE, $serviceNameValue))
+                ->identifier(SymfonyRuleIdentifier::NO_SERVICE_SAME_NAME_SET_CLASS)
+                ->line($methodCall->getStartLine())
+                ->build();
+
+            $ruleErrors[] = $identifierRuleError;
         }
 
-        if (count($node->getArgs()) !== 2) {
-            return [];
+        return $ruleErrors;
+    }
+
+    /**
+     * We look for:
+     *
+     * $services->set(SomeClass::class, SomeClass::class)
+     */
+    private function matchTwoArgsOfSameClassConstName(MethodCall $methodCall): ?string
+    {
+        if (count($methodCall->getArgs()) !== 2) {
+            return null;
         }
 
-        $serviceName = $node->getArgs()[0]->value;
-        $serviceType = $node->getArgs()[1]->value;
+        $serviceName = $methodCall->getArgs()[0]->value;
+        $serviceType = $methodCall->getArgs()[1]->value;
 
         if (! $serviceName instanceof ClassConstFetch) {
-            return [];
+            return null;
         }
 
         if (! $serviceType instanceof ClassConstFetch) {
-            return [];
+            return null;
         }
 
         $serviceNameValue = NamingHelper::getName($serviceName->class);
         if (! is_string($serviceNameValue)) {
-            return [];
+            return null;
         }
 
         $serviceTypeValue = NamingHelper::getName($serviceType->class);
         if (! is_string($serviceTypeValue)) {
-            return [];
+            return null;
         }
 
         if ($serviceNameValue !== $serviceTypeValue) {
-            return [];
+            return null;
         }
 
-        if (str_contains($serviceNameValue, '\\')) {
-            $serviceNameValue = Strings::after($serviceNameValue, '\\', -1);
-        }
-
-        $identifierRuleError = RuleErrorBuilder::message(sprintf(self::ERROR_MESSAGE, $serviceNameValue))
-            ->identifier(SymfonyRuleIdentifier::NO_SERVICE_SAME_NAME_SET_CLASS)
-            ->build();
-
-        return [$identifierRuleError];
+        return $serviceNameValue;
     }
 }

tests/Rules/Symfony/ConfigClosure/NoServiceSameNameSetClassRule/Fixture/SkipNonClosureConstantSet.php

@@ -0,0 +1,18 @@
+<?php
+
+namespace Symplify\PHPStanRules\Tests\Rules\Symfony\ConfigClosure\NoServiceSameNameSetClassRule\Fixture;
+
+use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
+use Symplify\PHPStanRules\Tests\Rules\Symfony\ConfigClosure\NoServiceSameNameSetClassRule\Source\SomeSetService;
+
+class SkipNonClosureConstantSet
+{
+    private const NAME = 'name';
+
+    private const TYPE = 'type';
+
+    public function run()
+    {
+        $this->set(self::NAME, self::TYPE);
+    }
+}

tests/Rules/Symfony/ConfigClosure/NoServiceSameNameSetClassRule/Fixture/SkipParametersSetConstant.php

@@ -0,0 +1,12 @@
+<?php
+
+namespace Symplify\PHPStanRules\Tests\Rules\Symfony\ConfigClosure\NoServiceSameNameSetClassRule\Fixture;
+
+use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
+use Symplify\PHPStanRules\Tests\Rules\Symfony\ConfigClosure\NoServiceSameNameSetClassRule\Source\ConstantList;
+use Symplify\PHPStanRules\Tests\Rules\Symfony\ConfigClosure\NoServiceSameNameSetClassRule\Source\SomeSetService;
+
+return function (ContainerConfigurator $container) {
+    $parameters = $container->parameters();
+    $parameters->set(ConstantList::NAME, ConstantList::NAME);
+};

tests/Rules/Symfony/ConfigClosure/NoServiceSameNameSetClassRule/Fixture/SomeConfigWithInvalidSet.php

@@ -1,6 +1,6 @@
 <?php
 
-namespace Symplify\PHPStanRules\Tests\Rules\Symfony\ConfigClosure\ServicesExcludedDirectoryMustExistRule\Fixture;
+namespace Symplify\PHPStanRules\Tests\Rules\Symfony\ConfigClosure\NoServiceSameNameSetClassRule\Fixture;
 
 use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
 use Symplify\PHPStanRules\Tests\Rules\Symfony\ConfigClosure\NoServiceSameNameSetClassRule\Source\SomeSetService;

tests/Rules/Symfony/ConfigClosure/NoServiceSameNameSetClassRule/NoServiceSameNameSetClassRuleTest.php

@@ -29,10 +29,21 @@ public static function provideData(): Iterator
                 11,
             ],
         ]];
+
+        yield [__DIR__ . '/Fixture/SkipNonClosureConstantSet.php', []];
+        yield [__DIR__ . '/Fixture/SkipParametersSetConstant.php', []];
+    }
+
+    /**
+     * @return string[]
+     */
+    public static function getAdditionalConfigFiles(): array
+    {
+        return [__DIR__ . '/config/configured_rule.neon'];
     }
 
     protected function getRule(): Rule
     {
-        return new NoServiceSameNameSetClassRule();
+        return self::getContainer()->getByType(NoServiceSameNameSetClassRule::class);
     }
 }

tests/Rules/Symfony/ConfigClosure/NoServiceSameNameSetClassRule/Source/ConstantList.php

@@ -0,0 +1,8 @@
+<?php
+
+namespace Symplify\PHPStanRules\Tests\Rules\Symfony\ConfigClosure\NoServiceSameNameSetClassRule\Source;
+
+class ConstantList
+{
+    public const NAME = 'name';
+}

tests/Rules/Symfony/ConfigClosure/NoServiceSameNameSetClassRule/config/configured_rule.neon

@@ -0,0 +1,2 @@
+rules:
+    - Symplify\PHPStanRules\Rules\Symfony\ConfigClosure\NoServiceSameNameSetClassRule