Skip to content

Add ReturnRemoval mutator#2296

Merged
sanmai merged 73 commits into
infection:masterfrom
sanmai:feat/ReturnRemoval
Jul 11, 2025
Merged

Add ReturnRemoval mutator#2296
sanmai merged 73 commits into
infection:masterfrom
sanmai:feat/ReturnRemoval

Conversation

@sanmai

@sanmai sanmai commented Jul 5, 2025

Copy link
Copy Markdown
Member

This PR:

NextConnectingVisitor

Added a new AST visitor that connects sequential statement nodes by adding a "next" attribute. This visitor:

  • Tracks execution order between statements only (ignores expressions)
  • Skips comment nodes (Nop)
  • Resets the connection chain at function boundaries
  • Enables the ReturnRemoval mutator to detect if there are statements after a return

ReturnRemoval Mutator

The mutator now intelligently removes return statements without causing language errors:

  • Removes any return in functions without return types (void or no type declaration)
  • Removes returns that have subsequent statements (detected via NextConnectingVisitor)
  • Always preserves the last return in functions with return types, preventing type errors
  • Does not mutate return; or return null; at the end of functions without return types, as these are semantically equivalent to implicit null returns

So we avoid both un-killable mutations and fatal errors.

Benefits

This implementation successfully avoids language errors while still exposing untested code paths. By preserving the last return statement in typed functions, we ensure the code remains syntactically valid while still being able to detect:

  • Unreachable code after return statements
  • Early returns that may not be properly tested
  • Redundant return statements in functions

The approach provides maximum mutation coverage without breaking PHP's type system. Just what we wanted 🙂


Per-mutator report:

Mutator Mutations Killed by Test Framework Test Timings min/avg/max Killed by Static Analysis Static Analysis Timings min/avg/max Escaped Errors Syntax Errors Timed Out (Limit: 25 secs) Skipped Ignored MSI (%s) Covered MSI (%s)
ReturnRemoval 569 407 0.30 / 0.49 / 5.57 secs 0 0.00 / 0.00 / 0.00 secs 70 3 0 1 0 0 72.23 85.45
Comment thread tests/phpunit/Mutator/Removal/ReturnRemovalTest.php Outdated
Comment thread tests/phpunit/Mutator/Removal/ReturnRemovalTest.php Outdated
Comment thread tests/phpunit/Mutator/Removal/ReturnRemovalTest.php Outdated
@staabm

staabm commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

looking at ReturnLastTaggingVisitor, I think we can do it simpler and without a nested NodeTraverser by taking inspiration from PHPParsers builtin NodeConnectingVisitor.

we need to find/tag Return_ nodes, which do not have a 'next' node, right?

@sanmai

sanmai commented Jul 8, 2025

Copy link
Copy Markdown
Member Author

Sadly NodeConnectingVisitor only sets the next attribute on nodes that share the same parent. I.e. these do not:

if ($foo) {
    return $bar;
}
return $baz;

But it is a good starting point for our own connecting visitor.

@staabm

staabm commented Jul 8, 2025

Copy link
Copy Markdown
Contributor

Yes, posted it for inspiration to implement a smaller/specific one for our use-case

Comment thread src/Mutator/Removal/ReturnRemoval.php Outdated
Comment on lines +70 to +71
$this->previous?->setAttribute(self::NEXT_ATTRIBUTE, $node);
$this->previous = $node;

@staabm staabm Jul 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should set references between AstNodes very carefully.
at best this visitor would be very specific for the use-case and set a more specific attribute, like "isLastReturn" and if-possible only on Return_ nodes.

references between ast nodes can be very costly, as they hinder the garbage collector.
see e.g. https://phpstan.org/blog/preprocessing-ast-for-custom-rules which describes the cost of a parent/next/previous attribute the NodeConnectingVisitor incurs.

@sanmai sanmai Jul 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If PHPStan has to keep the whole codebase in memory to do its thing, then it makes total sense for it. For us, it is a single file at a time. So it should not be a problem, and we already use the parent connecting visitor.

@sanmai sanmai requested a review from maks-rafalko July 9, 2025 13:37

@maks-rafalko maks-rafalko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so I've went through my todo list and the only one item left is to analyze overlapping, which I will postpone as I don't have time yet for this time-consuming activity.

Also, I've read seggested topics by Markus regarding weak references, and they look very interesting. I will create an issue to have a separate discussion on this topic so we can check if and how weak references help us not only here for the new visitor but for parent connector as well.

I've also analyzed ~20 escaped mutants generated for Infection and they do make sense.

Let's try!

@maks-rafalko maks-rafalko changed the title Add ReturnRemoval mutator Add ReturnRemoval mutator Jul 10, 2025
@sanmai sanmai merged commit e90a4c5 into infection:master Jul 11, 2025
47 checks passed
@sanmai sanmai deleted the feat/ReturnRemoval branch January 11, 2026 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4 participants