refactor container check (#8)

* refactor container check

* no message

* no message

* no message

.gitignore

@@ -1,3 +1,4 @@
 vendor
 composer.lock
 .php_cs.cache
+tests/_run

.php_cs.dist

@@ -0,0 +1,20 @@
+<?php
+
+$finder = PhpCsFixer\Finder::create()
+    ->in(__DIR__.'/src')
+    ->ignoreDotFiles(true)
+    ->ignoreVCS(true)
+    ->files()
+    ->name('*.php');
+
+return PhpCsFixer\Config::create()
+    ->setFinder($finder)
+    ->setRules([
+        '@Symfony' => true,
+        'array_syntax' => ['syntax' => 'short'],
+        'binary_operator_spaces' => ['align_double_arrow' => false],
+        'no_useless_else' => true,
+        'no_useless_return' => false,
+        'ordered_imports' => true,
+        'phpdoc_to_comment' => false,
+    ]);

.travis.yml

@@ -7,6 +7,7 @@ php:
 
 before_install:
   - phpenv config-rm xdebug.ini || true
+  - composer validate
 
 install:
   - composer install

README.md

@@ -11,11 +11,45 @@ vendor/bin/psalm-plugin enable seferov/symfony-psalm-plugin
 
 ### Features
 
-- Detect `ContainerInterface::get()` result type
+- Detect `ContainerInterface::get()` result type. Works better if you [configure](https://github.com/seferov/symfony-psalm-plugin/#configuration) compiled container XML file.
 - Fixes `PossiblyInvalidArgument` for `Symfony\Component\HttpFoundation\Request::getContent`.
 The plugin calculates real return type by checking the given argument and marks return type as either string or resource.
 - Complains when `Container` is injected to a service. Use dependency-injection.
 
+### Configuration
+
+If you followed installation instructions, psalm-plugin command would added plugin configuration to psalm.xml
+
+```xml
+<?xml version="1.0"?>
+<psalm
+    totallyTyped="true"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xmlns="https://getpsalm.org/schema/config"
+    xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
+>
+    <!--  project configuration -->
+
+    <plugins>
+        <pluginClass class="Seferov\SymfonyPsalmPlugin\Plugin" />
+    </plugins>
+</psalm>
+```
+
+To be able to detect return types of services using ID (generally starts with `@` in Symfony YAML config files. Ex: `logger` service)
+`containerXml` must be provided. Example:
+```xml
+<pluginClass class="Seferov\SymfonyPsalmPlugin\Plugin">
+    <containerXml>var/cache/dev/App_KernelDevDebugContainer.xml</containerXml>
+</pluginClass>
+```
+
+This file path may change based on your Symfony version, file structure and environment settings.
+Default file for Symfony versions:
+- Symfony 3: var/cache/dev/srcDevDebugProjectContainer.xml
+- Symfony 4: var/cache/dev/srcApp_KernelDevDebugContainer.xml
+- Symfony 5: var/cache/dev/App_KernelDevDebugContainer.xml
+
 ### Credits
 
 - [@weirdan](https://github.com/weirdan) for [codeception psalm module](https://github.com/weirdan/codeception-psalm-module)

codeception.yml

@@ -1,11 +1,11 @@
 namespace: Psalm\PhpUnitPlugin\Tests
 
 paths:
-  tests: tests
-  output: tests/_output
-  data: tests/_data
-  support: tests/_support
-  envs: tests/_envs
+  tests: tests/acceptance
+  output: tests/acceptance/_output
+  data: tests/acceptance/_data
+  support: tests/acceptance/_support
+  envs: tests/acceptance/_envs
 
 actor_suffix: Tester
 

composer.json

@@ -11,6 +11,7 @@
     ],
     "require": {
         "php": "^7.1",
+        "ext-simplexml": "*",
         "vimeo/psalm": "^3.7",
         "symfony/framework-bundle": "^3.0 || ^4.0 || ^5.0"
     },

src/Handler/ClassHandler.php

@@ -13,7 +13,6 @@ use Psalm\Plugin\Hook\AfterClassLikeAnalysisInterface;
 use Psalm\Plugin\Hook\AfterMethodCallAnalysisInterface;
 use Psalm\StatementsSource;
 use Psalm\Storage\ClassLikeStorage;
-use Psalm\Type\Atomic\TNamedObject;
 use Psalm\Type\Union;
 use Seferov\SymfonyPsalmPlugin\Issue\ContainerDependency;
 use Seferov\SymfonyPsalmPlugin\Issue\RepositoryStringShortcut;
@@ -21,11 +20,6 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
 
 class ClassHandler implements AfterClassLikeAnalysisInterface, AfterMethodCallAnalysisInterface
 {
-    /**
-     * @psalm-var array<string, string>
-     */
-    private static $classServiceMap = [];
-
     /**
      * {@inheritdoc}
      */
@@ -62,22 +56,6 @@ class ClassHandler implements AfterClassLikeAnalysisInterface, AfterMethodCallAn
         Union &$return_type_candidate = null
     ) {
         switch ($declaring_method_id) {
-            case 'Psr\Container\ContainerInterface::get':
-            case 'Symfony\Component\DependencyInjection\ContainerInterface::get':
-                if ($return_type_candidate && $expr->args[0]->value instanceof ClassConstFetch) {
-                    $className = (string) $expr->args[0]->value->class->getAttribute('resolvedName');
-                    $return_type_candidate = new Union([new TNamedObject($className)]);
-                }
-                if (!count(self::$classServiceMap)) {
-                    self::$classServiceMap = self::loadServiceFile($codebase);
-                }
-                if ($return_type_candidate && count(self::$classServiceMap) && $expr->args[0]->value instanceof Node\Scalar\String_) {
-                    $serviceName = (string) $expr->args[0]->value->value;
-                    if (isset(self::$classServiceMap[$serviceName])) {
-                        $return_type_candidate = new Union([new TNamedObject((string) self::$classServiceMap[$serviceName])]);
-                    }
-                }
-                break;
             case 'Symfony\Component\HttpFoundation\Request::getcontent':
                 if ($return_type_candidate) {
                     $removeType = 'resource';
@@ -99,45 +77,4 @@ class ClassHandler implements AfterClassLikeAnalysisInterface, AfterMethodCallAn
                 break;
         }
     }
-
-    /**
-     * @todo don't check every time if containerXml config is not set.
-     * @psalm-return array<string, string>
-     */
-    private static function loadServiceFile(Codebase $codebase): array
-    {
-        $classServiceMap = [];
-        if (count($codebase->config->getPluginClasses())) {
-            foreach ($codebase->config->getPluginClasses() as $pluginClass) {
-                if ($pluginClass['class'] === str_replace('Handler', 'Plugin', __NAMESPACE__)) {
-                    $simpleXmlConfig = $pluginClass['config'];
-                }
-            }
-        }
-
-        if (isset($simpleXmlConfig) && $simpleXmlConfig instanceof \SimpleXMLElement) {
-            $serviceFilePath = (string) $simpleXmlConfig->containerXml;
-            if (!file_exists($serviceFilePath)) {
-                return [];
-            }
-            $xml = simplexml_load_file($serviceFilePath);
-            if (!$xml->services instanceof \SimpleXMLElement) {
-                return $classServiceMap;
-            }
-            $services = $xml->services;
-            /** @psalm-suppress MixedAssignment */
-            if (count($services)) {
-                foreach ($services->service as $serviceObj) {
-                    if (isset($serviceObj) && $serviceObj instanceof \SimpleXMLElement) {
-                        $serviceAttributes = $serviceObj->attributes();
-                        if ($serviceAttributes && isset($serviceAttributes['id']) && isset($serviceAttributes['class'])) {
-                            $classServiceMap[(string) $serviceAttributes['id']] = (string) $serviceAttributes['class'];
-                        }
-                    }
-                }
-            }
-        }
-
-        return $classServiceMap;
-    }
 }

src/Handler/ContainerHandler.php

@@ -0,0 +1,39 @@
+<?php
+
+namespace Seferov\SymfonyPsalmPlugin\Handler;
+
+use PhpParser\Node\Expr;
+use PhpParser\Node\Expr\ClassConstFetch;
+use Psalm\Codebase;
+use Psalm\Context;
+use Psalm\Plugin\Hook\AfterMethodCallAnalysisInterface;
+use Psalm\StatementsSource;
+use Psalm\Type\Atomic\TNamedObject;
+use Psalm\Type\Union;
+
+class ContainerHandler implements AfterMethodCallAnalysisInterface
+{
+    /**
+     * {@inheritdoc}
+     */
+    public static function afterMethodCallAnalysis(
+        Expr $expr,
+        string $method_id,
+        string $appearing_method_id,
+        string $declaring_method_id,
+        Context $context,
+        StatementsSource $statements_source,
+        Codebase $codebase,
+        array &$file_replacements = [],
+        Union &$return_type_candidate = null
+    ) {
+        if (!in_array($declaring_method_id, ['Psr\Container\ContainerInterface::get', 'Symfony\Component\DependencyInjection\ContainerInterface::get'])) {
+            return;
+        }
+
+        if ($return_type_candidate && $expr->args[0]->value instanceof ClassConstFetch) {
+            $className = (string) $expr->args[0]->value->class->getAttribute('resolvedName');
+            $return_type_candidate = new Union([new TNamedObject($className)]);
+        }
+    }
+}

src/Handler/ContainerXmlHandler.php

@@ -0,0 +1,71 @@
+<?php
+
+namespace Seferov\SymfonyPsalmPlugin\Handler;
+
+use PhpParser\Node\Expr;
+use PhpParser\Node\Scalar\String_;
+use Psalm\Codebase;
+use Psalm\CodeLocation;
+use Psalm\Context;
+use Psalm\IssueBuffer;
+use Psalm\Plugin\Hook\AfterMethodCallAnalysisInterface;
+use Psalm\StatementsSource;
+use Psalm\Type\Atomic\TNamedObject;
+use Psalm\Type\Union;
+use Seferov\SymfonyPsalmPlugin\Issue\ServiceNotFound;
+use Seferov\SymfonyPsalmPlugin\SymfonyContainer;
+
+class ContainerXmlHandler implements AfterMethodCallAnalysisInterface
+{
+    /**
+     * {@inheritdoc}
+     */
+    public static function afterMethodCallAnalysis(
+        Expr $expr,
+        string $method_id,
+        string $appearing_method_id,
+        string $declaring_method_id,
+        Context $context,
+        StatementsSource $statements_source,
+        Codebase $codebase,
+        array &$file_replacements = [],
+        Union &$return_type_candidate = null
+    ) {
+        if (!in_array($declaring_method_id, ['Psr\Container\ContainerInterface::get', 'Symfony\Component\DependencyInjection\ContainerInterface::get'])) {
+            return;
+        }
+
+        if ($return_type_candidate && $expr->args[0]->value instanceof String_) {
+            $simpleXmlConfig = null;
+            if (count($codebase->config->getPluginClasses())) {
+                foreach ($codebase->config->getPluginClasses() as $pluginClass) {
+                    if ($pluginClass['class'] === str_replace('Handler', 'Plugin', __NAMESPACE__)) {
+                        $simpleXmlConfig = (string) $pluginClass['config'];
+                    }
+                }
+            }
+
+            if (!is_string($simpleXmlConfig)) {
+                throw new \LogicException('This hook is registered when xml file is set');
+            }
+
+            $symfonyContainer = new SymfonyContainer($simpleXmlConfig);
+            $serviceId = (string) $expr->args[0]->value->value;
+            $serviceDefinition = $symfonyContainer->get($serviceId);
+            if ($serviceDefinition) {
+                if ($serviceDefinition->isPublic()) {
+                    $class = $serviceDefinition->getClass();
+                    if ($class) {
+                        $return_type_candidate = new Union([new TNamedObject($class)]);
+                    }
+                }
+                // @todo: else emit "get private service" issue
+            } else {
+                IssueBuffer::accepts(
+                    new ServiceNotFound($serviceId, new CodeLocation($statements_source, $expr->args[0]->value)),
+                    $statements_source->getSuppressedIssues()
+                );
+            }
+        }
+    }
+}

src/Issue/ServiceNotFound.php

@@ -0,0 +1,14 @@
+<?php
+
+namespace Seferov\SymfonyPsalmPlugin\Issue;
+
+use Psalm\CodeLocation;
+use Psalm\Issue\CodeIssue;
+
+class ServiceNotFound extends CodeIssue
+{
+    public function __construct(string $id, CodeLocation $code_location)
+    {
+        parent::__construct(sprintf('Service "%s" not found', $id), $code_location);
+    }
+}

src/Plugin.php

@@ -2,18 +2,36 @@
 
 namespace Seferov\SymfonyPsalmPlugin;
 
+use Psalm\Exception\ConfigException;
 use Psalm\Plugin\PluginEntryPointInterface;
 use Psalm\Plugin\RegistrationInterface;
 use Seferov\SymfonyPsalmPlugin\Handler\ClassHandler;
+use Seferov\SymfonyPsalmPlugin\Handler\ContainerHandler;
+use Seferov\SymfonyPsalmPlugin\Handler\ContainerXmlHandler;
 use SimpleXMLElement;
 
 class Plugin implements PluginEntryPointInterface
 {
-    /** @return void */
+    /**
+     * {@inheritdoc}
+     */
     public function __invoke(RegistrationInterface $api, SimpleXMLElement $config = null)
     {
         require_once __DIR__.'/Handler/ClassHandler.php';
+        require_once __DIR__.'/Handler/ContainerHandler.php';
+        require_once __DIR__.'/Handler/ContainerXmlHandler.php';
 
         $api->registerHooksFromClass(ClassHandler::class);
+
+        if (isset($config->containerXml)) {
+            $containerXmlPath = realpath((string) $config->containerXml);
+            if (!$containerXmlPath) {
+                throw new ConfigException(sprintf('Container XML file (%s) does not exits', $containerXmlPath));
+            }
+
+            $api->registerHooksFromClass(ContainerXmlHandler::class);
+        } else {
+            $api->registerHooksFromClass(ContainerHandler::class);
+        }
     }
 }

src/SymfonyContainer.php

@@ -0,0 +1,43 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Seferov\SymfonyPsalmPlugin;
+
+use Psalm\Exception\ConfigException;
+use Symfony\Component\DependencyInjection\Definition;
+
+class SymfonyContainer
+{
+    /**
+     * @psalm-var array<string, Definition>
+     */
+    private $services = [];
+
+    public function __construct(string $containerXmlPath)
+    {
+        $xml = simplexml_load_file($containerXmlPath);
+        if (!$xml->services instanceof \SimpleXMLElement) {
+            throw new ConfigException('Not a valid container xml file');
+        }
+
+        /** @psalm-var \SimpleXMLElement $service */
+        foreach ($xml->services->service as $service) {
+            /** @psalm-var \SimpleXMLElement $serviceAttributes */
+            $serviceAttributes = $service->attributes();
+            $definition = new Definition((string) $serviceAttributes->class);
+            $definition->setPublic((bool) $serviceAttributes->public);
+
+            $this->services[(string) $serviceAttributes->id] = $definition;
+        }
+    }
+
+    public function get(string $id): ?Definition
+    {
+        if (isset($this->services[$id])) {
+            return $this->services[$id];
+        }
+
+        return null;
+    }
+}

tests/acceptance/acceptance/ContainerXml.feature

@@ -0,0 +1,59 @@
+Feature: Container XML config
+  Detect ContainerInterface::get() result type
+
+  Background:
+    Given I have the following config
+      """
+      <?xml version="1.0"?>
+      <psalm totallyTyped="true">
+        <projectFiles>
+          <directory name="."/>
+          <ignoreFiles> <directory name="../../vendor"/> </ignoreFiles>
+        </projectFiles>
+
+        <plugins>
+          <pluginClass class="Seferov\SymfonyPsalmPlugin\Plugin">
+            <containerXml>../../tests/acceptance/container.xml</containerXml>
+          </pluginClass>
+        </plugins>
+      </psalm>
+      """
+
+  Scenario: Asserting psalm recognizes return type of service got via 'ContainerInterface::get() using service ID'
+    Given I have the following code
+      """
+      <?php
+
+      class SomeController
+      {
+        use \Symfony\Component\DependencyInjection\ContainerAwareTrait;
+
+        public function index(): bool
+        {
+          return $this->container->get('service_container')->has('lorem');
+        }
+      }
+      """
+    When I run Psalm
+    Then I see no errors
+
+  # todo: emit a service not found error instead of PossiblyNullReference
+  Scenario: Psalm emits when service ID not found in container'
+    Given I have the following code
+      """
+      <?php
+
+      class SomeController
+      {
+        use \Symfony\Component\DependencyInjection\ContainerAwareTrait;
+
+        public function index(): void
+        {
+          $this->container->get('not_a_service')->has('lorem');
+        }
+      }
+      """
+    When I run Psalm
+    Then I see these errors
+      | Type            | Message                           |
+      | ServiceNotFound | Service "not_a_service" not found |

tests/acceptance/container.xml

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="utf-8"?>
+<container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd">
+    <parameters>
+        <parameter key="kernel.environment">dev</parameter>
+    </parameters>
+    <services>
+        <service id="service_container" class="Symfony\Component\DependencyInjection\ContainerInterface" public="true" synthetic="true"/>
+    </services>
+</container>