Rector rule on `AttributeGroup` nodeType not called on 0.18.4

[etshy]

Bug Report

Subject	Details
Rector version	0.18.4

I made a Rector rule targeting AttributeGroup with rector 0.18.3 and with the latest update, my rule is not called anymore.

    public function getNodeTypes(): array
    {
        return [Node\AttributeGroup::class];
    }

It was a custom rule i made to add/change parameter name after annotations were converted to Attributes.
In my context of migrating Swagger to OpenApi, it was to add parameter name before the new OA\Schema and new OA\Items
For example :

- #[OA\Property(type: 'array', new OA\Items(type: 'integer'))]
+ #[OA\Property(type: 'array', items: new OA\Items(type: 'integer'))]

the rule is called after the AnnotationToAttribute.

The rule worked great on 0.18.3 when i wrote it 2 weeks ago.

Edit : just tested on a project that still have rector 0.18.3, my rule targeting AttributeGroup is called correctly (on class that have attribute, or annotation transformed to attributes by previous rules)

Minimal PHP Code Causing Issue

Demo

Expected Behaviour

Rules targeting AttributeGroup should be called

[samsonasik]

You probably need to apply DocblockUpdater service to re-print phpdoc, see #8201

[etshy]

I'm not sure this is related to DocblockUpdater as my custom rule is not even called and the annotationToAttribute rule is working.

For example :
With Rector 0.18.3, in my config file I only call AnnotationToAttributeRector, with a list of OpenApi annotation and attribute, and then my Rule targeting AttributeGroup to add/change parameter name.

In this case everything work, the annotation is converted to attribute and param name is changed/added

diff result of reactor process:

-     * @OA\Property(type="array", @OA\Items(type="integer"))
+    #[OA\Property(type: 'array', items: new OA\Items(type: 'integer'))]

The exact same config (and rules) with 0.18.4:
the annotation are converted to attributes but my custom rule is not even called.

diff result of reactor process:

-     * @OA\Property(type="array", @OA\Items(type="integer"))
+    #[OA\Property(type: 'array', new OA\Items(type: 'integer'))]

Configurations are the same, targeted files are not the same but very similar

I tracked down the process until I found the rule is being skipped by the line

return $this->rectifiedAnalyzer->hasRectified($rectorClass, $node);

in Skipper class

I tracked again until RectifiedAnalyzer::hasConsecutiveCreatedByRule and found some weird things about the AttributeKey::CREATED_BY_RULE attribute
The method is called 2 times during the process (I guess because I have 2 rules in rector.php file ?) in both version I have always empty array
BUT, the 0.18.4 have a different condition

if ($createdByRule === []) {
    return !$originalNode instanceof Node && \count($node->getAttributes()) <= 1;
}

while 0.18.3 just returned false if $createdByRule === []

in my case, the \count($node->getAttributes()) is always returning 1 (0.18.3 and 0.18.4) so 0.18.4 will return true and skip my rule while 0.18.3 is returning false and my rule is running.

[samsonasik]

Thank you for tracking down, that was used to handle infinite loop:

• [CodingStyle][EarlyReturn] Fix infinite if else on BinarySwitchToIfElseRector+RemoveAlwaysElseRector rector-src#5057

could you debug what var_dump(array_keys($node->getAttributes()) is? probably we can have win win solution.

[etshy]

Yeah just found that PR while looking at the history of the RectifiedAnalyzer.php file

Sure here is the result of the dump

array(1) {
  [0]=>
  string(5) "scope"
}
array(1) {
  [0]=>
  string(5) "scope"
}

(called twice, like I said before)

FYI, I dumped inside a condition to limit to my useCase of AttributeGroup

private function hasConsecutiveCreatedByRule(string $rectorClass, Node $node, ?Node $originalNode) : bool
    {
        $createdByRuleNode = $originalNode ?? $node;
        /** @var class-string<RectorInterface>[] $createdByRule */
        $createdByRule = $createdByRuleNode->getAttribute(AttributeKey::CREATED_BY_RULE) ?? [];
        if ($node instanceof Node\AttributeGroup) {
            var_dump(array_keys($node->getAttributes()));
        }
        if ($createdByRule === []) {
            return !$originalNode instanceof Node && \count($node->getAttributes()) <= 1;
        }
        return \end($createdByRule) === $rectorClass;
    }

[samsonasik]

That only scope attribute seems the reason it was caused infinite loop, that's why check only <=1 attribute is there.

My currrent suggestion is to not use AttributeGroup as it rely on the Stmt it belong.

If you can create reproduced repo of the related custom rule that cause it, it usually 2 rules flip flop, we probably can have better solution.

[etshy]

I can create a quick gist with my custom rule and the rector.php file if that can help.
And an example target file, with just one annotation to convert too.

is that good ?

Edit : nvm, I'll create a minimalistic repo, it will be easier for everyone.

[samsonasik]

minimal repo should be good 👍

[etshy]

Made this quickly but it reproduce my problem.

I downgraded rector to 0.18.3 (composer is set to ^0.18.4, in case you want to downgrade too) and the param name is correctly added

[samsonasik]

I created PR to your repository to refactor the rule, to use Property node instead, and always return node on changed

• Fix AttributeGroup refactor etshy/rector-8237#1

That's the change will be as expected:

➜  rector-8237 git:(master) ✗ vendor/bin/rector --dry-run
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
1 file with changes
===================

1) src/Target/TargetClass.php:1

    ---------- begin diff ----------
@@ @@

 namespace Etshy\Rector8237\Target;

-use OpenApi\Annotations as OA;
+use OpenApi\Attributes as OA;

 class TargetClass
 {
     /**
      * @var array|null
-     * @OA\Property(type="array", @OA\Items(type="integer"))
      */
+    #[OA\Property(type: 'array', items: new OA\Items(type: 'integer'))]
     private ?array $property = [];
 }
    ----------- end diff -----------

Applied rules:
 * MyCustomRule
 * AnnotationToAttributeRector (https://wiki.php.net/rfc/attributes_v2)

[etshy]

Thanks a lot !

It wasn't needed in 0.18.3 and as I couldn't dump in my rule, and when I tracked in rector files I saw my rule was skipped, I didn't thought it was a problem in the rule itself.

Seems to work great so i'l close this issue

[etshy]

Juste something I though this mornig though.
Even if AttributeGroup is not listed in here, as it implement the Node interface people might try to target it, like I did, so maybe it should be listed as exception ?

And I guess there might be some other classes that implement Node but won"t work as rule's target.

[TomasVotruba]

Great idea. I think Rector should throw an exception to inform user immediately.

[samsonasik]

@etshy if the rule is return node on refactor, it will crash on refresh scope immediately, see

rector/src/Application/ChangedNodeScopeRefresher.php

Lines 105 to 106 in fb1a354

	$errorMessage = \sprintf('Complete parent node of "%s" be a stmt.', \get_class($node));
	throw new ShouldNotHappenException($errorMessage);

the reason you don't get error because you return null.

Some Nodes that not refreshable will not have scope refreshed

rector/src/NodeAnalyzer/ScopeAnalyzer.php

Lines 26 to 39 in fb1a354

	private const NON_REFRESHABLE_NODES = [Name::class, Identifier::class, Param::class, Arg::class, Variable::class];
	public function __construct(ScopeFactory $scopeFactory)
	{
	$this->scopeFactory = $scopeFactory;
	}
	public function isRefreshable(Node $node) : bool
	{
	foreach (self::NON_REFRESHABLE_NODES as $noScopeNode) {
	if ($node instanceof $noScopeNode) {
	return \false;
	}
	}
	return \true;
	}

which the scope is never changed.