Resolve Static throw points#4128
Conversation
3e23c43 to
fd0fa25
Compare
fd0fa25 to
021e818
Compare
ondrejmirtes
left a comment
There was a problem hiding this comment.
These are not the right places to do this fix. The MethodReflection has to already answer correctly.
The key lies in UnresolvedMethodPrototypeReflection (interface). ObjectType and StaticType return different implementations of this interface.
The two most important implementations are:
- CallbackUnresolvedMethodPrototypeReflection (used in StaticType)
- CalledOnTypeUnresolvedMethodPrototypeReflection (used in ObjectType)
When called on StaticType, the type has to stay the same/base class only being updated.
When called on ObjectType, the throw type has to be resolved to the current final type.
91d3962 to
afa2bdb
Compare
Ok, so I did the fix in CallbackUnresolvedMethodPrototypeReflection & CalledOnTypeUnresolvedMethodPrototypeReflection and it works fine for the method and the static method. But I still get error for the case 3 and 4 (constructor and property hooks) without a specific fix. Should I just split the PR in two and just test the case 1 and 2 for now (without the fix 3 and 4) which already close the issue and cover most of real-life cases ? |
afa2bdb to
98b340b
Compare
Yes please, then open a PR with failing tests after this one is merged |
bbf8370 to
af6d594
Compare
PR updated |
ondrejmirtes
left a comment
There was a problem hiding this comment.
- Please remove the property hook and the constructor throws and also the 8.4 requirement.
- Please add a thorough test for a dead catch rule to make it obvious this really works properly when calling a method on an object variable vs. on $this. Also that changeBaseClass works.
Sorry I'm not sure to understand what should be the test. Could you give me a snippet ? I don't see how TestDataException or ADataException will be reported as a dead catch |
|
@VincentLanglet Thinking about it, this should rather be a TypeInferenceTestCase in
|
ondrejmirtes
left a comment
There was a problem hiding this comment.
Also, one more place needs fixing. MutatingScope::enterClassMethod should also transformStaticType on $throwType.
Not sure how to test that. Maybe there's already a rule that asks that on $scope->getFunction(). Maybe we need to write a fake one just for tests.
I'm not sure about what I should catch since if you look at https://phpstan.org/r/030ef155-337f-4b7d-8361-7bf9f4f6c225
I assume it should be the same for enterPropertyHook ?
I dunno either since I'm not even sure about the impact of changing enterClassMethod/enterPropertyHook. |
|
Thank you. |
Closes phpstan/phpstan#11900