google-cloud-php icon indicating copy to clipboard operation
google-cloud-php copied to clipboard

Static analysis error with psalm due to missing @template tag

Open plsoucy-tapclicks opened this issue 1 year ago • 6 comments

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

  • OS: Any
  • PHP version: 8.1
  • Package name and version: google/cloud-storage 1.30.2

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

plsoucy-tapclicks avatar May 09 '23 02:05 plsoucy-tapclicks

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

orklah avatar May 09 '23 17:05 orklah

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.

ajupazhamayil avatar May 10 '23 15:05 ajupazhamayil

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

orklah avatar May 10 '23 17:05 orklah

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

ajupazhamayil avatar May 24 '24 10:05 ajupazhamayil

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

orklah avatar May 24 '24 16:05 orklah

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

ojhaujjwal avatar Aug 15 '24 06:08 ojhaujjwal