Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement always true or always false if and elseif conditions #1865

Open
manhunto opened this issue May 26, 2023 · 8 comments · May be fixed by #1866
Open

Implement always true or always false if and elseif conditions #1865

manhunto opened this issue May 26, 2023 · 8 comments · May be fixed by #1866
Labels
Feature Request Mutator

Comments

@manhunto
Copy link
Contributor

Hi, I have idea to implement new mutator. Let's look at an example.

final readonly class PaginationRequest
{
    public function __construct(
        private int $page,
        private int $size
    ) {
    }

    public function getSize(): int
    {
        return $this->size + 1;
    }

    public function getOffset(): int
    {
        if ($this->page === 1) {
            return 0;
        }

        return ($this->page - 1) * $this->size;
    }
}

And test for it

class PaginationRequestTest extends TestCase
{
    /**
     * @dataProvider paginationInput
     */
    public function testItPreparesValidPaginationInput(int $page, int $expected): void
    {
        $sut = new PaginationRequest($page, 25);

        self::assertSame($expected, $sut->getOffset());
        self::assertSame(26, $sut->getSize());
    }

    /**
     * @return iterable<array{0: int, 1: int}>
     */
    public function paginationInput(): iterable
    {
        yield [1, 0];
        yield [2, 25];
        yield [3, 50];
        yield [10, 225];
    }
}

Link to playground: https://infection-php.dev/r/ylkd

In this example, if in getOffset() method is redundant and can be delated.

        if ($this->page === 1) {
            return 0;
        }

What do you think about implementing a new mutator which will change if and elseif conditions to false. (and could be another mutator that set it to true)

This mutator will do changes like below

-        if ($this->page === 1) {
+        if (false) {
            return 0;
        }

If you think is worth implementing it, I can prepare PR with changes :)
@maks-rafalko
Copy link
Member

In my todo list there was an idea to implement if removal mutator, so IMO removing any if/elseif conditions completely would be a great addition to the mutator set.

and could be another mutator that set it to true

need to think more about it with some examples, probably worth trying to implement and check what it brings

@maks-rafalko maks-rafalko changed the title Always true if condition mutator Implement always true or always false if and elseif conditions May 26, 2023
@manhunto
Copy link
Contributor Author

Yes, I also thought about if/else removal. What do you think is a better idea? Removal or setting it to false? IMO it would bring the same value. It seems to me that. removing if will be hard to implement.

I also thought about boolean conditions (&& and ||) that are not in if/elseif statements. It could be done in the next PR. But I need to think about it or try to implement POC.

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

@manhunto
Copy link
Contributor Author

manhunto commented May 26, 2023 •
edited

It seems to me that. removing if will be hard to implement.

Ok, I have to take those words back.. I thought that there is no empty node, but I found it in repo Node\Stmt\Nop. So let me know which approach, in your opinion, would be better ;)

EDIT: but there is still problem when there is elseif statement. It will produce syntax error.

@maks-rafalko
Copy link
Member

let's try with changing condition to false then rather than removing. But feel free to change the strategy if removing will work better for some reason

@manhunto manhunto linked a pull request May 26, 2023 that will close this issue
Draft
4 tasks
@sanmai
Copy link
Member

sanmai commented May 28, 2023 •
edited

I'm afraid I'm going to upset you all. I think came up before, and the biggest argument against these mutations was that they are very easy to kill (or downright impossible), very obvious from the coverage report, and most of the time they are going to waste CPU cycles, skewing the metrics.

Here are all mutations this example currently generates: https://infection-php.dev/r/3w26

If I had a branch like this in my code, it is probably because it was added as an optimization to avoid costly code paths. So if I have to kill the mutation proposed here, I'm supposed to remove an optimization because there's no way to test it. How can we argue this is a good thing?

@manhunto
Copy link
Contributor Author

@sanmai you are right, I agree with you. Most of these mutation would not be useful but there will be a lot of it

@maks-rafalko
Copy link
Member

@manhunto did you do some analysis and have examples?

@manhunto
Copy link
Contributor Author

No, It is just my assumption and when I thought about it may be true. I will do more research after two weeks because I'm on holidays now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature Request Mutator
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add always false if and elseif mutators manhunto/infection
3 participants
@sanmai @maks-rafalko @manhunto