rectorphp / rector

Instant Upgrades and Automated Refactoring of any PHP 5.3+ code
https://getrector.com
MIT License
8.76k stars 687 forks source link

Syntax error in unknown file #6593

Closed Wirone closed 3 years ago

Wirone commented 3 years ago

Bug Report

Subject Details
Rector version e.g. v0.11.40

I am struggling with finding root cause of the problem with "syntax error" when I analyse files with Rector. I get:

vendor/bin/rector process -vvv --dry-run packages/Ecommerce/tests/Unit/Common/EcommerceCommandTest.php
[parsing] packages/Ecommerce/tests/Unit/Common/EcommerceCommandTest.php
[refactoring] packages/Ecommerce/tests/Unit/Common/EcommerceCommandTest.php
    [applying] Rector\CodeQuality\Rector\Name\FixClassCaseSensitivityNameRector
    [applying] Rector\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector
    [applying] Rector\Php52\Rector\Property\VarToPublicPropertyRector
    [applying] Rector\Php74\Rector\Property\TypedPropertyRector
    [applying] Rector\Php74\Rector\Property\RestoreDefaultNullToNullableTypePropertyRector

In ParserAbstract.php line 269:

  [PhpParser\Error]
  Syntax error, unexpected '}', expecting T_VARIABLE on line 792

Exception trace:
  at ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php:269
 PhpParser\ParserAbstract->doParse() at ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php:143
 PhpParser\ParserAbstract->parse() at ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/Parser/Multiple.php:48
 PhpParser\Parser\Multiple->tryParse() at ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/Parser/Multiple.php:31
 PhpParser\Parser\Multiple->parse() at ./vendor/rector/rector/packages/FamilyTree/Reflection/FamilyRelationsAnalyzer.php:114
 Rector\FamilyTree\Reflection\FamilyRelationsAnalyzer->getPossibleUnionPropertyType() at ./vendor/rector/rector/rules/Php74/Rector/Property/TypedPropertyRector.php:139
 Rector\Php74\Rector\Property\TypedPropertyRector->refactor() at ./vendor/rector/rector/src/Rector/AbstractRector.php:248
 Rector\Core\Rector\AbstractRector->enterNode() at ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:176
 PhpParser\NodeTraverser->traverseArray() at ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:105
 PhpParser\NodeTraverser->traverseNode() at ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:196
 PhpParser\NodeTraverser->traverseArray() at ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:105
 PhpParser\NodeTraverser->traverseNode() at ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:196
 PhpParser\NodeTraverser->traverseArray() at ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php:85
 PhpParser\NodeTraverser->traverse() at ./vendor/rector/rector/src/PhpParser/NodeTraverser/RectorNodeTraverser.php:52
 Rector\Core\PhpParser\NodeTraverser\RectorNodeTraverser->traverse() at ./vendor/rector/rector/src/Application/FileProcessor.php:54
 Rector\Core\Application\FileProcessor->refactor() at ./vendor/rector/rector/src/Application/FileProcessor/PhpFileProcessor.php:135
 Rector\Core\Application\FileProcessor\PhpFileProcessor->Rector\Core\Application\FileProcessor\{closure}() at ./vendor/rector/rector/src/Application/FileProcessor/PhpFileProcessor.php:147
 Rector\Core\Application\FileProcessor\PhpFileProcessor->tryCatchWrapper() at ./vendor/rector/rector/src/Application/FileProcessor/PhpFileProcessor.php:136
 Rector\Core\Application\FileProcessor\PhpFileProcessor->refactorNodesWithRectors() at ./vendor/rector/rector/src/Application/FileProcessor/PhpFileProcessor.php:91
 Rector\Core\Application\FileProcessor\PhpFileProcessor->process() at ./vendor/rector/rector/src/Application/ApplicationFileProcessor.php:76
 Rector\Core\Application\ApplicationFileProcessor->processFiles() at ./vendor/rector/rector/src/Application/ApplicationFileProcessor.php:57
 Rector\Core\Application\ApplicationFileProcessor->run() at ./vendor/rector/rector/src/Console/Command/ProcessCommand.php:145
 Rector\Core\Console\Command\ProcessCommand->execute() at ./vendor/rector/rector/vendor/symfony/console/Command/Command.php:274
 RectorPrefix20210725\Symfony\Component\Console\Command\Command->run() at ./vendor/rector/rector/vendor/symfony/console/Application.php:870
 RectorPrefix20210725\Symfony\Component\Console\Application->doRunCommand() at ./vendor/rector/rector/vendor/symfony/console/Application.php:266
 RectorPrefix20210725\Symfony\Component\Console\Application->doRun() at ./vendor/rector/rector/src/Console/ConsoleApplication.php:71
 Rector\Core\Console\ConsoleApplication->doRun() at ./vendor/rector/rector/vendor/symfony/console/Application.php:162
 RectorPrefix20210725\Symfony\Component\Console\Application->run() at ./vendor/rector/rector/bin/rector.php:61
 require_once() at ./vendor/rector/rector/bin/rector:5

process [-n|--dry-run] [-a|--autoload-file AUTOLOAD-FILE] [-o|--output-format [OUTPUT-FORMAT]] [--no-progress-bar] [--no-diffs] [--clear-cache] [--] [<source>...]

The point is that analyzed file doesn't have line 792. Looking at inheritance tree there are 2 files with this line, but each looks fine. I don't know how to find out what's the problem.

Minimal PHP Code Causing Issue

Not known.

Expected Behaviour

Rector should be more helpful with finding the real problem (logs, exception's message).

TomasVotruba commented 3 years ago

Hi,

thanks for reporting. When you run with --debug, what file it crashes on?

What PHP version do you use?

We'll try to improve the exact file location on the error.


From top of my head, there are 3 options (from least probable, to more probable):

Wirone commented 3 years ago

@TomasVotruba I use PHP 7.4.21. Provided output was with --debug (actually I've used -vvv). Most probably tokens are not correct after applying Rector refactoring, when file is about to be saved. But IMO it should not work this way 🙂 Will be great if Rector could handle this in a way that user would know that some rector rule is not able to produce valid syntax (if that's the case).

Wirone commented 3 years ago

@TomasVotruba FYI: the problem is in TypedPropertyRector (or maybe other property-type-related rectors) because when I manually add type to property I can successfully run Rector on that file and I don't get error anymore.

Wirone commented 3 years ago

Last info:

❯ vendor/bin/rector process --debug --dry-run --clear-cache packages/Ecommerce/tests/Unit/Common/EcommerceCommandTest.php
[parsing] packages/Ecommerce/tests/Unit/Common/EcommerceCommandTest.php
[refactoring] packages/Ecommerce/tests/Unit/Common/EcommerceCommandTest.php
    [applying] Rector\Renaming\Rector\Name\RenameClassRector
    [applying] Rector\CodeQuality\Rector\Name\FixClassCaseSensitivityNameRector
    [applying] Rector\CodeQuality\Rector\Class_\CompleteDynamicPropertiesRector
    [applying] Rector\Symfony\Rector\Class_\EventListenerToEventSubscriberRector
    [applying] Rector\Php52\Rector\Property\VarToPublicPropertyRector
    [applying] Rector\Php74\Rector\Property\TypedPropertyRector
    [applying] Rector\Php74\Rector\Property\RestoreDefaultNullToNullableTypePropertyRector
    [applying] Rector\Symfony\Rector\ConstFetch\ConstraintUrlOptionRector
PHP Fatal error:  Uncaught PhpParser\Error: Syntax error, unexpected '}', expecting T_VARIABLE on line 792 in ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php:269
Stack trace:
#0 ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php(143): PhpParser\ParserAbstract->doParse()
#1 ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/Parser/Multiple.php(48): PhpParser\ParserAbstract->parse('<?php\n\n// Start...', Object(PhpParser\ErrorHandler\Throwing))
#2 ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/Parser/Multiple.php(31): PhpParser\Parser\Multiple->tryParse(Object(PhpParser\Parser\Php7), Object(PhpParser\ErrorHandler\Throwing), '<?php\n\n// Start...')
#3 ./vendor/rector/rector/packages/FamilyTree/Reflection/FamilyRelationsAnalyzer.php(114): PhpParser\Parser\Multiple->parse('<?php\n\n// Start. in ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php on line 269
Fatal error: Uncaught PhpParser\Error: Syntax error, unexpected '}', expecting T_VARIABLE on line 792 in ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php:269
Stack trace:
#0 ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php(143): PhpParser\ParserAbstract->doParse()
#1 ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/Parser/Multiple.php(48): PhpParser\ParserAbstract->parse('<?php\n\n// Start...', Object(PhpParser\ErrorHandler\Throwing))
#2 ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/Parser/Multiple.php(31): PhpParser\Parser\Multiple->tryParse(Object(PhpParser\Parser\Php7), Object(PhpParser\ErrorHandler\Throwing), '<?php\n\n// Start...')
#3 ./vendor/rector/rector/packages/FamilyTree/Reflection/FamilyRelationsAnalyzer.php(114): PhpParser\Parser\Multiple->parse('<?php\n\n// Start. in ./vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/ParserAbstract.php on line 269

My colleague did some dump-driven-development and found out that PhpParser\ParserAbstract->parse('<?php\n\n// Start...', Object(PhpParser\ErrorHandler\Throwing)) refers to.... Core_c.php from PHPStorm's stubs, which is reaaaaaaaally weird and I totally don't get why since I don't even execute this command within PHPStorm 😅 I don't know if this will be helpful for you but I thought I can share that finding with you.

ondrejmirtes commented 3 years ago

I don't know how Rector is configured but with PHPStan on PHP 7.x I always parse phpstorm-stubs with Emulative lexer set to PHP 8 so it understands PHP 8-specific syntax in the stubs.

TomasVotruba commented 3 years ago

@Wirone

FYI: the problem is in TypedPropertyRector (or maybe other property-type-related rectors) because when I manually add type to property I can successfully run Rector on that file and I don't get error anymore.

Good! Could you private the minimal code that is causing it? We'll be able to fix it then

Wirone commented 3 years ago

@TomasVotruba I don't think it's strictly related to our code, most probably it's combination of Composer dependencies, PHP version, Rector, bootstrap file and other stuff 😅. Most probably it's a problem mentioned by @ondrejmirtes - PHPStorm's stubs include PHP8-specific code and if Rector uses them on PHP7 runtime it crashes (at least here). I don't know why these stubs are used internally, but looks like they are a root cause of the issue. I can try to provide some minimal code or reproducer-repo, but rather not today.

As I think of it, is it OK that you require PHP ^8.0 in rector-src, you run test suite on PHP 8.0 and then you somehow allow usage of rector/rector for ^7.1 🤔? It's neat, but can cause problems - as you can see.

ondrejmirtes commented 3 years ago

 You run test suite on PHP 8.0 and then you somehow allow usage of rector/rector for ^7.1

Yes, looks like an oversight. PHPStan (which inspired Rector for the rector-src workflow) runs unit tests in phpstan-src on all supported PHP versions - from PHP 7.1 to 8.0.

TomasVotruba commented 3 years ago

@Wirone We need the PHP code to be able to reproduce and then fix the bug.

puniserv commented 3 years ago

PHP 7.4.20 "rector/rector": "0.11.40"

FamilyRelationsAnalyzer::getPossibleUnionPropertyType

$fileName: rector/vendor/rector/rector/vendor/phpstan/phpstan/phpstan.phar/vendor/ondrejmirtes/better-reflection/src/SourceLocator/SourceStubber/../../../../../jetbrains/phpstorm-stubs/Core/Core_c.stub

$ancestorName: 'Countable' $varType: FullyQualifiedObjectType $propertyTypeNode: FullyQualified

Example to replicate issue:


declare(strict_types=1);

use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

return static function (ContainerConfigurator $containerConfigurator): void {
    $containerConfigurator->services()->set(\Rector\Php74\Rector\Property\TypedPropertyRector::class);
};
<?php

declare(strict_types=1);

namespace Hmm\OhNo;

use Hmm\OhNo\Some\BlaBla;
use PHPUnit\Framework\MockObject\MockObject;

use PHPUnit\Framework\TestCase;

class SomeTest extends TestCase
{
    /**
     * @var BlaBla
     */
    private $bla;
    /**
     * @var MockObject&BlaBla
     */
    private $mock;

    protected function setUp(): void
    {
        parent::setUp();
        $this->bla = new BlaBla();
        $this->mock = $this->createMock(BlaBla::class);
    }
}
<?php

declare(strict_types=1);

namespace Hmm\OhNo\Some;

class BlaBla implements \Countable
{
    public function count(): int
    {
        return 0;
    }
}
{
    "require-dev": {
        "rector/rector": "^0.11.40",
        "phpunit/phpunit": "8.5.18"
    },
    "autoload": {
        "psr-4": {
            "Hmm\\OhNo\\": "src"
        }
    }
}
chrisminett commented 3 years ago

Here's an example I was able to strip down to minimum to still produce the error:

rector.php

<?php

declare(strict_types=1);

use Rector\Core\Configuration\Option;
use Rector\Core\ValueObject\PhpVersion;
use Rector\Set\ValueObject\SetList;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

return static function (ContainerConfigurator $containerConfigurator): void {
    $parameters = $containerConfigurator->parameters();
    $parameters->set(Option::PATHS, [__DIR__ . '/src', __DIR__ . '/tests']);
    $parameters->set(Option::PHP_VERSION_FEATURES, PhpVersion::PHP_74);
    $containerConfigurator->import(SetList::PHP_74);
};

ValidateTraitTest.php

<?php

declare(strict_types=1);

namespace Phlib\Beanstalk;

use PHPUnit\Framework\TestCase;

class ValidateTraitTest extends TestCase
{
    /**
     * @var ValidateTrait
     */
    protected $validate;
}

output

$ ./vendor/bin/rector process tests/ValidateTraitTest.php                                                                                                                -1-
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 [ERROR] Could not process "tests/ValidateTraitTest.php" file, due to:
         "Syntax error, unexpected '}', expecting T_VARIABLE:792". On line: 269

env

rector/rector                      0.11.40

PHP 7.4.21 (cli) (built: Jul  5 2021 03:32:42) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.21, Copyright (c), by Zend Technologies
    with Xdebug v3.0.4, Copyright (c) 2002-2021, by Derick Rethans

macOS v11.4
TomasVotruba commented 3 years ago

Thanks for sharing code snippets :+1: Could you create a reproducible repository on Github, so we be sure we reproduce the same code?

git clone ... some-directory
cd some-directory 

composer install
vendor/bin/rector

I'll try to reproduce the bug and see if there is something we can do on our side.

Wirone commented 3 years ago

@TomasVotruba based on @puniserv's comment I've created reproducer repo. Steps:

Docker image should be built, container created and composer install && composer rector should be executed within isolated environment. Of course it should result with Syntax error, unexpected '}', expecting T_VARIABLE.

TomasVotruba commented 3 years ago

@Wirone Thanks for making repository. It will reproducing easier.

I tried it, for PHP 8.0 it works correctly but for PHP 7.4 I can see the bug:

image

Any tips how to fix it?

Thanks

ondrejmirtes commented 3 years ago

@TomasVotruba I've already told you: https://github.com/rectorphp/rector/issues/6593#issuecomment-887680134

ondrejmirtes commented 3 years ago

@TomasVotruba Also you need to run rector-src unit tests on all supported PHP versions, not just PHP 8. PHPStan does the same.

Wirone commented 3 years ago

@TomasVotruba there's Docker stack based on PHP7.4 in reproducer repo, you should run it as I wrote, not through local PHP 🙂 I don't know how to fix it, but I believe @ondrejmirtes' tip is the way to go 😉

TomasVotruba commented 3 years ago

@ondrejmirtes Not sure what should we change in Rector and where.

TomasVotruba commented 3 years ago

@Wirone I saw the pipe and assumed it's bash one. Got it 👍

TomasVotruba commented 3 years ago

@Wirone Running the full command, actually does not report any error for me:

docker-compose run php composer install && composer rector

Failing Github Action in your repository would help to verify if it's local issue or CI.

ondrejmirtes commented 3 years ago

@TomasVotruba Use different PhpParser instances in different places. Can't be more specific than that - I'm not familiar with Rector's codebase.

Wirone commented 3 years ago

@TomasVotruba I am out now, when I'll be at home I'll check, but I run this multiple times yesterday and had that error 😉 and it's what Docker encapsulation does - it's not my/your/local/ci problem since it's executed in the same environment. Try with --clear-cache since I was using docker-compose down between runs just to be sure if provided command will work.

I will try to add Github Actions later.

TomasVotruba commented 3 years ago

@Wirone Clear cache helped :)

@ondrejmirtes I'm looking into it, thanks :+1: I'm surprise Rector does not do the same by default.

TomasVotruba commented 3 years ago

@Wirone This might be the issue https://github.com/rectorphp/rector-src/pull/624

Wirone commented 3 years ago

Yeah, I've just checked and subsequent executions does not result in error, so I've added --clear-cache to composer rector script. Can't work on Github Actions now but maybe it's not required for now 🙂

I've just tried dev-main version in reproducer and it works properly 👍

TomasVotruba commented 3 years ago

@Wirone Thanks for testing :partying_face:

This can become chicken egg problem, as some stubs use PHP 8 and some PHP 7.x (e.g. match exists as keyword on first, but not on second). So it might require more attention and some custom php-stub-per-php-version-parser :D

We'll see. Thank for investigation of the right file and exact place where this fails. I'll relase a new version to make this into stable. Please report any further issues you have :+1:

TomasVotruba commented 3 years ago

Resolved via https://github.com/rectorphp/rector-src/pull/624

Released under https://github.com/rectorphp/rector/releases/tag/0.11.45