Native type does not know anything about purity #3797

VincentLanglet · 2025-01-25T19:47:52Z

ondrejmirtes

I disagree with this on a conceptual level. With a closure like fn () => sleep(5), only native types are involved and we still know it's impure.

With one-off fixes like that, we'd be chaising our own tail for a long time (finding more stuff to fix all the time, which is frustrating). I propose a different fix. In https://github.com/phpstan/phpstan-src/blob/2.1.x/src/Rules/PhpDoc/VarTagTypeRuleHelper.php there are about 9 isSuperTypeOf calls.

Instead of calling them directly on a type, introduce a private method that accepts $type, $varTagType and calls isSuperTypeOf inside. That way we only need to change a single place.
After calling this isSuperTypeOf, in case an error would be reported, run one more check. Take the $type (the native type of the expression), do Type::toPhpDocNode() and then create a Type again with TypeNodeResolver. This will "clean up" everything that's not expressible with PHPDocs.
Call isSuperTypeOf again with the new $type that's the result of operation from 2).

VincentLanglet · 2025-01-26T10:41:00Z

After calling this isSuperTypeOf, in case an error would be reported, run one more check. Take the $type (the native type of the expression), do Type::toPhpDocNode() and then create a Type again with TypeNodeResolver. This will "clean up" everything that's not expressible with PHPDocs.

TypeNodeResolver::resolve needs a NameScope. How do I get one here ?

ondrejmirtes · 2025-01-26T11:10:00Z

I think null is fine? Or an empty one. We won't be resolving any relative names, I hope.

ondrejmirtes · 2025-01-26T11:10:23Z

Anyway, this is just a guess what could work, maybe it's still going to break for some cases.

VincentLanglet · 2025-01-26T13:00:56Z

I think null is fine? Or an empty one. We won't be resolving any relative names, I hope.

Indeed.

I saw there was 5 occurences of new NameScope(null, []) do you think it could be useful to provide a
NameScope::createEmpty() method ?

VincentLanglet · 2025-01-26T13:23:36Z

src/Rules/PhpDoc/VarTagTypeRuleHelper.php

+
+	private function isSuperTypeOfVarType(Type $type, Type $varTagType): TrinaryLogic
+	{
+		$type = $this->typeNodeResolver->resolve($type->toPhpDocNode(), new NameScope(null, []));


After calling this isSuperTypeOf, in case an error would be reported, run one more check.

Seems like if I do this every time it works. I would say it's avoid two calls to isSuperTypeOf (?)

Also since sometimes checks are

!$this->isSuperTypeOfVarType($type, $varTagType)->yes()

and sometimes it's

$this->isSuperTypeOfVarType($type, $varTagType)->no()

It was unclear to me if I should have call toPhpDocNode for maybe results. So I did it every time.

Please do what I suggested. I don't want to break any existing use-cases, that's why I want to do the "new" thing right before when the original error message would appear. To decrease the number of reported errors, not to potentially incraese it.

Sure, but I don't understand how I'll know "an error would be reported" easily since sometime the usage will be
!$this->isSuperTypeOfVarType($type, $varTagType)->yes() and sometimes it will be
$this->isSuperTypeOfVarType($type, $varTagType)->no().

The following implementation

private function isSuperTypeOfVarType(Type $type, Type $varTagType): TrinaryLogic { $result = $type->isSuperTypeOf($varTagType); if ($result->yes()) { return $result; } $type = $this->typeNodeResolver->resolve($type->toPhpDocNode(), new NameScope(null, [])); return $type->isSuperTypeOf($varTagType); }

is not satisfying since for case where the usage is

$this->isSuperTypeOfVarType($type, $varTagType)->no()

I use the strategy toPhpDocNode for nothing since the first isSuperTypeOf wouldn't have report any error.

I have the implementation
d8b9c48

But I dunno if it's satisfying enough

ondrejmirtes · 2025-01-26T15:49:45Z

src/Rules/PhpDoc/VarTagTypeRuleHelper.php

+
+	private function isSuperTypeOfVarType(Type $type, Type $varTagType): TrinaryLogic
+	{
+		$type = $this->typeNodeResolver->resolve($type->toPhpDocNode(), new NameScope(null, []));


Please do what I suggested. I don't want to break any existing use-cases, that's why I want to do the "new" thing right before when the original error message would appear. To decrease the number of reported errors, not to potentially incraese it.

ondrejmirtes · 2025-01-26T21:53:36Z

We don't need to be too smart. We csn have two different methods - one for !yes(), one for no ().

VincentLanglet · 2025-01-27T23:13:59Z

We don't need to be too smart. We csn have two different methods - one for !yes(), one for no ().

I created isSuperTypeOfVarType and isAtLeastMaybeSuperTypeOfVarType

VincentLanglet · 2025-02-23T19:59:50Z

Friendly ping @ondrejmirtes ; this PR is ready to be re-reviewed :)

ondrejmirtes

I worry the empty NameScope might cause problems for generics. Can you please test it with @template T above a method and then using the T in inline @var PHPDoc tag?

With a failing test I'll come up with a solution to the problem.

ondrejmirtes · 2025-02-24T09:09:07Z

We need a test that executes the line with $type:

		if ($type->isSuperTypeOf($varTagType)->yes()) {
			return true;
		}

		$type = $this->typeNodeResolver->resolve($type->toPhpDocNode(), new NameScope(null, []));

		return $type->isSuperTypeOf($varTagType)->yes();

Please debug what $type is before and after, ideally with $type->describe(VerbosityLevel::precise()).

VincentLanglet · 2025-02-25T16:54:18Z

With a failing test I'll come up with a solution to the problem.

I fixed the test, it's failing now.

var_dump($type->describe(VerbosityLevel::precise()));
$type = $this->typeNodeResolver->resolve($type->toPhpDocNode(), new NameScope(null, []));
var_dump($type->describe(VerbosityLevel::precise()));

gives Closure(): list<T> before and after

ondrejmirtes · 2025-02-26T13:04:14Z

Now instead of new NameScope(null, []), you need to get the right NameScope by doing:

			$function = $scope->getFunction();
			$nameScope = $this->fileTypeMapper->getNameScope(
				$scope->getFile(),
				$scope->isInClass() ? $scope->getClassReflection()->getName() : null,
				$scope->isInTrait() ? $scope->getTraitReflection()->getName() : null,
				$function !== null ? $function->getName() : null,
			);

It's a new method I just added in 1.12.x.

VincentLanglet · 2025-02-26T14:01:27Z

Now instead of new NameScope(null, []), you need to get the right NameScope by doing:

			$function = $scope->getFunction();
			$nameScope = $this->fileTypeMapper->getNameScope(
				$scope->getFile(),
				$scope->isInClass() ? $scope->getClassReflection()->getName() : null,
				$scope->isInTrait() ? $scope->getTraitReflection()->getName() : null,
				$function !== null ? $function->getName() : null,
			);

It's a new method I just added in 1.12.x.

Thanks ! It works well.

ondrejmirtes · 2025-02-26T15:13:38Z

src/Rules/PhpDoc/VarTagTypeRuleHelper.php

+		try {
+			$type = $this->typeNodeResolver->resolve($type->toPhpDocNode(), $this->createNameScope($scope));
+		} catch (NameScopeAlreadyBeingCreatedException) {
+			return false;


Does this mean it's going to be reported, or it's not going to be reported? I'd rather opt for failing silently (to not report an error).

I didn't know exactly what this exception means.

Before my PR we were doing just $type->isSuperTypeOf($varTagType)->no().

Does this mean it's going to be reported,

Yes. But the error was also reported before this PR. This won't introduce new error.
That's why I didn't chose the failing silently solution.

Now, we doing

If $type->isSuperTypeOf($varTagType)->no() was OK before, early return

Try to create a namescope

If we cannot create a namescope => Report an error (But this error is currently already reported by PHPStan because the check $type->isSuperTypeOf($varTagType)->no() was not ok).

If we can create a namescope => use it to remove some false-positive.

Do you still prefer, I return true ?

Yes. This would be an error the user cannot do with much. So I don't want to report it.

src/Rules/PhpDoc/VarTagTypeRuleHelper.php

ondrejmirtes · 2025-02-26T15:53:09Z

Perfect, thank you! From the method naming to the logic, this is top-notch!

ondrejmirtes · 2025-02-26T16:12:28Z

Can you please submit these regression tests? Thanks.

VincentLanglet · 2025-02-26T20:25:22Z

Sure, I opened #3841

VincentLanglet changed the base branch from 2.1.x to 1.12.x January 25, 2025 19:48

VincentLanglet mentioned this pull request Jan 25, 2025

@var with type Closure(): list<HelloWorld> is not subtype of native type Closure(): array phpstan/phpstan#12458

Closed

ondrejmirtes requested changes Jan 26, 2025

View reviewed changes

VincentLanglet force-pushed the nativeTypePure branch from 25b2572 to 5afb299 Compare January 26, 2025 12:58

VincentLanglet force-pushed the nativeTypePure branch from 5afb299 to 45824e4 Compare January 26, 2025 13:01

VincentLanglet commented Jan 26, 2025

View reviewed changes

VincentLanglet requested a review from ondrejmirtes January 26, 2025 13:23

ondrejmirtes requested changes Jan 26, 2025

View reviewed changes

VincentLanglet requested a review from ondrejmirtes January 26, 2025 21:22

VincentLanglet force-pushed the nativeTypePure branch from c4acd25 to aa6ae1d Compare January 27, 2025 23:07

VincentLanglet force-pushed the nativeTypePure branch from aa6ae1d to 1b4409b Compare February 23, 2025 19:37

ondrejmirtes requested changes Feb 24, 2025

View reviewed changes

VincentLanglet requested a review from ondrejmirtes February 25, 2025 16:54

VincentLanglet added 5 commits February 26, 2025 14:09

Remove purity check from native type

3feb236

Refacto

d5c8413

Refacto

0ae5874

Add test about generic

1719af0

Fix test

2b8dae7

VincentLanglet force-pushed the nativeTypePure branch 2 times, most recently from dd605c7 to f0d2c7a Compare February 26, 2025 13:24

Fix

58be765

VincentLanglet force-pushed the nativeTypePure branch from f0d2c7a to 58be765 Compare February 26, 2025 13:26

Update test

e80620c

ondrejmirtes requested changes Feb 26, 2025

View reviewed changes

Silent error

210c0b3

VincentLanglet requested a review from ondrejmirtes February 26, 2025 15:37

ondrejmirtes merged commit 5668c05 into phpstan:1.12.x Feb 26, 2025
309 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native type does not know anything about purity #3797

Native type does not know anything about purity #3797

VincentLanglet commented Jan 25, 2025

ondrejmirtes left a comment

VincentLanglet commented Jan 26, 2025

ondrejmirtes commented Jan 26, 2025

ondrejmirtes commented Jan 26, 2025

VincentLanglet commented Jan 26, 2025

VincentLanglet Jan 26, 2025

ondrejmirtes Jan 26, 2025

VincentLanglet Jan 26, 2025

VincentLanglet Jan 26, 2025

ondrejmirtes Jan 26, 2025

ondrejmirtes commented Jan 26, 2025

VincentLanglet commented Jan 27, 2025

VincentLanglet commented Feb 23, 2025

ondrejmirtes left a comment

ondrejmirtes commented Feb 24, 2025

VincentLanglet commented Feb 25, 2025

ondrejmirtes commented Feb 26, 2025

VincentLanglet commented Feb 26, 2025

ondrejmirtes Feb 26, 2025

VincentLanglet Feb 26, 2025

VincentLanglet Feb 26, 2025

ondrejmirtes Feb 26, 2025

VincentLanglet Feb 26, 2025

ondrejmirtes commented Feb 26, 2025

ondrejmirtes commented Feb 26, 2025

VincentLanglet commented Feb 26, 2025

Native type does not know anything about purity #3797

Native type does not know anything about purity #3797

Conversation

VincentLanglet commented Jan 25, 2025

ondrejmirtes left a comment

Choose a reason for hiding this comment

VincentLanglet commented Jan 26, 2025

ondrejmirtes commented Jan 26, 2025

ondrejmirtes commented Jan 26, 2025

VincentLanglet commented Jan 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Jan 26, 2025

VincentLanglet commented Jan 27, 2025

VincentLanglet commented Feb 23, 2025

ondrejmirtes left a comment

Choose a reason for hiding this comment

ondrejmirtes commented Feb 24, 2025

VincentLanglet commented Feb 25, 2025

ondrejmirtes commented Feb 26, 2025

VincentLanglet commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Feb 26, 2025

ondrejmirtes commented Feb 26, 2025

VincentLanglet commented Feb 26, 2025