Skip named arguments when adding, modifying or removing parameters

[carlos-granados]

The ArgumentAdderRector, ReplaceArgumentDefaultValueRector, FunctionArgumentDefaultValueReplacerRector and RemoveMethodCallParamRector rules add, modify or remove parameters using the position of the parameter. This can be problematic if the function/method call uses named arguments as we may end up modifying the wrong parameter and producing invalid code. This PR modifies these rules so that if there are any named parameters in the calls, the rules are not applied.

Includes new fixtures which confirm the new functionality

[TomasVotruba]

Thanks for PR.
This would disable the upgrade path they're designed for. It should work for named args seamlessly.

What is the exact use case or a set this broke for you?

[carlos-granados]

It happened on a private project, unfortunately I cannot share the code. What I saw is that we had a subproject with a method with many parameters, similar to

public function addMediaFolder(
    string $name,
    ?string $description = null,
    string $mediaType = '',
    string $color = '',
    ... more parameters
 }

And we wanted to change all places where the function was called so that the $mediaType parameter would always be 'PNG'.

The problem is that we had some places where this function was called like this:

$manager->addMediaFolder($folderName, mediaType: '', color:'');

When we tried to replace the value for argument 2 with 'PNG' using the ReplaceArgumentDefaultValueRector rule, it changed the wrong parameter. I added a fix to the rule and decided to review similar rules as well.

(these are made up examples, just to give you the idea, as I said I cannot share the exact code)

See https://getrector.com/demo/14aed97f-da7d-4e27-aeb5-9c77dd3938d5

I don't think these rules can be safely applied if the calls use named parameters, that's why I added some code to disable them for this case, but maybe you can suggest something to keep them working for this usage?

[TomasVotruba]

I see. Can you add function addMediaFolder() to the demo link so it's more clear which arg is which?

The rule should be smart enough to pick up the right position regardless int or arg name.

[carlos-granados]

This is the updated example https://getrector.com/demo/a6398562-3904-4caa-be42-c881cd451692

Notice that the rule here also changes the default value in the MediaManager class. In our use case this does not happen because the code is actually in a different subproject so Rector won't touch it

What do you want to do here? I am not sure how these rules can really be changed so that they handle named arguments correctly

[TomasVotruba]

Thanks for the update!

The rule should work as follows:

-		$manager->addMediaFolder($folderName, mediaType: '', color:'');
+		$manager->addMediaFolder($folderName, mediaType: 'PNG', color: '');

[TomasVotruba]

Can you update it to handle this change? It would benefit all other use cases we already use this rules for.

[carlos-granados]

I'll try to take a look and see if I can think of a way to handle this case (and similar ones for the other rules)

[staabm]

Does this rector has the same problem for splat operator like

->method($a, ...$b);

?

(Splat can also contain named args)

[carlos-granados]

@TomasVotruba I have modified the ArgumentAdderRector rule to process named arguments in a more intelligent way. What it does now:

• If there are named arguments and there is already a named arguments whose name matches the name of the argument that we want to add, skip the rule as this means that the argument is already present
• If the argument is not present and we have named arguments, instead of adding the new argument positionally, we add it as a new named argument at the end of the argument list

Please let me know if you think this approach is correct. If you do, I'll work to implement something similar in the rules which replace or remove arguments

[samsonasik]

I think to get correct position, you can combine it with argumentName from:

$position = $this->argsAnalyzer->resolveArgPosition($args, $argumentName, $position);

see

rector-src/src/NodeAnalyzer/ArgsAnalyzer.php

Lines 35 to 50 in 2e1e9cc

	public function resolveArgPosition(array $args, string $name, int $defaultPosition): int
	{
	foreach ($args as $position => $arg) {
	if (! $arg->name instanceof Identifier) {
	continue;
	}
	if (! $this->nodeNameResolver->isName($arg->name, $name)) {
	continue;
	}
	return $position;
	}
	return $defaultPosition;
	}

[carlos-granados]

Ok, but not sure how that would be useful here

[samsonasik]

The logic is to compare parameter name vs arg name, if equal, use its position whenever they match, otherwise, use default position originally provided.

[carlos-granados]

That can be useful in the rule that tries to replace an argument, but I don't think we need this in this rule which just tries to add a new argument, if the argument already exists we need to skip the rule and if it doesn't, trying to add it positionally can be problematic, so better just add it as a new named argument

[carlos-granados]

@TomasVotruba were you able to take a look at the update I added for the ArgumentAdderRector rule? I wanted to make sure I was on the right track before I looked into the other rules. Thanks!

[TomasVotruba]

Thanks for the ping. Not yet, I usually review once @samsonasik approves to streamline merging if its ok for both of us

[carlos-granados]

@TomasVotruba this won't be ready for merging yet as I have just updated one of the rules I originally modified and just wanted to have your opinion on whether what I had implemented made sense before I worked on the other two cases. Should I go ahead and implement the other updates?

[TomasVotruba]

I have no time for deeper now. Let's do first rule, merge and then take it from there.