[BetterPhpDocParser] Handle parseString() got ShouldNotHappenException

[samsonasik]

Fixes #5267

[samsonasik]

@vhenzl @ERuban please verify if this can fix #5267 , You can try with the following steps:

composer config minimum-stability dev  
composer config prefer-stable true
composer remove rector/rector
composer require --dev rector/rector:dev-fix-5267

# run rector process to the file affected
composer rector -- process --no-progress-bar --dry-run --debug src/RectorBug.php

[samsonasik]

All checks have passed 🎉 @TomasVotruba it is ready for review.

[vhenzl]

Thanks @samsonasik for looking into that, but I don't think this is correct. I had just a quick glance, but the first impression is that you are here treating symptoms instead of the disease cause.

By ignoring the parsing error of a comment with @var and providing empty comment instead, you lose the property type ($abc would be mixed instead of Xyz).

Also, having a logic based on an exception named ShouldNotHappenException isn't IMO correct. As for its name, the exception is for cases that really shouldn't happen (and afaik used mostly to silence PHPStan when it complains about something being null but you know it never should be null - like it this case where CurrentNodeProvider instance should contain the current node).

See these two my comments: #5267 (comment) and #5267 (comment). I believe the problem was introduced in #5250 which removed PhpDocInfoNodeVisitor.php and as a result, PhpDocInfoFactory isn't called and the current node isn't set anymore.

[samsonasik]

@vhenzl thank you for verify, feel free to create other PR if you have better solution ;)

[samsonasik]

@vhenzl I created new commit to verify PropertyFetch by checking for its type from scope as the PropertyFetch is came from the method parameter.

Please verify if that is better solution ;)

[samsonasik]

rebased.

[samsonasik]

All checks have passed 🎉 @TomasVotruba it is ready for review.

[samsonasik]

updated with better approach with check PropertyFetch has PhpDocInfo 57db2fe

[samsonasik]

I refactored to ensure $phpDocInfo->getVarTagValueNode() instanceof VarTagValueNode and use the VarTagValueNode's TypeNode to check with StaticTypeMapper.

[samsonasik]

All checks have passed 🎉 @TomasVotruba it is ready for review.

[ERuban]

Good job, @samsonasik. The problem is disappeared in my project.
So, waiting the release with this fix.
Thank you.

[vhenzl]

@TomasVotruba @samsonasik This isn't correct. Yes, it fixes the issue, but the resolved type of a property fetch is wrong.

There isn't any test for PropertyFetchTypeResolver so I modified NodeTypeResolver::resolver() to output the resolved types like this:

// vendor/rector/rector/packages/node-type-resolver/src/NodeTypeResolver.php
    public function resolve(Node $node): Type
    {
        $type = $this->resolveFirstType($node);
        if (! $type instanceof TypeWithClassName) {
            echo "<1 " . __METHOD__ . ": {$node->getType()} ({$node->name}): ".$type->describe(VerbosityLevel::cache())."\n";
            return $type;
        }

        $type = $this->parentClassLikeTypeCorrector->correct($type);
        echo "<2 " . __METHOD__ . ": {$node->getType()} ({$node->name}): ".$type->describe(VerbosityLevel::cache())."\n";
        return $type;
    }

The original repro example isn't exactly good, so I modified it like this:

// src/Abc.php
class Abc
{
    public function get(string $s): string
    {
        return $s;
    }
}

// src/Xyz.php
class Xyz
{
    /**
     * @var Abc
     */
    public $abc;

    /**
     * @var string
     */
    public $str;
}

// src/RectorBug.php
class RectorBug
{
    public function fails(Xyz $xyz): void
    {
        $xyz->abc->get('xxx');
        $xyz->str->xxx();
    }
}

Rector 0.9.10 correctly prints:

...
[parsing] src/RectorBug.php
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_Variable (xyz): RectorBugTest\Xyz
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_PropertyFetch (abc): Abc
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_Variable (xyz): RectorBugTest\Xyz
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_Variable (xyz): RectorBugTest\Xyz
<1 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_PropertyFetch (str): string
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_Variable (xyz): RectorBugTest\Xyz
[refactoring] src/RectorBug.php
...

Rector with this PR resolves types incorrectly and prints:

[parsing] src/RectorBug.php
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_Variable (xyz): RectorBugTest\Xyz
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_PropertyFetch (abc): RectorBugTest\Xyz
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_Variable (xyz): RectorBugTest\Xyz
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_Variable (xyz): RectorBugTest\Xyz
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_PropertyFetch (str): RectorBugTest\Xyz
<2 Rector\NodeTypeResolver\NodeTypeResolver::resolve: Expr_Variable (xyz): RectorBugTest\Xyz
[refactoring] src/RectorBug.php

[samsonasik]

@vhenzl please create PR with the failing test and fixture for that, thank you.

[vhenzl]

@samsonasik See #5307