Trying to remove false positive on logical or.

[Neirda24]

This PR:

• Improve LogicalOr mutatability
• Covered by tests

Fixes #1796

It improves the LogicalOr mutatability by detecting if we are trying to mutate to something that doesn't make sense.
For example :

$myVar === 'hello' || $myVar === 'world';

cannot be mutated to

$myVar === 'hello' && $myVar === 'world';

I think there a lot more usecase that I don't cover such as

$myVar < 5 || $myVar > 10;

can't be mutated but

$myVar < 10 || $myVar > 5;

could. So not only should we check the variable name but also to what it compares. Although it would require to handle a lot of cases and Node class doesn't help much at the moment. Help appreciated to improve this further.

---

Edit : also handled some use cases regarding < & >

[maks-rafalko]

Cool, left a couple of small comments. Otherwise, looks good.

[maks-rafalko]

I think we don't need === true as in_array returns only boolean and nothing else

[Neirda24]

As you wish. I always add a strict comparison because sometimes we want it to be equal to false and the ! is easy to miss / remove. I'll adapt to your coding style.

[maks-rafalko]

get_class($node->left) - does it make sense to move to variable? As it's being called many times. Same for get_class($node->right)

[Neirda24]

Yes we could. When upgrading PHP we could then use $node->left::class and maybe we will have to rollback the variable.

[maks-rafalko]

ok, let's do it

[Neirda24]

done

[maks-rafalko]

still present === true, please check all cases :)

[maks-rafalko]

could you also please fix PHPStan issues? https://github.com/infection/infection/actions/runs/3985550828/jobs/6833052618#step:7:90

also, seems like more tests should be added as many mutants are escaped and MSI level is not reached, see them here https://github.com/infection/infection/pull/1801/files

[Neirda24]

@maks-rafalko : sorry for the delay. I've figured that some of what I was trying to fix shouldn't be. I'll update the PR with added tests soon !

[Neirda24]

@maks-rafalko : Okay I've pushed new code and fixed coding style using cs. I'll gadly take some help finishing this as it requires I think a lot of tests combinations to reduce the number of mutants. Some of those mutants can't be tested against I think. Let me know.

[maks-rafalko]

@Neirda24 could you please rebase on master?

Also, please run make psalm to see the issues reported by Psalm (or docker compose run php81 make psalm)

[Neirda24]

@maks-rafalko : here are the result of psalm :

$ docker compose run php81 make psalm
./.tools/psalm --threads=max

Deprecated: Return type of HumbugBox383\KevinGH\RequirementChecker\RequirementCollection::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///opt/infection/.tools/psalm/.box/src/RequirementCollection.php on line 15

Deprecated: Return type of HumbugBox383\KevinGH\RequirementChecker\RequirementCollection::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in phar:///opt/infection/.tools/psalm/.box/src/RequirementCollection.php on line 19
Target PHP version: 8.0 (inferred from composer.json)
Scanning files...
Analyzing files...

░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░  60 / 411 (14%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 120 / 411 (29%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 180 / 411 (43%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 240 / 411 (58%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 300 / 411 (72%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ 360 / 411 (87%)
░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░

ERROR: UndefinedPropertyFetch - src/Mutator/Boolean/LogicalOr.php:100:25 - Instance property PhpParser\Node\Expr::$left is not defined (see https://psalm.dev/039)
        $nodeLeftLeft = $nodeLeft->left;


ERROR: UndefinedPropertyFetch - src/Mutator/Boolean/LogicalOr.php:101:26 - Instance property PhpParser\Node\Expr::$right is not defined (see https://psalm.dev/039)
        $nodeLeftRight = $nodeLeft->right;


ERROR: UndefinedPropertyFetch - src/Mutator/Boolean/LogicalOr.php:103:26 - Instance property PhpParser\Node\Expr::$left is not defined (see https://psalm.dev/039)
        $nodeRightLeft = $nodeRight->left;


ERROR: UndefinedPropertyFetch - src/Mutator/Boolean/LogicalOr.php:104:27 - Instance property PhpParser\Node\Expr::$right is not defined (see https://psalm.dev/039)
        $nodeRightRight = $nodeRight->right;


ERROR: UndefinedMethod - src/Mutator/Boolean/LogicalOr.php:202:41 - Method PhpParser\Node\Expr::getOperatorSigil does not exist (see https://psalm.dev/022)
            return (match ("{$nodeLeft->getOperatorSigil()}::{$nodeRight->getOperatorSigil()}") {


ERROR: UndefinedMethod - src/Mutator/Boolean/LogicalOr.php:202:75 - Method PhpParser\Node\Expr::getOperatorSigil does not exist (see https://psalm.dev/022)
            return (match ("{$nodeLeft->getOperatorSigil()}::{$nodeRight->getOperatorSigil()}") {


------------------------------
6 errors found
------------------------------
416 other issues found.
You can display them with --show-info=true
------------------------------
Psalm can automatically fix 5 of these issues.
Run Psalm again with
--alter --issues=InvalidNullableReturnType,InvalidReturnType --dry-run
to see what it can fix.
------------------------------

But regarding ERROR: UndefinedPropertyFetch - src/Mutator/Boolean/LogicalOr.php:100:25 - Instance property PhpParser\Node\Expr::$left is not defined (see https://psalm.dev/039) $nodeLeftLeft = $nodeLeft->left; for example the issue is that right before here is the code :

        if (
            !property_exists($nodeLeft, 'left')
            || !property_exists($nodeLeft, 'right')
            || !property_exists($nodeRight, 'left')
            || !property_exists($nodeRight, 'right')
        ) {
            return true;
        }

        $nodeLeftLeft = $nodeLeft->left;
        $nodeLeftRight = $nodeLeft->right;

        $nodeRightLeft = $nodeRight->left;
        $nodeRightRight = $nodeRight->right;

looks like a psalm issue don't you think ? There is no Interface that ensure there is a left / right property.

[Neirda24]

@maks-rafalko : Fixed psalm but it looks ugly.... Let me know what you think

[maks-rafalko]

@maks-rafalko : Fixed psalm but it looks ugly.... Let me know what you think

yeah, no the best look, but I can't advice anything better, so I'm personally ok with that.

That's already a huge job! Could you look into PHPStan error as well and make Autoreview build green?

Some of those mutants can't be tested against I think. Let me know.

If they can't be tested, probably we can ignore them?

[Neirda24]

@maks-rafalko : there is an open issue about this (I thought I saw it merged with an article but couldn't find it) phpstan/phpstan#4451 . Basically PHPStan complains that there is no default but in this case the default would be an execption same as the default behavior without the default key. There is still an option to set teh default to true (In doubt mutate) otherwise. What do you prefer ? It seems to be the only thing left to fix (I'll also look into mutations in CI to see if I can fix more)

[maks-rafalko]

Probably exception will be better (if this is the same as default behavior when default is absent?

I'll also look into mutations in CI to see if I can fix more

👍

[Neirda24]

Ok I think I fixed everything I could. I added more tests and fixed psalm. Although introducing the default in match statement fixes psalm, it creates a new mutation that we cannot test (unreachable statement). Mutation still failing are Either LogicalOr | LogicalAnd or InstanceOf and I can't seems to be able to test them directly. I think we are dealing with other false positive here. Could you check and let me know what you think please @maks-rafalko ? Also I re-arramged a bit the tests.

[maks-rafalko]

I've fixed CS and added ignore rules.

Thank you for your contribution @Neirda24

[Neirda24]

Thank you for the final touch !