Scope $this override to the Artisan::command() callback closure#1127
Merged
Conversation
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).
Collaborator
Author
|
/psalm-delta |
Contributor
PR delta: Base (bca83dd) -> PR (7dceec4)Per-app delta — IssuesNo issue changes across the benchmarked apps.
Per-app delta — Time (seconds)No significant time change (all deltas within runner jitter). Per-app delta — Type coverage (%)No type-coverage changes. Informational only — never fails the PR. Reproduce locally: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue to Solve
Post-merge follow-up to #1122 (closed #1114). A review of the merged
ConsoleClosureScopeHandlerfound an Important bug: it setContext::$selftoClosureCommandfor the wholeArtisan::command(...)call, soself::/static::in the signature argument resolved againstClosureCommandinstead of the enclosing class:Related
Follow-up to #1122. Refs #1114.
Solution Description
Scope the
selfoverride to the callback closure node instead of the whole call:beforeExpressionAnalysisrecords thespl_object_idof theArtisan::command()callback argument (2nd positional or namedcallback, non-static); when that exact closure node is analysed, it overridesContext::$self, andafterExpressionAnalysisrestores it. Sibling arguments (the signature) keep the realself, soself::SIG/static::sig()resolve against the enclosing class.@methoddispatch analyses the callback argument more than once (AtomicStaticCallAnalyzer::analyzePseudoMethodCallre-runsArgumentsAnalyzerfor the data-flow pass), so each pass must re-apply the override. The id set is reset per file (beforeAnalyzeFile) to bound it and prevent anspl_object_idfrom one file matching an unrelated closure in another.Also addresses the review's suggestions: detection is restricted to the actual callback argument; the cheap AST checks run before the codebase queries; and the type tests are strengthened.
Verification (all empirical):
ConsoleClosureScopeSelfArgTest.phpt(new) is confirmed a true regression guard — it goes red on the pre-rewrite handler (UndefinedMagicMethod/InaccessibleMethod/MixedArgumentonself::sig()/static::sig()) and green on this one. The reviewer's literalself::SIGconst form was also probed directly and now reports no errors.static fnforms, a named-argument callback, a non-global option name (so the unknown-signature path is actually exercised), and an arrow: ClosureCommand => $thisreturn that proves the bound type rather than only "no issue".composer test:type(509),composer test:unit(797),composer test:app(realroutes/console.php),composer psalm(100% coverage),composer cs,composer rectorall green.Note for the reviewer: the literal repro used a class constant (
self::SIG); the committed regression test uses a private static method (self::sig()/static::sig()) because a bare untyped const tripsMissingClassConstTypeaterrorLevel=1. Same mechanism, and the const form is verified fixed.Checklist
tests/Type/)