Resolve Builder::find($mixed) widening to Collection|TModel|null#1002
Open
alies-dev wants to merge 3 commits into
Open
Resolve Builder::find($mixed) widening to Collection|TModel|null#1002alies-dev wants to merge 3 commits into
Builder::find($mixed) widening to Collection|TModel|null#1002alies-dev wants to merge 3 commits into
Conversation
Builder::find/findOrFail/findOrNew/findOr widen to
`Collection<int, TModel>|TModel|null` when the argument is `mixed`,
because Psalm's `TemplateInferredTypeReplacer` cannot place `mixed` in
either branch of the stub's `T is (array|Arrayable)` conditional and
combines both. Downstream `$model->prop = ...` then surfaces as
`UndefinedPropertyAssignment` on the spurious Collection branch — the
exact false positive invoiceninja hit on
`Client::withTrashed()->find($_client->client_id)` where
`$_client->client_id` is `mixed` from `\DB::select()`'s `list<stdClass>`.
Larastan ships an `@method` overload pair on the stub, but Psalm stores
pseudo-methods last-write-wins in `$storage->pseudo_methods` — overloads
are not portable. `BuilderFindMixedHandler` makes the same trade-off via
a return-type provider: when `$id` is `mixed`, collapse to the
scalar-id branch (`TModel|null` / `TModel` / `TModel|TValue` for
`findOr`). Concrete scalar/array arguments still narrow through the
stub conditional.
Covers `Builder` and the relation classes that re-declare the methods:
`BelongsToMany` (with the `&object{pivot: TPivotModel}` intersection
reconstructed from template index 2), `HasManyThrough`,
`HasOneOrManyThrough`, `HasOneOrMany`. `findOr`'s `TValue` is extracted
from the callback closure at arg 1 or arg 2.
The accepted soundness gap (caller passes `mixed` that is actually an
array → handler reports the scalar branch) is pinned in
`FindMixedTradeOffTest.phpt` so it stays visible. The taint regression
guard in `SafeSsrfEloquentFetch.phpt` drops the now-redundant
`MixedArgument` suppression; the `$id = \$request->input('id')`
assignment still needs `MixedAssignment`.
Closes #975
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
2 participants
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
Builder::find/findOrFail/findOrNew/findOrwiden toCollection<int, TModel>|TModel|nullwhen the argument ismixed. Psalm'sTemplateInferredTypeReplacercannot placemixedin either branch of the stub'sT is (array|Arrayable)conditional, so it combines both. Downstream$model->prop = ...then surfaces asUndefinedPropertyAssignmenton the spurious Collection branch.The motivating real-world case is invoiceninja's
Client::withTrashed()->find($_client->client_id)where$_clientis a\stdClassfrom\DB::select()— every property read ismixedeven though the runtime value is scalar.Related
Closes #975
Solution Description
Larastan ships an
@methodoverload pair on the stub. Psalm stores pseudo-methods last-write-wins in$storage->pseudo_methods, so the overload approach is not portable. NewBuilderFindMixedHandler(MethodReturnTypeProviderInterface) collapses the mixed case to the scalar-id branch via the same trade-off:find→TModel|nullfindOrFail/findOrNew→TModelfindOr→TModel|TValue(TValue from the callback closure at arg 1 or 2)Concrete scalar/array arguments still narrow through the stub conditional.
Covers
Builderand the four relation classes that re-declare the methods.BelongsToMany's&object{pivot: TPivotModel}intersection is reconstructed from template index 2; other relations have no pivot.The accepted soundness gap (mixed-truly-array → wrong narrowing) is pinned in
FindMixedTradeOffTest.phptso future readers see it.Verification: 423/423 type tests, 632 unit, app exit 0, psalm self-check clean, cs clean.
Checklist
.phptfiles undertests/Type/tests/Builder/)