Don't remember non-nullable properties as nullable #3943
Conversation
| class KeepsPropertyNonNullable { | ||
| private readonly int $i; | ||
|
|
||
| public function __construct() | ||
| { | ||
| $this->i = getIntOrNull(); | ||
| } | ||
|
|
||
| public function doFoo(): void { | ||
| assertType('int', $this->i); | ||
| assertNativeType('int', $this->i); | ||
| } | ||
| } | ||
|
|
||
| function getIntOrNull(): ?int { | ||
| if (rand(0, 1) === 0) { | ||
| return null; | ||
| } | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
main motivation was this case here, as with types remembered from constructors we otherwise get a lot of followup errors in instance methods
|
This pull request has been marked as ready for review. |
src/Analyser/NodeScopeResolver.php
Outdated
|
|
||
| $scope = $scope->assignExpression($var, TypeCombinator::intersect($assignedExprType->toCoercedArgumentType(true), $propertyNativeType), TypeCombinator::intersect($scope->getNativeType($assignedExpr)->toCoercedArgumentType(true), $propertyNativeType)); | ||
| if ($propertyReflection->hasNativeType() && $scope->isDeclareStrictTypes()) { |
There was a problem hiding this comment.
Please restructure this code. I don't really like two following if statements to repeat the first part of the condition.
if ($propertyReflection->hasNativeType() && $propertyNativeType->isNull()->no()) {
$assignedExprType = TypeCombinator::removeNull($assignedExprType);
$assignedNativeType = TypeCombinator::removeNull($assignedNativeType);
}
$scope = $scope->assignExpression($var, TypeCombinator::intersect($assignedExprType->toCoercedArgumentType(true), $propertyNativeType), TypeCombinator::intersect($scope->getNativeType($assignedExpr)->toCoercedArgumentType(true), $propertyNativeType));
if ($propertyReflection->hasNativeType() && $scope->isDeclareStrictTypes()) {should become:
if ($propertyReflection->hasNativeType()) {
if ($scope->isDeclareStrictTypes()) {
....
} elseif ($propertyNativeType->isNull()->no()) {
....
}
}I'm still not sure whether this is a good idea. I first need to have this code restructured so I can think about it clearly. Thanks.
There was a problem hiding this comment.
restructured, but its not
if ($propertyReflection->hasNativeType()) {
if ($scope->isDeclareStrictTypes()) {
....
} elseif ($propertyNativeType->isNull()->no()) {
....
}
}
because we want $propertyNativeType->isNull()->no() also be considered when strict-types=1
There was a problem hiding this comment.
Please submit a separate PR with test + fix what this code currently achieves with declare(strict_types = 1). I'm trying to understand it. I don't think there's a present bug about that right now.
There was a problem hiding this comment.
I just realized you are right. I can re-structure like you suggested, as there is no bug with strict-types=1 and my fix is only useful for strict-types=0
There was a problem hiding this comment.
I found a bug unrelated to property nullability and created a reported: phpstan/phpstan#12902
| @@ -398,7 +398,7 @@ private static function isFileLintSkipped(string $file): bool | |||
|
|
|||
| @fclose($f); | |||
|
|
|||
| if (preg_match('~<?php\\s*\\/\\/\s*lint\s*([^\d\s]+)\s*([^\s]+)\s*~i', $firstLine, $m) === 1) { | |||
| if (preg_match('~<?php\\s*(?:declare\\s*\([^)]+\)\\s*;\\s*)?\\/\\/\s*lint\s*([^\d\s]+)\s*([^\s]+)\s*~i', $firstLine, $m) === 1) { | |||
There was a problem hiding this comment.
added support for <?php declare(…); // lint >= 8.1-format in gatherAssertTypesFromDirectory
|
looks like we have a better alternative implementation in #3945 |
as it stands right now we only narrow property types from assigments when
declare(strict-types=1)because doing so in non-strict mode is very complicated.I think we can still handle the very common case of non-nullable vs. nullable in non-strict mode as this should work independent from strict-types.
I am not interessted in going down the rabbit hole for all possible types, but I think supporting null brings us already a big step forward
similar to phpstan/phpstan#12393
inspired by phpstan/phpstan#12889 (comment)