Skip to content

[PostRector] Keep imports used in an earlier docblock of a node with stacked docblocks - #8507

Closed
Adelagric wants to merge 1 commit into
rectorphp:mainfrom
Adelagric:fix/unused-import-stacked-docblocks
Closed

Adelagric wants to merge 1 commit into
rectorphp:mainfrom
Adelagric:fix/unused-import-stacked-docblocks

Conversation

@Adelagric

Copy link
Copy Markdown

With ->withImportNames(removeUnusedImports: true), a node carrying two docblocks loses the imports referenced only in the first one:

use App\Attr\First;
use App\Attr\Second;

final class Foo
{
    /**
     * @First
     */
    /**
     * @Second
     */
    public $bar;
}

use App\Attr\First; is removed, with no rule applied. The same two annotations in a single docblock are fine; with three stacked docblocks, the first two imports go.

Cause: UnusedImportRemovingPostRector::findNamesInDocBlocks() checks each docblock on a clone $node with setDocComment($doc), but the clone keeps the PHP_DOC_INFO attribute already resolved for the original node — i.e. its last docblock — so createFromNodeOrEmpty() returns that cached PhpDocInfo instead of parsing $doc. Only the last docblock is ever scanned. It shows as soon as anything resolved the node's docblock earlier in the run: DocblockNameImportingPostRector with importDocBlockNames (the default), or any rule reading the docblock — which is why the existing fixtures, whose configs do neither, pass.

Fix: clear the attribute on the clone. Fixture added to tests/Issues/NamespacedUseAutoImport (its config already enables both removeUnusedImports() and importNames()); it fails on main (-use Carbon\CarbonInterface;) and passes with the change. Full suite: 5290 tests green; ecs and phpstan report nothing new on the changed file.

…stacked docblocks

UnusedImportRemovingPostRector checks each docblock of a node on a clone
with setDocComment(), but the clone keeps the PHP_DOC_INFO attribute
already resolved for the original node (its last docblock), so the factory
returns that PhpDocInfo instead of parsing the docblock: only the last
docblock is scanned and the imports used by the earlier ones are removed.
Clear the attribute on the clone.
@TomasVotruba

Copy link
Copy Markdown
Member

This would break a docblock standard that is generally recognized and would lead to unexpected consequences.

Fix the docblock to one instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants