More precise types after assignment when strict-types=0

[staabm]

Improve types on top of #3945 but for the non-strict-types case.

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...?

closes phpstan/phpstan#12946

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

I don't get why this is needed and why the current logic from #3945 doesn't already work for that. Can you explain please?

[staabm]

The logic implemented in the already merged PR only takes the native type from the assignment, when one of the property-native-flattened types is a super-type of the assigment type.

In the given example the native type of the property is not a super-type of the assigment. Thats why the current logic doesn't take effect. Since we are in non-strict type-land the else-if also doesn't narrow the types.

New: as a last resort we therefore only use the native knowledge about non-nullability of the assignment, to narrow the property type in scope.

The example only works because of phpdoc types (and these types are not expressable with native types - aka param-out in this case; similar thing could happen in conditional return types)

[staabm]

scratch the previous comment.

before #3945 we always assigned a type, and since #3945 we no longer do so. we are missing a ELSE case

[phpstan-bot]

This pull request has been marked as ready for review.

[staabm]

with the last commit I made PHPStan always do type-coersion, even if non-strict-types are used.

I think we can do that in the changed areas, because we are working on natively typed properties and these always convert whatever you assign to them (independent from strict-types=0/1)

see e.g. https://3v4l.org/qnsMN

[ondrejmirtes]

See the reason why we can't simply do this:

class Foo
{

	public int $foo;

	public function doFoo(string $s): void
	{
		$this->foo = $s;
		assertType('int', $this->foo);
	}

	public function doBar(): void
	{
		$this->foo = 'foo';
		assertType('int', $this->foo);
	}

}

Reports:

15     Expected type int, actual: *NEVER*¨
21     Expected type int, actual: *NEVER*

On your PR.

I'm fine with the state of assigning from before all the toCoercedArgumentType changes when strict_types is 0. It simply looked like this:

phpstan-src/src/Analyser/NodeScopeResolver.php

Line 5215 in e6dc705

$scope = $scope->assignExpression($var, $assignedExprType, $scope->getNativeType($assignedExpr));

But ->toCoercedArgumentType(true) should only be called and assigned with strict_types = 1.

[staabm]

thanks for the pointer. just to double check: in PHPStan 2.0.12 we get this results for your test:

class Foo
{

	public int $foo;

	public function doFoo(string $s): void
	{
		$this->foo = $s;
		assertType('string', $this->foo); // should be int
	}

	public function doBar(): void
	{
		$this->foo = 'foo';
		assertType("'foo'", $this->foo); // should be int
	}

}

and this is good enough to get back to, even though its wrong for now, right?

[staabm]

copied the test 12393 and changed only the strict_types to 0

[ondrejmirtes]

See the reason why we can't simply do this:

class Foo
{

	public int $foo;

	public function doFoo(string $s): void
	{
		$this->foo = $s;
		assertType('int', $this->foo);
	}

	public function doBar(): void
	{
		$this->foo = 'foo';
		assertType('int', $this->foo);
	}

}

Reports:

15     Expected type int, actual: *NEVER*¨
21     Expected type int, actual: *NEVER*

On your PR.

I'm fine with the state of assigning from before all the toCoercedArgumentType changes when strict_types is 0. It simply looked like this:

phpstan-src/src/Analyser/NodeScopeResolver.php

Line 5215 in e6dc705

$scope = $scope->assignExpression($var, $assignedExprType, $scope->getNativeType($assignedExpr));

But ->toCoercedArgumentType(true) should only be called and assigned with strict_types = 1.

[ondrejmirtes]

I just forcepushed here how the code makes sense to me. We just need to fix the failing tests 😂

[staabm]

thanks for the hints.. I will look into the errors after a lunch break

[staabm]

We just need to fix the failing tests 😂

do you mean adjusting test expectations or fixing the underlying bugs?

[ondrejmirtes]

Fix the bugs

[staabm]

ready for another review. added loads of new tests

[ondrejmirtes]

I didn't mean you to start changing the toCoerced... method. It we do that, we need to do it right.

What I meant right now is to slightly adjust what the "else" branch in NodeScopeResolver does after my force-push.

It's up to you 😊

[staabm]

What I meant right now is to slightly adjust what the "else" branch in NodeScopeResolver does after my force-push.

I don't see a way how a slight change can fullfill the tests

It we do that, we need to do it right.

could you give me an idea whats "wrong" in the current PR?
do you see errors, or do you mean more precise results - like non-empty-string -> int cannot be 0 ..?