Infection: Family bc*-functions mutators (bcmath support)

[majkel89]

Similarly to https://github.com/infection/infection/issues/654 there should be bcmath support

Example

- $c = bcadd($a, b);
+ $c = $a + $b;

I think this mutator should be enabled on demand and not by default.

There should be one mutator for all bcmath functions

[maks-rafalko]

A big 👍 for this mutator.

I think this mutator should be enabled on demand and not by default.

Could you please provide arguments?

From my point of view, if developers of a particular project have ext-bcmath installed, and intentionally use slow bc-* functions - they know what are they doing.

And Infection should force them to write a test to prove bc-* function is used with a proper reason.

We probably should skip mutating if extension is not loaded, just for better performance:

// early return
protected function mutatesNode(Node $node): bool 
{
    if (!extension_loaded('bcmath')) {
        return false;
    }

    // ...
}

or even don't add this mutator to mutator list if the extension is not loaded, to prevent its execution for each traversed node.

[sanmai]

I think this mutator should be enabled on demand and not by default.

Could you please provide arguments?

For one because bcmath functions work with strings of numbers, and first return value check will have these mutators busted. Example:

var_dump('1' + '2'); \\ int(3)
var_dump(bcadd('1', '2')); \\ string(1) "3"

That assuming strict types are enabled.

[maks-rafalko]

We must cast to string then.

- $c = bcadd($a, b);
+ $c = (string) ($a + $b);

[sanmai]

This may work, but only before we get into the float territory. Bcmath functions do not exactly work with floats.

var_dump((string) ('11111111111111111111' + '2222222222222222222'));
// string(19) "1.3333333333333E+19"
var_dump(bcadd((string) ('11111111111111111111' + '2222222222222222222'), '1'));
// string(1) "1" - bummer!
var_dump(bcadd(bcadd('11111111111111111111', '2222222222222222222'), '1'));
// string(20) "13333333333333333334"

Otherwise put, just a single test that actually uses very large numbers will catch our mutations. That's a whole lot of mutations for all those functions busted by a single test.

[maks-rafalko]

Otherwise put, just a single test that actually uses very large numbers will catch our mutations.

But this is the goal of this mutator - to force developers to write such a test (with big numbers or with floats), isn't it?

What I mean:

We have a method that uses bcadd:

public function add(string $a, string $b): string
{
    return bcadd($a, $b);
}

Then, such test is useless:

assertSame('3', Calculator::add('1', '2'));

and mutant won't be killed. And escaped mutant shows that bcadd function is not needed in this method.

This will force the developer to update such test to:

assertSame('1333333333333333333', Calculator::add('11111111111111111111', '2222222222222222222');

if they really want to work with big numbers, or otherwise replace bcadd($a, $b with $a + $b.

[sanmai]

But this is the goal of this mutator - to force developers to write such a test (with big numbers or with floats), isn't it?

Agreed.

I wonder if @majkel89 have some other arguments against having this enabled by default.

[majkel89]

But this is the goal of this mutator - to force developers to write such a test (with big numbers or with floats), isn't it?

Agreed.

I wonder if @majkel89 have some other arguments against having this enabled by default.

I also agree. Mine only concern was performance but no I think it wont be a gig deal if we disable the mutator when the extension is not loaded.