Fix: canSee() narrowing was inert, replace with a working mechanism

.github/workflows/tests.yml

@@ -5,6 +5,7 @@ on:
     paths:
       - '**.php'
       - '**.phpstub'
+      - 'tests/fixtures/**'
       - 'phpunit.xml.dist'
       - 'composer.json'
       - '.github/workflows/tests.yml'

README.md

@@ -60,7 +60,7 @@ Hook methods that override a base declaration (`fields()`, `apply()`, `calculate
 
 ### Nova stubs
 
-The plugin ships stubs for `Action`, `Field`, `FieldElement`, `Element`, `PartitionResult`, `Panel`, `Resource`, `Filterable`, `AuthorizedToSee` and `Stack` that fix vendor signatures Psalm cannot resolve. `Resource` is templated, so a resource can declare its model with `@extends`:
+The plugin ships stubs for `Action`, `Field`, `FieldElement`, `Element`, `PartitionResult`, `Panel`, `Resource`, `Filterable` and `Stack` that fix vendor signatures Psalm cannot resolve. `Resource` is templated, so a resource can declare its model with `@extends`:
 
 ```php
 /** @extends \Laravel\Nova\Resource<\App\Models\User> */
@@ -72,7 +72,11 @@ final class User extends Resource
 
 The stubs are registered by the plugin itself; no `<stubs>` entry is needed in `psalm.xml`.
 
-`FieldElement`'s visibility callbacks (`showOnIndex()`, `showOnDetail()`, `hideFromIndex()`, …) are narrowed through a bounded template rather than a fixed union, so a closure typed against the resource's own model (`fn(NovaRequest $request, Post $post): bool`) is accepted instead of being rejected as too narrow. This is a deliberate trade-off, and it is wider than just wrong-model confusion: any type consistent with the bound (`Model|Fluent|array<array-key, mixed>|object`) is accepted for the resource parameter, so a closure typed against the *wrong* model (`fn(NovaRequest $request, Comment $comment)` on a field that only ever appears on `Post`) still type-checks, and so does one typed `stdClass`, `DateTimeImmutable`, or an unrelated array shape. Wrong request classes, wrong return types and wrong arity are still reported.
+`FieldElement`'s visibility callbacks (`showOnIndex()`, `showOnDetail()`, `hideFromIndex()`, …) are narrowed through a bounded template rather than a fixed union, so a closure typed against the resource's own model (`fn(NovaRequest $request, Post $post): bool`) is accepted instead of being rejected as too narrow. This is a deliberate trade-off, and it is wider than just wrong-model confusion: any type consistent with the bound (`Model|Fluent|array<array-key, mixed>|object`) is accepted for the resource parameter, so a closure typed against the *wrong* model (`fn(NovaRequest $request, Comment $comment)` on a field that only ever appears on `Post`) still type-checks, and so does one typed `stdClass`, `DateTimeImmutable`, or an unrelated array shape. Wrong request classes, wrong return types and wrong arity are still reported. This also makes the plugin *stricter* than Nova's own docblocks for the setters Nova types with a bare `mixed` second parameter (Nova only narrows some of them itself, via `@phpstan-param`): the bound now catches a resource parameter typed as something unrelated (e.g. `int`) that Nova's own bare `mixed` wouldn't have flagged.
+
+`canSee()` is narrowed to `NovaRequest` for every `Field`-derived class, but not through a stub: it's declared only on the `AuthorizedToSee` trait, which `Field`/`FieldElement` merely inherit, and a stub cannot override a method the stubbed class only inherits from a used trait. The plugin rewrites it programmatically post-populate instead, scoped to the `Field` hierarchy only — `Tool`, `Dashboard`, `Filters\Filter` and `Menu\*` share the same trait but can receive a plain `Illuminate\Http\Request` at runtime, so their `canSee()` stays wide.
+
+`Filterable::filterable()`'s query parameter accepts either a concrete `Illuminate\Database\Eloquent\Builder` or a `Relation`, since Nova passes a `Relation` for relationship-index requests. Typing a closure against only one of the two type-checks even for a field that's reachable through both request kinds — type against the shared `Illuminate\Contracts\Database\Eloquent\Builder` contract instead if a field needs to be safe against both.
 
 ## Requirements
 

composer.json

@@ -28,7 +28,10 @@
     "autoload-dev": {
         "psr-4": {
             "InteractionDesignFoundation\\PsalmLaravelNova\\Tests\\": "tests/"
-        }
+        },
+        "classmap": [
+            "tests/fixtures/fake-nova/"
+        ]
     },
     "config": {
         "allow-plugins": {

src/NovaFieldAuthorizationHandler.php

@@ -0,0 +1,101 @@
+<?php declare(strict_types=1);
+
+namespace InteractionDesignFoundation\PsalmLaravelNova;
+
+use Psalm\Codebase;
+use Psalm\Internal\MethodIdentifier;
+use Psalm\Plugin\EventHandler\AfterCodebasePopulatedInterface;
+use Psalm\Plugin\EventHandler\Event\AfterCodebasePopulatedEvent;
+use Psalm\Storage\ClassLikeStorage;
+use Psalm\Storage\MethodStorage;
+use Psalm\Type\Atomic\TClosure;
+use Psalm\Type\Atomic\TNamedObject;
+use Psalm\Type\Union;
+
+/**
+ * Narrows `canSee()` to `NovaRequest` for every `FieldElement` descendant, leaving `Tool`/`Dashboard`/
+ * `Filters\Filter`/`Menu\*` (same `AuthorizedToSee` trait, but can get a plain `Request`) untouched.
+ * A stub can't do this — `canSee()` is only inherited, never declared, on the classes in between, and
+ * a stub can override a declared method but not an inherited one (confirmed against real Nova). This
+ * rewrites `declaring_method_ids`/`methods` directly instead — the fields Psalm's method resolution
+ * actually reads — the same way `NovaResourceQueryMethodHandler` narrows query-builder params.
+ * @internal
+ */
+final class NovaFieldAuthorizationHandler implements AfterCodebasePopulatedInterface
+{
+    private const FIELD_ELEMENT = 'laravel\nova\fields\fieldelement';
+
+    private const AUTHORIZED_TO_SEE = 'laravel\nova\authorizedtosee';
+
+    private const CAN_SEE = 'cansee';
+
+    private const NOVA_REQUEST = 'Laravel\Nova\Http\Requests\NovaRequest';
+
+    #[\Override]
+    public static function afterCodebasePopulated(AfterCodebasePopulatedEvent $event): void
+    {
+        $codebase = $event->getCodebase();
+
+        foreach ($codebase->classlike_storage_provider::getAll() as $storage) {
+            $isFieldElement = mb_strtolower($storage->name) === self::FIELD_ELEMENT
+                || isset($storage->parent_classes[self::FIELD_ELEMENT]);
+            if (!$isFieldElement) {
+                continue;
+            }
+
+            self::narrowCanSee($codebase, $storage);
+        }
+    }
+
+    private static function narrowCanSee(Codebase $codebase, ClassLikeStorage $storage): void
+    {
+        $declaringId = $storage->declaring_method_ids[self::CAN_SEE] ?? null;
+        if ($declaringId === null || mb_strtolower($declaringId->fq_class_name) !== self::AUTHORIZED_TO_SEE) {
+            // canSee() is missing, or a user class overrode it somewhere in the chain — only ever
+            // narrow Nova's own trait method, never second-guess a user's own override.
+            return;
+        }
+
+        $declaringStorage = $codebase->methods->getStorage($declaringId);
+        $narrowedCallback = self::narrowCallbackParam($declaringStorage);
+        if ($narrowedCallback === null) {
+            // Nova's canSee() shape changed in a way we don't recognise: silence over false positives.
+            return;
+        }
+
+        $narrowed = clone $declaringStorage;
+        $narrowed->params = [$narrowedCallback];
+
+        $selfId = new MethodIdentifier($storage->name, self::CAN_SEE);
+        $storage->methods[self::CAN_SEE] = $narrowed;
+        $storage->declaring_method_ids[self::CAN_SEE] = $selfId;
+        $storage->appearing_method_ids[self::CAN_SEE] = $selfId;
+    }
+
+    /**
+     * `canSee(Closure $callback)`: rewrite the closure's own param type, not `$callback`'s.
+     * @psalm-mutation-free
+     */
+    private static function narrowCallbackParam(MethodStorage $canSee): ?\Psalm\Storage\FunctionLikeParameter
+    {
+        $callbackParam = $canSee->params[0] ?? null;
+        if ($callbackParam === null) {
+            return null;
+        }
+
+        $callbackType = $callbackParam->type;
+        if ($callbackType === null) {
+            return null;
+        }
+
+        $closure = $callbackType->getSingleAtomic();
+        if (!$closure instanceof TClosure || $closure->params === null || !isset($closure->params[0])) {
+            return null;
+        }
+
+        $narrowedRequestParam = $closure->params[0]->setType(new Union([new TNamedObject(self::NOVA_REQUEST)]));
+        $narrowedClosure = $closure->replace([$narrowedRequestParam], $closure->return_type);
+
+        return $callbackParam->setType(new Union([$narrowedClosure]));
+    }
+}

src/Plugin.php

@@ -20,18 +20,19 @@ public function __invoke(RegistrationInterface $registration, ?\SimpleXMLElement
         require_once __DIR__.'/NovaMakeSignatureHandler.php';
         require_once __DIR__.'/NovaWhenReturnTypeHandler.php';
         require_once __DIR__.'/NovaSuppressHandler.php';
+        require_once __DIR__.'/NovaFieldAuthorizationHandler.php';
 
         $registration->registerHooksFromClass(NovaResourceQueryMethodHandler::class);
         $registration->registerHooksFromClass(NovaMakeSignatureHandler::class);
         $registration->registerHooksFromClass(NovaWhenReturnTypeHandler::class);
         $registration->registerHooksFromClass(NovaSuppressHandler::class);
+        $registration->registerHooksFromClass(NovaFieldAuthorizationHandler::class);
 
         // Nova stubs that fix vendor signatures Psalm cannot resolve (and template Resource so a
         // resource can declare its model via @extends). Shipped with the package so it stays
         // self-contained.
         $stubsDir = __DIR__.'/../stubs/Nova';
         $registration->addStubFile($stubsDir.'/Actions/Action.phpstub');
-        $registration->addStubFile($stubsDir.'/AuthorizedToSee.phpstub');
         $registration->addStubFile($stubsDir.'/Fields/Field.phpstub');
         $registration->addStubFile($stubsDir.'/Fields/FieldElement.phpstub');
         $registration->addStubFile($stubsDir.'/Fields/Filterable.phpstub');

stubs/Nova/Fields/FieldElement.phpstub

@@ -21,19 +21,11 @@ namespace Laravel\Nova\Fields;
  *
  * `showOnCreating()`/`hideWhenCreating()` take request-only callbacks and need no change.
  *
- * `canSee()` is redeclared here (narrowed to `NovaRequest`) rather than left on the shared
- * `AuthorizedToSee` trait: field resolution (`ResolvesFields`) always goes through `NovaRequest`,
- * unlike `Tool`/`Dashboard`/`Filters\Filter`/`Menu\*`, which can receive a plain `Request` — see
- * `AuthorizedToSee.phpstub` for why the trait itself stays wide.
+ * `canSee()` is narrowed too, but not here — it's only inherited (via `AuthorizedToSee`), and a stub
+ * can't override an inherited method. See `NovaFieldAuthorizationHandler`.
  */
 abstract class FieldElement extends \Laravel\Nova\Element
 {
-    /**
-     * @param \Closure(\Laravel\Nova\Http\Requests\NovaRequest):bool $callback
-     * @return $this
-     */
-    public function canSee(\Closure $callback) {}
-
     /**
      * @template TResource of \Illuminate\Database\Eloquent\Model|\Laravel\Nova\Support\Fluent|array<array-key, mixed>|object
      * @param (callable(\Laravel\Nova\Http\Requests\NovaRequest, TResource):bool)|bool $callback

stubs/Nova/Fields/Filterable.phpstub

@@ -15,6 +15,9 @@ namespace Laravel\Nova\Fields;
  * the pattern used in `FieldElement.phpstub`: Psalm infers `TBuilder` from the closure's own param type
  * instead of checking against a fixed union, so a closure typed against either concrete class still
  * narrows correctly, while a genuinely wrong type (e.g. `stdClass`) is still rejected.
+ *
+ * Trade-off: a closure typed against only ONE of the two shapes still type-checks, even if the field
+ * is reachable via both request kinds — Psalm can't know. Use the shared contract type if that matters.
  */
 trait Filterable
 {

tests/AcceptanceTest.php

@@ -13,12 +13,12 @@
  * `fake-nova/` holds minimal Laravel/Nova declarations that reproduce the vendor docblocks the
  * plugin's stubs override. Both scenario files are analysed in a single Psalm run.
  *
- * @see tests/fixtures/psalm.xml for why the fakes are stub files rather than project files.
+ * @see tests/fixtures/psalm.xml for why the fakes are reached through the composer classmap.
  */
 #[CoversNothing]
 final class AcceptanceTest extends TestCase
 {
-    /** @var list<array{file_name: string, line_from: int, type: string, message: string}>|null */
+    /** @var list<array{file_name: string, line_from: int, selected_text: string, type: string, message: string}>|null */
     private static ?array $issues = null;
 
     #[Test]
@@ -37,29 +37,35 @@ public function genuinely_wrong_callbacks_are_still_reported(): void
         self::assertSame(
             [
                 // Wrong request class.
-                ['line' => 22, 'type' => 'InvalidArgument'],
+                ['text' => 'static fn(\stdClass $request, Post $post): bool => true', 'type' => 'InvalidArgument'],
                 // Wrong return type.
-                ['line' => 25, 'type' => 'InvalidArgument'],
+                ['text' => "static fn(NovaRequest \$request, Post \$post): string => 'nope'", 'type' => 'InvalidArgument'],
+                // Wrong resource param type on a setter with no upstream @phpstan-param (stricter
+                // than pre-plugin Nova, which left this bare `mixed`).
+                ['text' => 'static fn(NovaRequest $request, int $post): bool => true', 'type' => 'InvalidArgument'],
                 // Wrong param type on the authorisation callback.
-                ['line' => 28, 'type' => 'InvalidArgument'],
+                ['text' => 'static fn(int $request): bool => true', 'type' => 'InvalidArgument'],
                 // Wrong builder type on the filter callback.
-                ['line' => 31, 'type' => 'InvalidArgument'],
+                [
+                    'text' => 'static function (NovaRequest $request, \stdClass $wrongBuilder, mixed $value, string $attribute): void {}',
+                    'type' => 'InvalidArgument',
+                ],
                 // Stack line is not a valid class-string<Field>|callable|Field.
-                ['line' => 34, 'type' => 'InvalidArgument'],
+                ['text' => '[42]', 'type' => 'InvalidArgument'],
                 // Stack line via $lines is not a valid class-string<Field>|callable|Field either.
-                ['line' => 37, 'type' => 'InvalidArgument'],
+                ['text' => '[new \stdClass()]', 'type' => 'InvalidArgument'],
                 // Tool::canSee() narrowed to NovaRequest would be unsound: Tool can receive a plain Request.
-                ['line' => 51, 'type' => 'ArgumentTypeCoercion'],
+                ['text' => 'static fn(NovaRequest $request): bool => true', 'type' => 'ArgumentTypeCoercion'],
             ],
             array_map(
-                static fn(array $issue): array => ['line' => $issue['line_from'], 'type' => $issue['type']],
+                static fn(array $issue): array => ['text' => $issue['selected_text'], 'type' => $issue['type']],
                 $issues,
             ),
             "Unexpected Psalm issues in still_errors.php:\n".self::describe($issues),
         );
     }
 
-    /** @return list<array{file_name: string, line_from: int, type: string, message: string}> */
+    /** @return list<array{file_name: string, line_from: int, selected_text: string, type: string, message: string}> */
     private function issuesIn(string $fixtureRelativePath): array
     {
         return array_values(array_filter(
@@ -68,45 +74,62 @@ private function issuesIn(string $fixtureRelativePath): array
         ));
     }
 
-    /** @return list<array{file_name: string, line_from: int, type: string, message: string}> */
+    /** @return list<array{file_name: string, line_from: int, selected_text: string, type: string, message: string}> */
     private static function psalmIssues(): array
     {
         if (self::$issues !== null) {
             return self::$issues;
         }
 
         $projectRoot = \dirname(__DIR__);
-        $descriptors = [1 => ['pipe', 'w'], 2 => ['pipe', 'w']];
 
-        $process = proc_open(
-            [
-                \PHP_BINARY,
-                $projectRoot.'/vendor/bin/psalm',
-                '--no-cache',
-                '--no-progress',
-                '--output-format=json',
-                '-c',
-                'tests/fixtures/psalm.xml',
-            ],
-            $descriptors,
-            $pipes,
-            $projectRoot,
-        );
-        self::assertIsResource($process, 'Could not start Psalm.');
+        // stderr goes to a temp file, not a second pipe: draining stdout and stderr from two live
+        // pipes sequentially can deadlock if Psalm fills the undrained one while blocked writing to
+        // the other. A file has no such buffer limit.
+        $stderrFile = tempnam(sys_get_temp_dir(), 'psalm-plugin-nova-stderr-');
+        self::assertIsString($stderrFile, 'Could not create a temp file for stderr.');
+
+        try {
+            $process = proc_open(
+                [
+                    \PHP_BINARY,
+                    $projectRoot.'/vendor/bin/psalm',
+                    '--no-cache',
+                    '--no-progress',
+                    '--output-format=json',
+                    '-c',
+                    'tests/fixtures/psalm.xml',
+                ],
+                [1 => ['pipe', 'w'], 2 => ['file', $stderrFile, 'w']],
+                $pipes,
+                $projectRoot,
+            );
+            self::assertIsResource($process, 'Could not start Psalm.');
 
-        $stdout = (string) stream_get_contents($pipes[1]);
-        $stderr = (string) stream_get_contents($pipes[2]);
-        array_map(fclose(...), $pipes);
-        proc_close($process);
+            $stdout = (string) stream_get_contents($pipes[1]);
+            fclose($pipes[1]);
+            $exitCode = proc_close($process);
+            $stderr = (string) file_get_contents($stderrFile);
+        } finally {
+            unlink($stderrFile);
+        }
+
+        // 0 = no issues, 2 = issues were found (expected — still_errors.php is meant to raise some;
+        // see IssueBuffer::finish()). Anything else is Psalm itself failing to run, not an issue.
+        self::assertContains(
+            $exitCode,
+            [0, 2],
+            "Psalm exited with code {$exitCode}.\nstdout: {$stdout}\nstderr: {$stderr}",
+        );
 
-        /** @var list<array{file_name: string, line_from: int, type: string, message: string}>|null $issues */
+        /** @var list<array{file_name: string, line_from: int, selected_text: string, type: string, message: string}>|null $issues */
         $issues = json_decode($stdout, associative: true);
         self::assertIsArray($issues, "Psalm did not return JSON.\nstdout: {$stdout}\nstderr: {$stderr}");
 
         return self::$issues = $issues;
     }
 
-    /** @param list<array{file_name: string, line_from: int, type: string, message: string}> $issues */
+    /** @param list<array{file_name: string, line_from: int, selected_text: string, type: string, message: string}> $issues */
     private static function describe(array $issues): string
     {
         return implode("\n", array_map(