Protoc generator for php missing type hint on message getters (yet setters have it)

[frickenate]

What language does this apply to?
protoc generated code for php

Describe the problem you are trying to solve.
Setters on messages for repeated fields are docblock type hinted (thanks to PhpSetterTypeName() in src/google/protobuf/compiler/php/php_generator.cc) to include both \Google\Protobuf\Internal\RepeatedField as well as the underlying repeated field type—whether it's a native type (eg. int[] or string[]) or an embedded/nested message (eg. InnerMessage[]). This is perfect for integration with IDEs.

Unfortunately the getters for repeated fields, as implemented by PhpGetterTypeName(), only type hints \Google\Protobuf\Internal\RepeatedField, without adding the additional TYPE[]| type hint for the underlying type being repeated.

Example stub of what is currently generated, where the setter has the "useful type hint" as well as RepeatedField, but the getter only has RepeatedField and is missing the "useful type hint":

/**
 * Generated from protobuf message ...
 */
class OuterMessage extends \Google\Protobuf\Internal\Message
{
    /**
     * @return \Google\Protobuf\Internal\RepeatedField $var
     */
    public function getInnerMessages() {}

    /**
     * @param InnerMessage[]|\Google\Protobuf\Internal\RepeatedField $var
     */
    public function setInnerMessages($var) {}
}

Thus when writing PHP code in an intelligent IDE that uses the getters:

foreach ($outerMessage->getInnerMessages() as $innerMessage) {
    // since getInnerMessages() is not type hinted as InnerMessage[] in addition to
    // RepeatedField, IDE does know that $innerMessage is of type InnerMessage,
    // and the entirety of useful IDE functionality goes out the window
}

Describe the solution you'd like
The logic used by PhpSetterTypeName() to generate the @param phpdoc for setters should be replicated by PhpGetterTypeName() to generate the @return phpdoc for getters. The result of the earlier example stub should become:

/**
 * Generated from protobuf message ...
 */
class OuterMessage extends \Google\Protobuf\Internal\Message
{
    /**
     * @return InnerMessage[]|\Google\Protobuf\Internal\RepeatedField $var
     */
    public function getInnerMessages() {}

    /**
     * @param InnerMessage[]|\Google\Protobuf\Internal\RepeatedField $var
     */
    public function setInnerMessages($var) {}
}

Describe alternatives you've considered
N/A

Additional context
N/A

[TeBoring]

I don't think Foo[] should be added into return type.
We actually always return Google\Protobuf\Internal\RepeatedField which has similar api as array.

[frickenate]

The code already does this for setters, but not for getters. It's an addition to the docblock line, not changing the true return type (which stays as RepeatedField). The PHP client is extremely unfriendly to use due to this omission, with IDEs being unable to help with autocompleting off of getters. Again, the setters have this already.

[pelletiermaxime]

This is actually one of the first thing I found very weird. The getters for repeated fields would be a lot easier to use if they returned the right type, just like the setters do.
In the meantime I have to add manually the type hint for my ide to have auto completion:

/** @var InnerMessage[] $innerMessage */

[frickenate]

@TeBoring Can you please reread the issue to understand? It's about modifying the @return tag in the PHP doc block (/** ... **/, not changing the real return type. Again, the generator already does this for the setters... for some reason the getters (which are imo even more useful) were not given the same treatment.

[TeBoring]

The setter accept both php native array and our internal repeated field. However, the getter only returns our internal repeated field. If we change it to php native array, that would be a lie. Does php have some interface like iterable? Probably we can change both getter and setter to that. @haberman, how do you think?

…

On Mon, Jul 6, 2020 at 3:40 PM Nate Bessette ***@***.***> wrote: @TeBoring <https://github.com/TeBoring> Can you please reread the issue to understand? It's about modifying the @return tag in the docblock, not changing the real return type. Again, the generator already does this for the setters... for some reason the getters (which are imo even more useful) were excluded. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6145 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABHUPZLVY2ZHBCNBL3YO7A3R2JHGVANCNFSM4HNPTQYA> .

[haberman]

I agree with @TeBoring's analysis. The getters are documented to accept InnerMessage[] because they do, in fact, accept a real PHP array of InnerMessage instances. The getters are documented to return RepeatedField only, because that is the true type of the return value. RepeatedField is a type defined by the protobuf library. It behaves like a real PHP array in many ways, but it enforces that all elements are of a single type.

Ideally we could annotate the return type as something like RepeatedField<InnerMessage>. Or perhaps as ArrayAccess<InnerMessage>, since RepeatedField implements the ArrayAccess interface. Is such a thing possible?

If not, the question is whether is it acceptable and desirable to write @return InnerMessage[]|\Google\Protobuf\Internal\RepeatedField when the function will not actually ever return a PHP array. If writing both return types joined with | will solve the IDE problem, it seems harmless enough, as the true return type of RepeatedField is still in the list of types.

@bshaffer any thoughts here?

[frickenate]

If not, the question is whether is it acceptable and desirable to write @return InnerMessage[]|\Google\Protobuf\Internal\RepeatedField when the function will not actually ever return a PHP array. If writing both return types joined with | will solve the IDE problem, it seems harmless enough, as the true return type of RepeatedField is still in the list of types.

It’s this, exactly. Due to limitations of the php language, it’s very common to tell “white lies” in phpdoc comments about@param and @return types. We do the same with the Generator class/type - while the true type returned from a generator function is Generator, we will write the following for a generator that yields strings:

/** @return \Generator|string[] */
function generate(): \Generator
{
    yield “hello”;
    yield “world”;
}

The phpdoc for these sorts of things is really about helping out in the IDE. It has no effect on actual usage whatsoever. As @pelletiermaxime mentioned, right now your users are littering their code based with literally hundreds/thousands of manual /** @var */ comments every time a getter is called on a message (and every chained submessage). It’s truly an awful experience fully solved by the standard “php workaround” for this occurrence.

[pelletiermaxime]

I couldn't agree more with @frickenate. Yes, it's kind of a lie, but that why we always return both with @return InnerMessage[]|\Google\Protobuf\Internal\RepeatedField

[bshaffer]

I agree it's very important to add the return type of the iterated items, and t's ok to add the union return type for this cause (it's pretty clear what this means). In my testing (PHPStorm), I found the following worked as expected:

    /**
     * @return InnerMessage[]|RepeatedField
     * @return RepeatedField|InnerMessage[]
     */

And the following did not work:

    /**
     * @return RepeatedField[InnerMessage]
     * @return RepeatedField<InnerMessage>
     */

The first (InnerMessage[]|RepeatedField) is my recommended option because PHPStorm suggested the first when it autocompleted the docblock.

[haberman]

Sounds like we have consensus. I'll re-open the issue.

I'd be happy to accept a pull request for this.

[dwsupplee]

Hi all,

I did a little digging here, and it seems as though generics were supported by PHP storm at one point in time while PSR-5 was more active.

As PSR-5 developed, it looks as though generics support was dropped (at least for the time being, based on the verbiage). It looks to me this may be why PHP storm doesn't support generics today - however, there appears to be an on-going case to get the support back into the IDE (which is recently active).

If it seems like PHP storm may support generics soon, would we be open to using that syntax instead of union? It seems like a more accurate representation of the state of the code and can be supported in the interim with a metadata file. If the IDE doesn't add support, we can move back to using union instead. WDYT?

[pelletiermaxime]

Adding generics would help static analysis tools and maybe PhpStorm in the future, but to me it's a bit weird to add tool/ide specific annotations in generated code. That may be my own bias as I didn't use PhpStorm for a long while.

[frickenate]

Indeed, @return RepeatedField<InnerMessage> would be nice, but:

1. It's not part of the php documentor (phpdoc) standard.

2. PhpStorm will happily auto-complete the type when defining RepeatedField<InnerMessage>; ie. it doesn't consider the angle brackets an error, and even lets you click on InnerMessage to navigate to the class.

3. However, PhpStorm does not take it into account with code that uses the function/method documented to return that type. So a foreach() over such a getter does not offer auto-complete of anything from InnerMessage.

Regarding .phpstorm.meta.php:

1. Who is going to generate that? I'm not going to manually write those stubs for the hundreds of Message sub-classes we are already using.

2. It's PhpStorm-specific, and involves placing the same docblock @return statement that can just be added to the existing code generation. Why duplicate the work?

Whereas RepeatedField|InnerMessage[] is phpdoc standard, and as such is supported by IDEs other than PhpStorm as well (eg. Eclipse and even Netbeans). It's the only solution that works across the board today, and as I mentioned before is the standard idiomatic workaround.

[dwsupplee]

Fair and reasonable points @pelletiermaxime and @frickenate. I shouldn't have so broadly assumed folks using an IDE nowadays would be on PHP Storm.

Regarding .phpstorm.meta.php:

Who is going to generate that? I'm not going to manually write those stubs for the hundreds of Message sub-classes we are already using.

I was thinking it could be scripted, but I haven't proof on concepted that out yet to determine how feasible it is.

It's the only solution that works across the board today, and as I mentioned before is the standard idiomatic workaround.

I understand we're between a rock and a hard place here, I'm just bothered by the return block not being truthful - for each case it may help folks there could also be cases where it misleads others. Would you be able to share some examples where this is considered an idiomatic workaround? I'd be open to learning a bit more on the subject.

[frickenate]

• Symfony Example
• Laravel Example
• Yii2 Example

[dwsupplee]

Good data points, thanks. Alright I think I'm convinced this can be a reasonable path forward until (hopefully) generic support lands.

[stof]

an idea could be to add the generic phpdoc using @phpstan-return or @psalm-return (keeping the less precise type in @return). AFAIK both psalm and phpstan will also attempt to read the annotation of the other tool and convert it to their own if they support the equivalent feature.
this way, tools supporting phpstan or psalm (which include phpstan and psalm of course, but also recent versions of PhpStorm and maybe other IDEs too) could benefit from the more precise types thanks to generics.

[deannagarcia]

It looks like the consensus here is that this can be implemented, but it will not be a priority for the proto team. If you all would like this feature, we would be happy to review and merge a PR!