Page MenuHomePhorge

Fix invalid constructor call for ArcanistHardpointFutureList
ClosedPublic

Authored by aklapper on Jul 1 2024, 11:35.

Details

Summary

ArcanistHardpointFutureList does not have a constructor and must be instantiated without any parameters.
As the code checks if ($result instanceof Future) and tries to pass $result as a parameter, the intention seems to be calling newFromFutures($result) on the new ArcanistHardpointFutureList instance.

Closes T15836

Test Plan

Read the code in ArcanistHardpointFutureList.

Diff Detail

Repository
rARC Arcanist
Branch
T15836
Lint
Lint Errors
SeverityLocationCodeMessage
Errorsrc/hardpoint/ArcanistHardpointTask.php:108XHP1PHP Syntax Error!
Unit
Test Failures
Build Status
Buildable 1394
Build 1394: arc lint + arc unit

Unit TestsFailed

TimeTest
0 msPhutilLibraryTestCase::testEverythingImplemented
EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage' #0 [internal function]: PhutilErrorHandler::handleError(2, '...', '...', 703) #1 /var/www/html/phorge/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
0 msPhutilLibraryTestCase::testLibraryMap
EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage' #0 [internal function]: PhutilErrorHandler::handleError(2, '...', '...', 703) #1 /var/www/html/phorge/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
0 msPhutilLibraryTestCase::testMethodVisibility
EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage' #0 [internal function]: PhutilErrorHandler::handleError(2, '...', '...', 703) #1 /var/www/html/phorge/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)

Event Timeline

aklapper requested review of this revision.Jul 1 2024, 11:35

Sigh, xdebug... and seems I need to drop the new from the constructor when the class is final.

or am I supposed to $result = id(new ArcanistHardpointFutureList())->newFromFutures($result);? Yeez

Test plan needed. But I don't know what an "hardpoint" is.


✅ Your static call is right since newFromFutures() is a static method.

https://we.phorge.it/source/arcanist/browse/master/src/hardpoint/ArcanistHardpointFutureList.php$9

🔶 But it's designed to accept an array of futures, not a single one. Maybe just wrap in array($result).

P.S. a final class just means that other classes cannot extend that.


Until now, probably a new object was created but without "futures". So any future was just discarded when entering there. Since PHP probably has (had?) a default constructor that just constructs, discarding arguments in an implicit way.

What will happen now: Future will be kept. But still, no test plan.

But it's designed to accept an array of futures, not a single one. Maybe just wrap in array($result).

Right, thanks! Done now.

I don't know what an "hardpoint" is.

Me neither. :) Thus no test plan for dysfunctional code in the master branch.

I don't know what an "hardpoint" is.

Evan explained it to me once and did reference https://en.wikipedia.org/wiki/Hardpoint. The only relevant thing I could locate is https://secure.phabricator.com/D21716. As I understood it, a hardpoint is a "template" for something that needs to be populated. I believe this pattern exists to enable efficient queries, e.g. build a list of "hardpoints" that describe what's needed, then pass to some sort of locator/executor/query-system to then assemble efficient queries, which could be queries to a database or running git/hg commands to get data.

So this patch which changes (and hopefully also fixes) definitely broken code will be blocked for good due to lack of a test plan (which I won't be able to figure out)?
Or does anyone can come up with any other option?

In general the reporter should really find a small way to reproduce their problem, also because among all the things written here (https://we.phorge.it/book/contrib/article/bug_reports/), everybody may agree with the first point:

We can NOT help you with issues we can not reproduce

https://we.phorge.it/book/contrib/article/reproduction_steps/

As much as I have proactively tried, I'm sorry I have failed to reproduce.

Anyway... working again in a clean empty room without ways to reproduce...

Basically, premising that the constructor of newFromFutures() seems legit, it's not clear to me why its return result should make sense and be compatible with the expected one. If somebody can clarify this, maybe it's at least something more.

Basically, premising that the constructor of newFromFutures() seems legit, it's not clear to me why its return result should make sense and be compatible with the expected one. If somebody can clarify this, maybe it's at least something more.

I'm afraid I cannot offer more clarification than the second sentence in the commit message, as in "what else could this code be supposed to do". :-/

Uh, looking at line 112, the return status seems correct and expected. I had not seen that first. Thanks for fixing the constructor.

This revision is now accepted and ready to land.Sep 17 2024, 15:06