Format-preserving pretty-printing for Mutants #2448
Conversation
9814cc8 to
40b228a
Compare
# Conflicts: # tests/phpunit/Mutation/MutationTest.php
that's why I've created #2450 - to be able to test exactly that formatting that happens for Mutants. Currently, different prinetrs are used so it's impossible. But I will do it in separate PR what will target this one, as it's quite big |
# Conflicts: # src/Mutant/MutantCodeFactory.php
I've spent much more time on it and updated the tests to use format-preserving pretty printer. Many of the tests have been updated (as they obviously failed), unfortunately it's impossible to move it to a separate PR, so please take a look. I've found already ~4 potential bugs in |
| private readonly MutatedNode $mutatedNode, | ||
| private readonly int $mutationByMutatorIndex, | ||
| private readonly array $tests, | ||
| private readonly array $originalFileTokens, |
There was a problem hiding this comment.
since those are readonly maybe they could be public?
There was a problem hiding this comment.
I decided not to do it, because it won't be consistent. We can translate private readonly properties to public as a separate PR using Rector I guess on the project level
| * | ||
| * @return Token[] | ||
| */ | ||
| public function getTokens(): array |
There was a problem hiding this comment.
I'm not a fan of this API. It fits in the current usage but it lacks any cohesiveness. Can't we change FileParser to return a tuple instead?
| public function getTokens(): array | |
| /** | |
| * @throws UnparsableFile | |
| * | |
| * @return array{Stmt[], Token[]} | |
| */ | |
| public function parse(SplFileInfo $fileInfo): array | |
| { | |
| try { | |
| $statements = $this->parser->parse($fileInfo->getContents()) ?? []; | |
| $tokens = $this->parser->getTokens(); | |
| return [$statements, $tokens]; | |
| } catch (Throwable $throwable) { | |
| $filePath = $fileInfo->getRealPath() === false | |
| ? $fileInfo->getPathname() | |
| : $fileInfo->getRealPath() | |
| ; | |
| throw UnparsableFile::fromInvalidFile($filePath, $throwable); | |
| } | |
| } |
I appreciate it has a greater impact though
There was a problem hiding this comment.
Decided not to do it because ->parse($file) and ->getTokens() this is exactly the API that php-parser exposes. Yes, it's arguable, but we only have a wrapper.
Such change will affect many files and IMO if we really want it (I have doubts) it shouldn't be done here.
There was a problem hiding this comment.
It's already questionable in PHP-Parser and the advantage of having a wrapper is to be able to correct such API issues.
Such change will affect many files
Does it? From what I saw it only affects 3 files which you are already touching. That said this could be done in a separate PR.
In the end I don't think it's a fundamental change so I won't insist more than that, but it would be nicer to not introduce loose APIs.
This is an extracted PR to fix review comment #2448 (comment)
Format-preserving pretty printing is hard :| After a week of evenings of debugging, here we go.
https://github.com/nikic/PHP-Parser/blob/master/doc/component/Pretty_printing.markdown#formatting-preserving-pretty-printing
Why is this needed?
php-parserInstead of 1000 words, examples:
Before:
After:
As you can see, previously Infection modified the source code formatting, then created a Mutant, and then compared modified original code with mutant.
Now, original code is compared with a mutant as is - we ONLY change the mutated node. No more reformatting.
This gives us much more readable and clear diffs. Long multiline arrays are no longer changed to lengthy one-liners.
During implementation, I found several bugs in
php-parser:These are not blockers, but interesting to see the answers from Nikita. I have a feeling that either these are bugs or we don't use nodes replacements correctly (probably we should do them in-place) - let's see.