Statement mutators: Assign, MethodCall

[sanmai]

This PR:

• Adds a new type of Statement mutators
• Adds an Assign Statement mutator
• Adds a family of MethodCall Mutators
• Covered by tests
• Doc PR: TBD
• MSI percentages need to be updated.

Statement mutators

Statement mutators change or disable entire statements. Requested in #53.

Assign Statement mutator

This mutator tries to disable assignments, while trying to not to cause clear and straight errors. This feat is achieved by prepending an assignment with true ||, thus making the assignment essentially void.

This code:

if ($a = $foo->b()) {
    $c = true;
}

Becomes:

if (true || $a = $foo->b()) {
    $c = true;
}

Assign mutator will only mutate if...

• There's a variable or a property on the left of an assignment, and if
• There's a method call, function call, variable, or property on the right.

Constant assignments are left unscathed.

MethodCall Mutators

There's three mutators in this family: MethodCallFalse, MethodCallNull, MethodCallTrue.

These mutators replace any method or function call with null, true, or false. This way even if a method/function call is embedded in another construct, be it an if or another method call, there won't be a language error.

if ($bar || $var->foo()) {
}

Becomes either of:

if ($bar || false) {
}

if ($bar || true) {
}

if ($bar || null) {
}

Instead of $bar there could be another method call that would not be mutated on this iteration. And there could be logical and instead of or: these mutators cover all bases.

What gives?

Overall this should fix #53.

Please tell me if these are okay and I can go on with a doc PR.

---

BTW I'm OK if these mutators will go to the long discussed @nightmare profile since they add quite a lot of mutations.

Mutator	Mutations	Killed	Escaped	Errors
Assign	437	281	27	1
MethodCallFalse	1275	583	116	3
MethodCallNull	1275	582	117	3
MethodCallTrue	1275	563	135	4

$ softlimit -m 500000000 bin/infection --threads=4 --mutators=@statement
You are running Infection with xdebug enabled.
    ____      ____          __  _
   /  _/___  / __/__  _____/ /_(_)___  ____ 
   / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
 _/ // / / / __/  __/ /__/ /_/ / /_/ / / / /
/___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/
 
Running initial test suite...

PHPUnit version: 6.1.0

  450 [============================] 14 secs

Generate mutants...

Processing source code files: 186/186
Creating mutated files and processes: 4271/4271
.: killed, M: escaped, S: uncovered, E: fatal error, T: timed out

SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS   (  50 / 4271)
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS   ( 100 / 4271)

<skip>

...................................MMM........MM..   (4150 / 4271)
........MM...................................MM.M.   (4200 / 4271)
..MMMMMMM.MMMMM......M..............MM........M..M   (4250 / 4271)
.....................                                (4271 / 4271)

4271 mutations were generated:
    2032 mutants were killed
    1830 mutants were not covered by tests
     401 covered mutants were not detected
       8 errors were encountered
       0 time outs were encountered

Metrics:
         Mutation Score Indicator (MSI): 48%
         Mutation Code Coverage: 57%
         Covered Code MSI: 84%

Please note that some mutants will inevitably be harmless (i.e. false positives).

That's 4271 extra mutations to a thousand currently generated, but 401 escaped mutants and only 8 errors.

Errors

I'm not sure if these errors are avoidable:

-        $isDoubledLogicalNot = $node->expr instanceof Node\Expr\BooleanNot || $node->getAttribute('parent') instanceof Node\Expr\BooleanNot;
+        $isDoubledLogicalNot = $node->expr instanceof Node\Expr\BooleanNot || false instanceof Node\Expr\BooleanNot;

-        $updater->getStrategy()->setPackageName(self::PACKAGE_NAME);
+        false->setPackageName(self::PACKAGE_NAME);

[maks-rafalko]

Hi, sorry for the delay with the reply.

About Assign Mutator

<?php
$a = $b;
$var->a = $b;
$var->a = count($a);
$var->b = $var->a;
$var->c = $var->foo();
$d = $var->foo();
if ($bar->baz && $foo->a = $baz->b()) {}

1. Seems like all these expressions can be removed instead of replacing them with true || XXX. What is the point to leave them? Also, all spies (method is called X times) will fail in tests regardless of whether we remove these expressions or leave with true || XXX.

2. I believe removing $a = $b; (or replacing with true || $a = $b) is not correct because variable $a is likely used below in the code, so we will get undefined variable error almost always. Searching this variable in the current context (scope) is much more difficult task.

For such mutations, we should analyze generated logs carefully to decide what edge cases we need to address.

---

About MethodCall*** Mutators

- if ($var->foo()->bar() === 1) {
+ if (false === 1) {  // MethodCallFalse
+ if (true === 1) {   // MethodCallTrue
+ if (null === 1) {   // MethodCallNull

^ duplicate mutations, all of them are evaluated to false. I would say that we need something smarter here (unless it's ok to have really thousands of mutations in @nightmare). But I guess adding so many [duplicate] mutations will just kill the @nightmare feature. We will end up with hours of mutation testing for a small project.

I don't know how to "fix" the issue above, but I'm thinking about the following mutator:

# first case
- $var->foo()->bar() === 1
+ 1 === 1

# second case
- $var->foo()->bar() === 'some string'
+ 'some string' === 'some string'

With this ^ mutator we will check whether the test suite checks ->bar() is called. Positive and negative conditions in this comparison are covered by already existing mutator: === -> !==

Another example from these group of mutators:

- $a = bar();
+ $a = false;
+ $a = true;
+ $a = null;

If $a is used as an array later in e.g. array_map() function, we will have 3 mutations that are immediately killed because of warning: (array_map expects array but null given). In this mutation, we have to check bar()'s return type and replace function call according to it:

function bar(): array {
}

- $a = bar();
+ $a = [];
...
// later in the code

$b = array_map($a, function () {}); // works as expected

// NO mutation with $a = false / true / null;

---

About Errors

-        $isDoubledLogicalNot = $node->expr instanceof Node\Expr\BooleanNot || $node->getAttribute('parent') instanceof Node\Expr\BooleanNot;
+        $isDoubledLogicalNot = $node->expr instanceof Node\Expr\BooleanNot || false instanceof Node\Expr\BooleanNot;

-        $updater->getStrategy()->setPackageName(self::PACKAGE_NAME);
+        false->setPackageName(self::PACKAGE_NAME);

I'm pretty sure this can be avoided by checking other nodes in AST. Of course, this will be harder to achieve.

[sanmai]

No worries, we are not in a hurry with this PR, not at all. (But I really want you to consider #258)

As was discussed in #53, primitive removal cannot be expected to give meaningful results.

I understand you're looking at this mutator and see, like, woa, this mutator makes too much rubbish.
But as I look at it, I see it making interesting mutations which other mutators just cannot devise.

What these do is way more clever that a simple removal. Where a simple removal would just remove the whole control struct with its descendants:

+if ($foo = bar()) { /* */ }
-

A statement mutator will keep all of it:

+if ($foo = bar()) { /* */ }
-if (true || $foo = bar()) { /* still executed */ }

So goes for other control structs and other mutators.

But if you're only interested in control structs, you should be looking at them and only, right?
The problem is that current API does not let a single mutator produce several mutations.

+if ($foo = bar() && $baz = foo()) { /* */ }
-if ((true || $foo = bar()) && $baz = foo()) { /* */ }
-if ($foo = bar() && (true || $baz = foo())) { /* */ }

The above example isn't possible within the current API within a single mutator.
Therefore we left to "carpet bombing" mutators like the one you're looking at.

Let's remember that the whole purpose of mutation testing is to find weak spots in the test data, or
ineffective tests. With this mutator you can expect that your tests will be tried in a very surprising
way. As for finding ineffective tests, these mutators are at least somewhat resultative as shown in #248.

[sanmai]

Please excuse me if i'm wrong but it seems like it may be possible to limit mutators to insides of control structs. These's ReflectionVisitor::isInsideFunction and what it does is goes by the tree up above until it finds beginning of a function. And similarly one can see if there's a control struct above. I'm not sure if it's going to work right, but that's something worth trying.

[BackEndTea]

I think it would be cleaner to have an abstract MethodCall mutator, and have MethodCallTrue extend from that in the same way

[sanmai]

True. Done.

[BackEndTea]

And similarly one can see if there's a control struct above. I'm not sure if it's going to work right, but that's something worth trying.

This is probably worth looking into. My main concern with these mutators is that a lot of them will be duplicates, or get killed off due to language errors.

[maks-rafalko]

should we close this one? seems like it's dead

if it needs to be revised, I would suggest opening an RFC for each mutator separately, because currently, it's very hard to follow with so many things what is going on in this PR

[sanmai]

Yeah, that’s going to better.