Skip to content

Run post rectors after the rules reach a fixed point, so no rule sees an imported short name - #8909

Open
SanderMuller wants to merge 1 commit into
rectorphp:mainfrom
SanderMuller:post-rectors-after-rules-fixed-point
Open

SanderMuller wants to merge 1 commit into
rectorphp:mainfrom
SanderMuller:post-rectors-after-rules-fixed-point

Conversation

@SanderMuller

Copy link
Copy Markdown
Contributor

With withImportNames(), NameImportingPostRector replaces a FullyQualified name with a short Name. The file loop then runs the rules again, and PHPStan resolves that short name as written. When a global class has the same name, the rules get the wrong type.

For example, ExplicitNullableObjectInstanceRector turns $directory === null into ! $directory instanceof \Some\Directory. The import makes that Directory. In the next pass PHPStan reads it as the core \Directory class, so the condition is always true, and RemoveAlwaysTrueIfConditionRector replaces the method body with return;. I hit this with an enum Locale and the intl Locale class. A final class does the same.

Names in the parsed file stay FullyQualified with an originalName, so they resolve correctly. Only names that a post rector imports during the loop are affected.

FileProcessor now runs the rules until they reach a fixed point, and only then the post rectors. The post rectors still repeat until the output is stable, because some imports need a second pass, for example after a conflicting unused use is removed. The rules do not run again after that.

The new test fails on main for the enum and the final class fixtures, and passes with this change. The case with a short name in the source already passed. The full test suite and the e2e projects give the same result as on main.

@samsonasik

Copy link
Copy Markdown
Member

It seems cause rector-doctrine unit test error

https://github.com/rectorphp/rector-src/actions/runs/41433742105/job/125238402868?pr=8909

@samsonasik

Copy link
Copy Markdown
Member

you also may need to rebase latest main, that's may needed to ensure all green

@SanderMuller

Copy link
Copy Markdown
Contributor Author

Thanks, you were right about the rector-doctrine tests. Two helpers there compare a docblock name with Collection or Doctrine\Common\Collections\Collection. Another rule in the set writes \Doctrine\Common\Collections\Collection, with a leading backslash, so neither name matches. On main, the name importer changes it to Collection between passes, and the next pass matches. Without importNames(), the same two fixtures also fail on main.

rectorphp/rector-doctrine#548 adds ltrim() of the backslash in both helpers. With it, the rector-doctrine suite passes on main and on this PR.

The branch is already on the latest main (01a316d), so a rebase does not change this.

@samsonasik

Copy link
Copy Markdown
Member

I restarted the build 👍

Comment thread src/Application/FileProcessor.php Resolved
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