Bug Report
| Subject |
Details |
| Rector version |
2.6.6 |
After upgrading from 2.4.5 to 2.6.6 I noticed that ReplaceArgumentDefaultValueRector rewrites an argument that already holds the target constant, so it re-fires on every run
Diffs are printed with no rule listed under Applied rules:, which is probably why they stayed unnoticed before: on 2.4.5 the same input and config produce no diff at all.
Minimal PHP Code Causing Issue
Expected Behaviour
ReplaceArgumentDefaultValueRector: no change for the first call, the argument already holds Cookie::SAMESITE_LAX. It is matched again because ArgumentDefaultValueReplacer::processArgs() compares the resolved value, and the constant resolves back to 'lax'. The existing guard for this case only covers self::/static::/parent:: (isSpecialClassName()), not an imported class constant, and there is no such guard at all in processParams(). The second call (false -> null) is a valid migration, but it should be attributed to the rule in Applied rules.
Guess at the cause: both rules mutate the AST in place without reporting it. processReturn0ToMethod() appends the statement without setting $this->hasChanged, and refactorNew() checks $replacedNode !== $currentNode while processArgs() mutates the argument and returns the very same instance, so in both cases refactor() ends up returning null. On 2.4.5 such undeclared mutations seem to be dropped, on 2.6.6 they are printed, which would explain both the empty Applied rules: and why these rules looked fine before.
Bug Report
After upgrading from 2.4.5 to 2.6.6 I noticed that
ReplaceArgumentDefaultValueRectorrewrites an argument that already holds the target constant, so it re-fires on every runDiffs are printed with no rule listed under
Applied rules:, which is probably why they stayed unnoticed before: on 2.4.5 the same input and config produce no diff at all.Minimal PHP Code Causing Issue
ReplaceArgumentDefaultValueRector, configured with two entries copied verbatim fromconfig/sets/symfony/composer-based.php: https://getrector.com/demo/fa73ba3b-b356-4eaf-a135-a660dc87217cExpected Behaviour
ReplaceArgumentDefaultValueRector: no change for the first call, the argument already holdsCookie::SAMESITE_LAX. It is matched again becauseArgumentDefaultValueReplacer::processArgs()compares the resolved value, and the constant resolves back to'lax'. The existing guard for this case only coversself::/static::/parent::(isSpecialClassName()), not an imported class constant, and there is no such guard at all inprocessParams(). The second call (false->null) is a valid migration, but it should be attributed to the rule inApplied rules.Guess at the cause: both rules mutate the AST in place without reporting it.
processReturn0ToMethod()appends the statement without setting$this->hasChanged, andrefactorNew()checks$replacedNode !== $currentNodewhileprocessArgs()mutates the argument and returns the very same instance, so in both casesrefactor()ends up returningnull. On 2.4.5 such undeclared mutations seem to be dropped, on 2.6.6 they are printed, which would explain both the emptyApplied rules:and why these rules looked fine before.