Fix imprecise types after assignment when strict-types=1 by staabm · Pull Request #3945 · phpstan/phpstan-src

staabm · 2025-04-18T10:15:31Z

work with coecered type only when the assigned one is incompatible with the natively defined property type

supercedes #3943

staabm · 2025-04-18T10:31:28Z

tests/PHPStan/Analyser/nsrt/bug-12902.php

+		self::$i = getInt();
+		assertType('float|int', self::$i); // could be int
+		assertNativeType('float|int', self::$i); // could be int
+
+		$this->impureCall();
+		assertType('float|int', self::$i);
+		assertNativeType('float|int', self::$i);
+	}


the impureCall does not make the scope forget about already narrowed types of static property fetches, therefore we don't narrow in this scenario with this PR.

-> we only improve the instance-call case, because non-static fetches will be forgotten as the test in NarrowsNativeUnion proves

src/Analyser/NodeScopeResolver.php

phpstan-bot · 2025-04-18T12:24:04Z

This pull request has been marked as ready for review.

ondrejmirtes · 2025-04-18T13:29:47Z

I've analyzed this situation. It's much more clear what's going on in this code sample: https://phpstan.org/r/903891e3-323a-494b-83a0-27aff806eb88

It's happening because IntegerType::toCoercedArgumentType() returns TypeCombinator::union($this, $this->toFloat()). And we take that, intrsection that with int|float and assign the value.

But what should happen first is even before we call toCoercedArgumentType, we should check whether the original value (ConstantIntegerType 1) is a subtype of the native property type.

IntegerType->isSuperTypeOf(ConstantIntegerType) -> yes
FloatType->isSuperTypeOf(ConstantIntegerType) -> no

Because 1) matches, we don't need to call toCoercedArgumentType at all. We simply assign the value as it is.

To get a list of types to compare, we can call TypeUtils::flattenTypes on the native property type. That way we can compare it in a foreach even when there's just a single type like int.

What's nice about this is that it can solve the problem for strict_types = 0 as well. The workflow should be:

Go through the types, if we find one that matches, use the assigned expression.
If we don't succeed with 1), check if we're in strict_types = 1. If yes, use the current logic in NodeScopeResolver with toCoercedArgumentType.

If done correctly, this will remove the need for #3943 as well.

ondrejmirtes · 2025-04-19T07:19:05Z

src/Analyser/NodeScopeResolver.php

+								TypeCombinator::intersect($assignedNativeType->toCoercedArgumentType(true), $propertyNativeType),
+							);
+						} else {
+							$scope = $scope->assignExpression($var, $assignedExprType, $assignedNativeType);


This is gonna get executed if assigned type is compatible or if we're not in declare strict types. That's not what we want. We only want to execute this if assigned type is compatible.

so what to execute if not compatible and not strict types?

I think it works now as you imagined.

If not compatible and not strict types, we currently can't do anything. Once we properly implement toCoercedArgumentType in the whole typesystem, then we can use it same way we use it now with strict_types=1.

staabm · 2025-04-19T08:09:24Z

tests/PHPStan/Analyser/nsrt/bug-12902-non-strict.php

+		$this->impureCall();
+		assertType('int', self::$i); // should be float|int
+		assertNativeType('int', self::$i); // should be float|int


this is a unrelated bug, right?

Yeah, probably.

fix in #3950

ondrejmirtes · 2025-04-19T08:24:26Z

src/Testing/TypeInferenceTestCase.php

 			@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) {


This isn't compatible with how php-parallel-lint works.

ondrejmirtes · 2025-04-19T08:24:46Z

tests/PHPStan/Analyser/nsrt/bug-12902-non-strict.php

@@ -0,0 +1,88 @@
+<?php declare(strict_types = 0); // lint >= 8.1


The first line has to be <?php // lint >= 8.1 and the declare statement has to be on line 3.

ondrejmirtes · 2025-04-19T08:25:20Z

tests/PHPStan/Analyser/nsrt/bug-12902-non-strict.php

+		$this->impureCall();
+		assertType('int', self::$i); // should be float|int
+		assertNativeType('int', self::$i); // should be float|int


Yeah, probably.

ondrejmirtes · 2025-04-19T08:57:11Z

Thank you!

staabm · 2025-04-28T15:34:11Z

I am in the process of updating PHPStan to 2.1.13 and stumble now again over the problem, I initially tried to fix with my first iteration of this PR in #3943

see https://phpstan.org/r/aa5d5753-eca2-4735-bebf-6479e1ab1f42

do you think we could remove nullability from the property-type in scope, as the method we call returns a native non-nullable type, even if strict-types=0?

staabm · 2025-04-28T15:50:28Z

proposed a new PR so we can discuss whether its possible and my idea is clear: #3965

staabm commented Apr 18, 2025

View reviewed changes

staabm force-pushed the bug12902 branch from f965c99 to 01f3e93 Compare April 18, 2025 10:34

staabm commented Apr 18, 2025

View reviewed changes

src/Analyser/NodeScopeResolver.php Outdated Show resolved Hide resolved

staabm marked this pull request as ready for review April 18, 2025 12:23

staabm force-pushed the bug12902 branch from e847793 to 2fe66c5 Compare April 19, 2025 07:03

ondrejmirtes requested changes Apr 19, 2025

View reviewed changes

staabm mentioned this pull request Apr 19, 2025

Don't remember non-nullable properties as nullable #3943

Closed

staabm commented Apr 19, 2025

View reviewed changes

ondrejmirtes requested changes Apr 19, 2025

View reviewed changes

staabm added 16 commits April 19, 2025 10:36

Fix imprecise types after assignment when strict-types=1

ad88033

fix

96f8a78

namespace

602ff9c

Update bug-12902.php

7b99a87

works only on instance methods

e78b85a

Update bug-12902.php

7b43638

TypeInferenceTestCase: support declare(); before // lint

eeb6150

coerce native/non-native both or none

b2a788c

implement feedback

75fafa7

Update NodeScopeResolver.php

be51451

fix name collision

c6f0e16

fix

d0a40f8

static property fetch

7fdb85b

test nullable

52e4b42

Discard changes to src/Testing/TypeInferenceTestCase.php

179f0d9

fix lint condition

4e0fe58

staabm force-pushed the bug12902 branch from 1a5ad9d to 4e0fe58 Compare April 19, 2025 08:36

ondrejmirtes merged commit 67fb7a6 into phpstan:2.1.x Apr 19, 2025
416 of 417 checks passed

staabm deleted the bug12902 branch April 19, 2025 08:59

staabm mentioned this pull request Apr 28, 2025

More precise types after assignment when strict-types=0 #3965

Merged

		@@ -0,0 +1,88 @@
		<?php declare(strict_types = 0); // lint >= 8.1

Conversation

staabm commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staabm Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phpstan-bot commented Apr 18, 2025

Uh oh!

ondrejmirtes commented Apr 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ondrejmirtes commented Apr 19, 2025

Uh oh!

staabm commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staabm commented Apr 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

staabm commented Apr 18, 2025 •

edited

Loading

staabm Apr 18, 2025 •

edited

Loading

staabm Apr 19, 2025 •

edited

Loading

staabm commented Apr 28, 2025 •

edited

Loading