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

Updates for PHPStan 2.0 #48

Closed
wants to merge 2 commits into from

Conversation

carlos-granados
Copy link
Contributor

@carlos-granados carlos-granados commented Nov 15, 2024

Updates needed to update this to run with PHPStan 2.0

To install locally I had to remove the references to rector/rector and symplify/phpstan-extensions in composer.json. This should be updated once these dependencies are updated

@@ -40,12 +40,12 @@ public function resolve(MethodCall|MethodCallableNode $methodCallOrMethodCallabl
return null;
}

if (! $callerType instanceof TypeWithClassName) {
if (count($callerType->getObjectClassNames()) !== 1) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -13,7 +13,7 @@
use PhpParser\Node\NullableType;
use PhpParser\Node\Stmt\ClassMethod;
use PhpParser\Node\UnionType;
use PhpParser\PrettyPrinter\Standard;
use PHPStan\Node\Printer\Printer;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHPStan has removed the PhpParser\PrettyPrinter\Standard alias and now we need to use PHPStan\Node\Printer\Printer directly

{
$this->printerStandard = new Standard();
public function __construct(
private Printer $printer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHPStan complains if we try to instantiate this directly, it needs to be passed through dependency injection

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to create new Standard printer directly, as we lightweight. We don't need any special printer here

@@ -56,6 +54,7 @@ public function printArgTypesAsString(MethodCall $methodCall, ExtendedMethodRefl
return ResolvedTypes::UNKNOWN_TYPES;
}

// @phpstan-ignore phpstanApi.instanceofType
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case there is no good alternative to checking that it is an instance of IntersectionType (at least I was not able to find one), so I have added a ignore comment

Copy link
Member

Choose a reason for hiding this comment

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

Better place it in phpstan.neon, so we have all ignores in one place.

@@ -160,15 +159,15 @@ private function resolveSortedTypes(UnionType|NodeIntersectionType $paramType, ?

private function printTypeToString(Type $type): string
{
if ($type instanceof ClassStringType) {
if ($type->isClassString()->yes()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHPStan recommends using these alternatives

@@ -26,7 +27,7 @@ final class ReflectionParser
public function __construct()
{
$parserFactory = new ParserFactory();
$this->parser = $parserFactory->create(ParserFactory::PREFER_PHP7);
$this->parser = $parserFactory->createForVersion(PhpVersion::fromString('7.4'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHPParser 5 has removed the create method. I used the createForVersion instead, setting this as 7.4. I am not really sure that this is the right solution

Copy link
Member

Choose a reason for hiding this comment

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

I think there is some createForLatestPhp(), that should be used instead so we don't create fixed value here

Copy link
Member

Choose a reason for hiding this comment

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

@@ -110,10 +110,6 @@ private function validateArgVsParamTypes(array $args, MethodCall $methodCall, Sc
continue;
}

if (! $arg instanceof Arg) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$arg will always be an Arg, this check is not needed

@@ -43,6 +43,7 @@ public function getNodeType(): string
public function processNode(Node $node, Scope $scope): array
{
$varType = $scope->getType($node->var);
// @phpstan-ignore phpstanApi.instanceofType
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, there is no good alternative or at least I was not able to find one

@@ -55,6 +55,7 @@ public function processNode(Node $node, Scope $scope): array
}

$parentClassMethodReflection = $this->methodNodeAnalyser->matchFirstParentClassMethod($scope, $classMethodName);
// @phpstan-ignore phpstanApi.instanceofAssumption
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not covered by PHPStan's BC guarantee, for now we ignore it

@@ -16,7 +16,7 @@
final class NarrowPrivateClassMethodParamTypeRuleTest extends RuleTestCase
{
/**
* @param mixed[] $expectedErrorsWithLines
* @param list<array{0: string, 1: int, 2?: string|null}> $expectedErrorsWithLines
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHPStan asks us to add a more detailed type hint

@carlos-granados
Copy link
Contributor Author

@TomasVotruba @samsonasik updated

@@ -26,7 +26,7 @@ final class ReflectionParser
public function __construct()
{
$parserFactory = new ParserFactory();
$this->parser = $parserFactory->create(ParserFactory::PREFER_PHP7);
$this->parser = $parserFactory->createForNewestSupportedVersion();
Copy link
Member

Choose a reason for hiding this comment

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

it needs verify under php 7.x, since rector downgraded to php 7.2, will verify after PR on rector side finished

currently still wip :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I said I wasn't sure what to do here and why I initially used createForVersion(PhpVersion::fromString('7.4')) I'll let you decide what really needs to go here.

BTW the new Rector probably needs to downgrade to PHP 7.4 since PHPStan dropped support for 7.2 and 7.3, but again not really sure

Copy link
Member

Choose a reason for hiding this comment

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

I see, Rector require phpstan, so php 7.4 will be minimum then ;)

Copy link
Member

Choose a reason for hiding this comment

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

I tested on rector side, with ->createForNewestSupportedVersion(), it got error on curly array $string{0}

There was 1 error:

1) Rector\Tests\Php74\Rector\ArrayDimFetch\CurlyToSquareBracketArrayStringRector\CurlyToSquareBracketArrayStringRectorTest::test with data set #0 ('/Users/samsonasik/www/rector-...hp.inc')
PHPStan\Parser\ParserErrorsException: Syntax error, unexpected '{', expecting ';', Syntax error, unexpected '{', expecting ';', Syntax error, unexpected '{', expecting ';'

phar:///Users/samsonasik/www/rector-src/vendor/phpstan/phpstan/phpstan.phar/src/Parser/RichParser.php:70

updating to :

createForVersion(PhpVersion::fromString('7.0'))

make it working:

PHPUnit 10.5.38 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.12
Configuration: /Users/samsonasik/www/rector-src/phpunit.xml

.                                                                   1 / 1 (100%)

Time: 00:00.316, Memory: 62.50 MB

OK (1 test, 2 assertions)

see rectorphp/rector-src#6431 (comment)

this needs fallback to php 8.0 tho, I will keep updating ;)

Copy link
Contributor Author

@carlos-granados carlos-granados Nov 16, 2024

Choose a reason for hiding this comment

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

Let me know if I can help in any way

Copy link
Member

@samsonasik samsonasik Nov 17, 2024

Choose a reason for hiding this comment

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

I got it, on rector side, using ->createForNewestSupportedVersion() needs to be on default, then if it catch PHPStan\Parser\ParserErrorsException, fallback to use php 7 parser when doing parsing, see rectorphp/rector-src@3bb03c6

If parsing process doesn't involve phpstan, you may need to use PhpParser\Error for catch and retry with createForVersion(PhpVersion::fromString('7.0')), something like:

 try {
            // parse with newest supported version
        } catch (\PhpParser\Error) {
            // parse with php 7 parser
        }

@mitelg
Copy link
Contributor

mitelg commented Nov 29, 2024

hey @carlos-granados

thanks for your current work! 💪 we are currently waiting for this extension to be compatible with PHPStan 2, so I want to offer my help 🙂 let me know, if I can somehow support here

@carlos-granados
Copy link
Contributor Author

@mitelg it is all now in the hands of the Rector team. I am guessing that they first want to release Rector 2.0 (which is happening on 12/12) and then they'll look into the other dependencies. You may enquire in this issue rectorphp/rector#8815

@samsonasik
Copy link
Member

I cherry-picked your commits at PR:

to continue work on it.

@TomasVotruba
Copy link
Member

Cherry picked in #49,

Thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants