-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add FixerBlame class and one test #7865
base: master
Are you sure you want to change the base?
Changes from 1 commit
54f56a1
9410b9f
9319a3e
351f48c
a2bbbc2
3c91819
af14b86
2769ad3
622e675
cc98df0
d20f3aa
aa27b5d
be5a196
0e60e18
847a7b1
d873e38
396b5de
24c15b9
7e7e596
c8b83a9
60c72b1
dc2b8c1
7c8ecbe
c889d18
a4fb45a
63e4dfc
a566d41
1f80310
f25abdc
0cf69be
7986cb8
cefdfae
d54e66f
ddfb858
9ecdad4
0ea17b6
f680ea2
87172ca
295c088
4a1aa7e
e63922b
424711c
88559e7
c37cff3
43e39e1
dff0884
cac24b9
4e90bc3
3dfe8b3
0764ef8
0403f9b
b2ebcce
57bcf57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
/* | ||
* This file is part of PHP CS Fixer. | ||
* | ||
* (c) Fabien Potencier <[email protected]> | ||
* Dariusz Rumiński <[email protected]> | ||
* | ||
* This source file is subject to the MIT license that is bundled | ||
* with this source code in the file LICENSE. | ||
*/ | ||
|
||
namespace PhpCsFixer\FixerBlame; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please describe approach you want to take in RFC style before implementing it. it will help reviewers to recognise what and how you want to do, and help you to align on direction before spending time on implementing it. right now, I'm happy with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the problems with the implementation at the moment is that, because of the way The theory works anyway, because it can accurately track multi-step transformations, but it's also obvious that in its current form it doesn't just do what it's actually supposed to do. That's why I can't describe in any style what happens, because it doesn't happen the way I want to see it happen and 1000% I'll have to rewrite the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I failed to understand the comment. Please describe the problem you want to solve and describe the concept of solution, before you go deep into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the problem? What is a possible solution to this problem? What is the problem with which approach? a) while I'm all for something either working well or not existing at all, this is obviously not the answer What tools do we currently have to monitor changes?
The idea works as long as you don't put two violations in a line: if ($variable !== false && $variable2 !== false) That's two yoda style violations, but due to the limitations of diff I see this as one. So far I am not happy with the solution for this. As long as I am not satisfied with what my solution can do, there is no point in talking about how to integrate it nicely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, I'm now wondering if just changing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imagine the case: <?php
class A {
public method zzz() {} ## line 2
public method aaa() {
var_dump("a" == "b"); ## line 5
}
}
which lines should be reported as modified?
I suggest a new command/BC break that is not applying one fixer on top of another (like we need for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I achieved quite nice results on complex code with the solution I made ( https://github.com/connorhu/PHP-CS-Fixer/blob/57bcf57cde0cc5844070eb62ed38a5320085eb35/tests/Fixtures/Integration/misc/blame_test.test#L46-L56 ). The function will only work correctly if it finds the original position where the violation occurs, and now the code can do it (mostly). I'm between two extremes at the moment: in the In the beginning I did as you wrote. I made a "complex" code, set several fixers and saved each step in a separate file, then diffed them. I wrote down what I wanted to see (what the logical end result would be) and iterated until I got what the logical end result is.
I'll sleep on it and think about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I know, so I don't understand why the code you wrote is an issue. Fixing only fixes, checking only checks. Your example code results in two violations:
In both cases, char 0 is still an incorrect result, but this is a side effect of the diff. For me it's already an issue: var_dump("a" == "b", "c" == "b"); and var_dump("a" == "b");
var_dump("c" == "b"); My solution takes these as one violation. Which is not ok. I can see that you don't want to deal with such things, which you have the possibility to do anyway, by removing the output formats (or outsourcing them from the core code to a plugin system). |
||
|
||
final class CodeChange | ||
{ | ||
private string $content; | ||
private int $change; | ||
private ?int $newLineNumber; | ||
private ?int $oldLineNumber; | ||
|
||
public function __construct(string $content, int $change, ?int $newLineNumber = null, ?int $oldLineNumber = null) | ||
{ | ||
$this->content = $content; | ||
$this->change = $change; | ||
$this->newLineNumber = $newLineNumber; | ||
$this->oldLineNumber = $oldLineNumber; | ||
} | ||
|
||
public function getContent(): string | ||
{ | ||
return $this->content; | ||
} | ||
|
||
public function getChange(): int | ||
{ | ||
return $this->change; | ||
} | ||
|
||
public function getNewLineNumber(): ?int | ||
{ | ||
return $this->newLineNumber; | ||
} | ||
|
||
public function getOldLineNumber(): ?int | ||
{ | ||
return $this->oldLineNumber; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,297 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
/* | ||
* This file is part of PHP CS Fixer. | ||
* | ||
* (c) Fabien Potencier <[email protected]> | ||
* Dariusz Rumiński <[email protected]> | ||
* | ||
* This source file is subject to the MIT license that is bundled | ||
* with this source code in the file LICENSE. | ||
*/ | ||
|
||
namespace PhpCsFixer\FixerBlame; | ||
|
||
use PhpCsFixer\Fixer\FixerInterface; | ||
use PhpCsFixer\Tokenizer\Tokens; | ||
use SebastianBergmann\Diff\Differ; | ||
use SebastianBergmann\Diff\Output\StrictUnifiedDiffOutputBuilder; | ||
|
||
final class FixerBlame | ||
{ | ||
private Differ $differ; | ||
|
||
/** | ||
* @var array<array{ | ||
* fixerName: string, | ||
* source: string | ||
* }> | ||
*/ | ||
private array $changeStack = []; | ||
|
||
public function __construct() | ||
{ | ||
$this->differ = new Differ(new StrictUnifiedDiffOutputBuilder([ | ||
'collapseRanges' => true, | ||
'commonLineThreshold' => 1, | ||
'contextLines' => 1, | ||
'fromFile' => 'Original', | ||
'toFile' => 'New', | ||
])); | ||
} | ||
|
||
/** | ||
* @param string|Tokens $code | ||
*/ | ||
public function originalCode($code): void | ||
{ | ||
if ($code instanceof Tokens) { | ||
$code = $code->generateCode(); | ||
} | ||
|
||
$this->changeStack = [ | ||
[ | ||
'fixerName' => '__initial__', | ||
'source' => $code, | ||
], | ||
]; | ||
} | ||
|
||
public function snapshotCode(string $fixerName, string $source): void | ||
{ | ||
$this->changeStack[] = [ | ||
'fixerName' => $fixerName, | ||
'source' => $source, | ||
]; | ||
} | ||
|
||
public function snapshotTokens(FixerInterface $fixer, Tokens $tokens): void | ||
{ | ||
$this->changeStack[] = [ | ||
'fixerName' => $fixer->getName(), | ||
'source' => $tokens->generateCode(), | ||
]; | ||
} | ||
|
||
/** | ||
* @return list<FixerChange> | ||
*/ | ||
public function calculateChanges(): array | ||
{ | ||
$changes = []; | ||
|
||
foreach ($this->changeStack as $changeIndex => $change) { | ||
if (0 === $changeIndex) { | ||
continue; | ||
} | ||
|
||
$oldChangeContent = $this->changeStack[$changeIndex - 1]['source']; | ||
$newChangeContent = $change['source']; | ||
|
||
$fixerName = $change['fixerName']; | ||
|
||
$diffResults = $this->diff($oldChangeContent, $newChangeContent); | ||
$patches = $this->findPatches($diffResults); | ||
|
||
foreach ($patches as $patchInfo) { | ||
$patchContent = $patchInfo->getPatchContent($diffResults); | ||
|
||
$numberOfChanges = \count($patchContent); | ||
|
||
// simple remove | ||
if (1 === $numberOfChanges && Differ::REMOVED === $patchContent[0]->getChange()) { | ||
$changes[] = [ | ||
'fixerName' => $fixerName, | ||
'start' => $patchContent[0]->getOldLineNumber(), | ||
'changedSum' => $patchInfo->getChangeSum(), | ||
'changedAt' => 0, | ||
]; | ||
|
||
continue; | ||
} | ||
|
||
// line changed | ||
if (2 === $numberOfChanges && Differ::REMOVED === $patchContent[0]->getChange() && Differ::ADDED === $patchContent[1]->getChange()) { | ||
$addedLine = $patchContent[1]->getContent(); | ||
$removedLine = $patchContent[0]->getContent(); | ||
|
||
$changedAt = null; | ||
|
||
for ($i = 0; $i < min(\strlen($addedLine), \strlen($removedLine)); ++$i) { | ||
if ($addedLine[$i] !== $removedLine[$i]) { | ||
$changedAt = $i + 1; | ||
|
||
break; | ||
} | ||
} | ||
|
||
$changes[] = [ | ||
'fixerName' => $fixerName, | ||
'start' => $patchContent[0]->getOldLineNumber(), | ||
'changedSum' => $patchInfo->getChangeSum(), | ||
'changedAt' => $changedAt ?? \strlen($removedLine) + 1, | ||
]; | ||
|
||
continue; | ||
} | ||
|
||
$onlyRemove = 0x1; | ||
$onlyAdd = 0x1; | ||
|
||
foreach ($patchContent as $patchRow) { | ||
if (Differ::ADDED === $patchRow->getChange()) { | ||
$onlyAdd &= 0x1; | ||
} else { | ||
$onlyAdd &= 0; | ||
} | ||
|
||
if (Differ::REMOVED === $patchRow->getChange()) { | ||
$onlyRemove &= 0x1; | ||
} else { | ||
$onlyRemove &= 0; | ||
} | ||
} | ||
|
||
if (1 === $onlyAdd xor 1 === $onlyRemove) { | ||
if (1 === $onlyAdd) { | ||
$lineNumber = $patchContent[0]->getNewLineNumber(); | ||
} else { | ||
$lineNumber = $patchContent[0]->getOldLineNumber(); | ||
} | ||
|
||
$changes[] = [ | ||
'fixerName' => $fixerName, | ||
'start' => $lineNumber, | ||
'changedSum' => $patchInfo->getChangeSum(), | ||
'changedAt' => 0, | ||
]; | ||
|
||
continue; | ||
} | ||
if (Differ::ADDED === $patchContent[0]->getChange()) { | ||
throw new \RuntimeException('added lines first?'); | ||
} | ||
|
||
$changes[] = [ | ||
'fixerName' => $fixerName, | ||
'start' => $patchContent[0]->getOldLineNumber(), | ||
'changedSum' => $patchInfo->getChangeSum(), | ||
'changedAt' => 0, | ||
]; | ||
} | ||
} | ||
|
||
$changeSet = []; | ||
foreach ($changes as $index => $change) { | ||
$lineChanges = 0; | ||
for ($i = $index - 1; $i >= 0; --$i) { | ||
if ($changes[$i]['start'] >= $change['start']) { | ||
continue; | ||
} | ||
|
||
$lineChanges -= $changes[$i]['changedSum']; | ||
} | ||
|
||
$changeSet[] = new FixerChange($change['fixerName'], $change['start'] + $lineChanges, $change['changedAt']); | ||
} | ||
|
||
return $changeSet; | ||
} | ||
|
||
/** | ||
* @return array<CodeChange> | ||
*/ | ||
private function diff(string $oldCode, string $newCode): array | ||
{ | ||
$diffResults = $this->differ->diffToArray($oldCode, $newCode); | ||
|
||
$linePointerInOldContent = 1; | ||
$linePointerInNewContent = 1; | ||
|
||
$buffer = []; | ||
foreach ($diffResults as $diffResult) { | ||
if (Differ::ADDED === $diffResult[1]) { | ||
$buffer[] = new CodeChange($diffResult[0], Differ::ADDED, $linePointerInNewContent++); | ||
|
||
continue; | ||
} | ||
|
||
if (Differ::REMOVED === $diffResult[1]) { | ||
$buffer[] = new CodeChange($diffResult[0], Differ::REMOVED, null, $linePointerInOldContent++); | ||
|
||
continue; | ||
} | ||
|
||
$buffer[] = new CodeChange($diffResult[0], Differ::OLD, $linePointerInNewContent++, $linePointerInOldContent++); | ||
} | ||
|
||
return $buffer; | ||
} | ||
|
||
/** | ||
* @param array<CodeChange> $diffs | ||
* | ||
* @return array<PatchInfo> | ||
*/ | ||
private function findPatches(array $diffs): array | ||
{ | ||
/** @var array<PatchInfo> $patches */ | ||
$patches = []; | ||
$patchInfo = null; | ||
$state = 'file_start'; | ||
|
||
foreach ($diffs as $key => $diffResult) { | ||
if ('file_start' === $state) { | ||
if (Differ::OLD === $diffResult->getChange()) { | ||
$state = 'between_patch'; | ||
|
||
continue; | ||
} | ||
|
||
if (Differ::ADDED === $diffResult->getChange() || Differ::REMOVED === $diffResult->getChange()) { | ||
$patchInfo = new PatchInfo(); | ||
$patchInfo->setStartKey($key); | ||
$patchInfo->countChange($diffResult->getChange()); | ||
|
||
$state = 'in_patch'; | ||
|
||
continue; | ||
} | ||
} | ||
|
||
if ('between_patch' === $state && (Differ::ADDED === $diffResult->getChange() || Differ::REMOVED === $diffResult->getChange())) { | ||
$patchInfo = new PatchInfo(); | ||
$patchInfo->setStartKey($key); | ||
$patchInfo->countChange($diffResult->getChange()); | ||
|
||
$state = 'in_patch'; | ||
|
||
continue; | ||
} | ||
|
||
if ('in_patch' === $state && Differ::OLD === $diffResult->getChange()) { | ||
$state = 'between_patch'; | ||
|
||
$patchInfo->setEndKey($key); | ||
$patches[] = $patchInfo; | ||
$patchInfo = null; | ||
|
||
continue; | ||
} | ||
|
||
if ('in_patch' === $state) { | ||
$patchInfo->countChange($diffResult->getChange()); | ||
} | ||
} | ||
|
||
if ('in_patch' === $state) { | ||
$patchInfo->setEndKey(\count($diffs) - 1); | ||
$patches[] = $patchInfo; | ||
$patchInfo = null; | ||
} | ||
|
||
return $patches; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please avoid having all commits of Wirone in your PR, as they are other PR and making diff for your PR crazy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's an outcome of my comment, but I only asked for taking the parallel runner into consideration (make sure it will work), not to rebase on my work 😅. It could work if we had branches in the same repo, and a proper target branch was selected, but not like this. Hard to tell what is the best approach here, maybe a draft with a branch based on master (like before), but waiting until my PR is merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to align it with PR #7777 so I rebaseled it to see how much conflict there was (fortunately I only had to resolve 2-3 things). @Wirone's work is the more polished, more important and is expected to be in the master sooner. As mentioned in the PR description, once the parallel feature is merged this can be rebased back to the master and then the commits for the parallel run will be removed. There is no point in starting the review until there are implementation (and performance) issues with the PR anyway.