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.

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

Support custom mutators #1686

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

vss414
Copy link

@vss414 vss414 commented Apr 29, 2022

Infection supports only a set of Mutators which are based on AST and PHP-Parser project. In such situation library users can't use their own mutators (which may be useful in their projects but doesn't fit to be a part of the library).
This PR adds the ability to use custom mutators in projects that use the infection library.
Custom mutators should implement Infection\Mutator\Mutator interface.

To use custom mutators, user must add them to the mutator list in the infection config. User has to use the class name with the namespace of the custom mutator as a name of mutator in the config. An example of using a custom mutator in the infection config:

{
    "mutators": {
        "App\\Infection\\MyFirstCustomMutator": true,
        "App\\Infection\\MySecondCustomMutator": {
            "ignore": [
                "*Controller::*",
            ]
        }
    }
}

@maks-rafalko maks-rafalko self-requested a review April 29, 2022 10:03
@vss414 vss414 force-pushed the feature/support-custom-mutators branch from 04f5f36 to f49a2e7 Compare May 4, 2022 02:03
@maks-rafalko
Copy link
Member

maks-rafalko commented May 4, 2022

Overall, I'm ok with a feature itself, however it is much more complex.

Related to issues/comments discussed before

As it was already pointed out in those comments, we should consider the following cases (this list can be extended and should be taken into account/discussed during development of this feature here or in other PRs):

  • exposing Mutator interface. Now it's @internal, but should be "public". I was thinking about moving this interface to a separate package so that developers can depend on highly abstract package to develop their own Mutators (as we did here, but this can be done later). And unfortunately, Node interface from PHP-Parser is also sitting in the main package, which is bad from package design
  • PHAR distribution. Now, all the classes inside Infection are prefixed during that PHAR compilation. It means, Mutator interface will be prefixed (read as "namespace will be changed") as well, and this will break your custom mutator. We must to stop prefixing Mutator interface. For example, PHPStan does the same - see https://github.com/phpstan/phpstan-src/blob/1.7.x/compiler/build/scoper.inc.php#L268-L269. Note that Node interface shouldn't also be prefixed, unfortunately.
  • e2e test must be added (see tests/e2e and tests/add_new_e2e) to check custom Mutator works with infection/infection as well as with PHAR

By the way, what is your custom Mutator? Can you please provide an example? I'm thinking about what are your case, probably we will need to create another packages like infection-dotrine similar to what PHPStan does with its plugins.

resources/schema.json Outdated Show resolved Hide resolved
@michalbundyra
Copy link
Contributor

@vss414 This is great! Exactly what I was looking for. Can't wait to get my hands on it :)

@maks-rafalko My custom mutator will be to remove some calls while using fluent interfaces - like:

$query->select('*')->withTrashed();

so I'd add custom mutator to remove part of that statement:

Mutations:

$query->select('*');

and:

$query->withTrashed();

Ofc if we force our developers to not use fluent interfaces then it will be not needed.
But the thing right now is that we are using them and mutation like that would be very nice.
Not sure if it can be global mutation, that's why I would be ok to have it in my project.

I had another examples I wanted to use custom mutators, but I forgot now....

Thanks!

@blackwolf12333
Copy link

I can maybe add some examples from our codebase that would require support for custom mutators to be more effective.

Our codebase is using a custom framework in which we have a custom query language that I would like to be able to mutate. For example querying some database table on a field to be greater than 0 would look like:

new ApiConditionSimple('field', 0, '>')

It would be nice to be able to mutate the '>' string which infection currently can't do because it's just a string. But it would be nice to be able to apply the @conditional_boundary mutations to that for example.

@maks-rafalko maks-rafalko self-assigned this May 4, 2024
@maks-rafalko maks-rafalko force-pushed the feature/support-custom-mutators branch from f49a2e7 to 442d08f Compare May 4, 2024 16:39
@maks-rafalko
Copy link
Member

maks-rafalko commented May 4, 2024

I decided to move forward with this one, as it turned out we are very close to make it possible, and there is a need for this from Ondrej (maintainer of PHPStan), so we will have a good first consumer of our API.

This will be the first PR, in the following PRs I will extract our extension points to a separate package so that anyone who needs to implement custom mutator will require only small package infection/mutator (for example) with an interface instead of require the whole Infection.

But I have a problem here, seems like Box/Scoper have a bug or something like that.

@theofidry when I add Infection\Mutator\Mutator in the excluded files in scoper.inc.php, and run

cd tests/e2e/Custom_Mutator

../../../build/infection.phar

then I have an error:

PHP Fatal error:  Uncaught Error: Interface "Infection\Mutator\Mutator" not found in phar:///home/maksrafalko/apps/infection/build/infection.phar/src/Mutator/Arithmetic/Assignment.php:15
Stack trace:
#0 phar:///home/maksrafalko/apps/infection/build/infection.phar/.box/vendor/composer/ClassLoader.php(576): include()
#1 phar:///home/maksrafalko/apps/infection/build/infection.phar/.box/vendor/composer/ClassLoader.php(427): Composer\Autoload\{closure}()

which looks weird, as there is a non-scoped interface in the PHAR.


UPD: probably, I misunderstood exclude and expose concepts inside Scoper, so I re-read and updated scoper.inc.php, let's see if it works.

The bad thing that PHP-Parser's PhpParser\Node interface is not in the separate package, so we have to depend on the whole nikic/php-parser just to use 1 interface :| and because of that - do not prefix the whole parser package. I tried to not prefix only this interface, but then it doesn't work, probably I'm doing it wrong so @theofidry your review help would be appreciated

Stanislav Vozhov and others added 3 commits May 4, 2024 19:11
@maks-rafalko maks-rafalko force-pushed the feature/support-custom-mutators branch from c8a33e8 to 094ef17 Compare May 4, 2024 17:11
@maks-rafalko maks-rafalko force-pushed the feature/support-custom-mutators branch from ccbf248 to 484c5a8 Compare May 4, 2024 17:58
@theofidry
Copy link
Member

UPD: probably, I misunderstood exclude and expose concepts inside Scoper, so I re-read and updated scoper.inc.php, let's see if it works.

What you likely want there is expose indeed, exclude is to mark a symbol as "native to PHP" (how it is handled is... depends, e.g. you can declare native PHP symbols, for example that is what polyfills are for, or for handling symbols coming from third-party that is not in your vendor for WP plugins).

The bad thing that PHP-Parser's PhpParser\Node interface is not in the separate package, so we have to depend on the whole nikic/php-parser just to use 1 interface :| and because of that - do not prefix the whole parser package. I tried to not prefix only this interface, but then it doesn't work, probably I'm doing it wrong so @theofidry your review help would be appreciated

I don't think that would be possible, as a consumer you want to be able to consume any node no, so it's more than just the node interface?

I think for now it's best if you expose the whole PhpParser namespace. I can't review the code right now will try to do so ASAP

@maks-rafalko
Copy link
Member

Replacing exclude-namespaces with expose-namespaces for PHP-Parser namespaces leads to error during make compile (809d8f2):

PHP Fatal error:  Cannot declare class Infected\PhpParser\Internal\TokenPolyfill, because the name is already in use in phar:///home/runner/work/infection/infection/build/infection.phar/vendor/nikic/php-parser/lib/PhpParser/Internal/TokenPolyfill.php on line 8

https://github.com/infection/infection/actions/runs/8953112456/job/24591330644?pr=1686#step:15:339

So for now I will revert it.

…per config as per comments"

This reverts commit 809d8f2.
Copy link
Member

@theofidry theofidry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only a glance review; need to do a proper one still

],
'expose-namespaces' => [
// we have to expose it cause Mutator depends on PhpParser/Node interface, and it's not in a separate package
'/^PhpParser/',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be just the name:

Suggested change
'/^PhpParser/',
'PhpParser',

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I started with, but running infection.phar then fails with:

PHP Fatal error:  Uncaught Error: Class "Infection\PhpParser\FileParser" not found in phar:///home/maksrafalko/apps/infection/build/infection.phar/src/Container.php:185

It looks like it matches Infection\PhpParser\FileParser in the middle

src/Mutant/MutantExecutionResult.php Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants