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 #4533

[haberman]

Can we use ArrayAccess instead? RepeatedField is an internal type and we do not want to expose it to public interfaces.

[bshaffer]

This makes sense to me as long as there are no methods of RepeatedField that we want IDEs to typehint (e.g., we want the users to only see the objects as an array, and NOT as a RepeatedField).

Also, if we do that, we should also do that for the setter typehints, for consistency. In this case, I think the best approach would be to use [], such as @return string[].

As changes like this will effect most of the files in our repo, making them at the same time would be ideal. If that sounds good to you, I'll update this PR to remove RepeatedField entirely for both getters and setters, in favor of the type[] syntax!

[haberman]

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.

[haberman]

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?

[bshaffer]

@haberman yes that's correct! ArrayAccess will handle anything annotated with type[]

[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 <#11734 (comment)> : > 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 <#11734 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZMBP6FYCQ4VKRGZVDXXLWXLJXPANCNFSM6AAAAAAUM4NYKM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[github-actions]

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]

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.

[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 (#14666, #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]

aaa08d8 seems to have broken all the tests.

[JasonLunn]

@bshaffer - can you look into the test failures?

[bshaffer]

@JasonLunn the tests are passing! This is ready for review

[JasonLunn]

Nit: do we even need this header comment? If so, should it be this year's date?

[bshaffer]

Good question - I didn't update it since the file was essentially moved, but it did change a little... so maybe we should update it? I'm happy to do so if you'd like!