Add Flip Coalesce operator mutator

[sidz]

This PR:

• Adds new FlipCoalesce mutator
• Covered by tests
• Schema needs an update
• Doc PR: Update mutators.md with FlipCoalesce mutator site#185

Mutates:

$foo = 'foo';
$bar = 'bar';
- $foo ?? $bar;
+ $bar ?? $foo;

$foo = 'foo';
$bar = 'bar';
$baz = 'baz';
- $foo ?? $bar ?? $baz;
+ $foo ?? $baz ?? $bar;

$foo = 'foo';
$bar = 'bar';
$baz = 'baz';
- $foo ?? $bar ?? $baz;
+ $bar ?? $foo ?? $baz;

Requested in #1387

p.s. From my POV Infection\Mutator\Operator\Coalesce mutator is redundant when this PR is merged. Thoughts?

[maks-rafalko]

👍

[maks-rafalko]

p.s. From my POV Infection\Mutator\Operator\Coalesce mutator is redundant when this PR is merged. Thoughts?

I didn't do any analytics, but from the first look - agree, sounds like redundant

[sanmai]

Coalesce is redundant indeed.

[sanmai]

How about self::A ?? self::B? And with constants in general. These might be unkillable.

[maks-rafalko]

How about self::A ?? self::B?

is it a real case? I mean it's strange to use constant in if statement. It's either always true or always false

[sanmai]

It can be any kind of a constant expression.

define('TEST_ENVIRONMENT', null);
class B {
    const ALLOW_FEATURE_A = TEST_ENVIRONMENT;
}

No doubt it's a poor programming style, but shall we punish it?

[maks-rafalko]

Probably I don't understand the real case and that's why don't see an issue here. Any real examples? I just can't remember any cases with such code self::A ?? self::B, even with self::A ?? $variable

[sanmai]

The issue is that you might not be able to test self::A ?? self::B reliably. This could be a convenience method in a generated class. Or this could be a method to turn off a pending feature based on a hardcoded constant.

In either case I think this method either not worth the testing, or difficult to test. This makes a useless mutation, a waste of time.

[maks-rafalko]

So what are you suggesting? Do not mutate when the left side is a const?

Alternatively, if the user has such a weird case, they can ignore such line of the code, via ignoreSourceCodeByRegex:

{
    "mutators": {
        "global-ignoreSourceCodeByRegex": ["self::.*\\s\\?\\?\\sself::.*"]
    }
}

[sanmai]

Yes, I think it's a general enough case to ignore all mutations with a constant fetch on the left by default.

It is much cheaper for us to do that now, than for a user to invent complicated (as we can readily see!) regular expression just to avoid a pointless mutation.

[maks-rafalko]

TBH I'm not convinced, as I have never seen such code. But if you did - then ok.

[sanmai]

Let me put this way: in which circumstances do you think running this mutation against a constant on a left will be useful?

Again, it is not about this mutation is harmful, it's not, but rather it is not useful. Even annoyingly not useful.

One can argue this variant of this mutation isn't useful for most projects just like IdenticalEqual and NotIdenticalNotEqual not useful for the most modern-day projects.

[sanmai]

If I learnt something from the IdenticalEqual/NotIdenticalNotEqual debacle is that if a mutation might be seen as a punishment for a certain programming style, you shouldn't do it. Any such mutations make Infection less useful, and that's bad.

[sidz]

@sanmai done