googleapis / google-cloud-php

Google Cloud Client Library for PHP
https://cloud.google.com/php/docs/reference
Apache License 2.0
1.09k stars 433 forks source link

Static analysis error with psalm due to missing @template tag #6198

Open plsoucy-tapclicks opened 1 year ago

plsoucy-tapclicks commented 1 year ago

Note: this is not a blocker as we can psalm-suppress the error and the code actually works fine, but I'm reporting this here since it seems like an easy fix and others might encounter the same issue.

Environment details

Steps to reproduce

  1. Run psalm static analysis on code that calls $bucket->objects $objects = $bucket->objects(['prefix' => $gcs_path, 'fields' => 'items,nextPageToken']);

Details: https://github.com/vimeo/psalm/issues/9744

Code example

use Google\Cloud\Storage\StorageClient;

$key_file = 'foo';
$gcs_path = 'bar';
$storage_client = new StorageClient(['projectId' => 1234, 'keyFilePath' => $key_file]);

$bucket = $storage_client->bucket('bucket-name');
$objects = $bucket->objects(['prefix' => $gcs_path, 'fields' => 'items,nextPageToken']);
print !empty($objects)."\n";

Error as reported by psalm on that code:

ERROR: TooManyTemplateParams - src/test.php:10:12 - Google\Cloud\Storage\ObjectIterator<Google\Cloud\Storage\StorageObject> has too many template params, expecting 0 (see https://psalm.dev/184)
$objects = $bucket->objects(['prefix' => $gcs_path, 'fields' => 'items,nextPageToken']);

Per https://github.com/vimeo/psalm/issues/9744, this can be fixed by adding an @template annotation to the src/ObjectIterator.php class (I have verified this)

/**
 * Iterates over a set of {@see Google\Cloud\Storage\StorageObject} items.
 *
 * @template T
*/
class ObjectIterator implements \Iterator
orklah commented 1 year ago

Well, more precisely, Iterator has 2 templates. When it is implemented, a @template-implements is expected. It should look like

@template-implements \Iterator<TKey, TValue> where TKey and TValue are either template themselves or the type of the key or the value if it's fixed.

Here, ObjectIterator seems to use int as an offset (because key() in the trait return int), so TKey should be replaced by int

So you would have something like

/**
 * @template TValue
 *
 * @template-implements \Iterator<int, TValue>
 */

That way, users of ObjectIterator will still need to fill the TValue type for their own instantation/implementations

I don't know the library enough but based on the name, it could even be

/**
 * @template TValue of object
 *
 * @template-implements \Iterator<int, TValue>
 */

to restrain the possible type of the template to objects only

ajupazhamayil commented 1 year ago

I see the @template is nowhere used in google-cloud-php. We may need to change everywhere if we need to resolve this issue to have uniformity.

orklah commented 1 year ago

it's used here: https://github.com/googleapis/google-cloud-php/blob/main/Storage/src/Bucket.php#L651, not sure if it's used in other places

ajupazhamayil commented 3 months ago

I'm sorry, could you please confirm the given link has @template annotation?

orklah commented 3 months ago

Sorry, my answer was not clear. What I meant is that there is an annotation that is using ObjectIterator as if it had a template here: https://github.com/googleapis/google-cloud-php/blob/main/Storage/src/Bucket.php#L706

But ObjectIterator doesn't actually have one: https://github.com/googleapis/google-cloud-php/blob/main/Storage/src/ObjectIterator.php

So either the ObjectIterator<StorageObject> should become ObjectIterator in Bucket or a @template should be added to ObjectIterator

ojhaujjwal commented 1 month ago

I am being affected by this. please try to prioritize this folks. type-safety is pretty widespread now.