Page MenuHomePhabricator

Document Status and StatusValue as generic (templated) classes
Closed, ResolvedPublic

Description

Status and StatusValue, if we ignore their capacity to store error messages, are basically generic containers. $status = Status::newGood( $foo ) creates a container wrapping $foo, and $status->getValue() returns the same $foo. This is unfortunately not known to IDEs and static analysis tools, which will have no type information for $foo, and so e.g. PhpStorm will not autocomplete method calls, while Phan will not warn about using non-existent methods.

This requires us to annotate types explicitly (e.g. here or here, just two random examples), and when we want to avoid that, it instead leads us to implement unnecessary concrete subclasses (e.g. TempUser\CreateStatus, also see T358492).

The PHP ecosystem has some fairly consistent conventions to solve this, using @template PHPDoc annotations, although if there is an authoritative documentation for their formats, I did not find it – every IDE and static analysis tool seems to just wing it.

While we don't seem to use @template almost anywhere (2 results here: https://codesearch.wmcloud.org/search/?q=%40template&files=php), we actually annotate uses of Status with generic parameters fairly often: https://codesearch.wmcloud.org/search/?q=%40.%2B\b(Status|StatusValue)<&files=php (this doesn't help any IDEs or static analysis tools without the @template annotations, but it helps human readers).

I tried applying them to Status and StatusValue, and I got things working like I wanted in PhpStorm, but not in Phan – apparently, when a class is annotated to be generic but it's used in a return type without filling in the generic parameters, the type parameter leaks out and causes spurious errors everywhere. Since we annotate lots of things with @return Status instead of @return Status<Foo>, this makes it impossible to gradually introduce better typing. I filed this as a bug as https://github.com/phan/phan/issues/4982.

Getting this done will probably result in some Phan errors in various extensions (either real issues or false positives), but I think it's worth it. Hopefully it can become a common convention for other code later.

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
mediawiki/coremaster+48 -17
mediawiki/coremaster+3 -3
mediawiki/tools/codesniffermaster+47 -2
mediawiki/coremaster+39 -26
mediawiki/extensions/GrowthExperimentsmaster+2 -11
mediawiki/extensions/SecurePollmaster+10 -0
mediawiki/extensions/MediaModerationmaster+10 -0
mediawiki/coremaster+166 -8
mediawiki/coremaster+164 -7
mediawiki/extensions/CheckUsermaster+6 -0
mediawiki/extensions/GlobalBlockingmaster+5 -2
mediawiki/extensions/AbuseFiltermaster+6 -0
mediawiki/extensions/DiscussionToolsmaster+3 -0
mediawiki/extensions/CommunityConfigurationmaster+5 -1
mediawiki/extensions/Citemaster+16 -8
mediawiki/extensions/GrowthExperimentsmaster+4 -0
mediawiki/coremaster+7 -164
mediawiki/extensions/SyntaxHighlight_GeSHimaster+2 -2
mediawiki/tools/codesniffermaster+1 -43
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change #1163470 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] [WIP] Document Status and StatusValue as generic classes with @template

https://gerrit.wikimedia.org/r/1163470

Duplicate of T368541. This task has a better description, so I'm not sure in which direction we want to merge.

While we don't seem to use @template almost anywhere

Watch out: we use phan-template instead. In fact, PHPCS forbids @template and wants @phan-template to be used instead (I didn't check git history, but from what I remember: this was done out of an abundance of caution because we weren't sure how standard "@template" was, and in case the unprefixed version had different usage than the @phan- version).

I tried applying them to Status and StatusValue, and I got things working like I wanted in PhpStorm, but not in Phan – apparently, when a class is annotated to be generic but it's used in a return type without filling in the generic parameters, the type parameter leaks out and causes spurious errors everywhere.

I think this might be what led me to say that it «breaks type inference completely» in T368541. Fixing this upstream would be nice!

While we don't seem to use @template almost anywhere

Watch out: we use phan-template instead. In fact, PHPCS forbids @template and wants @phan-template to be used instead

…for functions. But not for classes. Oops?

(I didn't check git history, but from what I remember: this was done out of an abundance of caution because we weren't sure how standard "@template" was, and in case the unprefixed version had different usage than the @phan- version).

I didn't find a standard, but PHPStan and Psalm use the same syntax for the basic @template T syntax, which is the only thing Phan supports anyway. Both of them also support @template-covariant ... and @template T of ..., which Phan doesn't support (it parses them like @template T, so using them may cause some false warnings).

PhpStorm supports something, but it has no documentation; instead it links to the Psalm documentation, so I hope it supports the same things. While this page says "support covers a simple case of a function returning one of its parameters", I tested PhpStorm 2025.1.1 and it is able to recognize more than that: when I tried some class template annotations, it understood them, and provided method name autocompletion and warnings for calling non-existing methods.

(PHPStan also supports @template-contravariant, which is not supported or parsed by the others. Are there any other static analysis tool to think of?)

Forbidding @template seems inconvenient, since it prevents PhpStorm from reading these annotations (and I only launch PhpStorm when I really want method name autocompletion :P). I would suggest either allowing everything (and if you get Phan errors because you used something Phan doesn't support, that's your problem to solve); or forbidding the complex ones but allowing the basic @template T syntax, which is pretty standard everywhere (despite the lack of a standard) and offers 90% of the benefits.

While we don't seem to use @template almost anywhere

Watch out: we use phan-template instead. In fact, PHPCS forbids @template and wants @phan-template to be used instead

…for functions. But not for classes. Oops?

Oh yeah. Filed T398060.

Forbidding @template seems inconvenient, since it prevents PhpStorm from reading these annotations (and I only launch PhpStorm when I really want method name autocompletion :P). I would suggest either allowing everything (and if you get Phan errors because you used something Phan doesn't support, that's your problem to solve); or forbidding the complex ones but allowing the basic @template T syntax, which is pretty standard everywhere (despite the lack of a standard) and offers 90% of the benefits.

I think we can just allow everything, and possibly consider reversing the rule (i.e., forbid @phan-template).

Change #1166528 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/tools/codesniffer@master] Allow the `@template` annotation

https://gerrit.wikimedia.org/r/1166528

Change #1166528 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] Allow the `@template` annotation

https://gerrit.wikimedia.org/r/1166528

Change #1167995 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Add parameter to StatusValue constructor to allow inferring generic type

https://gerrit.wikimedia.org/r/1167995

Upcoming Phan 5.5.1, which includes several fixes I've made to its generics support, makes my MediaWiki patches pass type checks (as long as a last-minute fix is included, otherwise we'll have to wait for the next release). Upstream release task: https://github.com/phan/phan/pull/5019

Next up: improving all these hundreds of doc comments: https://codesearch.wmcloud.org/search/?q=%40(return|param|var).*%3F\b(Status|StatusValue)\b([^<]|%24)&files=\.php%24

Many uses of StatusValue don't actually have a meaningful value, they're only used to keep track of errors. They can be documented as StatusValue<null> (more intuitive, doesn't do anything special) or StatusValue<never> (perhaps less intuitive, causes a Phan warning if you attempt to access the value). I'm not sure which is better.

Change #1168245 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Document a couple of Status and StatusValue type params

https://gerrit.wikimedia.org/r/1168245

All of the Phan and PHPCS config dependencies have landed, and this is ready for review.

Change #1163470 merged by jenkins-bot:

[mediawiki/core@master] Document Status and StatusValue as generic classes with `@template`

https://gerrit.wikimedia.org/r/1163470

Change #1176659 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/SyntaxHighlight_GeSHi@master] Change Status to Status<string>

https://gerrit.wikimedia.org/r/1176659

Change #1176659 merged by jenkins-bot:

[mediawiki/extensions/SyntaxHighlight_GeSHi@master] Change Status to Status<string>

https://gerrit.wikimedia.org/r/1176659

I think this should've waited for all extensions to run phan 0.17.0 (which comes with proper generic handling). I'm seeing multiple errors due to generic types leaking out, e.g. in T401468 for Wikibase and some stuff in CampaignEvents. I am going to revert the core patch for the time being, as we don't know the extent of the breakage.

Change #1176689 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Revert "Document Status and StatusValue as generic classes with `@template`"

https://gerrit.wikimedia.org/r/1176689

I am going to revert the core patch for the time being, as we don't know the extent of the breakage.

Surely that’s going to break Wikibase again, now that we’ve merged several patches that rely on Status being generic?

I think this should've waited for all extensions to run phan 0.17.0 (which comes with proper generic handling). I'm seeing multiple errors due to generic types leaking out, e.g. in T401468 for Wikibase and some stuff in CampaignEvents. I am going to revert the core patch for the time being, as we don't know the extent of the breakage.

As an assurance that the upgrade will fix these issues: r1176686 has about 20 new issues complaining about a type T; the phan upgrade patch, depending on the other, has 0.

I am going to revert the core patch for the time being, as we don't know the extent of the breakage.

Surely that’s going to break Wikibase again, now that we’ve merged several patches that rely on Status being generic?

I think for the most part it shouldn't, I imagine the type specialization part would just be ignored. But even if it did break Wikibase again, I'm assuming that's preferable to potentially breaking dozens of other repos. I'll be happy to help get the phan update out asap.

I just tried it locally, it would in fact break Wikibase again:

repo/includes/EditEntity/EditEntityStatus.php:74 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue PhanTypeMismatchReturn on this line but this suppression is unused or suppressed elsewhere
repo/includes/TempUserStatus.php:12 UnusedPluginFileSuppression Plugin BuiltinSuppressionPlugin suppresses issue PhanGenericConstructorTypes in this file but this suppression is unused or suppressed elsewhere

I just tried it locally, it would in fact break Wikibase again:

repo/includes/EditEntity/EditEntityStatus.php:74 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue PhanTypeMismatchReturn on this line but this suppression is unused or suppressed elsewhere
repo/includes/TempUserStatus.php:12 UnusedPluginFileSuppression Plugin BuiltinSuppressionPlugin suppresses issue PhanGenericConstructorTypes in this file but this suppression is unused or suppressed elsewhere

Ah yes, unused suppressions would do that I guess. If you make a patch and add me as reviewer, I'll merge it ASAP. I don't think there's a realistic alternative to reverting the core change, considering that it probably breaks most if not all repos using Status(Value), i.e. a lot of repos.

I just tried it locally, it would in fact break Wikibase again:

repo/includes/EditEntity/EditEntityStatus.php:74 UnusedPluginSuppression Plugin BuiltinSuppressionPlugin suppresses issue PhanTypeMismatchReturn on this line but this suppression is unused or suppressed elsewhere
repo/includes/TempUserStatus.php:12 UnusedPluginFileSuppression Plugin BuiltinSuppressionPlugin suppresses issue PhanGenericConstructorTypes in this file but this suppression is unused or suppressed elsewhere

Fixed in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/1176690.

I don't think there's a realistic alternative to reverting the core change, considering that it probably breaks most if not all repos using Status(Value), i.e. a lot of repos.

I don’t understand why this is now being treated as a blocker, when previously it sounded like breaking CI in extensions was the expected consequence of this change? (“Getting this done will probably result in some Phan errors in various extensions” in the task description; “extensions that already used StatusValue<Foo> etc. in their PHPDocs will now have it validated by Phan, which may result in errors” in the commit message.)

When you revert the revert, can you please send an announcement to Wikitech-l so people at least have a chance to understand why their CI is failing? Several Wikibase developers were very confused this morning.

Change #1176689 merged by jenkins-bot:

[mediawiki/core@master] Revert "Document Status and StatusValue as generic classes with `@template`"

https://gerrit.wikimedia.org/r/1176689

I don't think there's a realistic alternative to reverting the core change, considering that it probably breaks most if not all repos using Status(Value), i.e. a lot of repos.

I don’t understand why this is now being treated as a blocker, when previously it sounded like breaking CI in extensions was the expected consequence of this change? (“Getting this done will probably result in some Phan errors in various extensions” in the task description; “extensions that already used StatusValue<Foo> etc. in their PHPDocs will now have it validated by Phan, which may result in errors” in the commit message.)

I think the expected consequence was to break CI in some extensions. The reason I'm treating this as a blocker is that all extensions are still using mediawiki-phan-config 0.16.0, which comes with a version of phan (5.5.0) where handling of generic types is severely buggy. @matmarex did an excellent jobs finding and fixing all these bugs upstream, but the fixes were only included in the 5.5.1 release, which is what mw-phan-config 0.17.0 ships with.

Repurposing my example from T397781#11070821: CI is currently broken for CampaignEvents with 20 nonsense warnings about a phantom "T" type. Upgrading the extension to phan 0.17.0 will get rid of all those warnings without any code changes. That's the extent of the breakage we're talking about. I obviously haven't proven it, but I imagine that everything using StatusValue is currently broken, whereas with phan 0.17.0, only certain usages would be broken.

When you revert the revert, can you please send an announcement to Wikitech-l so people at least have a chance to understand why their CI is failing? Several Wikibase developers were very confused this morning.

Yep, I think that's a good idea.

Change #1176692 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Revert^2 "Document Status and StatusValue as generic classes with `@template`"

https://gerrit.wikimedia.org/r/1176692

Sorry about that, I should have planned the transition better and documented the dependency on mediawiki-phan-config 0.17 (Phan 5.5.1). I meant to write a Wikitech-l message too, but I haven't done it yet, since I didn't expect the change to be merged so enthusiastically.

I hope we can try again, with less disruption, in a week or two, once the dependencies are bumped in most repos. Some problems are unavoidable, as you all noted, but almost all of the existing uses of Status/StatusValue/etc. should be fine with no changes once we're using latest Phan (whereas with older Phan they almost all generate incorrect warnings).

Change #1177317 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Cite@master] Document the values returned by StatusValue

https://gerrit.wikimedia.org/r/1177317

Change #1177366 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/AbuseFilter@master] Use generic type on AbuseFilterPermissionStatus

https://gerrit.wikimedia.org/r/1177366

Change #1177385 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/CheckUser@master] Use generic type on CheckUserPermissionStatus

https://gerrit.wikimedia.org/r/1177385

Change #1177389 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/CommunityConfiguration@master] Use generic type on ValidationStatus

https://gerrit.wikimedia.org/r/1177389

Change #1177391 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/DiscussionTools@master] Use generic type on ContentThreadItemSetStatus

https://gerrit.wikimedia.org/r/1177391

Change #1177392 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/GlobalBlocking@master] Use generic type on GlobalBlockStatus

https://gerrit.wikimedia.org/r/1177392

Change #1177395 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/GrowthExperiments@master] Use generic type on AbuseFilterPermissionStatus

https://gerrit.wikimedia.org/r/1177395

Change #1177396 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/MediaModeration@master] Use generic type on ImageContentsLookupStatus

https://gerrit.wikimedia.org/r/1177396

Change #1177397 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/SecurePoll@master] Use generic type on AbuseFilterPermissionStatus

https://gerrit.wikimedia.org/r/1177397

Change #1177395 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Use generic type on LinkRecommendationEvalStatus

https://gerrit.wikimedia.org/r/1177395

Change #1177317 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Document the values returned by StatusValue

https://gerrit.wikimedia.org/r/1177317

Change #1177389 merged by jenkins-bot:

[mediawiki/extensions/CommunityConfiguration@master] Use generic type on ValidationStatus

https://gerrit.wikimedia.org/r/1177389

Change #1177391 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Set generic type on ContentThreadItemSetStatus

https://gerrit.wikimedia.org/r/1177391

Change #1177366 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Use generic type on AbuseFilterPermissionStatus

https://gerrit.wikimedia.org/r/1177366

It looks like almost all repos are on mediawiki-phan-config 0.17 (Phan 5.5.1), except for a handful which seem to have other CI problems. I would like to re-land the change (1176692) and the patches for extensions tagged against this task and the subtask (thanks for preparing them).

I'm planning to do it on Sunday evening to minimize disruption while the patches get merged (and in case we missed any repos that need fixes, they can get fixed on Monday morning instead of disrupting things in the middle of a day). I'll send a Wikitech-l message at the time too.

Change #1177392 merged by jenkins-bot:

[mediawiki/extensions/GlobalBlocking@master] Use generic type on GlobalBlockStatus

https://gerrit.wikimedia.org/r/1177392

Change #1177385 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Use generic type on CheckUserPermissionStatus

https://gerrit.wikimedia.org/r/1177385

Change #1176692 merged by jenkins-bot:

[mediawiki/core@master] Revert^2 "Document Status and StatusValue as generic classes with `@template`"

https://gerrit.wikimedia.org/r/1176692

Change #1177396 merged by jenkins-bot:

[mediawiki/extensions/MediaModeration@master] Use generic type on ImageContentsLookupStatus

https://gerrit.wikimedia.org/r/1177396

Change #1177397 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@master] Use generic type on BallotStatus

https://gerrit.wikimedia.org/r/1177397

matmarex claimed this task.

Change #1183648 had a related patch set uploaded (by Michael Große; author: Michael Große):

[mediawiki/extensions/GrowthExperiments@master] fix: adjust LinkRecommendationEvalStatus type to make CI pass

https://gerrit.wikimedia.org/r/1183648

Change #1183648 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] fix: adjust LinkRecommendationEvalStatus type to make CI pass

https://gerrit.wikimedia.org/r/1183648

Change #1168245 merged by jenkins-bot:

[mediawiki/core@master] Document a couple of Status and StatusValue type params

https://gerrit.wikimedia.org/r/1168245

Change #1184919 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/tools/codesniffer@master] HISTORY: Tag as v48.0.0

https://gerrit.wikimedia.org/r/1184919

Change #1184919 merged by jenkins-bot:

[mediawiki/tools/codesniffer@master] HISTORY: Tag as v48.0.0

https://gerrit.wikimedia.org/r/1184919

Change #1191379 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Add type information to some places that return StatusValues

https://gerrit.wikimedia.org/r/1191379

Change #1191379 merged by jenkins-bot:

[mediawiki/core@master] Add type information to some places that return StatusValues

https://gerrit.wikimedia.org/r/1191379

Change #1167995 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] Add parameter to StatusValue constructor to allow inferring generic type

Reason:

I'm not interested in pursuing this at the moment.

https://gerrit.wikimedia.org/r/1167995