I'm not sure how this wasn't reported earlier, I guess the module cache prevented everyone from encountering it?

Yeah, I think this didn't crop up sooner through a combination of the cache and there being relatively few files with multiple @phutil-external-symbol tags.

(I caught something similar in Diviner that I'll take a look at, although possibly D16484 covers it.)

We should probably change this API to always return array(), the "sometimes array, sometimes scalar" return value is pretty unintuitive. And any code which fixed this by adding (array) will just work.

The other fix I applied in the wake of D16431 was D16433.

To test all currently-known code depending on this stuff:

• arc liberate --all
• bin/celerity map
• bin/diviner generate --clean

I'm going to retarget this to make the API more consistent.

We have a parser for comment docblocks, used to generate documentation and extract metadata from source code. This is a docblock:

/**
 * @key value
 */

Source code is text files containing stories that computers can read.

D16431 improved this parser by providing a more structured way to extract multiple repeats of a given @key. However, it left us with a return value that is sometimes a scalar (if there was only one @key) and sometimes a list (if there was more than one @key).

This is surprising and caused a few issues at parser callsites:

• D16433: Update Celerity map parser for new docblock code
• D16484: Use new format for docblock specials when parsing symbols
• D16487: Update Diviner for array-valued `@doc-stuff` return values from DocblockParser

I think we should just always return an array instead, so consumers don't need to know that they have to test. Currently, it's possible to write a consumer which works fine until someone writes @key twice, then explodes.

To test all the stuff that hit issues:

phabricator/ $ arc liberate --all
phabricator/ $ ./bin/celerity map
phabricator/ $ ./bin/diviner generate --clean

I think there's another one of these in DivinerAtomController->composeTasks(). The other getDocblockMetaValue() calls anywhere in the codebase are also suspicious.