Skip to content

Native type does not know anything about purity #3797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Feb 26, 2025
Merged

Native type does not know anything about purity #3797

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3feb236
Remove purity check from native type
VincentLanglet Jan 26, 2025
d5c8413
Refacto
VincentLanglet Jan 26, 2025
0ae5874
Refacto
VincentLanglet Jan 27, 2025
1719af0
Add test about generic
VincentLanglet Feb 24, 2025
2b8dae7
Fix test
VincentLanglet Feb 25, 2025
58be765
Fix
VincentLanglet Feb 26, 2025
e80620c
Update test
VincentLanglet Feb 26, 2025
210c0b3
Silent error
VincentLanglet Feb 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 71 additions & 17 deletions src/Rules/PhpDoc/VarTagTypeRuleHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@

use PhpParser\Node;
use PhpParser\Node\Expr;
use PHPStan\Analyser\NameScope;
use PHPStan\Analyser\Scope;
use PHPStan\Node\Expr\GetOffsetValueTypeExpr;
use PHPStan\PhpDoc\NameScopeAlreadyBeingCreatedException;
use PHPStan\PhpDoc\Tag\VarTag;
use PHPStan\PhpDoc\TypeNodeResolver;
use PHPStan\Rules\IdentifierRuleError;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\ArrayType;
use PHPStan\Type\FileTypeMapper;
use PHPStan\Type\Generic\GenericObjectType;
use PHPStan\Type\MixedType;
use PHPStan\Type\ObjectType;
Expand All @@ -24,7 +28,12 @@
final class VarTagTypeRuleHelper
{

public function __construct(private bool $checkTypeAgainstPhpDocType, private bool $strictWideningCheck)
public function __construct(
private TypeNodeResolver $typeNodeResolver,
private FileTypeMapper $fileTypeMapper,
private bool $checkTypeAgainstPhpDocType,
private bool $strictWideningCheck,
)
{
}

Expand Down Expand Up @@ -76,7 +85,7 @@ public function checkExprType(Scope $scope, Node\Expr $expr, Type $varTagType):
$errors = [];
$exprNativeType = $scope->getNativeType($expr);
$containsPhpStanType = $this->containsPhpStanType($varTagType);
if ($this->shouldVarTagTypeBeReported($expr, $exprNativeType, $varTagType)) {
if ($this->shouldVarTagTypeBeReported($scope, $expr, $exprNativeType, $varTagType)) {
$verbosity = VerbosityLevel::getRecommendedLevelByType($exprNativeType, $varTagType);
$errors[] = RuleErrorBuilder::message(sprintf(
'PHPDoc tag @var with type %s is not subtype of native type %s.',
Expand All @@ -86,7 +95,7 @@ public function checkExprType(Scope $scope, Node\Expr $expr, Type $varTagType):
} else {
$exprType = $scope->getType($expr);
if (
$this->shouldVarTagTypeBeReported($expr, $exprType, $varTagType)
$this->shouldVarTagTypeBeReported($scope, $expr, $exprType, $varTagType)
&& ($this->checkTypeAgainstPhpDocType || $containsPhpStanType)
) {
$verbosity = VerbosityLevel::getRecommendedLevelByType($exprType, $varTagType);
Expand Down Expand Up @@ -127,22 +136,22 @@ private function containsPhpStanType(Type $type): bool
return false;
}

private function shouldVarTagTypeBeReported(Node\Expr $expr, Type $type, Type $varTagType): bool
private function shouldVarTagTypeBeReported(Scope $scope, Node\Expr $expr, Type $type, Type $varTagType): bool
{
if ($expr instanceof Expr\Array_) {
if ($expr->items === []) {
$type = new ArrayType(new MixedType(), new MixedType());
}

return $type->isSuperTypeOf($varTagType)->no();
return !$this->isAtLeastMaybeSuperTypeOfVarType($scope, $type, $varTagType);
}

if ($expr instanceof Expr\ConstFetch) {
return $type->isSuperTypeOf($varTagType)->no();
return !$this->isAtLeastMaybeSuperTypeOfVarType($scope, $type, $varTagType);
}

if ($expr instanceof Node\Scalar) {
return $type->isSuperTypeOf($varTagType)->no();
return !$this->isAtLeastMaybeSuperTypeOfVarType($scope, $type, $varTagType);
}

if ($expr instanceof Expr\New_) {
Expand All @@ -151,42 +160,87 @@ private function shouldVarTagTypeBeReported(Node\Expr $expr, Type $type, Type $v
}
}

return $this->checkType($type, $varTagType);
return $this->checkType($scope, $type, $varTagType);
}

private function checkType(Type $type, Type $varTagType, int $depth = 0): bool
private function checkType(Scope $scope, Type $type, Type $varTagType, int $depth = 0): bool
{
if ($this->strictWideningCheck) {
return !$type->isSuperTypeOf($varTagType)->yes();
return !$this->isSuperTypeOfVarType($scope, $type, $varTagType);
}

if ($type->isConstantArray()->yes()) {
if ($type->isIterableAtLeastOnce()->no()) {
$type = new ArrayType(new MixedType(), new MixedType());
return $type->isSuperTypeOf($varTagType)->no();
return !$this->isAtLeastMaybeSuperTypeOfVarType($scope, $type, $varTagType);
}
}

if ($type->isIterable()->yes() && $varTagType->isIterable()->yes()) {
if ($type->isSuperTypeOf($varTagType)->no()) {
if (!$this->isAtLeastMaybeSuperTypeOfVarType($scope, $type, $varTagType)) {
return true;
}

$innerType = $type->getIterableValueType();
$innerVarTagType = $varTagType->getIterableValueType();

if ($type->equals($innerType) || $varTagType->equals($innerVarTagType)) {
return !$innerType->isSuperTypeOf($innerVarTagType)->yes();
return !$this->isSuperTypeOfVarType($scope, $innerType, $innerVarTagType);
}

return $this->checkType($innerType, $innerVarTagType, $depth + 1);
return $this->checkType($scope, $innerType, $innerVarTagType, $depth + 1);
}

if ($type->isConstantValue()->yes() && $depth === 0) {
return $type->isSuperTypeOf($varTagType)->no();
if ($depth === 0 && $type->isConstantValue()->yes()) {
return !$this->isAtLeastMaybeSuperTypeOfVarType($scope, $type, $varTagType);
}

return !$type->isSuperTypeOf($varTagType)->yes();
return !$this->isSuperTypeOfVarType($scope, $type, $varTagType);
}

private function isSuperTypeOfVarType(Scope $scope, Type $type, Type $varTagType): bool
{
if ($type->isSuperTypeOf($varTagType)->yes()) {
return true;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still prefer, I return true ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

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

private function isAtLeastMaybeSuperTypeOfVarType(Scope $scope, Type $type, Type $varTagType): bool
{
if (!$type->isSuperTypeOf($varTagType)->no()) {
return true;
}

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

return !$type->isSuperTypeOf($varTagType)->no();
}

/**
* @throws NameScopeAlreadyBeingCreatedException
*/
private function createNameScope(Scope $scope): NameScope
{
$function = $scope->getFunction();

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

}
4 changes: 1 addition & 3 deletions tests/PHPStan/Analyser/AnalyserIntegrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1198,9 +1198,7 @@ public function testBug5091(): void
public function testBug9459(): void
{
$errors = $this->runAnalyse(__DIR__ . '/data/bug-9459.php');
$this->assertCount(1, $errors);
$this->assertSame('PHPDoc tag @var with type callable(): array is not subtype of native type Closure(): array{}.', $errors[0]->getMessage());
$this->assertSame(10, $errors[0]->getLine());
$this->assertCount(0, $errors);
}

public function testBug9573(): void
Expand Down
9 changes: 8 additions & 1 deletion tests/PHPStan/Rules/PhpDoc/VarTagChangedExpressionTypeRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

namespace PHPStan\Rules\PhpDoc;

use PHPStan\PhpDoc\TypeNodeResolver;
use PHPStan\Rules\Rule;
use PHPStan\Testing\RuleTestCase;
use PHPStan\Type\FileTypeMapper;

/**
* @extends RuleTestCase<VarTagChangedExpressionTypeRule>
Expand All @@ -13,7 +15,12 @@ class VarTagChangedExpressionTypeRuleTest extends RuleTestCase

protected function getRule(): Rule
{
return new VarTagChangedExpressionTypeRule(new VarTagTypeRuleHelper(true, true));
return new VarTagChangedExpressionTypeRule(new VarTagTypeRuleHelper(
self::getContainer()->getByType(TypeNodeResolver::class),
self::getContainer()->getByType(FileTypeMapper::class),
true,
true,
));
}

public function testRule(): void
Expand Down
17 changes: 16 additions & 1 deletion tests/PHPStan/Rules/PhpDoc/WrongVariableNameInVarTagRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace PHPStan\Rules\PhpDoc;

use PHPStan\PhpDoc\TypeNodeResolver;
use PHPStan\Rules\Rule;
use PHPStan\Testing\RuleTestCase;
use PHPStan\Type\FileTypeMapper;
Expand All @@ -23,7 +24,12 @@ protected function getRule(): Rule
{
return new WrongVariableNameInVarTagRule(
self::getContainer()->getByType(FileTypeMapper::class),
new VarTagTypeRuleHelper($this->checkTypeAgainstPhpDocType, $this->strictWideningCheck),
new VarTagTypeRuleHelper(
self::getContainer()->getByType(TypeNodeResolver::class),
self::getContainer()->getByType(FileTypeMapper::class),
$this->checkTypeAgainstPhpDocType,
$this->strictWideningCheck,
),
$this->checkTypeAgainstNativeType,
);
}
Expand Down Expand Up @@ -182,6 +188,15 @@ public function testBug4505(): void
$this->analyse([__DIR__ . '/data/bug-4505.php'], []);
}

public function testBug12458(): void
{
$this->checkTypeAgainstNativeType = true;
$this->checkTypeAgainstPhpDocType = true;
$this->strictWideningCheck = true;

$this->analyse([__DIR__ . '/data/bug-12458.php'], []);
}

public function testEnums(): void
{
if (PHP_VERSION_ID < 80100) {
Expand Down
29 changes: 29 additions & 0 deletions tests/PHPStan/Rules/PhpDoc/data/bug-12458.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?php declare(strict_types = 1);

namespace Bug12458;

class HelloWorld
{
/**
* @param list<HelloWorld> $a
*/
public function test(array $a): void
{
/** @var \Closure(): list<HelloWorld> $c */
$c = function () use ($a): array {
return $a;
};
}

/**
* @template T of HelloWorld
* @param list<T> $a
*/
public function testGeneric(array $a): void
{
/** @var \Closure(): list<T> $c */
$c = function () use ($a): array {
return $a;
};
}
}
Loading