Page MenuHomePhorge

Add return statements for PhutilChannelChannel::readBytes()/writeBytes()
ClosedPublic

Authored by aklapper on Jul 28 2024, 17:24.

Details

Summary

PHPStan complains that Method PhutilChannelChannel::readBytes() should return string but return statement is missing. and Method PhutilChannelChannel::writeBytes() should return int but return statement is missing.

All other subclasses of PhutilChannel implementing these two methods either provide a return value or directly throw an exception.
PhutilChannelChannel throws an exception for both methods but hadn't explicitly declared that as a return statement.

Test Plan

Run static code analysis; read/grep the code.

Diff Detail

Repository
rARC Arcanist
Branch
phutilChannelChannelBytesReturn
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1525
Build 1525: arc lint + arc unit

Event Timeline

Uhm. Before the change, the method returns void. After the change, the method returns void. So this proposed change would be "to make PHPStan happy" but I expect it will highlight in future versions of PHPStan as well.

Probably authors of PHPStan would like to see this line. Or probably not, since they have 1 thousand issues opened 🤔 Maybe related:

https://github.com/phpstan/phpstan/issues?q=is%3Aissue+is%3Aopen+%22should+return+string+but+return+statement+is+missing%22

Before and after it throws an exception; after it involves the return command to do so.

I expect it will highlight in future versions of PHPStan as well.

Not sure I get you. What is "it" after this change? (If you imply declining this patch and consider this a bug in PHPStan I'm fine with that.)

Little premise, in this method the return statement is dead, unreachable code:

function asd() {
    return throw new Exception("lel");
}

Same, dead unreachable return:

// PHPStan does not see any return. So, assume "void" return type.
function asd() {
    throw new Exception("lel");

    return 420; // Something secret that will never be returned.
}

Before the change, without any return statement, PHPStan was able to detect at 100% that it was impossible to respect the int return type. So it's error.

After the change, PHPStan is "just" tricked, can't detect that the method still does not return int at runtime (also because the parent still has return void), so still cannot respect the parent int return type. But I expect PHPStan can be improved in the future to detect this "obvious situation from the perspective of a static analyzer".

Proposed resolution, just add a return statement, but also let's return an int (not void). Example:

protected function readBytes($length) {
  $this->throwOnRawByteOperations();
  return -1; // Never returned. Makes static analyzers happy.
}

lol whatever, but please consider this instead to make happy also future/better versions of PHPStan

src/channel/PhutilChannelChannel.php
54–57
60–61
This revision is now accepted and ready to land.Aug 2 2024, 14:23

Do as Valerio said. Tested before and after, two errors less in PHPStan output.