Remember narrowed types from the constructor when analysing other methods #3930
Conversation
src/Analyser/NodeScopeResolver.php
Outdated
| return 0; | ||
| } | ||
|
|
||
| return [!$a->isStatic(), $a->name->toLowerString() !== '__construct'] <=> [!$b->isStatic(), $b->name->toLowerString() !== '__construct']; |
There was a problem hiding this comment.
We have a better way to ask for constructor, that works for same-named constructors as class on PHP 7.
There was a problem hiding this comment.
ExtendedMethodReflection has no isConstructor method.
I took inspiration from
which seems to be also wrong then?There was a problem hiding this comment.
I looked into adding ExtendedMethodReflection->isConstructor(): TrinaryLogic, but this is incompatible with
PhpMethodFromParserNodeReflection->isConstructor(): bool
any preferences how you want this to be resolved?
There was a problem hiding this comment.
I think we can leave as it is. This is mostly useful for readonly properties anyway, which means __construct() constructors...
|
re-ordering the order in which methods get analyzed affected the ordering of errors in tests... I am not yet sure where/how to compensate that |
|
Here's this line: phpstan-src/src/Testing/RuleTestCase.php Line 244 in 375f68e You could call |
|
I have put it behind bleeding edge and added corresponding opt-in methods for the test-base-classes so we can move forward without the need to adjust all tests |
|
This pull request has been marked as ready for review. |
|
sorry for push bombing :-) - I am finished now |
|
I just realized there's one more thing we could use this for: with natively typed properties (non-nullable), expressions like We should preserve PropertyInitializationExpr and then check for that in IssetCheck. This is for non-readonly properties too. |
great idea. done |
src/Rules/IssetCheck.php
Outdated
| && $expr->name instanceof Node\Identifier | ||
| && $expr->var instanceof Expr\Variable | ||
| && $expr->var->name === 'this' | ||
| && !TypeCombinator::containsNull($propertyReflection->getNativeType()) |
There was a problem hiding this comment.
- Doing
Type::isNull()would make more sense. This example doesn't covermixed(which can include null). - But I think we should pay more attention to
$typeMessageCallbackpassed to IssetCheck. For both the type check and the error message. Something a little bit different happens in EmptyRule which should be covered by a set of tests. (Becauseempty($x)is!isset($x) || $x == false).
src/Rules/IssetCheck.php
Outdated
| $this->propertyDescriptor->describeProperty($propertyReflection, $scope, $expr), | ||
| $operatorDescription, | ||
| ), | ||
| )->identifier(sprintf('%s.neverNullOrUninitialized', $identifier))->build(); |
There was a problem hiding this comment.
This identifier should include property
staabm
force-pushed
the
constr-scope
branch
2 times, most recently
from
75ca7d5 to
e9343d7
Compare
|
Perfect, thank you! |
|
Hi, this looks pretty neat, thx! I was just wondering why e.g. https://phpstan.org/r/9c85e54d-6c87-42ee-84d5-6db7447c3e4e does not work in the same way? Is it possible to remember such expressions too? UPDATE: changed playground snippet, there was a bug.. Sorry, haven't thought too much about how this works, is it related to the fact that other methods might write / change the property of the wrapper object anytime in between the constructor and method call? |
|
we should be able to remember anything which cannot change after constructor was executed (e.g. because its readonly or because the runtime does not allow to change it once set by design). here is the code which decides what will be remembered and what skipped: |
|
cool, thx! I think my real case I experienced yesterday was not using readonly props, so it wouldn't work.. and this one is somewhat super special. I'll think about it maybe :) |
closes phpstan/phpstan#12860
closes phpstan/phpstan#10048
closes phpstan/phpstan#11828
closes phpstan/phpstan#9075
closes phpstan/phpstan#6063
closes phpstan/phpstan#12723