Add pre-filters configuration option #1327

sanmai · 2020-09-08T08:52:08Z

This PR:

Adds optional pre-filters
Fixes Plugin system to allow pre-/post- filtering and modification of mutations #1323
Covered by tests (pending pre-approval)
PHPStan isn't unhappy
Doc PR: TBD

Premise

The static analysis plugin uses only $mutant->getFilePath(); to infer whenever a mutation is good to go or not, but there might be additional optimizations to check for new defects, for example if we consider that we know which line we're mutating.

Design

To address the needs of this or similar plugins this PR introduces the notion of a plugin.

Plugins specified in a configuration section named "plugins" as FQCN.
- Every plugin must come with a static constructor method appropriate for the type, currently it is only ::create()
Plugins must extend any of the public interfaces. Right now there's only one: MutantFilterPlugin
- It must implement getMutantFilter() method returning a callable.

Example filter

namespace PreMutantFilter;

use Infection\Plugins\MutantFilterPlugin;
use Infection\Plugins\Configuration;
use Infection\Plugins\Mutant;

class ExampleFilter implements MutantFilterPlugin
{
    private $count;

    private function __construct()
    {
        $this->count = 0;
    }

    public static function create(Configuration $configuration)
    {
        // This Configuration object doesn't do anything yet, but it leave a
        // possibility to export anything we want from the original container,
        // or add some plugin-specific configuration options.
        return new static();
    }

    public function getMutantFilter(): ?callable
    {
        return function (Mutant $mutant) {
            // Do whatever there's need to be done.
            $mutant->getFilePath();
            $mutant->getMutatedCode();
            $mutant->getDiff();
            $mutant->isCoveredByTest();
            $mutant->getMutatorName();

            ++$this->count;

            // Allow only two first mutants
            return $this->count <= 2;
        };
    }
}

For this filter to work infection.json needs to be updated like so:

{
    "timeout": 25,
    "plugins": [
        "\\PreMutantFilter\\ExampleFilter"
    ]
}

Future new filters should not require configuration changes since we can infer filter usage by looking at its type.

These filters work with infection.phar just as well as with Composer-installed Infection.

Ocramius

This gets the work done, but is not really an acceptable approach.

I suggest having a plugins string reference, which requires a list<class-string<InfectionPlugin>>, where:

interface InfectionPlugin
{
    // @TODO singleton? I would really rather just talk to the infection container instead of this
    public static function instance(): static; 

     /** @psalm-return null|callable(Mutant): bool */
     public function preFilterMutant(): ?callable;
}

Requiring zero-argument-constructor is not really viable

src/Configuration/Configuration.php

Ocramius · 2020-09-08T15:07:10Z

src/Mutant/MutantFiltersHandler.php

+        foreach ($this->filters as $filterClassName) {
+            Assert::classExists($filterClassName);
+
+            new $filterClassName($mutants);


This looks exceptionally ugly :D

Can we just have the filter instances injected? They will come with their complex DI setup anyway...

It is very peculiar indeed.

You mean like with $container->getMutantFiltersHandler()->addFilter(...)? Would this mean a filter will have own executable? How one could have several filters?

Ideally (yes, below is a callable producing another callable):

/** @psalm-var callable(Container): callable(Mutant): bool $factory */ $container->registerFilter($factory);

Plugin system could then be something like:

interface InfectionPlugin { public static function register(Container $container): void; }

No runtime operation: the plugin can only manipulate or use the infection DIC before it is given to Application:

infection/src/Console/Application.php

Line 72 in b28f2e2

public function __construct(Container $container)

Registering plugins is then a matter of having a list<class-string<InfectionPlugin>> as part of infection.json.dist

Note: we can certainly keep InfectionPlugin as @internal and say "it's experimental - expect breakages in minor releases" or such

What will InfectionPlugin::register() do?

Whatever the plugin needs to do to start up.

For example, in my case I need to get the CWD and find psalm's config file, others may register autoloading, etc.

I mean, does it really need to have an instance of Container?

Yes, because that's how you are going to manipulate and decorate/replace infection components.

tests/e2e/PreMutantFilter/filter/ExampleFilter.php

src/Configuration/Schema/SchemaConfiguration.php

theofidry · 2020-09-08T15:33:56Z

I can't have a deep look at it atm but I feel it would be easier if we leverage a PHP config as an extension point like for example done in PHP-Scoper with patchers or the isolated finder: it allows us to expose a common API, PHAR or not and the user can freely do things, he/she only has to provide the object instances we need

sanmai · 2020-09-14T03:27:36Z

@Ocramius Please see the updated API.

Right now I'm not so sure about giving access to the full Containter object because if your plugin becomes a success, which I hope it will, and if we'll have to break things inside Container, which I have some ideas about, there will be BC breaks, and they'll become upgrade blockers like with have now with PHPUnit 9.3.

If we can avoid these, we better do, so if you have an idea exactly what you want to know from the container object, or exactly what you want to do with it, we can make an explicit API just for that, ensuring future BC breaks would be less of an issue.

Ocramius · 2020-09-14T03:30:34Z

Hopefully reviewing later today then 👍

src/Mutant/MutantFiltersHandler.php

tests/e2e/PreMutantFilter/filter/ExampleFilter.php

sanmai · 2020-09-16T02:23:13Z

@Ocramius I've added exportable interfaces for Container and Mutant. If you need anything specifically from Container or underlying object like Configuration, we can add these now.

Test are broken, I'll fix them if we decide this is the right approach.

sanmai · 2020-09-21T02:50:49Z

@Ocramius @maks-rafalko

I'd like to do something about #1283, and this might require more configuration changes, and I really don't want to step on my own toes there and here, so if you might find a moment, what if we can reach a certain decision here?

Ocramius · 2020-09-21T03:03:37Z

I totally missed to check this one as I hoped to do earlier last week, sorry.

Will try allocating time for it on Monday/Tuesday this starting week :|

theofidry · 2020-09-21T07:13:41Z

src/Mutant/Mutant.php

@@ -101,4 +108,9 @@ public function getTests(): array
    {
        return $this->mutation->getAllTests();
    }
+
+    public function getMutantWrapper(): PublicMutant


Suggested change

public function getMutantWrapper(): PublicMutant

public function getPublicMutant(): PublicMutant

theofidry · 2020-09-21T07:13:51Z

src/Mutant/Mutant.php

+    /**
+     * @var PublicMutant|null
+     */
+    private $wrapper;


Suggested change

private $wrapper;

private $publicMutant;

theofidry · 2020-09-21T07:21:06Z

src/Plugins/Plugin.php

+    /** @return Plugin */
+    public static function create(Configuration $configuration);


Suggested change

/** @return Plugin */

public static function create(Configuration $configuration);

public static function create(Configuration $configuration): Plugin;

That's going to be a major headache PHPStan-wise, unfortunately. I know this because the original design had exactly this signature, and, well, suffice to say that return type covariance isn't yet implemented in PHP 7.3.

But I'll try to come back at this problem again.

If we merge the two plugin interfaces that problem will be solved no?

theofidry · 2020-09-21T07:22:12Z

src/Plugins/Wrapper/ContainerWrapper.php

+ */
+final class ContainerWrapper implements Configuration


Is Configuration appropriate? In the end should not be Container?

It should not be Container, yes. It may include some features on an actual Container, but I have yet to see why a plugin can't roll their own DI container.

I believe it's not so much a question of whether they have their own DI, but about either exposing some elements of our Container. For example here you could have a Container::registerMutantFilter() hence you don't need to call the plugin afterwards, you just create it, it will register what it needs to and done.

So maybe an alternative approach is to have this public Container interface where we have the registerMutantFilter() and make our container implement that public one: that way we keep one container instance on our side but be very clear on what is exposed to the Plugin API

theofidry · 2020-09-21T07:24:19Z

src/Plugins/MutantFilterPlugin.php

+     *
+     * @psalm-return null|callable(Mutant): bool
+     */
+    public function getMutantFilter(): ?callable;


Might be more straightforward to pass an array of such callables instead? This way we can also require the array to be a callbable[] i.e. not allow null

Design-wise it won't matter for us will it be an iterable<callable(Mutant)> or just a callable. @Ocramius, do you think you'll be better off with a list of callables here?

theofidry · 2020-09-21T07:25:12Z

src/Mutant/MutantFiltersHandler.php

+ * @internal
+ * @final
+ */
+class MutantFiltersHandler


Suggested change

class MutantFiltersHandler

class MutantFilterer

?

src/Mutant/MutantFiltersHandler.php

theofidry · 2020-09-21T07:28:04Z

src/Plugins/MutantFilterPlugin.php

+ * Plugin to filter mutations with external tools.
+ */
+interface MutantFilterPlugin extends Plugin
+{


Maybe for now we can merge MutantFilterPlugin with Plugin? It's not too constraining to implement getMutantFilter()

We can, but that'll cost us BC breaks in the future. For example, there's an idea to make #1326 an internal plugin. It may require a different interface, and a different injection point. Merging these interfaces will create an inconvenience.

theofidry · 2020-09-21T07:29:09Z

src/Process/Runner/MutationTestingRunner.php

@@ -73,6 +75,7 @@ class MutationTestingRunner
    public function __construct(
        MutantProcessFactory $processFactory,
        MutantFactory $mutantFactory,
+        MutantFiltersHandler $pluginsHandler,


Suggested change

MutantFiltersHandler $pluginsHandler,

MutantFiltersHandler $mutantFilterer,

In a sense we don't care where it comes from at this point, plugin or elsewhere. I'm even wondering if the two filter() calls should be moved to such filterers

sanmai · 2020-09-21T07:53:58Z

@Ocramius I really appreciate that! I don't want you spend time reviewing implementation (I don't think I spend enough time polishing it, and there are pending suggestions), but if you can tell that the external interface is sufficient or not, that'll be great.

I'm thinking about adding another object to the callback, named, say, Result, to make it callable(Mutant, Result) to make the filtering API more explicit if we'd want to not just filter the mutations but do something about them.

theofidry · 2020-09-21T08:41:19Z

I'm thinking about adding another object to the callback, named, say, Result, to make it callable(Mutant, Result) to make the filtering API more explicit if we'd want to not just filter the mutations but do something about them.

I quite like that idea and push it even a bit further:

class MutantExecutionResult {
}

class MutantExecution {
    public readonly Mutant $mutant;
    public readonly MutantExecutionResult $result;
}

public function run(iterable $mutations, string $testFrameworkExtraOptions): void
    {
        $numberOfMutants = IterableCounter::bufferAndCountIfNeeded($mutations, $this->runConcurrently);
        $this->eventDispatcher->dispatch(new MutationTestingWasStarted($numberOfMutants));

        $processes = take($mutations)
            ->map(function (Mutation $mutation): Mutant {
                return new MutantExecution(
                    $this->mutantFactory->create($mutation),
                    new MutantExecutionResult()
                );
            })
            ->tap(function (MutantExecution $execution) {
                if (!$execution->isCoveredByTest()) {
                    $execution->result->status = 'notCovered';
                }
            })
            ->tap(function (MutantExecution $execution) {
                $result = $execution->result;

                if ($result->status !== 'pending') {
                    return;
                }

                $mutant = $execution->mutant;
                $mutatorName = $mutant->getMutation()->getMutatorName();

                if (!array_key_exists($mutatorName, $this->ignoreSourceCodeMutatorsMap)) {
                    return;
                }

                foreach ($this->ignoreSourceCodeMutatorsMap[$mutatorName] as $sourceCodeRegex) {
                    if ($this->diffSourceCodeMatcher->matches($mutant->getDiff(), $sourceCodeRegex)) {
                        $result->status = 'ignored';
                        return;
                    }
                }
            })
            ->tap(function (MutantExecution $execution) {
                if ($execution->mutant->getMutation()->getNominalTestExecutionTime() > $this->timeout) {
                    $execution->result->status = 'skipped';
                }
            })
            // Note that the commend from the PR still applies: the two previous
            // taps could be moved in the handler in some way.
            // I would however prefer an API we we can wire the call like so
            // Instead of doing a $this->filterer->applyFilters($pipeline);
            // Also note that this comes _after_ all the previous call to allow
            // the plugins to take advantage of some infection built-in processing
            // and correct the information if they see fit before we do any
            // actual filtering
            ->tap($this->pluginsHandler->tap())
            ->filter(function (MutantExecution $execution) {
                $status = $execution->result->status;
                $mutant = $execution->mutant;

                if ($status === 'notCovered') {
                    $this->eventDispatcher->dispatch(new MutantProcessWasFinished(
                        MutantExecutionResult::createFromNonCoveredMutant($mutant)
                    ));

                    return false;
                }

                if ($status === 'ignored') {
                    // Do not appear in the results at all
                    return false;
                }

                if ($status === 'skipped') {
                    $this->eventDispatcher->dispatch(new MutantProcessWasFinished(
                        MutantExecutionResult::createFromTimeSkippedMutant($mutant)
                    ));

                    return false;
                }

                return !$execution->result->executed;
            })
            ->map(function (MutantExecution $execution) use ($testFrameworkExtraOptions): ProcessBearer {
                $mutant = $execution->mutant;

                $this->fileSystem->dumpFile($mutant->getFilePath(), $mutant->getMutatedCode());

                $process = $this->processFactory->createProcessForMutant($mutant, $testFrameworkExtraOptions);

                return $process;
            })
        ;

        $this->processRunner->run($processes);

        $this->eventDispatcher->dispatch(new MutationTestingWasFinished());
    }

maks-rafalko

I'm really sorry for the delay with the feedback, tough days here in Belarus...

As for me - I agree with several of Theo comments. Design for me looks good, I'm ok with existing architecture.

Let's see what @Ocramius will say as he is the main consumer at the moment.

The only one major issue here is package design principles. I don't say we should do anything right now, let's agree on the architecture first, but then - we will need to move interfaces out of infection/infection.

Idea is similar to what I described here for Test Framework Adapters.

For example, one of the reasons of moving interfaces to separate packages is to allow developers not to depend on infection/infection in their plugins.

Currently, Roave/infection-static-analysis-plugin depends on infection/infection:0.17.5 and will depend on the same package if we merge it as is.

Instead, it would depend on infection/abstract-plugin:^1.0.0 (name is just for example) and would work with any version of infection/infection that works properly with infection/abstract-plugin:^1.0.0

and so on...

So, 👍 on what is already here

maks-rafalko · 2020-10-21T10:00:07Z

@Ocramius friendly reminder about this PR

VincentLanglet · 2022-11-01T18:48:35Z

What is the next step of this PR ?

sanmai · 2022-11-01T23:10:56Z

I think it is time for someone to make a fresh start 😓

sanmai · 2022-11-01T23:12:06Z

Few years later I would emphasize @Ocramius' suggestion to let the plugins operate on the container.

maks-rafalko · 2022-11-02T13:41:02Z

I already started investigating it and have new ideas in mind (the current solution for me is not what we need for Psalm/PHPStan plugins, because we should not pre-filter mutations but kill mutations by SA tool only after tests can not kill them (this is btw what happens on infection-static-analysis-plugin), because tests are much faster than running another tool under the hood).

@VincentLanglet I will (hopefully) open RFC somewhere this year, this is the next feature I will work on (read - most prioritized)

VincentLanglet · 2022-11-02T13:53:37Z

I will (hopefully) open RFC somewhere this year, this is the next feature I will work on (read - most prioritized)

Awesome, glad to hear it's on the todo-list :)

maks-rafalko · 2024-05-04T09:29:08Z

I got back to this idea.

I decided to simplify the things first:

not trying to come up with a very generic post- / pre- filters, but instead "test" the idea by implementing SA support inside Infection core
decided to go with PHPStan, as I personally see that it's more actively developed
after Ondrej implements what we need here at Infection, I will test integrating PHPStan as pre- and post- job. I would like to see what is faster: to run tests and then trying to kill Mutant with PHPStan, or run PHPStan to decide if this Mutant is even valid. nobody can answer to this question without real benchmark I guess

and only then we can think if we really need any "plugin system". If it's only PHPStan and all other virtual potential cases are just imaginary ones, I don't think we need to spend time on it, and we will just integrate PHPStan in the core first, opt-in (TBD)

sanmai marked this pull request as ready for review September 8, 2020 14:58

sanmai requested a review from maks-rafalko September 8, 2020 14:59

sanmai mentioned this pull request Sep 8, 2020

Plugin system to allow pre-/post- filtering and modification of mutations #1323

Open

Ocramius suggested changes Sep 8, 2020

View reviewed changes

sanmai commented Sep 8, 2020

View reviewed changes

src/Configuration/Schema/SchemaConfiguration.php Outdated Show resolved Hide resolved

sanmai added the Feature label Sep 14, 2020

sanmai commented Sep 14, 2020

View reviewed changes

src/Mutant/MutantFiltersHandler.php Outdated Show resolved Hide resolved

sanmai commented Sep 15, 2020

View reviewed changes

tests/e2e/PreMutantFilter/filter/ExampleFilter.php Outdated Show resolved Hide resolved

sanmai mentioned this pull request Sep 16, 2020

Add an ability to ignore Mutators for source code line(s) by regular expression #1326

Merged

3 tasks

Adds pre-filters configuration option

4d4f6d7

sanmai force-pushed the pr/2020-09/preFilters branch from 9308bcf to 4d4f6d7 Compare September 17, 2020 08:08

theofidry self-requested a review September 20, 2020 09:55

theofidry approved these changes Sep 21, 2020

View reviewed changes

maks-rafalko reviewed Sep 28, 2020

View reviewed changes

Merge branch 'master' into pr/2020-09/preFilters

f5f9a9a

maks-rafalko mentioned this pull request Nov 1, 2020

Update to infection 0.20 Roave/infection-static-analysis-plugin#79

Closed

sanmai closed this Nov 1, 2022

sanmai deleted the pr/2020-09/preFilters branch November 1, 2022 23:10

	public function getMutantWrapper(): PublicMutant
	public function getPublicMutant(): PublicMutant

		/** @return Plugin */
		public static function create(Configuration $configuration);

	MutantFiltersHandler $pluginsHandler,
	MutantFiltersHandler $mutantFilterer,

Add pre-filters configuration option #1327

Add pre-filters configuration option #1327

Conversation

sanmai commented Sep 8, 2020 • edited

Premise

Design

Example filter

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius Sep 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented Sep 8, 2020 • edited

sanmai commented Sep 14, 2020 • edited

Ocramius commented Sep 14, 2020

sanmai commented Sep 16, 2020

sanmai commented Sep 21, 2020

Ocramius commented Sep 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry Sep 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanmai commented Sep 21, 2020 • edited

theofidry commented Sep 21, 2020

maks-rafalko left a comment • edited

Choose a reason for hiding this comment

maks-rafalko commented Oct 21, 2020

VincentLanglet commented Nov 1, 2022

sanmai commented Nov 1, 2022

sanmai commented Nov 1, 2022

maks-rafalko commented Nov 2, 2022 • edited

VincentLanglet commented Nov 2, 2022

maks-rafalko commented May 4, 2024 • edited

sanmai commented Sep 8, 2020 •

edited

Ocramius Sep 8, 2020 •

edited

theofidry commented Sep 8, 2020 •

edited

sanmai commented Sep 14, 2020 •

edited

theofidry Sep 21, 2020 •

edited

sanmai commented Sep 21, 2020 •

edited

maks-rafalko left a comment •

edited

maks-rafalko commented Nov 2, 2022 •

edited

maks-rafalko commented May 4, 2024 •

edited