feat(php): improve return typehint when repeatedfield

[bshaffer]

Adds the type of the RepeatedField to the PHPDoc, e.g.

/**
 * @return \Google\Protobuf\Internal\RepeatedField<int>
 */

Whereas before, the <int> part was not included.

This is the "getter" counterpart for https://github.com/protocolbuffers/protobuf/pull/4533

[bshaffer]

We do!

On Mon, Feb 13, 2023, 7:55 PM Joshua Haberman @.***> wrote:

@.**** commented on this pull request.

In src/google/protobuf/compiler/php/php_generator.cc https://github.com/protocolbuffers/protobuf/pull/11734#discussion_r1105178194 :

 case FieldDescriptor::TYPE_GROUP: return "null";

 default: assert(false); return "";

}

• if (field->is_repeated()) {
• // accommodate for edge case with multiple types.
• size_t start_pos = type.find('|');
• if (start_pos != std::string::npos) {

•  type.replace(start_pos, 1, ">|\\Google\\Protobuf\\Internal\\RepeatedField<");
  

That sounds good to me. The only caveat is that I'm not sure if we support assignment of PHP arrays to repeated fields or not.

— Reply to this email directly, view it on GitHub https://github.com/protocolbuffers/protobuf/pull/11734#discussion_r1105178194, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZMBP6FYCQ4VKRGZVDXXLWXLJXPANCNFSM6AAAAAAUM4NYKM . You are receiving this because you authored the thread.Message ID: @.***>

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.

This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

[bshaffer]

This is kinda bizarre - so if you ignore a PR for long enough you use that as justification to close it?

[haberman]

The PR is kept open if there are any comments on the issue. It's to gauge whether there is still interest from the author.

I'll reopen this one.

[haberman]

Hi @bshaffer , can you confirm an answer to my most recent question above?

Ok sounds good, yes let's remove RepeatedField in getters and setters. If we use type[] we should be satisfying that by implementing ArrayAccess, yes?

[haberman]

It looks like the PR is still using RepeatedField. Can you update it to use type[] instead?

[bshaffer]

@haberman so a few thoughts on this...

There are a few issues with typing things as we have now. This depends on the context of. the typehint:

Typehinting a setter as type[]

Protobuf Message setters accept both array and RepeatedField types. Using the type[] type-hint says that we expect this variable to be an array. This is usually okay, since normally these are passed in as a user-defined array. However, static analyzers will complain if it detects a RepeatedField being passed in (even though we accept them). I could see this happening when the getter from one message is used as a setter for another. For example:

$a = $message1->getA();
$message2->setA($a);

This would not be a problem if we typehint the return types as type[] as well, but that is problematic in its own way (see below).

Type-hinting return types as type[]

If we use type[] for return type-hints, instead of RepeatedField<type>, we are telling our users/static analyzers/IDEs that these things need to be arrays. This may cause issues with static analyzers and IDEs. The main issue is that there are methods which work for array types which are not available to RepeatedField. For instance, array_merge and the + operator do not work on RepeatedField objects. The static analyzers will miss them, and a user may try to use them and be unpleasantly surprised to find they're not actually arrays.

Additionally, because the static analyzer expects an array but gets a RepeatedField, it may send out an error. In my testing, this doesn't actually happen in Protobuf because the static analyzer doesn't know what's being returned. But potentially someday it may throw an error here.

So our choices are as follows

1. Set return type-hints as type[] and hope nobody minds. I personally think this is a bad way to go, as users may be surprised to find that objects which they thought were arrays are in fact RepeatedField
2. Set return type-hints to RepeatedField<type>. One thing we can do is shorten this to not contain the namespace, since we know that in every case where we use RepeatedField, we import it at the top of the class. This would at least make the typing less verbose. The problem here is in the case above, this would throw a static analysis error for setters because they expect type[].
3. Use Traversable<type> instead. This takes advantage of the Traversable interface in PHP, and is essentially the opposite of using the type[] typehint, in the sense that we are typing it stricter than it actually is. We are saying it can be used for foreach, but nothing else. This means in some cases, things like $value = $repeatedField[0] would throw a static analysis error. Since that usage is probably discouraged anyway, maybe this isn't a problem. But it would definitely be an issue for MapField types (which is out of scope for this PR). But again, this would throw a static analysis error for setters because they expect type[].

Conclusion

• If we want to be 100% accurate, we should type parameters as type[]|RepeatedField<type> and returns as RepeatedField<type>.
• If we want to hide the RepeatedField type (and we think that the potential issues with static analyzers and IDEs are negligible, which is probably true), we should type parameters as type[] and returns as Traversable<type>

[haberman]

What about ArrayAccess<T>? That seems like the most accurate type to return.

[bshaffer]

What about ArrayAccess<T>? That seems like the most accurate type to return.

ArrayAccess is similar to marking as Traversable in the sense that it's a more narrowly scoped type. It would still encounter the issues I mentioned for Traversable. IMHO Traversable is better because I imagine the normal usage of RepeatedField is to traverse it (e.g. in foreach) or call iterator_to_array to make it into an array. Neither of these are possible with ArrayAccess (and would result in a static analysis error). I think ArrayAccess is better suited for MapField types.

[bshaffer]

Another thought on this is we could typehint the returns as ArrayAccess<T>&Traversable<T> using the Intersection Types in PHP 8.1. This mirrors the actual RepeatedField implementation, which implements ArrayAccess, IteratorAggregate (which extends Traversable), and Countable.

[bshaffer]

Heads up @haberman , we are receiving a few issues (https://github.com/protocolbuffers/protobuf/issues/14666, https://github.com/protocolbuffers/protobuf/pull/15673) related to this. I think we should make a decision on how to support them!

[jontro]

I would also like to add that UnaryCall, ServerStreamingCall, ClientStreamingCall and BidiStreamingCall would be nice to type this way too.

[bshaffer]

@jontro can you describe a little bit more what you're asking for? This repo is for protobuf, you may be looking for the gRPC repo, where those classes are defined, or the google cloud PHP repo, where the APIs are defined.

If you could provide an example, that would be very helpful.

[jontro]

@bshaffer Sorry for the confusion. Found a corresponding issue in the grpc repo for this in case anyone else is looking https://github.com/grpc/grpc/issues/33431