feat: add dynamic parameter type extensions#4290
Conversation
49c7da8 to
58e51fb
Compare
58e51fb to
79de0a6
Compare
|
This pull request has been marked as ready for review. |
|
I tested it by porting my previous implementation for Larastan to this one, and looks like it works fine! Will try to implement more use cases and see if I can find any bugs. Also will try to test it on real projects.But so far so good. Thanks for this! I'll also try to review this PR (though Ondrej would do a better job 😄 ) |
79de0a6 to
7c605c1
Compare
ondrejmirtes
left a comment
There was a problem hiding this comment.
This looks solid, just a few questions as a start, will do deep review later 👍
|
@calebdw @ondrejmirtes Sorry to bother you, but I wanted to ask how we can move this forward? I'm really interested in this feature. |
|
No worries, just been busy with personal life, I'll try to get to the comments soon |
b9ce6ac to
b293d32
Compare
|
@ondrejmirtes, @canvural, sorry it took so long to address these comments---just been busy with personal life but hoping to move this forward now 🙏 |
b293d32 to
16430bf
Compare
|
@calebdw - is this still planned on going ahead? Does this fix the issue with using whereHas with custom builders? I've seen multiple projects now look to using custom builders since the implementation of the attribute & official support from Laravel last year, only to find issues when trying to type the arguments in eloquent method for type hints / auto complete, etc. https://laravel-news.com/defining-a-dedicated-query-builder-in-laravel-12-with-php-attributes In the meantime, we have been using whereRelation, which compiles to whereHas under the hood, but it would be nice to use whereHas directly.
|
|
My team are also eagerly awaiting the outcome of this PR! Hope it can be pushed forward 🙏 |
08bc956 to
7823716
Compare
|
@CamKemBell, yes I just rebased and resolved all the conflicts @ondrejmirtes, this is ready to review 👍 |
7823716 to
9e68ce4
Compare
|
What is the status on this PR? |
|
Hi @calebdw sorry for the delay there is now more review for the PHPStan codebase. Are you still interested by the PR ? Thanks ! |
9e68ce4 to
c67e140
Compare
781f1dc to
f59eab2
Compare
|
f59eab2 to
46c462d
Compare
VincentLanglet
left a comment
There was a problem hiding this comment.
-
Please rebase for the conflict
-
This will need a PR on https://github.com/phpstan/phpstan-nette after to use the new non-deprecated interface (should we wait before deprecating the old one (?))
-
The
integration-tests / Integration - shipmonk-rnd/dead-code-detectorfailure seems not normal https://github.com/phpstan/phpstan-src/actions/runs/22313350451/job/64551526195?pr=4290
64bf789 to
0a17a77
Compare
|
What is this one waiting for? 😊 |
|
I think it's mainly conflict and targeting 2.2.x |
|
@VincentLanglet @staabm Once you give a green light and you're happy with this PR, I will take over, take a look and bring it over the finish line 😊 |
| use PHPStan\Reflection\ParameterReflection; | ||
|
|
||
| /** | ||
| * This is the interface for dynamic parameter type extensions for functions. |
There was a problem hiding this comment.
I am looking at this for the first time, and my initial question is:
where to draw the line between FunctionTypeSpecifyingExtension, FunctionParameterOutTypeExtension, DynamicFunctionParameterTypeExtension? when to use which?
(same for Method and StaticMethod ofc)
all of the above can specify/narrow/change types of funtion calls (or function call parameters).
I feel extension implementators will have this question and after we properly discussed the differences, it might make sense to document these
There was a problem hiding this comment.
This is more of a documentation comment right? Not blocking this PR.
There was a problem hiding this comment.
its not blocking, but I am interessted in the anwser. it would help me to get a better understanding for the review
There was a problem hiding this comment.
FunctionTypeSpecifyingExtension is for narrowing the types after a function call. We do not influence the type itself inside that function. And here we have access to the context so we know if we are in a if statement.
FunctionParameterOutTypeExtension I always thought for parameters that are passed by reference, but documentation gives an example without reference 😄 So I guess, it works for normal parameters too. But again we specify how the passed variables type changes after the function call.
DynamicFunctionParameterTypeExtension is when you want the variable type changed for that function body. So it'll be analyzed by PHPStan with the changed type. So compared to others this affects the types for the function body itself. Not after the call. And currently it only works for arrays and closures.
These are my understandings. Hope it's clear and I'm not wrong 😄
There was a problem hiding this comment.
Is everything clear here? 👀
Sorry, if I come up as very pushy but I really want this to move forward. Especially when Ondrej said he can take it over the finish line after approval from other maintainers.
There was a problem hiding this comment.
I think it looks mostly good, a few questions
- if I did not miss it, it seems none of the tests is covering a error case?
- $overriddenType is passed thru a lot of handlers. I think we should have a few more tests which show that the overridden type gets correctly passed thru complex expressions
- I wonder if we really need end-to-end test-cases only or whether we could at least have some in-phpunit suite tests (these are easier to run, debug and maintain)
- you should look into issue-bot results and judge whether these are expected/related (some of them might go-away after rebase)
- we need this PR to target 2.2.x
| } elseif ($keyType instanceof ConstantIntegerType) { | ||
| $nextAutoIndex = $keyType->getValue() + 1; |
There was a problem hiding this comment.
could keyType be a integer range here?
or e.g. null|4
|
@calebdw Would you have time to address the comments? Or if you allow, I can open another PR with your code and fix there. |
|
Yes, sorry been busy---I'll get to this shortly |
0a17a77 to
79dd1ad
Compare
| path: phpstan-dist | ||
| token: ${{ secrets.PHPSTAN_BOT_TOKEN }} | ||
| ref: 2.1.x | ||
| ref: 2.2.x |
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| ref: 2.1.x | ||
| ref: 2.2.x |
There was a problem hiding this comment.
zizmor found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
|
||
| - name: "Install bashunit" | ||
| run: "curl -s https://bashunit.typeddevs.com/install.sh | bash -s e2e/ 0.37.0" | ||
| uses: "TypedDevs/bashunit@ffa9c79e71ecbb9990e777348bc9ba12314b62d0" # 0.39.1 |
|
|
||
| - name: "Install bashunit" | ||
| run: "curl -s https://bashunit.typeddevs.com/install.sh | bash -s e2e/ 0.37.0" | ||
| uses: "TypedDevs/bashunit@ffa9c79e71ecbb9990e777348bc9ba12314b62d0" # 0.39.1 |
79dd1ad to
edb86a0
Compare
edb86a0 to
3e2e16d
Compare
3e2e16d to
9b99f72
Compare
|
Tested the latest PHAR, still works alright 👍🏽 Static analysis fails on CI looks legit though. But didn't happen at runtime 🤔 Maybe didn't hit that code path. |
| @@ -1,3 +1,4 @@ | |||
| --- | |||

Closes phpstan/phpstan#11707, closes phpstan/phpstan#12585
Supersedes #3828, supersedes #3131, supersedes #3823
Hello!
This adds generalized dynamic parameter type extensions and deprecates the parameter closure type extensions per phpstan/phpstan#11707 (comment).
This also fixes the
return.typeand theargument.typeerrors described in phpstan/phpstan#12585 when the parameter type is overridden via the extension.Note
This currently only works for array and closure arguments, if support is desired for other argument types in the future then that will have to be added.
CC: @canvural, @Neol3108
Thanks!