fix: overly strict ArraySpreadInsteadOfArrayMergeRector

[calebdw]

Hello!

The ArraySpreadInsteadOfArrayMergeRector was unnecessarily skipping valid transformations when it couldn't determine that parameters were explicitly typed as ArrayType or ConstantArrayType. This prevented the rector from transforming many valid array_merge() calls to the more modern spread operator syntax:

class FrameworkParent
{
    /**
     * Get the map of resource methods to ability names.
     *
     * @return array
     */
    protected function resourceAbilityMap()
    {
        return [
            'index' => 'viewAny',
            'show' => 'view',
            'create' => 'create',
            'store' => 'create',
            'edit' => 'update',
            'update' => 'update',
            'destroy' => 'delete',
        ];
    }
}

class Child extends FrameworkParent
{
    #[Override]
    protected function resourceAbilityMap(): array
    {
        return array_merge(parent::resourceAbilityMap(), [
            'cancel'   => 'update',
            'complete' => 'update',
            'hold'     => 'update',
            'ready'    => 'update',
            'start'    => 'update',
        ]);
    }
}

This PR removes overly strict type requirement that prevented these valid transformations---array_merge() itself enforces array types at runtime, it has a strict signature and throws a TypeError if non-arrays are passed.

This is safe for the following reasons:

• Runtime equivalence: If code currently works with array_merge($var1, $var2), then $var1 and $var2 are guaranteed to be arrays at runtime. Converting to [...$var1, ...$var2] maintains the exact same runtime behavior and type requirements.
• Same error behavior: Both array_merge($nonArray) and [...$nonArray] throw errors when given non-arrays, so the transformation doesn't change error handling (https://3v4l.org/2YVCi)
• Advantages: The spread operator has several advantages over array_merge(), there's really no reason to not use it
• PHP < 8.1 compatibility preserved: The rector's existing PHP version checks remain unchanged. For PHP versions below 8.1, the transformation is still correctly skipped for arrays with string keys or when the type is not known

Thanks!

[samsonasik]

since it optional rule and not part of set, I am fine with this 👍

[samsonasik]

Thank you @calebdw

[staabm]

it has a strict signature and throws a TypeError if non-arrays are passed.

This is only true for php8+

https://3v4l.org/oSJcb#veol

[samsonasik]

@staabm oops, yes, working with non array at https://3v4l.org/OWtO5#v7.4.33
I think php version < 8.0 is needed with is array check.

I will create PR for it.

[samsonasik]

@staabm it seems there is already test for php 7.4, see

rector-src/rules-tests/CodingStyle/Rector/FuncCall/ArraySpreadInsteadOfArrayMergeRector/FixturePhp74/skip_simple_array_merge.php.inc

Lines 5 to 11 in 15eb8c2

	class SkipSimpleArrayMerge
	{
	public function run($iter1, $iter2)
	{
	$values = array_merge($iter1, $iter2);
	}
	}

so, the issue is probably it should skip @param array $iter which it rely on docblock on php 7.4, should be skipped. I will look into it.

[staabm]

I am not sure whether there is a bug. Just wanted to point out that one of the assumptions does not work for php7

[samsonasik]

@staabm it is a bug, I created PR:

• [CodingStyle] Skip non-native array type on PHP 7.4 for ArraySpreadInsteadOfArrayMergeRector #7248

to avoid regression.

[github-actions]

This pull request has been automatically locked because it has been closed for 150 days. Please open a new PR if you want to continue the work.