Leave only one element in the non empty returned array

[maks-rafalko]

This PR:

• Adds new Mutator described in RFC: leave only one element in the non empty returned array #690
• Covered by tests
• Doc PR: Add ArrayOneItem mutator site#131

Closes #690

[BackEndTea]

I was thinking maybe we could use array_key_first, but because array_value_first didn't get implemented, that isn't as useful.

[BackEndTea]

👍

[maks-rafalko]

Thank you @sanmai & @BackEndTea

[morozov]

The fact that the mutation produces a conditional statement opens up the possibility of false positives. At the code execution time, depending on the size of the collection, the mutation may or may not effectively happen but it will also be counted as such.

Some collections have one element by design. For instance, all sorts of JSON-based API responses that have one top-level element:

{
   "response": { /* ... */}
}

The code that consumes this API and the corresponding tests don't expect any other elements in the collection, therefore the mutation (that effectively doesn't change anything) passes.

[maks-rafalko]

Agree. And this is where the user can disable mutator for particular line, method or completely for the project.

Do you have any suggestions here?

This mutator is useful for the opposite scenario, where we are working with e.g. Doctrine repositories and expect > 1 elements in the returned array.

[morozov]

This mutator is useful for the opposite scenario, where we are working with e.g. Doctrine repositories and expect > 1 elements in the returned array.

Do you mean that they expect more than one item but only check the first one? Shouldn't the ArrayItemRemoval mutation flag such tests?

Do you have any suggestions here?

Judging by the documentation, this is the only mutation that generates a conditional, one branch of which is the same as the original code. I suggest to avoid that and make sure the code is mutated unconditionally (however it's still not clear how it's different from ArrayItemRemoval).

Alternatively, the mutated code, instead of returning the one-element collection as is could throw an exception indicating that the mutation didn't happen and shouldn't be counted.

[maks-rafalko]

Shouldn't the ArrayItemRemoval mutation flag such tests?

ArrayItemRemoval mutates the raw arrays, not variables. When you literally have ['a' => 1, 2, 3, 'd' => 4] for example.

This mutator, ArrayOneItem, mutates returned array variables. That's the difference.

[morozov]

Indeed, even item removal cannot be done unconditionally. You cannot remove it from an empty array. Still, throwing an exception on an unsuccessful mutation should help avoid false positives.

[maks-rafalko]

even item removal cannot be done unconditionally. You cannot remove it from an empty array

we don't mutate empty array with ArrayItermRemoval mutator :) So there are no conditions.

Still, throwing an exception on an unsuccessful mutation should help avoid false positives.

Do you have false-positives with ArrayOneItem mutator because of conditions? What is the real source code?

I'm open to change it to throw an exception, just need more information about real cases and then we can create issue and discuss farther / implement.

[morozov]

we don't mutate empty array with ArrayItermRemoval mutator :) So there are no conditions.

That's the thing. You can see if the array literal is empty at mutation time and avoid false positives but you cannot do the same with a variable.

Do you have false-positives with ArrayOneItem mutator because of conditions? What is the real source code?

Yes. See morozov/kih#13.

[BackEndTea]

I have put this into an issue so we can track this a bit better: #803