From cc89974d16ceda9f7e235e8a74f612e8d3ea44df Mon Sep 17 00:00:00 2001
From: Farhad Safarov <farhad.safarov@gmail.com>
Date: Sun, 14 Feb 2021 12:09:40 +0300
Subject: [PATCH] [doctrine] query builder explicit parameter type for
 performance (#144)

---
 .github/workflows/integrate.yaml              |  3 +-
 psalm.xml                                     |  1 -
 src/Handler/DoctrineQueryBuilderHandler.php   | 67 ++++++++++++++++++
 src/Issue/QueryBuilderSetParameter.php        | 17 +++++
 src/Plugin.php                                |  6 ++
 .../acceptance/DoctrineQueryBuilder.feature   | 68 +++++++++++++++++++
 6 files changed, 160 insertions(+), 2 deletions(-)
 create mode 100644 src/Handler/DoctrineQueryBuilderHandler.php
 create mode 100644 src/Issue/QueryBuilderSetParameter.php
 create mode 100644 tests/acceptance/acceptance/DoctrineQueryBuilder.feature

diff --git a/.github/workflows/integrate.yaml b/.github/workflows/integrate.yaml
index 7da83ba..99753b5 100644
--- a/.github/workflows/integrate.yaml
+++ b/.github/workflows/integrate.yaml
@@ -25,7 +25,7 @@ jobs:
         dependencies:
           - highest
           - lowest
-        
+
         exclude:
           - php-version: 7.3
             dependencies: lowest
@@ -77,6 +77,7 @@ jobs:
     runs-on: ubuntu-latest
 
     strategy:
+      fail-fast: false
       matrix:
         php-version:
           - 7.1
diff --git a/psalm.xml b/psalm.xml
index fb26475..29f4729 100644
--- a/psalm.xml
+++ b/psalm.xml
@@ -1,6 +1,5 @@
 <?xml version="1.0"?>
 <psalm
-    errorLevel="1"
     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"
diff --git a/src/Handler/DoctrineQueryBuilderHandler.php b/src/Handler/DoctrineQueryBuilderHandler.php
new file mode 100644
index 0000000..b1edefd
--- /dev/null
+++ b/src/Handler/DoctrineQueryBuilderHandler.php
@@ -0,0 +1,67 @@
+<?php
+
+namespace Psalm\SymfonyPsalmPlugin\Handler;
+
+use PhpParser\Node\Expr;
+use Psalm\Codebase;
+use Psalm\CodeLocation;
+use Psalm\Context;
+use Psalm\IssueBuffer;
+use Psalm\Plugin\Hook\AfterMethodCallAnalysisInterface;
+use Psalm\StatementsSource;
+use Psalm\SymfonyPsalmPlugin\Issue\QueryBuilderSetParameter;
+use Psalm\Type\Union;
+
+class DoctrineQueryBuilderHandler 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
+    ): void {
+        if ('Doctrine\ORM\QueryBuilder::setparameter' === $declaring_method_id) {
+            if (isset($expr->args[2])) {
+                return;
+            }
+
+            $value = $expr->args[1]->value;
+
+            if (self::isValueObject($value, $context)) {
+                IssueBuffer::accepts(
+                    new QueryBuilderSetParameter(new CodeLocation($statements_source, $value)),
+                    $statements_source->getSuppressedIssues()
+                );
+            }
+        }
+    }
+
+    private static function isValueObject(Expr $value, Context $context): bool
+    {
+        if ($value instanceof Expr\Variable) {
+            $varName = $value->name;
+            if (is_string($varName)) {
+                $varName = '$'.$varName;
+                if ($context->hasVariable($varName)) {
+                    $type = $context->vars_in_scope[$varName];
+
+                    return $type->hasObjectType();
+                }
+            }
+        }
+
+        if ($value instanceof Expr\New_) {
+            return true;
+        }
+
+        return false;
+    }
+}
diff --git a/src/Issue/QueryBuilderSetParameter.php b/src/Issue/QueryBuilderSetParameter.php
new file mode 100644
index 0000000..264d799
--- /dev/null
+++ b/src/Issue/QueryBuilderSetParameter.php
@@ -0,0 +1,17 @@
+<?php
+
+namespace Psalm\SymfonyPsalmPlugin\Issue;
+
+use Psalm\CodeLocation;
+use Psalm\Issue\PluginIssue;
+
+/**
+ * @see https://www.doctrine-project.org/projects/doctrine-orm/en/2.8/reference/query-builder.html#binding-parameters-to-your-query
+ */
+class QueryBuilderSetParameter extends PluginIssue
+{
+    public function __construct(CodeLocation $code_location)
+    {
+        parent::__construct('To improve performance set explicit type for objects', $code_location);
+    }
+}
diff --git a/src/Plugin.php b/src/Plugin.php
index 978c346..16c62bd 100644
--- a/src/Plugin.php
+++ b/src/Plugin.php
@@ -10,6 +10,7 @@ use Psalm\SymfonyPsalmPlugin\Handler\AnnotationHandler;
 use Psalm\SymfonyPsalmPlugin\Handler\ConsoleHandler;
 use Psalm\SymfonyPsalmPlugin\Handler\ContainerDependencyHandler;
 use Psalm\SymfonyPsalmPlugin\Handler\ContainerHandler;
+use Psalm\SymfonyPsalmPlugin\Handler\DoctrineQueryBuilderHandler;
 use Psalm\SymfonyPsalmPlugin\Handler\DoctrineRepositoryHandler;
 use Psalm\SymfonyPsalmPlugin\Handler\HeaderBagHandler;
 use Psalm\SymfonyPsalmPlugin\Handler\RequiredSetterHandler;
@@ -56,12 +57,17 @@ class Plugin implements PluginEntryPointInterface
         require_once __DIR__.'/Handler/ConsoleHandler.php';
         require_once __DIR__.'/Handler/ContainerDependencyHandler.php';
         require_once __DIR__.'/Handler/RequiredSetterHandler.php';
+        require_once __DIR__.'/Handler/DoctrineQueryBuilderHandler.php';
 
         $api->registerHooksFromClass(HeaderBagHandler::class);
         $api->registerHooksFromClass(ConsoleHandler::class);
         $api->registerHooksFromClass(ContainerDependencyHandler::class);
         $api->registerHooksFromClass(RequiredSetterHandler::class);
 
+        if (class_exists(\Doctrine\ORM\QueryBuilder::class)) {
+            $api->registerHooksFromClass(DoctrineQueryBuilderHandler::class);
+        }
+
         if (class_exists(AnnotationRegistry::class)) {
             require_once __DIR__.'/Handler/DoctrineRepositoryHandler.php';
             /** @psalm-suppress DeprecatedMethod */
diff --git a/tests/acceptance/acceptance/DoctrineQueryBuilder.feature b/tests/acceptance/acceptance/DoctrineQueryBuilder.feature
new file mode 100644
index 0000000..590f0c2
--- /dev/null
+++ b/tests/acceptance/acceptance/DoctrineQueryBuilder.feature
@@ -0,0 +1,68 @@
+@symfony-common
+Feature: Doctrine QueryBuilder
+
+  Background:
+    Given I have the following config
+      """
+      <?xml version="1.0"?>
+      <psalm>
+        <projectFiles>
+          <directory name="."/>
+          <ignoreFiles> <directory name="../../vendor"/> </ignoreFiles>
+        </projectFiles>
+
+        <plugins>
+          <pluginClass class="Psalm\SymfonyPsalmPlugin\Plugin"/>
+        </plugins>
+      </psalm>
+      """
+    And I have the following code preamble
+      """
+      <?php
+
+      use Doctrine\ORM\QueryBuilder;
+
+      /**
+       * @psalm-suppress InvalidReturnType
+       * @return QueryBuilder
+       */
+      function qb() {}
+      """
+
+  Scenario: No complaint about explicit type for non-objects
+    Given I have the following code
+      """
+      $qb = qb();
+      $qb->setParameter('string', 'string');
+      $qb->setParameter('integer', 1);
+      $qb->setParameter('bool', true);
+
+      $string = 'string';
+      $int = 1;
+      $bool = false;
+      $qb->setParameter('string', $string);
+      $qb->setParameter('integer', $int);
+      $qb->setParameter('bool', $bool);
+      """
+    When I run Psalm
+    Then I see no errors
+
+  Scenario: Complaint about not setting explicit type for objects
+    Given I have the following code
+      """
+      $qb = qb();
+
+      $qb->setParameter('date', new \DateTimeImmutable());
+
+      $date = new \DateTimeImmutable();
+      $qb->setParameter('date', $date);
+
+      $qb->setParameter('qb', $qb);
+      """
+    When I run Psalm
+    Then I see these errors
+      | Type                       | Message                                                |
+      | QueryBuilderSetParameter   | To improve performance set explicit type for objects   |
+      | QueryBuilderSetParameter   | To improve performance set explicit type for objects   |
+      | QueryBuilderSetParameter   | To improve performance set explicit type for objects   |
+    And I see no other errors