Resolve container binding arguments by name in ContainerBindConcreteWithClosureOnlyRector - #566
Open
chinmaypurav wants to merge 1 commit into
Conversation
ContainerBindConcreteWithClosureOnlyRector read the abstract and concrete
arguments positionally, so named arguments produced invalid code:
$app->singleton(abstract: Foo::class, concrete: fn () => new Foo())
became `$app->singleton(concrete: fn () => new Foo())`, which fatals with
an ArgumentCountError because $abstract is required. Arguments are now
matched to parameters by name where present and by position otherwise, so
reversed named arguments resolve correctly too, and the closure loses its
name when it moves into the abstract position.
The $shared argument was silently dropped as well. Laravel routes a
closure abstract through bindBasedOnClosureReturnTypes(), which overwrites
$concrete, so `bind(Foo::class, $fn, true)` became `bind($fn, true)` and
stopped being shared. It is now emitted as `bind($fn, shared: true)`.
Drop bindIf() and singletonIf() from the rule entirely. Both pass the
abstract to bound(), which uses it as an array offset, so a closure
abstract throws "Cannot access offset of type Closure in isset or empty".
Every transform the rule made on those two methods produced code that
fatals at boot.
Also remove the `$classString instanceof Const_` guard: Arg::$value is
always an Expr and Const_ extends NodeAbstract, so it could never be true.
Fixes driftingly#397
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Fixes #397.
Named arguments produced fatal code
The rule read the abstract and concrete arguments positionally and then dropped index 0, so a call using named arguments lost
$abstract:Container::singleton($abstract, $concrete = null)still requires$abstract, so the result is anArgumentCountError.Arguments are now matched to parameters by
Arg::$namewhen present and by position otherwise. That also picks up the reversed form,singleton(concrete: $fn, abstract: SomeClass::class), which the old positional read skipped entirely. The closure has its name stripped when it moves into the abstract position, so the output is plainsingleton($fn).$sharedwas silently droppedNot part of the issue, but found while writing fixtures for it.
bind(SomeClass::class, $fn, true)becamebind($fn, true). Laravel routes a closure abstract throughbindBasedOnClosureReturnTypes($abstract, $concrete, $shared), which overwrites$concretewith the closure — so thetruelanded in$concreteand was discarded, quietly turning a shared binding into a non-shared one. It is now emitted asbind($fn, shared: true).bindIf()/singletonIf()removed from the ruleAlso outside the issue, and the largest behavioral change here. Both forward the abstract to
bound(), which doesisset($this->bindings[$abstract]). With a closure abstract that throwsTypeError: Cannot access offset of type Closure in isset or empty— neither method has the closure handling thatbind()gained. So every transform this rule made onbindIf/singletonIfproduced code that fatals at boot, named arguments or not. Happy to split this into its own PR if you'd rather review it separately.Minor
Removed the
$classString instanceof Const_guard.Arg::$valueis always anExprandConst_extendsNodeAbstract, so it could never be true; PHPStan flags it asinstanceof.alwaysFalseonce the argument type is narrowed. No behavior change.Tests
New fixtures:
fixture_named_arguments,fixture_shared_argument,skip_conditional_binding,skip_spread_arguments.PHPStan (level max),
duster lint,rector --dry-run, andstructarmedare clean. The full suite goes 673 → 677 tests with the same 1 error / 14 failures that already fail onmain, all in unrelated rules.🤖 Generated with Claude Code