From 0722401aaf4b41928438fb7a921d5c85ca6221f9 Mon Sep 17 00:00:00 2001 From: Barney Laurance Date: Sat, 26 Jun 2021 21:59:40 +0100 Subject: [PATCH 1/4] Trim code snippets on both sides when comparing current issues against baseline Should fix issue #5979 We could also trim code issues when writing to baseline, but I think that's a minor BC break, so probably shouldn't happen until Psalm 9. --- src/Psalm/ErrorBaseline.php | 7 +++++- src/Psalm/Internal/Analyzer/IssueData.php | 4 +++- tests/ErrorBaselineTest.php | 28 +++++++++++++++++++++++ tests/IssueBufferTest.php | 22 +++++++++++++++++- 4 files changed, 58 insertions(+), 3 deletions(-) diff --git a/src/Psalm/ErrorBaseline.php b/src/Psalm/ErrorBaseline.php index 9ab0cbfc7d6..5d589d9d062 100644 --- a/src/Psalm/ErrorBaseline.php +++ b/src/Psalm/ErrorBaseline.php @@ -19,6 +19,7 @@ use function preg_replace_callback; use function str_replace; use function strpos; +use function trim; use function usort; use const LIBXML_NOBLANKS; @@ -115,7 +116,7 @@ public static function read(FileProvider $fileProvider, string $baselineFile): a $codeSamples = $issue->getElementsByTagName('code'); foreach ($codeSamples as $codeSample) { - $files[$fileName][$issueType]['s'][] = $codeSample->textContent; + $files[$fileName][$issueType]['s'][] = trim($codeSample->textContent); } } } @@ -273,6 +274,10 @@ function (string $extension) : string { foreach ($existingIssueType['s'] as $selection) { $codeNode = $baselineDoc->createElement('code'); + /** @todo in major version release (e.g. 5.0.0) replace $selection with trim($selection) + * This will be a minor BC break as baselines generated will then not be compatible with Psalm + * versions from before PR https://github.com/vimeo/psalm/pull/6000 + */ $codeNode->textContent = $selection; $issueNode->appendChild($codeNode); } diff --git a/src/Psalm/Internal/Analyzer/IssueData.php b/src/Psalm/Internal/Analyzer/IssueData.php index 363e8578e7f..4dddfbb7426 100644 --- a/src/Psalm/Internal/Analyzer/IssueData.php +++ b/src/Psalm/Internal/Analyzer/IssueData.php @@ -2,6 +2,8 @@ namespace Psalm\Internal\Analyzer; +use function trim; + class IssueData { /** @@ -154,7 +156,7 @@ public function __construct( $this->file_name = $file_name; $this->file_path = $file_path; $this->snippet = $snippet; - $this->selected_text = $selected_text; + $this->selected_text = trim($selected_text); $this->from = $from; $this->to = $to; $this->snippet_from = $snippet_from; diff --git a/tests/ErrorBaselineTest.php b/tests/ErrorBaselineTest.php index 92e846f2a1f..56be18758f3 100644 --- a/tests/ErrorBaselineTest.php +++ b/tests/ErrorBaselineTest.php @@ -67,6 +67,34 @@ public function testLoadShouldParseXmlBaselineToPhpArray() ); } + public function testLoadShouldIgnoreLineEndingsInIssueSnippet(): void + { + $baselineFilePath = 'baseline.xml'; + + $this->fileProvider->fileExists($baselineFilePath)->willReturn(true); + $this->fileProvider->getContents($baselineFilePath)->willReturn( + " + + + + foo\r + + + " + ); + + $expectedParsedBaseline = [ + 'sample/sample-file.php' => [ + 'MixedAssignment' => ['o' => 1, 's' => ['foo']], + ], + ]; + + $this->assertSame( + $expectedParsedBaseline, + ErrorBaseline::read($this->fileProvider->reveal(), $baselineFilePath) + ); + } + /** * @return void */ diff --git a/tests/IssueBufferTest.php b/tests/IssueBufferTest.php index d350906e295..6cef39c9d9e 100644 --- a/tests/IssueBufferTest.php +++ b/tests/IssueBufferTest.php @@ -54,11 +54,31 @@ public function testFinishDoesNotCorruptInternalState(): void 0, 0 ) - ] + ], + '/path/three.php' => [ + new \Psalm\Internal\Analyzer\IssueData( + "error", + 0, + 0, + "MissingPropertyType", + 'Message', + "three.php", + "/path/three.php", + "snippet-3-has-carriage-return" . "\r", + "snippet-3-has-carriage-return" . "\r", + 0, + 0, + 0, + 0, + 0, + 0 + ) + ], ]); $baseline = [ 'one.php' => ['MissingPropertyType' => ['o' => 1, 's' => ['snippet-1']] ], 'two.php' => ['MissingPropertyType' => ['o' => 1, 's' => ['snippet-2']] ], + 'three.php' => ['MissingPropertyType' => ['o' => 1, 's' => ['snippet-3-has-carriage-return']] ], ]; $analyzer = $this->createMock(Analyzer::class); From eff9acbd9a697c0fc723fbf5bb83436b0ad2dde2 Mon Sep 17 00:00:00 2001 From: Barney Laurance Date: Sun, 27 Jun 2021 00:08:03 +0100 Subject: [PATCH 2/4] Remove unecassary concatentation of string literals --- tests/IssueBufferTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/IssueBufferTest.php b/tests/IssueBufferTest.php index 6cef39c9d9e..d12ef833484 100644 --- a/tests/IssueBufferTest.php +++ b/tests/IssueBufferTest.php @@ -64,8 +64,8 @@ public function testFinishDoesNotCorruptInternalState(): void 'Message', "three.php", "/path/three.php", - "snippet-3-has-carriage-return" . "\r", - "snippet-3-has-carriage-return" . "\r", + "snippet-3-has-carriage-return\r", + "snippet-3-has-carriage-return\r", 0, 0, 0, From 097ccf16b47e54eac343ab8a6c75fec1252b1659 Mon Sep 17 00:00:00 2001 From: Barney Laurance Date: Sun, 27 Jun 2021 10:18:42 +0100 Subject: [PATCH 3/4] Mention Psalm 5 in comment Co-authored-by: Bruce Weirdan --- src/Psalm/ErrorBaseline.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Psalm/ErrorBaseline.php b/src/Psalm/ErrorBaseline.php index 5d589d9d062..e56a65527e6 100644 --- a/src/Psalm/ErrorBaseline.php +++ b/src/Psalm/ErrorBaseline.php @@ -274,7 +274,7 @@ function (string $extension) : string { foreach ($existingIssueType['s'] as $selection) { $codeNode = $baselineDoc->createElement('code'); - /** @todo in major version release (e.g. 5.0.0) replace $selection with trim($selection) + /** @todo in major version release (e.g. Psalm 5) replace $selection with trim($selection) * This will be a minor BC break as baselines generated will then not be compatible with Psalm * versions from before PR https://github.com/vimeo/psalm/pull/6000 */ From 140cf01a9180c8d22db32c5ac8057914e69ac14a Mon Sep 17 00:00:00 2001 From: Barney Laurance Date: Sun, 27 Jun 2021 10:33:09 +0100 Subject: [PATCH 4/4] Trim issue snippet at time of comparison with baseline, not in IssueData constructor --- src/Psalm/Internal/Analyzer/IssueData.php | 4 +--- src/Psalm/IssueBuffer.php | 3 ++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/IssueData.php b/src/Psalm/Internal/Analyzer/IssueData.php index 4dddfbb7426..363e8578e7f 100644 --- a/src/Psalm/Internal/Analyzer/IssueData.php +++ b/src/Psalm/Internal/Analyzer/IssueData.php @@ -2,8 +2,6 @@ namespace Psalm\Internal\Analyzer; -use function trim; - class IssueData { /** @@ -156,7 +154,7 @@ public function __construct( $this->file_name = $file_name; $this->file_path = $file_path; $this->snippet = $snippet; - $this->selected_text = trim($selected_text); + $this->selected_text = $selected_text; $this->from = $from; $this->to = $to; $this->snippet_from = $snippet_from; diff --git a/src/Psalm/IssueBuffer.php b/src/Psalm/IssueBuffer.php index bc5492b2780..f9954a9cf6e 100644 --- a/src/Psalm/IssueBuffer.php +++ b/src/Psalm/IssueBuffer.php @@ -48,6 +48,7 @@ use function sprintf; use function str_repeat; use function str_replace; +use function trim; use function usort; use const DEBUG_BACKTRACE_IGNORE_ARGS; @@ -518,7 +519,7 @@ function (IssueData $d1, IssueData $d2) : int { if (isset($issue_baseline[$file][$type]) && $issue_baseline[$file][$type]['o'] > 0) { if ($issue_baseline[$file][$type]['o'] === count($issue_baseline[$file][$type]['s'])) { $position = array_search( - $issue_data->selected_text, + trim($issue_data->selected_text), $issue_baseline[$file][$type]['s'], true );