Fix the issue with anonymous classes inside arrays #1745

Slamdunk · 2022-10-18T10:18:35Z

This PR shows #1735 in action: https://github.com/infection/infection/actions/runs/3272547451/jobs/5383690401#step:13:265

Despite been related to PublicVisibility (and ProtectedVisibility as well), I have not been able to get the error in a Unit Test within \Infection\Tests\Mutator\FunctionSignature\PublicVisibilityTest.

I'd like to fix this and propose it here, but I'm stuck: I have no clue where to start, can you help me please with some hits?

Quick command to get the error locally: (cd tests/e2e/Github1735/ && php ../../../bin/infection -v)

Fixes #1735

Slamdunk · 2022-10-18T10:58:46Z

Man, Generators are awesome, except when you have to debug an infinite chain of them :(

sanmai · 2022-10-18T11:04:36Z

We can see where it happens in the stack trace:

Exception trace:
  at vendor/webmozart/assert/src/Assert.php:2042
 Webmozart\Assert\Assert::reportInvalidArgument() at vendor/webmozart/assert/src/Assert.php:762
 Webmozart\Assert\Assert::eq() at vendor/webmozart/assert/src/Assert.php:1727
 Webmozart\Assert\Assert::count() at src/TestFramework/Coverage/XmlReport/TestLocator.php:86
 Infection\TestFramework\Coverage\XmlReport\TestLocator->getTestsForFunctionSignature() at n/a:n/a

sanmai · 2022-10-18T11:20:14Z

This is where we initialize the range:

infection/src/TestFramework/Coverage/LineRangeCalculator.php

Lines 47 to 56 in c770354

    
           public function calculateRange(Node $node): NodeLineRangeData 
        
           { 
        
               $node = $this->getOuterMostArrayNode($node); 
        
               $endLine = $node->getAttribute(ReflectionVisitor::IS_ON_FUNCTION_SIGNATURE, false) === true 
        
                   ? $node->getStartLine() // function signature node should always be 1-line range: (start, start) 
        
                   : $node->getEndLine(); 
        
               return new NodeLineRangeData($node->getStartLine(), $endLine); 
        
           }

It looks like ReflectionVisitor::IS_ON_FUNCTION_SIGNATURE detection is failing here.

sanmai · 2022-10-18T11:38:51Z

There's something very confusing because if IS_ON_FUNCTION_SIGNATURE is set we should be getting a single element array, and what's more confusing is that NodeMutationGenerator refers to the same flag and it isn't the same there.

infection/src/Mutator/NodeMutationGenerator.php

Lines 177 to 180 in 8042277

    
           $testsMemoized = $this->trace->getAllTestsForMutation( 
        
               $this->lineRangeCalculator->calculateRange($this->currentNode), 
        
               $this->isOnFunctionSignature() 
        
           );

I don't think I have the mental capacity to understand this right now.

sanmai · 2022-10-18T11:46:57Z

It is interesting that if I restructure the fixture like so, the error goes away:

--- a/tests/e2e/Github1735/src/SourceClass.php
+++ b/tests/e2e/Github1735/src/SourceClass.php
@@ -6,13 +6,15 @@ class SourceClass
 {
     public function hello(): array
     {
+        $class = new class() {
+            public function foo(): bool
+            {
+                return true;
+            }
+        };
+
         $var = [
-            'class' => new class() {
-                public function foo(): bool
-                {
-                    return true;
-                }
-            },
+            'class' => $class,
         ];
 
         return $var;

… node to detect start/end lines of the method in an anonymous class instead of using outermost array start/end lines Example: ```php [1] $var = [ [2] 'class' => new class() { [3] public function foo(): bool [4] { [5] return true; [6] } [7] }, [8] ]; ``` Here, start line must be 3, end line 6. Previously it was 1 and 8 correspondingly

maks-rafalko · 2022-10-18T20:12:20Z

Thank you @Slamdunk for reporting the issue and creating a reproducer, this helps so much. Thanks @sanmai for the valuable feedback, helped me quicker understand the issue.

So, the bug in because of line

infection/src/TestFramework/Coverage/LineRangeCalculator.php

Line 49 in 4053dfd

$node = $this->getOuterMostArrayNode($node);

what's more confusing is that NodeMutationGenerator refers to the same flag and it isn't the same there.

this is because here

infection/src/Mutator/NodeMutationGenerator.php

Lines 177 to 180 in 8042277

    
           $testsMemoized = $this->trace->getAllTestsForMutation( 
        
               $this->lineRangeCalculator->calculateRange($this->currentNode), 
        
               $this->isOnFunctionSignature() 
        
           );

we use original node, but here

infection/src/TestFramework/Coverage/LineRangeCalculator.php

Lines 47 to 56 in c770354

    
           public function calculateRange(Node $node): NodeLineRangeData 
        
           { 
        
               $node = $this->getOuterMostArrayNode($node); 
        
               $endLine = $node->getAttribute(ReflectionVisitor::IS_ON_FUNCTION_SIGNATURE, false) === true 
        
                   ? $node->getStartLine() // function signature node should always be 1-line range: (start, start) 
        
                   : $node->getEndLine(); 
        
               return new NodeLineRangeData($node->getStartLine(), $endLine); 
        
           }

we use "outermost array node", wich is completely separate node for which IS_ON_FUNCTION_SIGNATURE is no longer true but false.

That being said, the update I pushed is just to use original node (public function foo()) to determine the start/end line of the method inside an anonymous class, instead of using outermost array node that contains this anonymous class.

The reported bug is fixed.

HOWEVER, I still think Infection can't (or is not able to currently) working with anonymous classes properly, even if this particular test passes.

Why?

This is how the XML coverage report look like:

<?xml version="1.0"?>
<phpunit xmlns="https://schema.phpunit.de/coverage/1.0">
  <file name="SourceClass.php" path="/">
    <totals>
      <lines total="21" comments="0" code="21" executable="4" executed="4" percent="100.00"/>
      <methods count="1" tested="1" percent="100.00"/>
      <functions count="0" tested="0" percent="0"/>
      <classes count="1" tested="1" percent="100.00"/>
      <traits count="0" tested="0" percent="0"/>
    </totals>
    <class name="Github1735\SourceClass" start="5" executable="4" executed="4" crap="1">
      <namespace name="Github1735"/>
      <method name="hello" signature="hello(): array" start="7" end="19" crap="1" executable="4" executed="4" coverage="100"/>
    </class>
    <coverage>
      <line nr="9">
        <covered by="Github1735\Test\SourceClassTest::test_hello"/>
      </line>
      <line nr="10">
        <covered by="Github1735\Test\SourceClassTest::test_hello"/>
      </line>
      <line nr="13">
        <covered by="Github1735\Test\SourceClassTest::test_hello"/>
      </line>
      <line nr="18">
        <covered by="Github1735\Test\SourceClassTest::test_hello"/>
      </line>
    </coverage>
    <source>
      <line no="1">
        <token name="T_OPEN_TAG">&lt;?php</token>
      </line>
      <line no="2"/>
      <line no="3">
        <token name="T_NAMESPACE">namespace</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_STRING">Github1735</token>
        <token name="T_SEMICOLON">;</token>
      </line>
      <line no="4"/>
      <line no="5">
        <token name="T_CLASS">class</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_STRING">SourceClass</token>
      </line>
      <line no="6">
        <token name="T_OPEN_CURLY">{</token>
      </line>
      <line no="7">
        <token name="T_WHITESPACE">    </token>
        <token name="T_PUBLIC">public</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_FUNCTION">function</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_STRING">hello</token>
        <token name="T_OPEN_BRACKET">(</token>
        <token name="T_CLOSE_BRACKET">)</token>
        <token name="T_COLON">:</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_ARRAY">array</token>
      </line>
      <line no="8">
        <token name="T_WHITESPACE">    </token>
        <token name="T_OPEN_CURLY">{</token>
      </line>
      <line no="9">
        <token name="T_WHITESPACE">        </token>
        <token name="T_VARIABLE">$var</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_EQUAL">=</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_OPEN_SQUARE">[</token>
      </line>
      <line no="10">
        <token name="T_WHITESPACE">            </token>
        <token name="T_CONSTANT_ENCAPSED_STRING">'class'</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_DOUBLE_ARROW">=&gt;</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_NEW">new</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_CLASS">class</token>
        <token name="T_OPEN_BRACKET">(</token>
        <token name="T_CLOSE_BRACKET">)</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_OPEN_CURLY">{</token>
      </line>
      <line no="11">
        <token name="T_WHITESPACE">                </token>
        <token name="T_PUBLIC">public</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_FUNCTION">function</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_STRING">foo</token>
        <token name="T_OPEN_BRACKET">(</token>
        <token name="T_CLOSE_BRACKET">)</token>
        <token name="T_COLON">:</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_STRING">bool</token>
      </line>
      <line no="12">
        <token name="T_WHITESPACE">                </token>
        <token name="T_OPEN_CURLY">{</token>
      </line>
      <line no="13">
        <token name="T_WHITESPACE">                    </token>
        <token name="T_RETURN">return</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_STRING">true</token>
        <token name="T_SEMICOLON">;</token>
      </line>
      <line no="14">
        <token name="T_WHITESPACE">                </token>
        <token name="T_CLOSE_CURLY">}</token>
      </line>
      <line no="15">
        <token name="T_WHITESPACE">            </token>
        <token name="T_CLOSE_CURLY">}</token>
        <token name="T_COMMA">,</token>
      </line>
      <line no="16">
        <token name="T_WHITESPACE">        </token>
        <token name="T_CLOSE_SQUARE">]</token>
        <token name="T_SEMICOLON">;</token>
      </line>
      <line no="17"/>
      <line no="18">
        <token name="T_WHITESPACE">        </token>
        <token name="T_RETURN">return</token>
        <token name="T_WHITESPACE"> </token>
        <token name="T_VARIABLE">$var</token>
        <token name="T_SEMICOLON">;</token>
      </line>
      <line no="19">
        <token name="T_WHITESPACE">    </token>
        <token name="T_CLOSE_CURLY">}</token>
      </line>
      <line no="20">
        <token name="T_CLOSE_CURLY">}</token>
      </line>
      <line no="21"/>
    </source>
  </file>
</phpunit>

Look at this piece:

<class name="Github1735\SourceClass" start="5" executable="4" executed="4" crap="1">
      <namespace name="Github1735"/>
      <method name="hello" signature="hello(): array" start="7" end="19" crap="1" executable="4" executed="4" coverage="100"/>
</class>

Only 1 method (hello) is marked as covered, while we have 2 methods (hello, foo) inside the class SourceClass:

hello is a method of SourceClass
foo is a method of class@anonymous

class SourceClass
{
    public function hello(): array
    {
        $var = [
            'class' => new class() {
                public function foo(): bool
                {
                    return true;
                }
            },
        ];

        return $var;
    }
}

What it breaks is that Infection incorrectly determines that the test that covers hello method also covers foo method. While it is the case in the current state of this PR, I can easily break this assumption by this change in the test:

    public function test_hello()
    {
        $sourceClass = new SourceClass();
-       self::assertTrue($sourceClass->hello()['class']->foo());
+       self::assertIsArray($sourceClass->hello());
    }

In this case, test_hello test case does cover SourceClass::hello() method, but does not cover class@anonymous::foo() method.

In practice, this leads to a bug with --only-covered option.

With the change above to the test, if I run

cd tests/e2e/Github1735/ && php ../../../bin/infection --only-covered

then I still get 2 mutations, one of them is escaped. However, class@anonymous::foo() is not covered and should not be mutated.

What to do with this? I don't know, because PHPUnit doesn't provide a coverage data Infection needs to build TestLocations for foo() method.

I think we should leave it as is for now, as I don't see a proper solution but at the same time mutating methods of anonymous classes located in other methods of real classes still sounds good and helpful.

Slamdunk · 2022-10-19T05:49:45Z

src/TestFramework/Coverage/LineRangeCalculator.php


-        return new NodeLineRangeData($node->getStartLine(), $endLine);
+        return new NodeLineRangeData($outerMostArrayNode->getStartLine(), $outerMostArrayNode->getEndLine());


When I first started a fix a week ago, I came to a conclusion similar to this, but all other tests were failing.
Glad to see I was in the right path. Still, the Generator thing makes debugging so painful 😩

Putting iterator_to_array everywhere has been tedious, I'd like to have a magic php settings to turn every Generator into an array immediately for debug purposes.

Still, the Generator thing makes debugging to painful weary

Same feelings here. I had to debug them a while ago and they just blew my mind, it's very hard. Though I didn't do any investigation on how to improve this process.

Same here...

It isn't that generators are at the fault but the structure we have here isn't friendly to generator-style programming. When you call one thing that calls another, and yet another, creating an implicit generator chain, it gets really convoluted. Much better to make explicit generators and chain them like we do in MutationTestingRunner.

Slamdunk · 2022-10-19T05:50:00Z

Thank you for your help :)

Prove infection#1735

88f7ae0

sanmai mentioned this pull request Oct 18, 2022

[TestLocator::getTestsForFunctionSignature] InvalidArgumentException: Expected an array to contain 1 elements. Got: 8. #1735

Closed

sanmai added 2 commits October 18, 2022 20:16

Add a test confirming the mutator isn't at the fault

65e7578

Typo

6c04aa4

sanmai requested a review from maks-rafalko October 18, 2022 11:47

maks-rafalko marked this pull request as ready for review October 18, 2022 20:12

Replace the old phpunit.xml config style with new, actual one

ca696ea

maks-rafalko approved these changes Oct 18, 2022

View reviewed changes

maks-rafalko added the Bugfix label Oct 18, 2022

maks-rafalko changed the title ~~Prove #1735~~ Fix the issue with anonymous classes inside arrays Oct 18, 2022

sanmai approved these changes Oct 19, 2022

View reviewed changes

Slamdunk commented Oct 19, 2022

View reviewed changes

maks-rafalko merged commit df3254c into infection:master Oct 19, 2022

Slamdunk deleted the github_1735 branch October 19, 2022 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the issue with anonymous classes inside arrays #1745

Fix the issue with anonymous classes inside arrays #1745

Slamdunk commented Oct 18, 2022 •

edited by maks-rafalko

Slamdunk commented Oct 18, 2022

sanmai commented Oct 18, 2022

sanmai commented Oct 18, 2022

sanmai commented Oct 18, 2022

sanmai commented Oct 18, 2022

maks-rafalko commented Oct 18, 2022 •

edited

Slamdunk Oct 19, 2022 •

edited

maks-rafalko Oct 19, 2022

theofidry Oct 19, 2022

sanmai Oct 20, 2022 •

edited

Slamdunk commented Oct 19, 2022


		return new NodeLineRangeData($node->getStartLine(), $endLine);
		return new NodeLineRangeData($outerMostArrayNode->getStartLine(), $outerMostArrayNode->getEndLine());

Fix the issue with anonymous classes inside arrays #1745

Fix the issue with anonymous classes inside arrays #1745

Conversation

Slamdunk commented Oct 18, 2022 • edited by maks-rafalko

Slamdunk commented Oct 18, 2022

sanmai commented Oct 18, 2022

sanmai commented Oct 18, 2022

sanmai commented Oct 18, 2022

sanmai commented Oct 18, 2022

maks-rafalko commented Oct 18, 2022 • edited

Slamdunk Oct 19, 2022 • edited

Choose a reason for hiding this comment

maks-rafalko Oct 19, 2022

Choose a reason for hiding this comment

theofidry Oct 19, 2022

Choose a reason for hiding this comment

sanmai Oct 20, 2022 • edited

Choose a reason for hiding this comment

Slamdunk commented Oct 19, 2022

Slamdunk commented Oct 18, 2022 •

edited by maks-rafalko

maks-rafalko commented Oct 18, 2022 •

edited

Slamdunk Oct 19, 2022 •

edited

sanmai Oct 20, 2022 •

edited