Skip to content
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.

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

No longer verify types, when TypeSpecifyingExtension uses $overwrite=true #1011

Merged
merged 5 commits into from Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Rules/Comparison/ImpossibleCheckTypeHelper.php
Expand Up @@ -158,6 +158,12 @@ public function findSpecifiedType(
}

$specifiedTypes = $this->typeSpecifier->specifyTypesInCondition($scope, $node, TypeSpecifierContext::createTruthy());

// don't validate types on overwrite
if ($specifiedTypes->shouldOverwrite()) {
return null;
}

$sureTypes = $specifiedTypes->getSureTypes();
$sureNotTypes = $specifiedTypes->getSureNotTypes();

Expand Down
@@ -0,0 +1,44 @@
<?php declare(strict_types = 1);

namespace PHPStan\Rules\Comparison;

use PHPStan\Rules\Rule;
use PHPStan\Testing\RuleTestCase;

/**
* @extends RuleTestCase<ImpossibleCheckTypeMethodCallRule>
*/
class ImpossibleCheckTypeGenericOverwriteRuleTest extends RuleTestCase
{

private bool $treatPhpDocTypesAsCertain;

public function getRule(): Rule
{
return new ImpossibleCheckTypeMethodCallRule(
new ImpossibleCheckTypeHelper(
$this->createReflectionProvider(),
$this->getTypeSpecifier(),
[],
$this->treatPhpDocTypesAsCertain,
),
true,
$this->treatPhpDocTypesAsCertain,
);
}

public function testNoReportedErrorOnOverwrite(): void
{
$this->treatPhpDocTypesAsCertain = false;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be a property if you're not testing true/false scenarios. So please put "true" directly in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


$this->analyse([__DIR__ . '/data/generic-type-override.php'], []);
}

public static function getAdditionalConfigFiles(): array
{
return [
__DIR__ . '/impossible-check-type-generic-overwrite.neon',
];
}

}
@@ -0,0 +1,58 @@
<?php declare(strict_types = 1);

namespace PHPStan\Rules\Comparison;

use PhpParser\Node\Expr\MethodCall;
use PHPStan\Analyser\Scope;
use PHPStan\Analyser\SpecifiedTypes;
use PHPStan\Analyser\TypeSpecifier;
use PHPStan\Analyser\TypeSpecifierAwareExtension;
use PHPStan\Analyser\TypeSpecifierContext;
use PHPStan\Reflection\MethodReflection;
use PHPStan\Type\MethodTypeSpecifyingExtension;
use PHPStan\Type\StringType;
use PHPStan\Type\Generic\GenericObjectType;

class GenericTypeOverride implements MethodTypeSpecifyingExtension, TypeSpecifierAwareExtension
{

/** @var TypeSpecifier */
private $typeSpecifier;

public function setTypeSpecifier(TypeSpecifier $typeSpecifier): void
{
$this->typeSpecifier = $typeSpecifier;
}

public function getClass(): string
{
return \GenericTypeOverride\Foo::class;
}

public function isMethodSupported(
MethodReflection $methodReflection,
MethodCall $node,
TypeSpecifierContext $context
): bool
{
return $methodReflection->getName() === 'setFetchMode';
}

public function specifyTypes(
MethodReflection $methodReflection,
MethodCall $node,
Scope $scope,
TypeSpecifierContext $context
): SpecifiedTypes
{
$newType = new GenericObjectType(\GenericTypeOverride\Foo::class, [\GenericTypeOverride\Bar::class]);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't how the constructor should be called. The constructor looks like this:

	/**
	 * @api
	 * @param array<int, Type> $types
	 */
	public function __construct(
		string $mainType,
		private array $types,
		?Type $subtractedType = null,
		private ?ClassReflection $classReflection = null,
	)

You need new ObjectType(\GenericTypeOverride\Bar::class) instead.

Copy link
Contributor Author

@staabm staabm Feb 11, 2022

Choose a reason for hiding this comment

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

thx for catching. with this fix, the unit test works like expected:

$ vendor/bin/phpunit tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeGenericOverwriteRuleTest.php
Xdebug: [Step Debug] Time-out connecting to debugging client, waited: 200 ms. Tried: localhost:9003 (through xdebug.client_host/xdebug.client_port) :-(
PHPUnit 9.5.7 by Sebastian Bergmann and contributors.

Warning:       XDEBUG_MODE=coverage or xdebug.mode=coverage has to be set

F                                                                   1 / 1 (100%)

Time: 00:00.424, Memory: 52.00 MB

There was 1 failure:

1) PHPStan\Rules\Comparison\ImpossibleCheckTypeGenericOverwriteRuleTest::testNoReportedErrorOnOverwrite
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'
+'13: Call to method GenericTypeOverride\Foo<int>::setFetchMode() will always evaluate to false.
 '

C:\dvl\Workspace\phpstan-src-staabm\src\Testing\RuleTestCase.php:140
C:\dvl\Workspace\phpstan-src-staabm\tests\PHPStan\Rules\Comparison\ImpossibleCheckTypeGenericOverwriteRuleTest.php:34

I am wondering whether phpstan should have errored about this when scanning the test-case?
as can be seen in the github actions, it did not catch this problem


return $this->typeSpecifier->create(
$node->var,
$newType,
TypeSpecifierContext::createTruthy(),
true
);
}

}
39 changes: 39 additions & 0 deletions tests/PHPStan/Rules/Comparison/data/generic-type-override.php
@@ -0,0 +1,39 @@
<?php

namespace GenericTypeOverride;

use function PHPStan\Testing\assertType;

class Test {
public function doFoo() {
$foo = $this->createGenericFoo();
assertType('Foo<int>', $foo);

// $foo generic will be overridden via MethodTypeSpecifyingExtension
$foo->setFetchMode();
assertType('Foo<Bar>', $foo);
}

/**
* @return Foo<int>
*/
public function createGenericFoo() {

}
}


/**
* @template T
*/
class Foo
{
public function setFetchMode() {

}
}


class Bar
{
}
@@ -0,0 +1,5 @@
services:
-
class: PHPStan\Rules\Comparison\GenericTypeOverride
tags:
- phpstan.typeSpecifier.methodTypeSpecifyingExtension