Mark all methods that never return as such

[greg0ire]

This should allow static analyzers to understand better why methods
calling these methods also never return.

[codecov]

Codecov Report

Merging #4187 into 8.5 will increase coverage by 1.33%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                8.5    #4187      +/-   ##
============================================
+ Coverage     84.23%   85.56%   +1.33%     
- Complexity     3934     6555    +2621     
============================================
  Files           154      222      +68     
  Lines          9779    15984    +6205     
============================================
+ Hits           8237    13677    +5440     
- Misses         1542     2307     +765     

Impacted Files	Coverage Δ	Complexity Δ
src/Framework/Assert.php	92.72% <ø> (+2.96%)	453.00 <0.00> (+190.00)
src/Framework/Constraint/Constraint.php	100.00% <ø> (ø)	25.00 <0.00> (+13.00)
src/Framework/Constraint/JsonMatches.php	100.00% <ø> (ø)	9.00 <0.00> (ø)
src/Framework/WarningTestCase.php	100.00% <ø> (ø)	4.00 <0.00> (ø)
src/Framework/TestSuite.php	88.94% <100.00%> (+2.38%)	225.00 <1.00> (+110.00)
src/Util/TestDox/TextResultPrinter.php	81.25% <0.00%> (-7.64%)	6.00% <0.00%> (+2.00%)	⬇️
src/Util/Blacklist.php	75.55% <0.00%> (-7.06%)	25.00% <0.00%> (+13.00%)	⬇️
src/Util/TestDox/HtmlResultPrinter.php	96.96% <0.00%> (-3.04%)	13.00% <0.00%> (+6.00%)	⬇️
src/Framework/Constraint/Count.php	97.01% <0.00%> (-2.99%)	23.00% <0.00%> (+5.00%)	⬇️
src/Runner/StandardTestSuiteLoader.php	88.13% <0.00%> (-2.98%)	42.00% <0.00%> (+16.00%)	⬇️
... and 120 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8474e22...21fbbc7. Read the comment docs.

[sebastianbergmann]

Does @psalm-return never-return really provide a benefit here as all these methods are declared void?

[greg0ire]

I think so. Before the fix, after the fix. But it's better if @muglug reviews this, that way we'll be sure.

I needed to add that annotation on the child class because it overrides another one that is not void

    /**
     * {@inheritdoc}
     */
    protected function getQuotedAlterTableChangeColumnLengthSQL() : array
    {
        $this->markTestIncomplete('Not implemented yet');
    }

and got the following error:

ERROR: InvalidReturnType - tests/Doctrine/Tests/DBAL/Platforms/SQLAnywherePlatformTest.php:885:69 - No return statements were found for method Doctrine\Tests\DBAL\Platforms\SQLAnywherePlatformTest::getQuotedAlterTableChangeColumnLengthSQL but return type 'array<array-key, string>' was expected (see https://psalm.dev/011)
    protected function getQuotedAlterTableChangeColumnLengthSQL() : array

[sebastianbergmann]

Can you send this against 8.5, please? Thanks!

[greg0ire]

Done, sorry, that's what I intended to do in the first place 😅 Apparently it tripped up the roave BC-check, but that job probably does not care about annotations.

[muglug]

Looks good!

[sebastianbergmann]

@muglug Thanks for looking into this.

@greg0ire I'll merge this. Thanks!

@muglug Can you explain why @psalm-return never-return is necessary? Psalm should see that the code does not have a return statement. I feel like I am missing something here.

[greg0ire]

Apparently, in the "Other" section of this page, it says that void and no-return are separate things:

void - can be used in a return type when a function does not return a value.`

vs

no-return is the 'return type' for a function that can never actually return, such as die(), exit(), or a function that always throws an exception. It may also be written as never-return or never-returns, and is also known as the bottom type.

And I think I understood why that is.

In my case we have

TestCase::markTestIncomplete(): void;

called by getQuotedAlterTableChangeColumnLengthSQL() : array

with just that and

{
    $this->markTestIncomplete('Not implemented yet');
}

a static analyzer will think that getQuotedAlterTableChangeColumnLengthSQL is supposed to return an array, because void returns nothing, but does not stop the execution flow, so you will get an error asking why there is no return [] in that method.