Add rule for detecting expensive calls on a Collection

extension.neon

@@ -33,6 +33,18 @@ parameters:
     bootstrap: %rootDir%/../../nunomaduro/larastan/bootstrap.php
     reportUnmatchedIgnoredErrors: false
     checkGenericClassInNonGenericObjectType: false
+    noUnnecessaryCollectionCall: true
+    noUnnecessaryCollectionCallOnly: []
+    noUnnecessaryCollectionCallExcept: []
+
+parametersSchema:
+    noUnnecessaryCollectionCall: bool()
+    noUnnecessaryCollectionCallOnly: listOf(string())
+    noUnnecessaryCollectionCallExcept: listOf(string())
+
+conditionalTags:
+    NunoMaduro\Larastan\Rules\NoUnnecessaryCollectionCallRule:
+    		phpstan.rules.rule: %noUnnecessaryCollectionCall%
 
 services:
     -
@@ -194,6 +206,12 @@ services:
         tags:
             - phpstan.phpDoc.typeNodeResolverExtension
 
+    -
+        class: NunoMaduro\Larastan\Rules\NoUnnecessaryCollectionCallRule
+        arguments:
+            onlyMethods: %noUnnecessaryCollectionCallOnly%
+            excludeMethods: %noUnnecessaryCollectionCallExcept%
+
     -
         class: NunoMaduro\Larastan\Types\GenericEloquentBuilderTypeNodeResolverExtension
         tags:

src/Rules/NoUnnecessaryCollectionCallRule.php

@@ -0,0 +1,324 @@
+<?php
+
+declare(strict_types=1);
+
+namespace NunoMaduro\Larastan\Rules;
+
+use Illuminate\Database\Eloquent\Builder as EloquentBuilder;
+use Illuminate\Database\Eloquent\Relations\Relation;
+use Illuminate\Database\Query\Builder as QueryBuilder;
+use Illuminate\Database\Eloquent\Model;
+use Illuminate\Support\Collection;
+use Illuminate\Support\Str;
+use NunoMaduro\Larastan\Properties\ModelPropertyExtension;
+use PhpParser\Node;
+use PhpParser\Node\Expr\MethodCall;
+use PhpParser\Node\Identifier;
+use PHPStan\Analyser\Scope;
+use PHPStan\Reflection\ReflectionProvider;
+use PHPStan\Rules\Rule;
+use PHPStan\Type\MixedType;
+use PHPStan\Type\ObjectType;
+use PHPStan\Type\Type;
+
+/**
+ * This rule checks for unnecessary heavy operations on the Collection class
+ * that could have instead been performed on the Builder class.
+ *
+ * For example:
+ * User::all()->count()
+ * could be simplified to:
+ * User::count()
+ *
+ * In addition, this code:
+ * User::whereStatus('active')->get()->pluck('id')
+ * could be simplified to:
+ * User::whereStatus('active')->pluck('id')
+ */
+class NoUnnecessaryCollectionCallRule implements Rule
+{
+    /**
+     * The method names that can be applied on a Collection, but should be applied on a Builder.
+     * @var string[]
+     */
+    protected const RISKY_METHODS = [
+        'count',
+        'isempty',
+        'isnotempty',
+        'last',
+        'pop',
+        'skip',
+        'slice',
+        'take',
+        // Eloquent collection
+        'diff',
+        'except',
+        'find',
+        'modelkeys',
+        'only',
+    ];
+
+    /**
+     * The method names that can be applied on a Collection, but should in some cases be applied
+     * on a builder depending on what parameter is passed to the method.
+     * @var string[]
+     */
+    protected const RISKY_PARAM_METHODS = [
+        'average',
+        'avg',
+        'firstwhere',
+        'max',
+        'min',
+        'pluck',
+        'sum',
+        'where',
+        'wherestrict',
+    ];
+
+    /** @var string[] The method names that should be checked by this rule - can be configured by the user. */
+    protected $shouldHandle;
+
+    /**
+     * @var \PHPStan\Reflection\ReflectionProvider
+     */
+    protected $reflectionProvider;
+
+    /**
+     * @var \NunoMaduro\Larastan\Properties\ModelPropertyExtension
+     */
+    protected $propertyExtension;
+
+    /**
+     * NoRedundantCollectionCallRule constructor.
+     * @param ReflectionProvider $reflectionProvider
+     * @param ModelPropertyExtension $propertyExtension
+     * @param string[] $onlyMethods
+     * @param string[] $excludeMethods
+     */
+    public function __construct(
+        ReflectionProvider $reflectionProvider,
+        ModelPropertyExtension $propertyExtension,
+        array $onlyMethods,
+        array $excludeMethods
+    )
+    {
+        $this->reflectionProvider = $reflectionProvider;
+        $this->propertyExtension = $propertyExtension;
+        $allMethods = array_merge(
+            static::RISKY_METHODS,
+            static::RISKY_PARAM_METHODS,
+            [
+                'first',
+                'contains',
+                'containsstrict',
+            ]
+        );
+
+        if (! empty($onlyMethods)) {
+            $this->shouldHandle = $onlyMethods;
+        } else {
+            $this->shouldHandle = $allMethods;
+        }
+
+        if (! empty($excludeMethods)) {
+            $this->shouldHandle = array_diff($this->shouldHandle, $excludeMethods);
+        }
+        $this->shouldHandle = array_map(function (string $methodName): string {
+            return Str::lower($methodName);
+        }, $this->shouldHandle);
+    }
+
+    /**
+     * @return string
+     */
+    public function getNodeType(): string
+    {
+        return MethodCall::class;
+    }
+
+    /**
+     * @param Node $node
+     * @param Scope $scope
+     * @return string[]
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        /** @var \PhpParser\Node\Expr\MethodCall $node */
+        if (! $node->name instanceof Identifier) {
+            return [];
+        }
+
+        /** @var \PhpParser\Node\Identifier $name */
+        $name = $node->name;
+
+        if ($this->isNotCalledOnCollection($node->var, $scope)) {
+            // Method was called not called on a collection, so no errors.
+            return [];
+        }
+
+        $previousCall = $node->var;
+
+        if (! $this->callIsQuery($previousCall, $scope)) {
+            // Previous call wasn't on a Builder, so no errors.
+            return [];
+        }
+
+        /** @var Node\Expr\MethodCall|Node\Expr\StaticCall $previousCall */
+        if (! ($previousCall->name instanceof Identifier)) {
+            // Previous call was made dynamically e.g. User::query()->{$method}()
+            // Can't really analyze it in this scenario so no errors.
+            return [];
+        }
+
+        if (! in_array($name->toLowerString(), $this->shouldHandle, true)) {
+            return [];
+        }
+
+        if ($name->toLowerString() === 'first') {
+            if (count($node->args) === 0) {
+                // 'first', also accepts a closure as an argument.
+                return [$this->formatError($name->toString())];
+            }
+        } elseif ($this->isRiskyMethod($name)) {
+            return [$this->formatError($name->toString())];
+        } elseif ($this->isRiskyParamMethod($name)) {
+            if (count($node->args) === 0) {
+                // Calling e.g. DB::table()->pluck('age')-sum()
+                return [$this->formatError($name->toString())];
+            }
+            if ($this->firstArgIsDatabaseColumn($node, $scope)) {
+                return [$this->formatError($name->toString())];
+            }
+        } elseif ($name->toLowerString() === 'contains' || $name->toLowerString() === 'containsstrict') {
+            // 'contains' can also be called with Model instances or keys as its first argument
+            /** @var \PhpParser\Node\Arg[] $args */
+            $args = $node->args;
+            if (count($args) === 1 && ! ($args[0]->value instanceof Node\Expr\Closure)) {
+                return [$this->formatError($name->toString())];
+            } elseif ($this->firstArgIsDatabaseColumn($node, $scope)) {
+                return [$this->formatError($name->toString())];
+            }
+        }
+
+        return [];
+    }
+
+    /**
+     * Determines whether the first argument is a string and references a database column.
+     * @param MethodCall $node
+     * @param Scope $scope
+     * @return bool
+     */
+    protected function firstArgIsDatabaseColumn(MethodCall $node, Scope $scope): bool
+    {
+        /** @var \PhpParser\Node\Arg[] $args */
+        $args = $node->args;
+        if (count($args) === 0 || ! ($args[0]->value instanceof Node\Scalar\String_)) {
+            return false;
+        }
+
+        $iterableType = $scope->getType($node->var)->getIterableValueType();
+        if ($iterableType instanceof MixedType) {
+            $previous_call = $node->var;
+            if ($previous_call instanceof MethodCall) {
+                $query_builder_type = $scope->getType($previous_call->var);
+                if ((new ObjectType(QueryBuilder::class))->isSuperTypeOf($query_builder_type)->yes()) {
+                    // We encountered a DB query such as DB::table(..)->get()->max('id')
+                    // We assume max('id') could have been retrieved without calling get().
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
+        /** @var \PHPStan\Type\ObjectType $iterableType */
+        if ((new ObjectType(Model::class))->isSuperTypeOf($iterableType)->yes()) {
+            $modelReflection = $this->reflectionProvider->getClass($iterableType->getClassName());
+
+            /** @var \PhpParser\Node\Scalar\String_ $firstArg */
+            $firstArg = $args[0]->value;
+
+            return $this->propertyExtension->hasProperty($modelReflection, $firstArg->value);
+        }
+        return false;
+    }
+
+    /**
+     * Returns whether the method call is a call on a builder instance.
+     * @param Node\Expr $call
+     * @param Scope $scope
+     * @return bool
+     */
+    protected function callIsQuery(Node\Expr $call, Scope $scope): bool
+    {
+        if ($call instanceof MethodCall) {
+            $calledOn = $scope->getType($call->var);
+
+            return $this->isBuilder($calledOn);
+        } else if ($call instanceof Node\Expr\StaticCall) {
+            /** @var Node\Name $class */
+            $class = $call->class;
+            if ($class instanceof Node\Name) {
+                $modelClassName = $class->toCodeString();
+                return is_subclass_of($modelClassName, Model::class);
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * Returns whether the method is one of the risky methods.
+     * @param Identifier $name
+     * @return bool
+     */
+    protected function isRiskyMethod(Identifier $name): bool
+    {
+        return in_array($name->toLowerString(), self::RISKY_METHODS, true);
+    }
+
+    /**
+     * Returns whether the method might be a risky method depending on the parameters passed.
+     * @param Identifier $name
+     * @return bool
+     */
+    protected function isRiskyParamMethod(Identifier $name): bool
+    {
+        return in_array($name->toLowerString(), self::RISKY_PARAM_METHODS, true);
+    }
+
+    /**
+     * Returns whether its argument is some builder instance.
+     * @param Type $type
+     * @return bool
+     */
+    protected function isBuilder(Type $type): bool
+    {
+        return ! (new ObjectType(EloquentBuilder::class))->isSuperTypeOf($type)->no()
+            || ! (new ObjectType(QueryBuilder::class))->isSuperTypeOf($type)->no()
+            || ! (new ObjectType(Relation::class))->isSuperTypeOf($type)->no();
+    }
+
+    /**
+     * Returns whether the Expr was not called on a Collection instance.
+     * @param Node\Expr $expr
+     * @param Scope $scope
+     * @return bool
+     */
+    protected function isNotCalledOnCollection(Node\Expr $expr, Scope $scope): bool
+    {
+        $calledOnType = $scope->getType($expr);
+        return ! (new ObjectType(Collection::class))->isSuperTypeOf($calledOnType)->yes();
+    }
+
+    /**
+     * Formats the error.
+     * @param string $method_name
+     * @return string
+     */
+    protected function formatError(string $method_name): string
+    {
+        return "Called '{$method_name}' on collection, but could have been retrieved as a query.";
+    }
+}

tests/Application/app/User.php

@@ -10,6 +10,7 @@
 use Illuminate\Database\Eloquent\Relations\MorphTo;
 use Illuminate\Foundation\Auth\User as Authenticatable;
 use Illuminate\Notifications\Notifiable;
+use Illuminate\Support\Str;
 use Tests\Application\HasManySyncable;
 
 /**
@@ -45,6 +46,11 @@ public function id(): int
         return $this->id;
     }
 
+    public function getAllCapsName(): string
+    {
+        return Str::upper($this->name);
+    }
+
     public function scopeActive(Builder $query): Builder
     {
         return $query->where('active', 1);

tests/Features/Methods/ModelExtension.php

@@ -21,11 +21,6 @@ public function testAll()
         return User::all();
     }
 
-    public function testAllFirst(): ?User
-    {
-        return User::all()->first();
-    }
-
     public function testReturnThis(): Builder
     {
         $user = User::join('tickets.tickets', 'tickets.tickets.id', '=', 'tickets.sale_ticket.ticket_id')
@@ -237,7 +232,7 @@ public function scopeValid(Builder $query): Builder
 
     public static function testFindOnStaticSelf(): ?Thread
     {
-        return self::query()->where('foo', 'bar')->get()->first();
+        return self::query()->where('foo', 'bar')->first();
     }
 
     public function getCustomPropertyAttribute(): string