Add negation mutators #1753

manhunto · 2022-11-27T00:31:05Z

This PR:

Add mutators that negate whole condition in if and else if (it only mutates simple expressions like function call, method call etc. to not duplicate with other existing mutators)
Add mutators that negate whole condition in LogicalAnd and LogicalOr expressions
Add mutators that negate every sub expression together in LogicalAnd and LogicalOr expressions (even in complex conditions)
Add mutators that negate every sub expression separately in LogicalAnd and LogicalOr expressions (even in complex conditions, only simple sub expressions like in if and else if)
Covered by tests
Doc PR: Add docs for "if condition negation" mutators site#253

Negation of entire simple expression in if.

-if ($this->fooBar()) {
+if (!$this->fooBar()) {

Negation of entire simple expression in elseif.

if ($this->fooBar() {
- } elseif ($this->fooBaz()) {
+ } elseif (!$this->fooBaz()) {
}

Negation of entire && and || condition (even in if statements)

- $var = $a && $b;
+ $var = !($a && $b);

- $var = $a || $b;
+ $var = !($a || $b);

Negation of every sub expression in && and || condition (even in if statements)

- $var = $a && $b;
+ $var = !$a && !$b;

- $var = $a || $b;
+ $var = !$a || !$b;

Negation of every sub expression separately in && and || condition (even in if statements)

- $var = $a && $b;
# Mutation 1
+ $var = !$a && $b;
# Mutation 2
+ $var = $a && !$b;

- $var = $a || $b;
# Mutation 1
+ $var = !$a || $b;
# Mutation 2
+ $var = $a || !$b;

I've decided to create one PR for all mutators because they are related. They share traits and I think it is better to review it together, but I can split it if you think that it will be easier to maintain.

Closes: #1752

theofidry

Sorry for the delay in the review; good job! 👏

Despite the number of comments I think it's mostly nitpicks, it looks pretty good to me.

It's been too long since I've had a good view on the mutators and I didn't completely follow the conversation with @maks-rafalko previously neither so I do think it deserve another round of review. As such I wouldn't recommend to address all the points yet just in case another maintainer disagrees with them :)

src/Mutator/Boolean/LogicalAndAllSubExprNegation.php

theofidry · 2022-12-06T21:51:00Z

tests/phpunit/Mutator/Boolean/LogicalAndAllSubExprNegationTest.php

+            <<<'PHP'
+<?php
+
+$var = a() && b();


just for the sake of it I would add another test making use of the and and AND notation instead of &&

I think that require implementation of \PhpParser\Node\Expr\BinaryOp\LogicalAnd node in mutators. Currently, we operate on BooleanAnd node.

tests/phpunit/Mutator/Boolean/LogicalAndAllSubExprNegationTest.php

src/Mutator/Boolean/LogicalAndNegation.php

src/Mutator/SimpleExpression.php

theofidry · 2022-12-06T22:08:39Z

src/Mutator/SimpleExpression.php

+{
+    private function isSimpleExpression(Node\Expr $expr): bool
+    {
+        return $expr instanceof Node\Expr\FuncCall


looking at the implementation the method could be static

theofidry · 2022-12-06T22:10:03Z

src/Mutator/SimpleExpression.php

+/**
+ * @internal
+ */
+trait SimpleExpression


From this alone I am not convinced there is any benefit of it being a trait. It could very well be a regular stateless class with a static method and a private constructor.

But maybe the rest of the team prefers traits... I'll let them voice their opinion on it

I agree with you, I also prefer working with static methods that with traits. Making it static class require to write unit tests for it (project requirement). So let me know which way I supposed to choose ;)

theofidry · 2022-12-06T22:12:04Z

src/Mutator/SimpleExpression.php

+            || $expr instanceof Node\Expr\StaticCall
+            || $expr instanceof Node\Expr\Variable
+            || $expr instanceof Node\Expr\ArrayDimFetch
+            || $expr instanceof Node\Expr\ClassConstFetch;


Note for other reviewers, I did not verify the list completeness

src/Mutator/Util/AbstractSingleSubExprNegation.php

manhunto · 2022-12-10T08:37:59Z

Thank you for review @theofidry I've introducer some of changes proposed by you and I'm waiting for comments from another reviewers:)

manhunto · 2023-02-02T13:53:32Z

Hi @maks-rafalko, any updates on this PR? :)

maks-rafalko · 2023-04-02T10:01:04Z

Oh, somehow I missed this one, sorry. Could you please rebase so we can move forward? And thank you for you patience and work!

Co-authored-by: Théo FIDRY <5175937+theofidry@users.noreply.github.com>

maks-rafalko · 2023-04-04T21:52:33Z

Ok, I managed to rebase it myself.

Notes for reviewers: we need to analyze if the provided mutators somehow generate duplicate muations (with existing in master mutators and between each other).

My idea is to create boolean matrices (as I did here #1752 (comment)) and compare all of them, finding duplicate results.

icanhazstring · 2023-04-05T05:28:15Z

Just out of curiosity as I don't know the full functionality behind.

Isn't there a hashmap containing a list of, let's say, md5 hashes of the content of each mutated file? So that there can be no duplicate mutation?

Or is it because that some mutations look different but produce the same logical outcome?

maks-rafalko · 2023-04-05T07:40:41Z

@icanhazstring I'm more about boolean matrices created by new mutators here. The simplest thing to understand it is De Morgan's law

where !$a && !$b === !($a || $b). By introducing many new different mutations, we can have duplicates in boolean matrices, which is not efficient as the same (logically) mutators will consume more and more time.

However, your question about hashes gives some source for thinking. We do have md5 hashing for each mutation, but I don't think we check uniqueness to avoid duplicates on physical, not logical level:

infection/src/Mutation/Mutation.php

Lines 173 to 186 in 0228b46

    
           private function createHash(): string 
        
           { 
        
               $hashKeys = [ 
        
                   $this->originalFilePath, 
        
                   $this->mutatorName, 
        
                   $this->mutationByMutatorIndex, 
        
               ]; 
        
               foreach ($this->attributes as $attribute) { 
        
                   $hashKeys[] = $attribute; 
        
               } 
        
               return md5(implode('_', $hashKeys)); 
        
           }

icanhazstring · 2023-04-05T07:45:08Z

Maybe we could improve this hash based on the uniqueness of these logical mutations?
Don't know if we could come up with a parser implementation based on de morgan's law to figure out duplicates.

…utate identical expressions

…f abstract methods

…tate identical expressions with boolean ($a === false), because it's same as TrueValue/FalseValue

maks-rafalko

@manhunto this is absolutely awesome work. I've spent this day investigating the code and checking for duplicates. Made and update to not negate $a === false & $b !== true because it's duplicate of TrueValue / FalseValue mutators. Did the same for Or analogue.

Also, added more tests and simplified child classes to reduce the number of abstract methods.

I agree that from maintenance POV it's better to keep classes separate, at least for now, because the logic is quite complex.

Thank you for your great contribution, going to merge it as soon as the build passes.

Related to infection/infection#1753

manhunto · 2023-05-13T20:26:54Z

@maks-rafalko thank you for helping me with this PR -- for the changes you introduced in this PR and docs :)

manhunto mentioned this pull request Nov 27, 2022

If condition negation mutator #1752

Closed

maks-rafalko self-requested a review November 28, 2022 09:01

theofidry self-requested a review November 30, 2022 23:25

maks-rafalko added the Mutator label Dec 4, 2022

theofidry approved these changes Dec 6, 2022

View reviewed changes

manhunto added 22 commits April 5, 2023 00:45

Proof of concept

4e75d2e

Add missing final

5fda89c

Rewrite && subexpressions mutators to traverser

d878332

Refactor

d3183bf

Move code from visitor to mutator

b779263

Introduce simple expression

9fa0c97

Refactor single sub expr visitor to abstract class

a9348dd

Add missing license

0104c76

Move canMutate to abstract class

d5e0b83

Improve unit tests

17dd2f9

Fix phpdocs

78b4e58

Change methods scope to protected in abstract class

572b6d7

Fix phpdocs

4282257

Fix styles

c3a427c

Fixes for autoreview

4f43f88

Fix styles

fb9572e

Fix for phpstan

ea32869

Fix phpdocs

07c1e62

Simplify code in negate all sub expr mutators

f6b12e6

Fix styles

15390bd

Rename mutators

5889308

Fix handling attributes 9E88

52b478e

manhunto and others added 9 commits April 5, 2023 00:45

Fix typo

1765a1b

Fix description

9464b3d

Rewrite description

2ed165d

Co-authored-by: Théo FIDRY <5175937+theofidry@users.noreply.github.com>

Rewrite description

27854e4

Co-authored-by: Théo FIDRY <5175937+theofidry@users.noreply.github.com>

Rewrite elseifnegation mutator description

f152140

Co-authored-by: Théo FIDRY <5175937+theofidry@users.noreply.github.com>

Changes after code review

efb6967

Fix for cs

4609f0d

Fix phpdoc for BinaryOp Expression

6d98285

Fix description

71b2fb9

maks-rafalko force-pushed the feature/negation-in-if-and-boolean branch from 6a1ca8c to 71b2fb9 Compare April 4, 2023 21:46

maks-rafalko mentioned this pull request Apr 5, 2023

Investigate is it possible to check duplicates of generated mutators (physical and logical) #1838

Open

maks-rafalko added 4 commits May 12, 2023 21:41

Add more tests for LogicalAndAllSubExprNegation, update it to not m…

cd799b3

…utate identical expressions

Simplify abstract class and children classes by reducing the number o…

23209ac

…f abstract methods

Add more tests for LogicalOrAllSubExprNegation, update it to not mu…

81c669f

…tate identical expressions with boolean ($a === false), because it's same as TrueValue/FalseValue

Extract logic to abstract class to reuse the code and avoid duplication

8a956d6

maks-rafalko enabled auto-merge (squash) May 12, 2023 19:54

maks-rafalko approved these changes May 12, 2023

View reviewed changes

maks-rafalko merged commit 62441a0 into infection:master May 12, 2023

maks-rafalko added a commit to infection/site that referenced this pull request May 12, 2023

Add docs for "if condition negation" mutators

860b774

Related to infection/infection#1753

maks-rafalko mentioned this pull request May 12, 2023

Add docs for "if condition negation" mutators infection/site#253

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add negation mutators #1753

Add negation mutators #1753

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add negation mutators #1753

Add negation mutators #1753

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!