Add Mutators: SpreadAssignment, SpreadRemoval #1529
Conversation
| Assert::allNotNull($node->items); | ||
|
|
||
| return $node->items[0]->unpack; |
There was a problem hiding this comment.
the assertion is more for PHPStan because from what I understand and see in other examples the items will be ArrayItem, but the type here is ArrayItem|null. Let me know if I'm missing something, it's the first time I'm working with the php parser
AlexandruGG
changed the title
|
@maks-rafalko hey 👋, can you please run the workflows on this? Also, it should be ready for review |
| null, | ||
| <<<'DIFF' | ||
| - $x = [...$collection, 4, 5]; | ||
| + $x = [$collection, 4, 5]; |
There was a problem hiding this comment.
This particular mutator will be killed by static analyzers, which makes it useless.
Please, see the following Psalm examples:
$x = [...$collection, 4, 5];- https://psalm.dev/r/4b159a69f1 - no issues$x = [$collection, 4, 5];- https://psalm.dev/r/9e9b78e2e1 Psalm error -11:12 - Argument 1 of acceptsInt expects int, array{1, 2, 3} provided
in other words, if we are using for example https://github.com/Roave/infection-static-analysis-plugin - this mutant will always be killed, resulting in wasted resources.
So I would vote to not do this mutation. Does it make sense?
There was a problem hiding this comment.
interesting point, I'm familiar with that plugin, however, in your example we are making an assumption: we assume that the array is a list of a single type, which might not be the case. There are many situations where an array will have mixed types and this mutator becomes valuable.
Here's the simplest example, based on yours: https://psalm.dev/r/6ba915ee93. I've added the assertions at the end to show that the first element of each new array is not the same.
There will be other cases where the union type isn't even mentioned because an array can have many different types, so the developer might go with mixed.
|
Ok, let's merge then. Thank you @AlexandruGG I'm starting thinking about somehow gathering user's statistics about mutators (not just killing/escaping ratio, but usefulness from developers points of view during working on real projects), as described in this great article #1526 |
|
@maks-rafalko @sanmai thank you both for reviewing and merging this so quickly! 🙌 |
|
Very nice stuff ! :) |
| final class SpreadAssignmentTest extends BaseMutatorTestCase | ||
| { | ||
| /** | ||
| * @requires PHP >= 7.4 |
There was a problem hiding this comment.
I'm late to the party.
@requires is not needed as minimum compatible with Infection version of PHP is 7.4
There was a problem hiding this comment.
Good catch, should probably be removed from other places as well then. I might create a PR specifically for those
There was a problem hiding this comment.
I might create a PR specifically for those
yes please. Thanks
This PR:
...)SpreadMutator toSpreadOneItemCloses: #1528
I manually tested with the
.pharthe behaviour provided as an example in the issue.