Add reflection classes to mutators. Do no mutate public method visibility if parent has the same one by maks-rafalko · Pull Request #67 · infection/infection

maks-rafalko · 2017-11-06T20:13:10Z

This is an attempt to add native PHP Reflection to Mutator Operators. Fixes #63.

\ReflectionClass instance for each Node inside functions/methods
Move parsed source code from test files (string variables) to separate real files to make them autoloadable.
Try PHP-Scoper to prefix namespaces inside infection.phar. This is needed because now we load the source code of a target project (creating instance of \ReflectionClass loads the target class). ~~BLOCKED by PSR-0 dependencies are not correctly prefixed humbug/php-scoper#127~~
~~BLOCKED by Correctly determine Type of Node when PHP-Parser's namespaces are prefixed nikic/PHP-Parser#441~~
Test as a project dependency.
Test on many different projects as a PHAR.

As a result:

Many useless mutants are not created anymore for PublicVisibility and ProtectedVisibility mutators (-130 mutants for infection itself).
Now we can start to think/implement new mutators that was not possible to implement before without Reflection.

Please note:

Now Infection loads project's autoload.php (see app/bootstrap.php) in order to instantiate \ReflectionClass() instances

Ho to prefix:

cd infection
composer install --no-dev
/path/to/php-scoper/bin/php-scoper add-prefix --output-dir=build-prefixed --force --prefix=Mutant

composer dump-autoload --working-dir=build-prefixed/ --classmap-authoritative --no-dev

sudo chmod +x build-prefixed/bin/infection

Feedback/review is much appreciated.

UPD: Finally, I was able to prefix Infection lib with PHP-Scoper without errors, got the same MSI/Covered MSI, number of mutations, mutated files with Prefixed PHAR as with the original source code.

BackEndTea · 2018-01-22T17:35:36Z

Both humbug/php-scoper#127 and nikic/PHP-Parser#441 are fixed now.

theofidry · 2018-01-22T17:45:32Z

I would try PHP-Scoper a bit later and in a dedicated PR as I'm sure it's gonna be a work on its own. Infection may be fine itself, but guaranteed to find weird cases with PhpSpec & PHPUnit

maks-rafalko · 2018-01-23T10:44:12Z

Both humbug/php-scoper#127 and nikic/PHP-Parser#441 are fixed now.

great. Will continue working on this PR next week

but guaranteed to find weird cases with PhpSpec & PHPUnit

it shouldn't affect prefixing Infection as I understand since we don't have such dependencies in --no-dev mode

maks-rafalko · 2018-02-06T20:30:37Z

@infection/core I think this is ready for review.

I've updated initial comment, please see details. If you have some time - could you please check it with some of your project(s)? With prefixed PHAR and as a dependency.

Not sure why builds are failing on PHP 7.2 with phpdbg. Didn't dig in yet.

@theofidry your feedback on PHP-Scoper usage is highly appreciated

sidz · 2018-02-10T18:37:00Z

@borNfreee I've run Infection from this PR under phpdbg (php7.2) and got the same issue with segfault. Potentially it's an issue with php itself. It'll be nice to submit an issue (if it's an php issue) to the php bug-tracker

maks-rafalko · 2018-02-13T21:38:57Z

Potentially it's an issue with php itself

I was able to reproduce it with PHP 7.2.2 today. It fails with phpdbg -qrr vendor/bin/phpunit but works with php vendor/bin/phpunit.

Will continue git-bisecting tomorrow.

theofidry

I left some comments. I'll have to check that in more details as there's significant changes I need to pay more attention to it.

Regarding PHP Scoper I think it's mandatory to have an e2e test with the scoped file if you wanna integrate it. That said maybe this can be done in a separate PR.

Also I didn't see any change on how we are building the PHAR to pick up the scoped files, did I miss something?

theofidry · 2018-02-15T17:09:48Z

scoper.inc.php

+    // heart contents.
+    //
+    // For more see: https://github.com/humbug/php-scoper#patchers
+    'patchers' => [


you can remove this entry as you're not using it atm

theofidry · 2018-02-15T17:10:00Z

scoper.inc.php

+    // (the class name) as an argument and return a boolean (true meaning the class is going to prefixed).
+    //
+    // For more, see https://github.com/humbug/php-scoper#global-namespace-whitelisting
+    'global_namespace_whitelist' => [


I don't think we need this

theofidry · 2018-02-15T17:10:07Z

scoper.inc.php

+    // that this does not work with functions or constants neither with classes belonging to the global namespace.
+    //
+    // Fore more see https://github.com/humbug/php-scoper#whitelist
+    'whitelist' => [


neither this one

theofidry · 2018-02-15T17:11:27Z

src/TestFramework/Config/MutationConfigBuilder.php

 CONTENT;
    }
+
+    private function getInterceptorNamespacePrefix()


you can add the string return type

theofidry · 2018-02-15T17:14:07Z

src/Visitor/FullyQualifiedClassNameVisitor.php

+            $this->namespace = $node->name;
+        } elseif ($node instanceof Stmt\ClassLike) {
+            if (null !== $node->name) {
+                $node->fullyQualifiedClassName = Name::concat($this->namespace, $node->name);


Is this supposed to work for any class? Because if that's the case this won't be enough as you need to account for when an alias is used with the use statement (if any)

I'm not sure I got it. This visitor's goal is to build FQCL for the current class, not for other imported (via use statement) classes.

When we have:

namespace Infection\Mutator; use SomeClass; use Some\OtherClass as Aliased; class Plus { }

this visitor should build a Infection\Mutator\Plus FQCN and save it to the node to fullyQualifiedClassName property.

It does not need to work with SomeClass, OtherClass, Aliased.

Then we use it to build reflection in ReflectionVisitor:

if ($node instanceof Node\Stmt\ClassLike) { $this->reflectionClass = new \ReflectionClass($node->fullyQualifiedClassName->toString()); }

Oh right, I had my head too much in php-scoper. Should be alright then :)

theofidry · 2018-02-15T17:16:22Z

tests/Mutator/FunctionSignature/ProtectedVisibilityTest.php

+
+        $mutatedCode = $this->mutate($code);
+
+        $expectedMutatedCode = <<<'CODE'


I think 'PHP' would be better here as IDEs can pick that up for inspections

theofidry · 2018-02-15T17:17:09Z

tests/Mutator/FunctionSignature/ProtectedVisibilityTest.php

+
+    public function test_it_does_not_mutate_if_parent_class_has_same_protected_method()
+    {
+        $code = file_get_contents(__DIR__ . '/../../Fixtures/Autoloaded/ProtectedVisibility/pv-same-method-parent.php');


instead of having __DIR__ . '/../../Fixtures/Autoloaded[...] all the time everywhere maybe you can introduce a constant in the test case with the base path

sidz

Ok with me 👌

BackEndTea · 2018-02-20T13:08:34Z

Now Infection loads project's autoload.php (see app/bootstrap.php) in order to instantiate \ReflectionClass() instances

I don't know how relevant this is, but maybe we can allow users to define the location of the autoload file in the infection config. Similar to how phpunit does things.

maks-rafalko · 2018-02-25T10:59:32Z

Finally, rebased on master, ready for review again.

Regarding PHPScoper config, I think we will use Theo's one from humbug/php-scoper#172 later (but here PHPParser has been upgraded, so not all patchers are needed)

Also, I think we will release 0.8.0 and this PR will be shipped with the next major release as it is not ready yet.

I don't know how relevant this is, but maybe we can allow users to define the location of the autoload file in the infection config. Similar to how phpunit does things.

what does it bring for us?

sidz

Still ok 👍

BackEndTea · 2018-02-26T08:03:09Z

src/Visitor/ReflectionVisitor.php


    public function enterNode(Node $node)
    {
+        if ($node instanceof Node\Stmt\ClassLike) {


What happens here if we declare an anonymous class within our class?
Will all the nodes after that still be seen as instances of that anonymous class?

It crashes on anonymous classes in my test cases.
This could be fixed by

public function enterNode(Node $node) { if ($node instanceof Stmt\Namespace_) { $this->namespace = $node->name; } elseif ($node instanceof Stmt\ClassLike) { if (null !== $node->name) { $node->fullyQualifiedClassName = Name::concat($this->namespace, $node->name); } else { $node->fullyQualifiedClassName = null; } } }

For the FullyQualifiedClassNameVisitor

and

public function enterNode(Node $node) { if ($node instanceof Node\Stmt\ClassLike && $node->fullyQualifiedClassName !== null) { $this->reflectionClass = new \ReflectionClass($node->fullyQualifiedClassName->toString()); }

in the ReflectionVisitor

Thank you @BackEndTea, I've added a test case.

BackEndTea · 2018-02-26T19:10:04Z

what does it bring for us?

I realize now that it not that usefull for us. Phpunits version is for bootstrapping an application, not just autoloading

BackEndTea · 2018-02-26T20:04:29Z

Looks good to me. Id just like to see a test for the reflection visitor that checks if the reflection class is correctly set.

…ibility if parent has the same one Signed-off-by: borNfreee <b0rn@list.ru> User the correct namespace for Interceptor when code is prefixed with random character(s) Signed-off-by: borNfreee <b0rn@list.ru> Rebase and refactor. Add more tests Update PHPParser to 3.1.4 Do not analyze Fixture files by PHPStan as it loads the code and can't parse PHP7.1 syntax. Skip tests on PHP7.0 that requires 7.1+ Do not analyze Fixture files by PHPStan as it loads the code and can't parse PHP7.1 syntax. Skip tests on PHP7.0 that requires 7.1+ Add PHP-Scoper config. Decrease MSI since phpdbg results with the lower number than xdebug Rebase, resolve conflicts Remove empty files with PHP extension. This causes a segfault with phpdbg on PHP 7.2

…from master

BackEndTea

Looks good to me 👍

BackEndTea · 2018-02-27T18:57:07Z

src/Mutator/FunctionSignature/ProtectedVisibility.php

            return false;
        }

+        if ($this->hasSameProtectedParentMethod($node)) {


Just a small thing, but maybe its slightly faster to first check if the node is protected, before checking if it has a protected parent.

And ofcourse the same goes for public

maks-rafalko · 2018-03-05T19:34:55Z

Ok, I think it's ready to be merged (this PR also unlocks Profiles implementation).

Thanks everyone for the review and help.

maks-rafalko added the feature label Nov 6, 2017

maks-rafalko force-pushed the reflection2 branch from 77108bd to aa9b318 Compare November 6, 2017 20:14

maks-rafalko changed the title ~~[WIP] Add reflection classes to mutators. Do no mutate protected method visibility if parent has the same one~~ [WIP] Add reflection classes to mutators. Do no mutate public method visibility if parent has the same one Nov 6, 2017

maks-rafalko self-assigned this Nov 9, 2017

maks-rafalko mentioned this pull request Nov 9, 2017

PSR-0 dependencies are not correctly prefixed humbug/php-scoper#127

Closed

BackEndTea mentioned this pull request Jan 10, 2018

Filter mutations to only cover php classes #111

Closed

maks-rafalko force-pushed the reflection2 branch 6 times, most recently from 92fb97e to fea4fd7 Compare February 6, 2018 20:02

maks-rafalko requested review from sidz and theofidry February 6, 2018 20:36

BackEndTea mentioned this pull request Feb 9, 2018

Fix: Don't mutate abstract methods #169

Merged

2 tasks

BackEndTea mentioned this pull request Feb 11, 2018

Feature:disable certain mutators #177

Merged

4 tasks

This was referenced Feb 14, 2018

ProtectedVisibility False positive on implementation of abstract methods #180

Closed

Define a level of strictness #89

Closed

theofidry reviewed Feb 15, 2018

View reviewed changes

sidz approved these changes Feb 17, 2018

View reviewed changes

maks-rafalko added the in progress label Feb 23, 2018

maks-rafalko force-pushed the reflection2 branch from 6549cac to 20ad10d Compare February 25, 2018 10:29

sidz approved these changes Feb 25, 2018

View reviewed changes

BackEndTea reviewed Feb 26, 2018

View reviewed changes

maks-rafalko added 7 commits February 26, 2018 23:29

Rebase on master. Refactor tests to use new abstract test case class …

7e1c172

…from master

CS Fixes, add return typehints

f2fb2ba

Fix tests

17ca552

Tests refactoring

3cd2375

Add test for anonymous classes

eff38bf

Test reflectionClass is properly set

ec27507

maks-rafalko force-pushed the reflection2 branch from ef5101a to ec27507 Compare February 26, 2018 20:31

BackEndTea approved these changes Feb 27, 2018

View reviewed changes

BackEndTea reviewed Feb 27, 2018

View reviewed changes

maks-rafalko mentioned this pull request Feb 27, 2018

Fix Infection tests humbug/php-scoper#176

Merged

Reorder checks to run faster methods first

efd7048

maks-rafalko merged commit 926372c into master Mar 5, 2018

maks-rafalko deleted the reflection2 branch March 5, 2018 19:35

maks-rafalko removed the in progress label Mar 5, 2018

maks-rafalko added this to the 0.9.0 milestone Mar 5, 2018

maks-rafalko changed the title ~~[WIP] Add reflection classes to mutators. Do no mutate public method visibility if parent has the same one~~ Add reflection classes to mutators. Do no mutate public method visibility if parent has the same one Mar 5, 2018

maks-rafalko mentioned this pull request Mar 24, 2018

Add infection to PHIVE (PHAR Installation and Verification Environment) #134

Closed

maks-rafalko mentioned this pull request Apr 23, 2018

Build PHAR with Humbug Box #312

Merged


		$mutatedCode = $this->mutate($code);

		$expectedMutatedCode = <<<'CODE'

Uh oh!

Conversation

maks-rafalko commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ho to prefix:

Uh oh!

BackEndTea commented Jan 22, 2018

Uh oh!

theofidry commented Jan 22, 2018

Uh oh!

maks-rafalko commented Jan 23, 2018

Uh oh!

maks-rafalko commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sidz commented Feb 10, 2018

Uh oh!

maks-rafalko commented Feb 13, 2018

Uh oh!

theofidry left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidz left a comment

Choose a reason for hiding this comment

Uh oh!

BackEndTea commented Feb 20, 2018

Uh oh!

maks-rafalko commented Feb 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sidz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BackEndTea Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BackEndTea commented Feb 26, 2018

Uh oh!

BackEndTea commented Feb 26, 2018

Uh oh!

BackEndTea left a comment

Choose a reason for hiding this comment

Uh oh!

BackEndTea Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maks-rafalko commented Mar 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maks-rafalko commented Nov 6, 2017 •

edited

Loading

maks-rafalko commented Feb 6, 2018 •

edited

Loading

maks-rafalko commented Feb 25, 2018 •

edited

Loading

BackEndTea Feb 26, 2018 •

edited

Loading

BackEndTea Feb 27, 2018 •

edited

Loading