[psr-0001] fix(rules): ForbidEloquentMutationInControllersRule misses method-local receivers

CHANGELOG.md

@@ -8,6 +8,7 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and
 
 ### Changed
 
+- `ForbidEloquentMutationInControllersRule` — registration moved off `Class_` onto `CallLike` (per-node), closing a receiver-type blind spot. The rule previously registered on `Class_` and manually walked every method body, resolving each receiver via `$scope->getType($node->var)` against the **class-entry** scope — a scope carrying no flow-derived knowledge of method-local variables, so any receiver born inside the method body (`$m = new Model; $m->save();`; `$m = Model::query()->…->firstOrFail(); $m->delete();`; a `Builder` held in a local var) resolved to `mixed` and **silently never fired** — only receivers typed from a method signature (typed parameters) matched. Empirically proven on tc-api PR #133 (PHPStan-baseline entries for exactly these shapes came back "unmatched" — the rule never emitted them). Now registers on `CallLike` and branches `MethodCall` / `StaticCall` (mirrors `EnforceCurrentUserAttributeRule` / `LogRule`), so PHPStan supplies a method-level flow scope and local-variable receivers resolve to their real Model / Builder types. The namespace gate (`controllerNamespacePrefixes`, unchanged), the `checkInstanceCall` / `checkStaticCall` type-matching, the blocklist, the error identifier `forbidEloquentMutationInControllers.eloquentMutationInController`, and the message format are all preserved byte-for-byte; the containing-controller FQCN for the message now comes from `$scope->getClassReflection()?->getName()` at the call site (the manual `Class_`-scope `resolveClassFqcn` + `walkNodes` helpers are deleted). The static-call path (`Model::destroy($id)`) is unaffected — it never depended on flow scope. Every pre-existing fixture still fires at the same line; new positive fixtures cover the three local-receiver shapes and a new compliant fixture pins that a local var of a NON-Model class stays clean (the type gate still discriminates under flow scope). Nullsafe `$m?->delete()` calls are covered without a dedicated branch: PHPStan's `NodeScopeResolver` emits a synthetic non-null-narrowed `MethodCall` node (attribute `virtualNullsafeMethodCall`) for every `?->` call, so the plain `MethodCall` branch already fires once — an explicit `NullsafeMethodCall` branch double-reports (the real node plus its synthetic twin). `TypeCombinator::removeNull()` is applied to the receiver before the type gate for the DISTINCT plain-call-on-nullable shape (a plain `->delete()` on a `?Post` from `->first()`: `Post|null` is only a `maybe()` Model supertype, never `yes()`, so without the strip it never fires; a nullable Model receiver carries the same audit-bypass risk). Per-node registration additionally reaches trait bodies analysed through a using class: a mutation in a trait declared under a controllers namespace (e.g. `App\Http\Controllers\Concerns\*`) now fires, message naming the using class — intended coverage the old `Class_` walk structurally never had (a trait file has no `Class_` node). These are pinned by new fixtures (`ViolationNullsafeDelete` — single-fire, no double-report; `ViolationPlainNullableDelete` — the `removeNull` path; `ViolationInTraitFile`). **Versioning: candidate MAJOR per ADR-0021 §Versioning** — surfaces new errors in previously-clean consumer code (known: tc-api `EducationController::store` / `destroy`; the nullsafe + trait paths are additional new-error surface under the same candidacy); per the pre-1.0 caret convention `^0.7` excludes the next minor, so tagging auto-adopts nobody — each consumer runs a pre-cascade audit and remediates on its own pin-bump PR. Seed: tc-api PR #133 baseline-unmatched finding.
 - `EnforceResourceDataValidatorOptInRule` + `EnforceFormRequestToDtoRule` + `EnforceAuditModelProtectionsRule` — migrated the internal inheritance gate off the deprecated `ClassReflection::isSubclassOf(string)` API onto `isSubclassOfClass(ClassReflection)` (war-room enforcement queue #112). `isSubclassOf(string)` is `@deprecated Use isSubclassOfClass instead.` in PHPStan 2.2+ and **removed in PHPStan 3.x** — a latent break for every consumer's static analysis the day this package (the fleet's canonical static-analysis backbone) targets PHPStan 3. Each of the three rules now injects `PHPStan\Reflection\ReflectionProvider` (constructor DI, autowired by the PHPStan/Nette container — no `extension.neon` argument wiring needed, proven by each rule's container-resolution test) and resolves its configured/known base FQCN via `reflectionProvider->hasClass()/getClass()` before calling `isSubclassOfClass()`. **Behaviour is byte-for-byte preserved:** the migration inlines the deprecated method's own body (`if (!hasClass($fqcn)) return false; return isSubclassOfClass(getClass($fqcn));`), so the load-bearing unknown-base-class no-op — a tree lacking the configured base class (or `Illuminate\Database\Eloquent\Model` for the audit rule) silently does not fire, the "consumers analysing non-Laravel trees are unaffected" guarantee — is reproduced exactly. Pinned by a new base-class-absent no-op test per rule (a configured base FQCN absent from the analysed tree ⇒ zero errors); all existing positive tests stay green. **Versioning: PATCH per ADR-0021 §Versioning** (internal API migration / future-proofing — adds no errors, removes none, no consumer-visible behaviour change). Seed: war-room enforcement queue #112.
 
 ## [0.8.0] — 2026-07-13

src/Rules/ForbidEloquentMutationInControllersRule.php

@@ -7,22 +7,20 @@
 use Illuminate\Database\Eloquent\Builder as EloquentBuilder;
 use Illuminate\Database\Eloquent\Model;
 use PhpParser\Node;
+use PhpParser\Node\Expr\CallLike;
 use PhpParser\Node\Expr\MethodCall;
 use PhpParser\Node\Expr\StaticCall;
 use PhpParser\Node\Identifier;
 use PhpParser\Node\Name;
-use PhpParser\Node\Stmt\Class_;
-use PhpParser\Node\Stmt\ClassMethod;
 use PHPStan\Analyser\Scope;
 use PHPStan\Rules\IdentifierRuleError;
 use PHPStan\Rules\Rule;
 use PHPStan\Rules\RuleErrorBuilder;
 use PHPStan\Type\ObjectType;
 use PHPStan\Type\Type;
+use PHPStan\Type\TypeCombinator;
 
-use function array_filter;
 use function in_array;
-use function is_array;
 use function mb_strrpos;
 use function mb_substr;
 use function sprintf;
@@ -66,22 +64,62 @@
  *      opts them in by adding the prefix to `controllerNamespacePrefixes` in
  *      its `phpstan.neon` — mirrors the `formRequestBaseClass` /
  *      `resourceDataBaseClass` parameter precedent.
- *   2. For every `ClassMethod` in the class node, recursively walk the method
- *      body collecting `MethodCall` and `StaticCall` nodes.
- *   3. **MethodCall:** resolve the receiver expression's type via
- *      `$scope->getType($node->var)`. Fire if the type is a subtype of
+ *   2. **MethodCall:** resolve the receiver expression's type via
+ *      `$scope->getType($node->var)`, then strip null via
+ *      `TypeCombinator::removeNull()`. Fire if the result is a subtype of
  *      `Illuminate\Database\Eloquent\Model` OR a subtype of
  *      `Illuminate\Database\Eloquent\Builder` (the generic parameter is not
  *      unwrapped — `ObjectType::isSuperTypeOf()` handles `Builder<User>` as a
  *      subtype of the unparameterized `Builder` cleanly without brittle
- *      generic introspection). Method name must be in the blocklist.
- *   4. **StaticCall:** resolve the class name via `$scope->resolveName()`. Fire
+ *      generic introspection). Method name must be in the blocklist. The
+ *      null-strip is load-bearing for a plain `->delete()` on a nullable
+ *      `?Model` receiver: `Post|null` is only a `maybe()` supertype of `Model`,
+ *      never `yes()`, so without it that shape never fires. Nullsafe `?->`
+ *      calls are covered here too — see the implementation note below.
+ *   3. **StaticCall:** resolve the class name via `$scope->resolveName()`. Fire
  *      if the FQCN is a Model subclass and the method name is in the blocklist.
  *      The class-name resolution path covers `User::create([...])`,
  *      `User::destroy($id)`, `User::updateOrCreate(...)`. Class-name expressions
  *      that are not a literal `Name` node (`$class::create(...)`) are out of
  *      scope.
  *
+ * Implementation note: `getNodeType()` returns `CallLike::class` so PHPStan
+ * hands each call node a method-level flow scope — mirrors `LogRule` /
+ * `EnforceCurrentUserAttributeRule`. An earlier revision registered on `Class_`
+ * and walked each method body manually, resolving receiver types against the
+ * CLASS-entry scope; that scope carries no flow-derived knowledge of
+ * method-local variables, so receivers born inside the body
+ * (`$m = new Model; $m->save();`, `$m = Model::where(...)->firstOrFail(); $m->delete();`,
+ * a `Builder` held in a local var) resolved to `mixed` and NEVER fired — only
+ * receivers typed from a method signature (typed parameters) matched. Per-node
+ * registration closes that blind spot: PHPStan supplies the flow scope, so
+ * local-variable receivers resolve to their real Model / Builder types. The
+ * `CallLike` registration hands the rule `MethodCall` and `StaticCall` nodes
+ * (plus `New_` / `FuncCall`, which fall through to no-op). The containing-
+ * controller FQCN for the message comes from
+ * `$scope->getClassReflection()?->getName()` at the call site (non-null once the
+ * namespace gate has passed, since the gate implies an in-class scope).
+ *
+ * Nullsafe calls: `$m?->delete()` is NOT matched via a `NullsafeMethodCall`
+ * branch. PHPStan's `NodeScopeResolver` emits, for every `?->` call, a synthetic
+ * `MethodCall` node (attribute `virtualNullsafeMethodCall === true`) analysed
+ * under the non-null assumption, so the plain `MethodCall` branch already fires
+ * once on nullsafe calls with the receiver already null-narrowed. Registering a
+ * SEPARATE `NullsafeMethodCall` branch double-reports (CI-proven: the real
+ * `NullsafeMethodCall` node AND its synthetic `MethodCall` twin both fire) — so
+ * there is deliberately no such branch. `removeNull` therefore earns its keep on
+ * the DISTINCT plain-call-on-nullable shape (`$m = ...->first(); $m->delete();`
+ * where `$m` is `?Model`), not the nullsafe form.
+ *
+ * Trait coverage: per-node registration also reaches trait bodies — a mutation
+ * inside a trait declared in a controllers namespace (e.g.
+ * `App\Http\Controllers\Concerns\*`) fires, and the message names the USING
+ * class, because PHPStan analyses a trait through each using class and the
+ * call-site `$scope->getClassReflection()` resolves to that using class (the
+ * namespace gate reads the file's namespace). The old `Class_`-scope walk
+ * structurally never ran on a trait file (a trait file has no `Class_` node), so
+ * this is new-but-intended coverage.
+ *
  * Method-name blocklist — full ADR-0011 + ADR-0019 mutation surface (24 entries):
  *
  *   save, saveOrFail, saveQuietly,
@@ -109,7 +147,7 @@
  *   - Dynamic method names (`$method = 'save'; $model->{$method}()`) — would
  *     need value-flow analysis. Acceptable miss; rely on reviewer.
  *
- * @implements Rule<Class_>
+ * @implements Rule<CallLike>
  */
 final class ForbidEloquentMutationInControllersRule implements Rule
 {
@@ -151,7 +189,7 @@ public function __construct(
 
     public function getNodeType(): string
     {
-        return Class_::class;
+        return CallLike::class;
     }
 
     public function processNode(Node $node, Scope $scope): array
@@ -162,19 +200,31 @@ public function processNode(Node $node, Scope $scope): array
             return [];
         }
 
-        $classFqcn = $this->resolveClassFqcn($node, $namespace);
         $modelType = new ObjectType(Model::class);
         $builderType = new ObjectType(EloquentBuilder::class);
 
-        $errors = [];
+        if ($node instanceof MethodCall) {
+            $violation = $this->checkInstanceCall($node, $scope, $modelType, $builderType);
+        } elseif ($node instanceof StaticCall) {
+            $violation = $this->checkStaticCall($node, $scope, $modelType);
+        } else {
+            return [];
+        }
 
-        foreach ($node->getMethods() as $method) {
-            foreach ($this->collectViolations($method, $scope, $modelType, $builderType) as $violation) {
-                $errors[] = $this->buildError($classFqcn, $violation);
-            }
+        if ($violation === null) {
+            return [];
+        }
+
+        // At a call-site scope the class reflection resolves (the namespace gate
+        // already implies an in-class scope). Guard defensively — a call outside
+        // any class yields no controller FQCN, so there is nothing to report.
+        $classReflection = $scope->getClassReflection();
+
+        if ($classReflection === null) {
+            return [];
         }
 
-        return $errors;
+        return [$this->buildError($classReflection->getName(), $violation)];
     }
 
     /**
@@ -193,53 +243,21 @@ private function namespaceIsController(string $namespace): bool
         return false;
     }
 
-    /**
-     * @return list<array{type: string, method: string, node: MethodCall|StaticCall}>
-     */
-    private function collectViolations(
-        ClassMethod $method,
-        Scope $scope,
-        ObjectType $modelType,
-        ObjectType $builderType,
-    ): array {
-        $violations = [];
-
-        if ($method->stmts === null) {
-            return $violations;
-        }
-
-        $this->walkNodes(
-            $method->stmts,
-            function(Node $node) use (&$violations, $scope, $modelType, $builderType): void {
-                if ($node instanceof MethodCall) {
-                    $violation = $this->checkInstanceCall($node, $scope, $modelType, $builderType);
-
-                    if ($violation !== null) {
-                        $violations[] = $violation;
-                    }
-
-                    return;
-                }
-
-                if ($node instanceof StaticCall) {
-                    $violation = $this->checkStaticCall($node, $scope, $modelType);
-
-                    if ($violation !== null) {
-                        $violations[] = $violation;
-                    }
-                }
-            },
-        );
-
-        return $violations;
-    }
-
     /**
      * Match `$model->mutation(...)` or `$builder->mutation(...)`. Receiver type
      * must be a subtype of `Illuminate\Database\Eloquent\Model` OR
      * `Illuminate\Database\Eloquent\Builder`; method name must be in the
      * blocklist.
      *
+     * Null is stripped from the receiver type before the gate:
+     * `ObjectType::isSuperTypeOf(Post|null)` is `maybe()`, not `yes()`, so a
+     * plain `->delete()` on a nullable `?Post` receiver would otherwise slip
+     * through. A nullable Model receiver carries the same audit-bypass risk.
+     * (The nullsafe `$m?->delete()` form is handled separately — PHPStan feeds a
+     * synthetic non-null-narrowed `MethodCall` node for it, see the class
+     * docblock — so `removeNull`'s live surface is the plain-call-on-nullable
+     * shape.)
+     *
      * @return array{type: string, method: string, node: MethodCall}|null
      */
     private function checkInstanceCall(
@@ -258,7 +276,7 @@ private function checkInstanceCall(
             return null;
         }
 
-        $receiverType = $scope->getType($node->var);
+        $receiverType = TypeCombinator::removeNull($scope->getType($node->var));
 
         $receiverFqcn = $this->matchedReceiverFqcn($receiverType, $modelType, $builderType);
 
@@ -343,20 +361,6 @@ private function buildError(string $classFqcn, array $violation): IdentifierRule
             ->build();
     }
 
-    /**
-     * Resolve the fully-qualified class name from the AST node + namespace.
-     * Avoids depending on `$scope->getClassReflection()`, which can return
-     * null during fixture-mode analysis where the class isn't autoloadable.
-     */
-    private function resolveClassFqcn(Class_ $node, string $namespace): string
-    {
-        if ($node->name === null) {
-            return $namespace;
-        }
-
-        return $namespace . '\\' . $node->name->toString();
-    }
-
     private function shortName(string $fqcn): string
     {
         $pos = mb_strrpos($fqcn, '\\');
@@ -367,39 +371,4 @@ private function shortName(string $fqcn): string
 
         return mb_substr($fqcn, $pos + 1);
     }
-
-    /**
-     * Recursively walk a list of nodes, invoking `$callback` on each one.
-     * Mirrors `EnforceActionTransactionsRule::walkNodes()` /
-     * `EnforceAuditSnapshotOnRetryRule::walkNodes()` /
-     * `EnforceAuditTransactionScopeRule::walkNodes()` /
-     * `EnforceResourceDataValidatorOptInRule::walkNodes()` for parity —
-     * re-evaluate at v1.0 once the four-way duplication trigger is acted
-     * on (Standing Concern #29).
-     *
-     * @param array<int|string, Node|null> $nodes
-     */
-    private function walkNodes(array $nodes, callable $callback): void
-    {
-        foreach ($nodes as $node) {
-            if (!$node instanceof Node) {
-                continue;
-            }
-
-            $callback($node);
-
-            foreach ($node->getSubNodeNames() as $name) {
-                $subNode = $node->{$name};
-
-                if ($subNode instanceof Node) {
-                    $this->walkNodes([$subNode], $callback);
-                } elseif (is_array($subNode)) {
-                    $this->walkNodes(
-                        array_filter($subNode, static fn(mixed $item): bool => $item instanceof Node),
-                        $callback,
-                    );
-                }
-            }
-        }
-    }
 }

tests/Fixtures/ForbidEloquentMutationInControllers/CompliantLocalVarNonModel.php

@@ -0,0 +1,20 @@
+<?php
+
+declare(strict_types = 1);
+
+namespace App\Http\Controllers;
+
+use App\Services\MyService;
+
+final class CompliantLocalVarNonModel
+{
+    public function run(): void
+    {
+        // Local var of a NON-Model class calling `save()` — the receiver-type
+        // gate still discriminates under flow scope. Only
+        // `Illuminate\Database\Eloquent\Model` / `Builder` subtypes fire.
+        $service = new MyService;
+        $service->save();
+        $service->delete();
+    }
+}

tests/Fixtures/ForbidEloquentMutationInControllers/ViolationInTraitFile.php

@@ -0,0 +1,29 @@
+<?php
+
+declare(strict_types = 1);
+
+namespace App\Http\Controllers\Concerns;
+
+use App\Models\User;
+
+trait MutatesUsers
+{
+    public function persist(): void
+    {
+        // Mutation inside a trait declared in a controllers namespace. Per-node
+        // registration reaches trait bodies analysed through the using class —
+        // the old `Class_` walk never did (a trait file has no `Class_` node).
+        // Fires; the message names the USING class (call-site class reflection).
+        $user = new User;
+        $user->save();
+    }
+}
+
+namespace App\Http\Controllers;
+
+use App\Http\Controllers\Concerns\MutatesUsers;
+
+final class ViolationInTraitFile
+{
+    use MutatesUsers;
+}

tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarBuilderUpdate.php

@@ -0,0 +1,21 @@
+<?php
+
+declare(strict_types = 1);
+
+namespace App\Http\Controllers;
+
+use App\Models\User;
+
+final class ViolationLocalVarBuilderUpdate
+{
+    public function deactivateInactive(): int
+    {
+        // Builder held in a method-local variable — distinct from the inline
+        // `User::query()->...->update()` chain. The old `Class_`-scope walk
+        // resolved `$query` to `mixed`; the flow scope resolves it to a
+        // `Illuminate\Database\Eloquent\Builder` subtype.
+        $query = User::query()->where('email', 'inactive@example.test');
+
+        return $query->update(['name' => 'INACTIVE']);
+    }
+}

tests/Fixtures/ForbidEloquentMutationInControllers/ViolationLocalVarFirstOrFailDelete.php

@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types = 1);
+
+namespace App\Http\Controllers;
+
+use App\Models\Post;
+
+final class ViolationLocalVarFirstOrFailDelete
+{
+    public function destroy(int $id): void
+    {
+        // `Post::query()->where(...)->firstOrFail()` returns a hydrated `Post`,
+        // held in a method-local variable. The old `Class_`-scope walk could not
+        // resolve this receiver; the flow scope now does. (`query()` is used
+        // rather than the static `Post::where()` magic forward so the Builder
+        // generic resolves under vanilla PHPStan — larastan is not loaded in the
+        // fixture environment.)
+        $post = Post::query()->where('id', $id)->firstOrFail();
+        $post->delete();
+    }
+}