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! |
closes phpstan/phpstan#12860
closes phpstan/phpstan#10048
closes phpstan/phpstan#11828
closes phpstan/phpstan#9075
closes phpstan/phpstan#6063
closes phpstan/phpstan#12723