Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TypeDeclaration] Handle double declare(strict_types=1) addition on DeclareStrictTypesRector + IncreaseDeclareStrictTypesRector #5928

Merged
merged 7 commits into from
May 30, 2024

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented May 30, 2024

While user should choose one of the rules, when it added both, it cause double addition:

 <?php

+declare(strict_types=1);
+
+declare(strict_types=1);
+

which should only one, ref #5926

@samsonasik
Copy link
Member Author

Fixed 🎉

@TomasVotruba
Copy link
Member

These 2 rules should never be run together, as they approach same issue differently.
Instead we should raise an error and warn about this, like ECS does for conflicting rules.

@samsonasik
Copy link
Member Author

Ok, when using $nodes, the skipping is working ok, but Name is not yet transformed into FullyQualified

@samsonasik
Copy link
Member Author

I am looking for general issue on rector transformation, while both rules should not run together, but the order of stmt should be kept, so it should already skip already.

@samsonasik
Copy link
Member Author

I got it, the current patch is by check current nodes as well, as ->getNewStmts() seems may overlapped with the $nodes passed.

This handle "just added nodes", which the rule maybe different with both, as the ->getNewStmts() is regenerated after all rules applied, see

// this is needed for new tokens added in "afterTraverse()"
$file->changeNewStmts($postNewStmts);

@samsonasik
Copy link
Member Author

The another solution is ensure call:

        // ensure update new stmts on each rules applied
        $this->file->changeNewStmts($nodes);

on AbstractRector::beforeTraverse() itself, so file->getNewStmts() is always updated on each rule that just passed.

This will works on general transformation, not only this kind of conflict usage.

@samsonasik samsonasik marked this pull request as draft May 30, 2024 13:01
@samsonasik samsonasik marked this pull request as ready for review May 30, 2024 14:39
@samsonasik
Copy link
Member Author

Ok, using $nodes seems working fine, refactored with check on $nodes 6bc8e02

It seems we should not use $file->getNewStmts() on rules when possible, as $file->changeNewStmts() only called after Rector rules and Post Rector executed

// this is needed for new tokens added in "afterTraverse()"
$file->changeNewStmts($postNewStmts);

@TomasVotruba I am going to merge it as it may related with how this rule behaviour cross with other rule that change first level stmts ;)

@samsonasik samsonasik merged commit b2d1c9d into main May 30, 2024
39 checks passed
@samsonasik samsonasik deleted the double-declare branch May 30, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants