Page MenuHomePhabricator

OOUI docblocks trip Phan analysis
Closed, ResolvedPublic

Description

While upgrading Phan for AbuseFilter, I noticed a decent amount of warnings coming from OOUI, see P8143. Looking at OOUI codebase, it's pretty obvious why this happens: many (if not all) widgets, layouts etc. have something like this in the constructor:

	/**
	 * @param array $config Configuration options
	 * @param string $config['align'] ...
	 * @param array $config['errors'] ...
	 * @param array $config['notices'] ...
	 * @param string|HtmlSnippet $config['help'] ...
	 */
         public function __construct( array $config = [] ) {

Which is to say, every element of the $config array is documented as a stand-alone parameter. This is very confusing for Phan, and I'm unsure whether it's a valid docblock annotation. It also has to be fixed somehow, because otherwise any piece of code using OOUI will have to add tons of suppressions.
However, before starting to mass-edit all docblocks, I'd like to know what the proposed solution is. An idea is to replace @param with some padding, so to have:

(Prop 1)

	/**
	 * @param array $config Configuration options
	 *        string $config['align'] ...
	 *        array $config['errors'] ...

or similar format.

Prop 1b

	/**
	 * @param array $config Configuration options
	 *        $config['align'] string Blah blah
	 *        $config['errors'] array Blah blah

Prop 2

	/**
	 * @param array $config Configuration options
         *     $config = [
	 *        'align' => string Blah blah
	 *        'errors' => array Foo bar baz

Prop 2b

	/**
         * $config = [
         *    here goes description in one of the above styles
         * ]
         *
	 * @param array $config Configuration options

Event Timeline

Daimona created this task.Mar 2 2019, 10:11 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 2 2019, 10:11 AM
Daimona triaged this task as High priority.Mar 2 2019, 4:57 PM

Triaging as high because any extension using OOUI will be affected while switching to the new Phan.

Daimona updated the task description. (Show Details)Mar 4 2019, 1:11 PM

Added a couple of proposals in the task description. CC @matmarex, who AFAICS wrote most of the docs for OOUI classes.

Whatever format we use, it must also generate something sensible in the Doxygen generated documentation: https://doc.wikimedia.org/oojs-ui/master/php/classOOUI_1_1FieldLayout.html#a43b3e6ec62c92ad3f7cc8ddc23a6e53a

For reference, currently it looks like this, which is maybe a little weird, but readable:

You're correct though that it is not actually valid syntax. Apparently $config['foo'] Blah blah is interpreted as documenting two different parameters $config and ['foo'] with "Blah blah". From http://www.doxygen.nl/manual/commands.html#cmdparam: "Note that you can also document multiple parameters with a single \param command using a comma separated list.".

I've looked for similar cases in MediaWiki and it looks like we settled there on formatting the parameters as a list, for example here: https://github.com/wikimedia/mediawiki/blob/3290e7b3cc7b33287ec380d4b0af5c37aa1fad10/includes/libs/filebackend/FileBackend.php#L132 which generates this documentation: https://doc.wikimedia.org/mediawiki-core/master/php/classFileBackend.html#abc5ac617ee8f273d07ab461038f2ed6d

This format doesn't specify the type in a nice way though. But something like your prop 1 together with the dashes to make it formatted as a list would be fine, I guess?


It seems we can improve Phan's type checking for the config array by specifying the types of each item in the array (Phan calls this the "shape" of an array) using syntax described here: https://github.com/phan/phan/wiki/About-Union-Types

@param array{align:string,errors:array,notices:array,help:string|HtmlSnippet} $config Configuration options

This looks awful though…

Is there some different format to document the types for Phan, so that we could keep @param sane for Doxygen?

Oh, there is also a third consumer of the doc comments, the Ruby scripts in bin/ that generate test cases for all PHP widgets. But we can easily change these to parse any new format, it's just a bunch of regexp and string code in docparser.rb.

I've looked for similar cases in MediaWiki and it looks like we settled there on formatting the parameters as a list [...] This format doesn't specify the type in a nice way though.

Indeed, I think having the types specified (which is great) is what takes away a bit of flexibility.

But something like your prop 1 together with the dashes to make it formatted as a list would be fine, I guess?

I like it!

It seems we can improve Phan's type checking for the config array by specifying the types of each item in the array (Phan calls this the "shape" of an array) using syntax described here: https://github.com/phan/phan/wiki/About-Union-Types

Ugh, a bit hard to read. Also, while it'd help phan, IMHO it would also make the docblock less readable, unless we still add types in the description of every element. Which leads us back to the same problem.

Is there some different format to document the types for Phan, so that we could keep @param sane for Doxygen?

I don't think so.

I have read of several proposal to include a specific syntax for array elements in PSR, but it's far from being concrete.

So in the meanwhile we could go on with Prop1 + dashes?

Change 495669 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[oojs/ui@master] Change docblock style for array elements in $config

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

Daimona claimed this task.Mar 11 2019, 11:30 AM

The patch above implements prop1 + dashes. I'm currently checking it for errors.

@matmarex Alright, now the Ruby test parser has to be updated. I guess the regex could be changed to allow [-@] at the beginning of the line, and the following case should have an extra when for it. However I don't know Ruby and I'm leaving it up to someone else to avoid messing up.

Change 495669 merged by jenkins-bot:
[oojs/ui@master] Change docblock style for array elements in $config

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

Daimona closed this task as Resolved.Mar 11 2019, 9:40 PM
Daimona removed a project: Patch-For-Review.

Awesome, thanks!

Thank you for working on this!

Change 496356 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/core@master] Update OOUI to v0.31.0

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

Change 496356 merged by jenkins-bot:
[mediawiki/core@master] Update OOUI to v0.31.0

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