[dx] add test to check that a rule is not applied if it does not change the code #6794

carlos-granados · 2025-03-20T19:14:19Z

In several cases we have found previously that a rule was applied (it returned a Node object from the refactor() method) even when it had not actually changed the code. A couple of examples:

Fixes the MessageHandlerInterfaceToAttributeRector rule reported as always applied rector-symfony#584
Do not change method if name does not need to change rector-symfony#700

This generates two problems:

We have reduced performance as we have to deal with a changed node even though it has not actually changed
And, more important, we may be reporting that a rule has been applied without any real code change and this can be quite confusing for the users who might be left wondering why a rule seems to have been applied without need

In this PR we implement a fail-safe check that applies to all our tests where we test that a file has not changed. If in this case we detect that a rule has been applied, then we make this test fail so that this kind of problems can be detected early while the rule is in development.

This PR also fixes several cases in our current tests which had this problem.

If this gets accepted, I will work to fix similar cases in the rector-symfony, rector-doctrine, rector-phpunit and rector-downgrade-php repos

...s/Rector/FuncCall/FunctionArgumentDefaultValueReplacerRector/Fixture/version_compare.php.inc

rules-tests/Php70/Rector/FuncCall/MultiDirnameRector/Fixture/fixture.php.inc

rules-tests/Php70/Rector/FuncCall/MultiDirnameRector/Fixture/skip.php.inc

rules/Arguments/ArgumentDefaultValueReplacer.php

rules/CodeQuality/Rector/If_/ConsecutiveNullCompareReturnsToNullCoalesceQueueRector.php

rules/Php70/Rector/FuncCall/MultiDirnameRector.php

rules/Transform/Rector/ClassMethod/WrapReturnRector.php

rules/TypeDeclaration/Rector/ClassMethod/AddParamTypeBasedOnPHPUnitDataProviderRector.php

rules/TypeDeclaration/Rector/ClassMethod/AddReturnArrayDocblockBasedOnArrayMapRector.php

rules/TypeDeclaration/Rector/ClassMethod/ParamTypeByMethodCallTypeRector.php

carlos-granados · 2025-03-20T19:17:50Z

src/Application/FileProcessor.php

            $file->changeHasChanged(true);
+        }

+        $rectorWithLineChanges = $file->getRectorWithLineChanges();


If we have applied any rector rule, we create a FileDiff object even if the code has not actually changed. This FileDiff object will have an empty diff.

carlos-granados · 2025-03-20T19:18:05Z

src/Testing/PHPUnit/AbstractRectorTestCase.php

            $this->assertStringMatchesFormat($expectedFileContents, $changedContents, $failureMessage);
        }
+
+        if ($inputFileContents === $expectedFileContents && $numAppliedRectorClasses > 0) {


If we expect that the code should not change but there has been any Rector rule applied then we throw an error to let the user know that this is incorrect

carlos-granados · 2025-03-20T19:18:18Z

src/Testing/PHPUnit/ValueObject/RectorTestResult.php

        $rectorClasses = [];

-        foreach ($this->processResult->getFileDiffs() as $fileDiff) {
+        foreach ($this->processResult->getFileDiffs(false) as $fileDiff) {


Here we want to get all file diffs, including those where there was no actual change

carlos-granados · 2025-03-20T19:18:32Z

src/ValueObject/ProcessResult.php

     * @return FileDiff[]
     */
-    public function getFileDiffs(): array
+    public function getFileDiffs(bool $onlyWithChanges = true): array


In most cases we will just be interested in the file diffs where there was an actual change, so we filter out those where there was no change recorded

carlos-granados · 2025-03-20T19:18:47Z

tests/Issues/ScopeNotAvailable/ForeachValueScopeTest.php

    #[DataProvider('provideData')]
    public function test(string $filePath): void
    {
+        $this->expectException(RectorRuleShouldNotBeAppliedException::class);


In this particular test, the refactor() function returns on purpose a Node element even though it does not change it. This is done to confirm that the code that handles this node does not throw an error. So in this particular case we need to confirm that we know that this rule would be applied even though it does not change the code

use ob_start() and ob_get_clean() to get echo warning should be the way on this on purpose test

carlos-granados · 2025-03-20T19:19:03Z

tests/Testing/RectorRuleShouldNotBeApplied/RectorRuleShouldNotBeAppliedTest.php

+    #[DataProvider('provideData')]
+    public function test(string $filePath): void
+    {
+        $this->expectException(RectorRuleShouldNotBeAppliedException::class);


To test the new functionality we run a rule that returns a Node even though it does not change it and then we confirm that we catch this case and throw an exception informing us of the problem

TomasVotruba · 2025-03-20T19:22:21Z

Thanks for the PR 👍 It seems quite heavy to review.
Could you extract some easy-pick PRs, so we can handle it sooner?

carlos-granados · 2025-03-20T20:23:49Z

@TomasVotruba I can separate the fixes to the existing tests from the actual new test that revealed them, let me do that

carlos-granados · 2025-03-20T20:53:08Z

I have created some PRs, need to leave now, will continue tomorrow

carlos-granados · 2025-03-21T14:24:33Z

@TomasVotruba now that all the changes to the existing rules have been merged I have rebased this PR to only leave the changes for the new test

samsonasik · 2025-03-21T14:31:41Z

src/Testing/PHPUnit/AbstractRectorTestCase.php

+        if ($inputFileContents === $expectedFileContents && $numAppliedRectorClasses > 0) {
+            $failureMessage = $fixtureNameMessage . PHP_EOL . PHP_EOL
+                . 'File not changed but some Rector rules applied:' . PHP_EOL . $appliedRulesList;
+            throw new RectorRuleShouldNotBeAppliedException($failureMessage);


I prefer symfony warning instead of throw to make less noise or make return on purpose on test

Thanks @samsonasik, not sure I understand what you mean, can you provide some example code?

Just simple echo seems enough:

echo 'File not changed but some Rector rules applied:' . PHP_EOL . $appliedRulesList . PHP_EOL,

Actually, there is SymfonyStyle service, but seems skipped under unit test

rector-src/src/Console/Command/ProcessCommand.php

Lines 111 to 115 in 9939bd2

$this->symfonyStyle->warning(sprintf(

'These rules are registered in both sets and "withRules()". Remove them from "withRules()" to avoid duplications: %s* %s',

PHP_EOL . PHP_EOL,

implode(' * ', $setAndRulesDuplicatedRegistrations) . PHP_EOL

));

so only e2e test can show the warning.

carlos-granados · 2025-03-21T16:08:49Z

@samsonasik I have updated the code to print a warning instead of throwing an error, you can see how it looks here, for example: https://github.com/rectorphp/rector-src/actions/runs/13995973300/job/39191131180?pr=6794

samsonasik · 2025-03-22T20:01:47Z

Looks good 👍


Runtime:       PHP 8.2.28
Configuration: /home/runner/work/rector-src/rector-src/phpunit.xml

...............................................................  63 / 184 ( 34%)
..............
WARNING: On fixture file "skip_already_added.php.inc" for test "Rector\Doctrine\Tests\CodeQuality\Rector\Class_\YamlToAttributeDoctrineMappingRector\YamlToAttributeDoctrineMappingRectorTest"
File not changed but some Rector rules applied:
 * Rector\Doctrine\CodeQuality\Rector\Class_\YamlToAttributeDoctrineMappingRector

Test keep running ok so we can continue improve while wait for improving on the rector rules 👍

samsonasik · 2025-03-27T15:05:03Z

This may need verify on changed by DocblockUpdater service, which change the comment attribute

carlos-granados · 2025-03-27T18:52:09Z

This may need verify on changed by DocblockUpdater service, which change the comment attribute

I don't think this would be a problem. Even if the DocblockUpdater updates the comments, this would either change the final code of the file or not. If it does not change it and we have a test that checks that it does not change and a rule is applied, then we flag this as a warning. Any other case is ignored by this code

samsonasik · 2025-03-27T18:59:43Z

I checked on rector-symfony rules, it seems somehow change docblock not really detected, which may need to be verified more...

carlos-granados · 2025-03-27T19:01:20Z

I checked on rector-symfony rules, it seems somehow change docblock not really detected, which may need to be verified more...

Mmm, let me test this over the weekend, I'll let you know

carlos-granados · 2025-03-28T06:22:19Z

@samsonasik I tested the rector-symfony repo with this code and I could not see any case of docblock change not detected. I fixed all the cases detected by this PR and created another PR in that repo rectorphp/rector-symfony#711 so from my side I am not seeing anything that does not work as expected. If you still see some issues, can you please provide more information? Thanks!

carlos-granados · 2025-03-31T09:00:48Z

While working on the Doctrine repo changes I realised that we need to change the PhpDocTypeChanger to return a boolean when changing the type for a @var doc tag. Is this what you meant, @samsonasik ? I have added the change here, let me know if you would prefer to see it as a separate PR

samsonasik · 2025-03-31T09:17:20Z

Yes, separate PR would be better 👍

carlos-granados · 2025-03-31T09:32:37Z

Yes, separate PR would be better 👍

OK, moved to a new PR #6813

…ge the code

carlos-granados · 2025-04-12T09:03:40Z

@TomasVotruba @samsonasik all the issues in the rector/* repos have now been fixed so this should be ready to go if you believe it is OK

samsonasik · 2025-04-12T11:51:18Z

I am restarting packages tests to see if no longer warning in tests

samsonasik · 2025-04-12T15:33:29Z

package tests seems still have warnings, ref https://github.com/rectorphp/rector-src/actions/runs/14168592413/job/40439865009?pr=6794

samsonasik · 2025-04-12T15:36:23Z

rectorphp/rector-phpunit#475

samsonasik · 2025-04-12T15:42:33Z

It seeems ok now, let's give it a try, thank you @carlos-granados

TomasVotruba · 2025-04-12T16:43:29Z

Thank you @carlos-granados 🥳