From 7dceec4bc98c3d144ea00da2d8776dd6dee6dc94 Mon Sep 17 00:00:00 2001 From: Alies Lapatsin <5278175+alies-dev@users.noreply.github.com> Date: Sun, 21 Jun 2026 15:34:30 +0200 Subject: [PATCH] fix(console): scope $this override to the callback closure #1114 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #1122. That handler set Context::$self to ClosureCommand for the whole Artisan::command() call, so self::/static:: in the *signature* argument resolved against ClosureCommand — e.g. Artisan::command(self::SIG, fn ...) inside a class reported a false UndefinedConstant/UndefinedMethod. Override self only while the callback closure node itself is analysed (record the callback arg's spl_object_id at the call, flip self when that node is visited, restore after), leaving sibling arguments on the real self. The callback id is not consumed on first match: the facade @method dispatch analyses the argument more than once, so each pass re-applies the override; the id set is reset per file to bound it and avoid spl_object_id reuse across files. Also restricts detection to the actual callback argument (2nd positional or named `callback`), runs the cheap AST checks before codebase queries, and strengthens the type tests (self::/static:: signature regression, arrow and static-arrow forms, named-argument callback, non-global option). --- .../Console/ConsoleClosureScopeHandler.php | 220 +++++++++++------- .../tests/ConsoleClosureScopeSelfArgTest.phpt | 32 +++ .../tests/ConsoleClosureScopeStaticTest.phpt | 6 +- tests/Type/tests/ConsoleClosureScopeTest.phpt | 17 +- 4 files changed, 191 insertions(+), 84 deletions(-) create mode 100644 tests/Type/tests/ConsoleClosureScopeSelfArgTest.phpt diff --git a/src/Handlers/Console/ConsoleClosureScopeHandler.php b/src/Handlers/Console/ConsoleClosureScopeHandler.php index 80ebf6d4..988ebd21 100644 --- a/src/Handlers/Console/ConsoleClosureScopeHandler.php +++ b/src/Handlers/Console/ConsoleClosureScopeHandler.php @@ -6,7 +6,6 @@ use Illuminate\Foundation\Console\ClosureCommand; use Illuminate\Support\Facades\Artisan; -use PhpParser\Node\Expr; use PhpParser\Node\Expr\ArrowFunction; use PhpParser\Node\Expr\Closure; use PhpParser\Node\Expr\StaticCall; @@ -16,9 +15,11 @@ use Psalm\Internal\Analyzer\StatementsAnalyzer; use Psalm\Plugin\EventHandler\AfterExpressionAnalysisInterface; use Psalm\Plugin\EventHandler\BeforeExpressionAnalysisInterface; +use Psalm\Plugin\EventHandler\BeforeFileAnalysisInterface; use Psalm\Plugin\EventHandler\BeforeStatementAnalysisInterface; use Psalm\Plugin\EventHandler\Event\AfterExpressionAnalysisEvent; use Psalm\Plugin\EventHandler\Event\BeforeExpressionAnalysisEvent; +use Psalm\Plugin\EventHandler\Event\BeforeFileAnalysisEvent; use Psalm\Plugin\EventHandler\Event\BeforeStatementAnalysisEvent; /** @@ -44,21 +45,21 @@ * **How it works.** A bound `$this` at file scope needs *two* things, and Psalm * gates them separately — exactly the pair a `@var ClosureCommand $this` var * docblock would set (see {@see \Psalm\Internal\Analyzer\StatementsAnalyzer}'s - * handling of a `$this` var-comment), so we reproduce both: + * handling of a `$this` var-comment), so we reproduce both, scoped to the + * callback closure node alone: * * 1. **The `$this` variable's type.** * {@see \Psalm\Internal\Analyzer\ClosureAnalyzer::analyzeExpression()} derives * a closure body's `$this` purely from the *enclosing* `Context::$self` * (`new TNamedObject($context->self)`; it ignores any pre-set `vars_in_scope`). - * {@see self::beforeExpressionAnalysis()} sets `Context::$self` to - * `ClosureCommand` for the span of the `Artisan::command()` call — the - * argument is analysed against that same `Context` (no clone on the facade - * `@method` dispatch path) — and {@see self::afterExpressionAnalysis()} - * restores it once the call is analysed so no sibling expression sees the - * override. (The restore rides Psalm's after-hook, which is skipped if the - * call hard-fails analysis; the enclosing statement list is abandoned in that - * case, so a still-overridden `self` is never reused, and the after-hook - * re-validates the node to stay safe against `spl_object_id` recycling.) + * When {@see self::beforeExpressionAnalysis()} sees an `Artisan::command()` + * call it records the `spl_object_id` of its callback argument; when that exact + * closure node is then analysed, the same hook sets `Context::$self` to + * `ClosureCommand` for that node's span only, and + * {@see self::afterExpressionAnalysis()} restores it. Overriding at the closure + * node (not the whole call) keeps the *other* arguments on the real `self`, so + * `Artisan::command(self::SIG, fn ...)` still resolves `self::SIG` against the + * enclosing class. * 2. **The structural `$this->...` guard.** `MethodCallAnalyzer` (and the * property-fetch analyzer) reject `$this` whenever * `StatementsAnalyzer::getFQCLN()` is null — a separate check that step 1 does @@ -66,8 +67,16 @@ * `InvalidScope: Use of $this in non-class context`. * {@see self::beforeStatementAnalysis()} calls `setFQCLN(ClosureCommand)` on * the closure body's analyzer (detected by `Context::$self` already being - * `ClosureCommand`, which only our own step 1 produces), clearing the guard - * on a per-body analyzer that is discarded when the closure finishes. + * `ClosureCommand`, which only step 1 produces), clearing the guard on a + * per-body analyzer that is discarded when the closure finishes. + * + * The recorded callback id is intentionally *not* consumed on first use: the + * facade `@method` dispatch analyses the callback argument more than once + * (`AtomicStaticCallAnalyzer::analyzePseudoMethodCall` re-runs `ArgumentsAnalyzer` + * for the data-flow pass), so each pass must re-apply the override. The id set and + * the saved-`self` map are cleared per file ({@see self::beforeAnalyzeFile()}), + * which bounds them and prevents an `spl_object_id` recorded in one file from + * matching an unrelated closure in another. * * **Why a handler and not a stub.** The clean fix lives upstream in Psalm: * PHPStan — and therefore Larastan, see its `Foundation/Console/Kernel.stub` — @@ -87,30 +96,37 @@ * - The instance form `$kernel->command(...)` on the concrete * `Illuminate\Foundation\Console\Kernel` (the method is not on the * `Contracts\Console\Kernel` interface) — detection matches static calls only - * ({@see self::isArtisanCommandClosureCall()} requires a `StaticCall`), and - * this instance form is rare in practice. - * - A `static function` callback — a static closure cannot be rebound at - * runtime, so `$this` inside it is a genuine error. We skip it at detection - * ({@see self::hasInlineBindableClosureArg()}), leaving Psalm's `InvalidScope` - * to fire as it should. - * - * The override spans the whole `command()` call, so in the rare in-method form - * `Artisan::command(self::SIG, fn ...)` a `self::`/`static::` reference in the - * signature argument would resolve against ClosureCommand. Harmless in practice: - * the signature argument is a string literal in real code. + * ({@see self::callbackToBind()} requires a `StaticCall`), and this instance + * form is rare in practice. + * - A `static function`/`static fn` callback — a static closure cannot be + * rebound at runtime, so `$this` inside it is a genuine error. We skip it at + * detection ({@see self::bindableCallbackArg()}), leaving Psalm's + * `InvalidScope` to fire as it should. */ final class ConsoleClosureScopeHandler implements BeforeExpressionAnalysisInterface, BeforeStatementAnalysisInterface, - AfterExpressionAnalysisInterface + AfterExpressionAnalysisInterface, + BeforeFileAnalysisInterface { private const ARTISAN_FACADE = Artisan::class; private const CLOSURE_COMMAND = ClosureCommand::class; /** - * Outer `Context::$self` saved per `Artisan::command()` call node (keyed by - * the node's `spl_object_id`) so it can be restored once the call has been + * `spl_object_id`s of callback closure nodes seen as the callback argument of + * an `Artisan::command()` call in the file currently being analysed. Recorded + * when the call is visited and matched when the closure node is analysed (kept, + * not consumed: the callback is analysed more than once per call). Cleared in + * {@see self::beforeAnalyzeFile()}. + * + * @var array + */ + private static array $callbackIds = []; + + /** + * Outer `Context::$self` saved per overridden callback closure node (keyed by + * the node's `spl_object_id`) so it can be restored once the closure has been * analysed. Values are nullable because the dominant call site is file scope * (`routes/console.php`), where `self` is null. * @@ -123,16 +139,34 @@ public static function beforeExpressionAnalysis(BeforeExpressionAnalysisEvent $e { $expr = $event->getExpr(); - if (!self::isArtisanCommandClosureCall($expr, $event->getCodebase())) { + // First pass: an Artisan::command() call — remember its callback node so + // the override fires on that node alone (below), not across sibling args. + if ($expr instanceof StaticCall) { + $callback = self::callbackToBind($expr, $event->getCodebase()); + if ($callback !== null) { + self::$callbackIds[\spl_object_id($callback)] = true; + } + return null; } - // Inject ClosureCommand as the closure's `$this` by overriding the - // enclosing `self` for the span of this call's analysis. The callback - // body is analysed within that span, so ClosureAnalyzer picks it up. - $context = $event->getContext(); - self::$savedSelf[\spl_object_id($expr)] = $context->self; - $context->self = self::CLOSURE_COMMAND; + // The callback closure node itself — override `self` so ClosureAnalyzer + // types the body `$this` as ClosureCommand. + if ($expr instanceof Closure || $expr instanceof ArrowFunction) { + $context = $event->getContext(); + + // Skip non-callbacks, and skip when our override is already in place: + // the callback is analysed more than once, and a re-entrant call must + // not capture ClosureCommand itself as the "outer" self to restore. + if (!isset(self::$callbackIds[\spl_object_id($expr)]) + || $context->self === self::CLOSURE_COMMAND + ) { + return null; + } + + self::$savedSelf[\spl_object_id($expr)] = $context->self; + $context->self = self::CLOSURE_COMMAND; + } return null; } @@ -153,14 +187,14 @@ public static function beforeStatementAnalysis(BeforeStatementAnalysisEvent $eve // `StatementsAnalyzer::getFQCLN()`, which stays null for a file-scope // closure and emits `InvalidScope: Use of $this in non-class context`. // - // The closure body is the only place that runs with `self` already set to - // ClosureCommand (we set it solely for `Artisan::command()` callbacks and - // restore it straight after), so that identity reliably marks "inside a - // console-command closure". Calling `setFQCLN()` on the body analyzer is - // exactly what a `/** @var ClosureCommand $this */` docblock does - // (StatementsAnalyzer sets `fake_this_class` from such a comment), so it - // clears the structural guard the same sanctioned way — scoped to this - // body analyzer, which is discarded when the closure finishes. + // The callback body is the only place that runs with `self` set to + // ClosureCommand (we set it solely on the registered callback node), so + // that identity reliably marks "inside a console-command closure". Calling + // `setFQCLN()` on the body analyzer is exactly what a + // `/** @var ClosureCommand $this */` docblock does (StatementsAnalyzer sets + // `fake_this_class` from such a comment), so it clears the structural guard + // the same sanctioned way — scoped to this body analyzer, which is + // discarded when the closure finishes. if ($event->getContext()->self !== self::CLOSURE_COMMAND) { return null; } @@ -178,7 +212,7 @@ public static function afterExpressionAnalysis(AfterExpressionAnalysisEvent $eve { $expr = $event->getExpr(); - if (!$expr instanceof StaticCall) { + if (!$expr instanceof Closure && !$expr instanceof ArrowFunction) { return null; } @@ -190,59 +224,73 @@ public static function afterExpressionAnalysis(AfterExpressionAnalysisEvent $eve return null; } - $saved = self::$savedSelf[$id]; - unset(self::$savedSelf[$id]); + $context = $event->getContext(); - // Restore `self` only on the node we actually overrode. If - // `handleExpression()` returned false for an earlier `Artisan::command()` - // call, its paired after-hook was skipped (Psalm dispatches it only on - // success), leaving a stale entry; `spl_object_id` can then be recycled - // onto an unrelated StaticCall. Re-detecting the node — a stable property - // that always holds for the call we saved — rejects that recycled id so - // we never write a stale `self` onto someone else's context. Any id the - // after-hook observes is dropped above, so the map stays bounded — a - // hard-failed call's own entry lingers only until its id is recycled (or - // the worker exits), never growing without bound. - if (self::isArtisanCommandClosureCall($expr, $event->getCodebase())) { - $event->getContext()->self = $saved; + // Restore only while our override is still live. If the closure hard-failed + // analysis Psalm skips this after-hook; the `self === ClosureCommand` guard + // means a later node reusing this id (after the entry leaked) drops the + // stale entry below without being clobbered. + if ($context->self === self::CLOSURE_COMMAND) { + $context->self = self::$savedSelf[$id]; } + unset(self::$savedSelf[$id]); + return null; } - private static function isArtisanCommandClosureCall(Expr $expr, Codebase $codebase): bool + /** + * Per-file reset, at file start. Bounds the static maps and ensures an + * `spl_object_id` recorded for one file's callback cannot match an unrelated + * closure node in the next file (ids are unique only among live objects). + * Resetting at the *start* (rather than end) keeps it correct even if a prior + * file's analysis threw before any end-of-file hook could run. + * + * @psalm-external-mutation-free + */ + #[\Override] + public static function beforeAnalyzeFile(BeforeFileAnalysisEvent $event): void { - if (!$expr instanceof StaticCall) { - return false; - } + self::$callbackIds = []; + self::$savedSelf = []; + } + /** + * The inline, bindable callback of an `Artisan::command()` static call, or + * null when this is not such a call. Cheap AST checks (method name, named + * receiver, the callback argument) run before the codebase queries so + * unrelated `::command()` calls bail early. + */ + private static function callbackToBind(StaticCall $expr, Codebase $codebase): Closure|ArrowFunction|null + { // Method names are case-insensitive in PHP; match `command` exactly so // siblings like `commandStartedAt()` are not picked up. if (!$expr->name instanceof Identifier || \strtolower($expr->name->name) !== 'command') { - return false; + return null; } // Statically-named receiver only (not `$class::command(...)`). if (!$expr->class instanceof Name) { - return false; + return null; } - $className = $expr->class->getAttribute('resolvedName'); - if (!\is_string($className) || !self::resolvesToArtisanFacade($className, $codebase)) { - return false; + $callback = self::bindableCallbackArg($expr); + if ($callback === null) { + return null; } - // Only an inline, non-static closure/arrow function is rebound by Laravel - // at runtime. A variable holding a callable was analysed in its own scope - // already, and a `static` closure cannot be rebound — `$this` inside it is - // a genuine error that must keep surfacing. - if (!self::hasInlineBindableClosureArg($expr)) { - return false; + $className = $expr->class->getAttribute('resolvedName'); + if (!\is_string($className) || !self::resolvesToArtisanFacade($className, $codebase)) { + return null; } // ClosureAnalyzer will call classlike_storage_provider->get() on the // injected self; bail if ClosureCommand was never scanned to avoid a throw. - return $codebase->classlike_storage_provider->has(self::CLOSURE_COMMAND); + if (!$codebase->classlike_storage_provider->has(self::CLOSURE_COMMAND)) { + return null; + } + + return $callback; } /** @psalm-external-mutation-free */ @@ -257,16 +305,32 @@ private static function resolvesToArtisanFacade(string $className, Codebase $cod && $codebase->classExtends($className, self::ARTISAN_FACADE); } - private static function hasInlineBindableClosureArg(StaticCall $expr): bool + /** + * The `command(string $signature, Closure $callback)` callback argument — the + * 2nd positional arg or a named `callback:` arg — when it is an inline, + * non-static closure/arrow function (the only form Laravel rebinds). Any other + * argument (including a closure passed as the signature) is ignored. + */ + private static function bindableCallbackArg(StaticCall $expr): Closure|ArrowFunction|null { + $positional = 0; + foreach ($expr->getArgs() as $arg) { - $value = $arg->value; + $isCallback = $arg->name instanceof Identifier + ? $arg->name->name === 'callback' + : $positional++ === 1; - if (($value instanceof Closure || $value instanceof ArrowFunction) && !$value->static) { - return true; + if (!$isCallback) { + continue; } + + $value = $arg->value; + + return ($value instanceof Closure || $value instanceof ArrowFunction) && !$value->static + ? $value + : null; } - return false; + return null; } } diff --git a/tests/Type/tests/ConsoleClosureScopeSelfArgTest.phpt b/tests/Type/tests/ConsoleClosureScopeSelfArgTest.phpt new file mode 100644 index 00000000..2b322efd --- /dev/null +++ b/tests/Type/tests/ConsoleClosureScopeSelfArgTest.phpt @@ -0,0 +1,32 @@ +--FILE-- +comment('x'); + }); + + Artisan::command(static::sig(), function (): void { + $this->comment('y'); + }); + } + + private static function sig(): string + { + return 'do:thing'; + } +} +?> +--EXPECTF-- diff --git a/tests/Type/tests/ConsoleClosureScopeStaticTest.phpt b/tests/Type/tests/ConsoleClosureScopeStaticTest.phpt index 90fe4ff9..777570ff 100644 --- a/tests/Type/tests/ConsoleClosureScopeStaticTest.phpt +++ b/tests/Type/tests/ConsoleClosureScopeStaticTest.phpt @@ -3,10 +3,14 @@ use Illuminate\Support\Facades\Artisan; -// static closure can't be rebound → $this still errors (handler stays out). +// static closures (function + fn) can't be rebound → $this still errors +// (handler stays out for both forms). Artisan::command('inspire', static function (): void { $this->comment('x'); }); + +Artisan::command('inspire-fn', static fn (): mixed => $this->comment('x')); ?> --EXPECTF-- InvalidScope on line %d: Invalid reference to $this in a static context +InvalidScope on line %d: Invalid reference to $this in a static context diff --git a/tests/Type/tests/ConsoleClosureScopeTest.phpt b/tests/Type/tests/ConsoleClosureScopeTest.phpt index 7e4d57f7..15955354 100644 --- a/tests/Type/tests/ConsoleClosureScopeTest.phpt +++ b/tests/Type/tests/ConsoleClosureScopeTest.phpt @@ -8,18 +8,25 @@ Artisan::command('inspire', function (): void { /** @psalm-check-type-exact $this = Illuminate\Foundation\Console\ClosureCommand&static */ $this->comment('Display an inspiring quote'); $this->info('done'); - // unknown signature → no false InvalidConsole*Name - $this->argument('name'); - $this->option('verbose'); + // unknown signature (non-global names) → no false InvalidConsole*Name + $this->argument('target'); + $this->option('frequency'); })->purpose('Display an inspiring quote'); -// arrow fn -Artisan::command('inspire-arrow', fn () => $this->comment('x')); +// arrow fn: declared return proves $this is the bound ClosureCommand (a wrong +// or mixed $this would trip InvalidReturnStatement here). +Artisan::command('inspire-arrow', fn (): \Illuminate\Foundation\Console\ClosureCommand => $this); // \Artisan global alias \Artisan::command('inspire-alias', function (): void { /** @psalm-check-type-exact $this = Illuminate\Foundation\Console\ClosureCommand&static */ $this->comment('x'); }); + +// named-argument form: callback identified by name, not position +Artisan::command(signature: 'inspire-named', callback: function (): void { + /** @psalm-check-type-exact $this = Illuminate\Foundation\Console\ClosureCommand&static */ + $this->comment('x'); +}); ?> --EXPECTF--