psalm icon indicating copy to clipboard operation
psalm copied to clipboard

Psalm is not able to infer types from doc blocks of Google\Protobuf with 'protobuf' extension installed

Open benkelukas opened this issue 2 years ago • 5 comments

Hi, I have the protobuf extension installed in container in which I run Psalm, this causes psalm to not be able to infer types from Google\Protobuf classes, resulting in MixedAssignment and other errors, please see example below:

ERROR: MixedAssignment - index.php:3:1 - Unable to determine the type that $t is being assigned to (see https://psalm.dev/032)
$t = (new \Google\Protobuf\Timestamp())->setSeconds(0)->setNanos(0);

If I remove the protobuf extension, Psalm no longer reports those errors.

I've created a repository with reproducible example here: https://github.com/benkelukas/psalm-google-protobuf-issue

For now, I'm using container without protobuf for Psalm, but somebody might not have the options, so wanted to leave this here in case somebody else has this weird problem.

I've done some digging as well, and from my very much limited knowledge of Psalm, it seems that with the protobuf extension, the Google\Protobuf classes are not located via composer, but analyzed using reflection instead, and these classes do not have return types specified in actual code, but rather via annotations, so maybe that is what is breaking this? If you compare output of --debug-by-line with the extension and without it is different when processing the Google\Protobuf classes.

Some info on my environment:

  • Psalm version: 4.24.0
  • PHP: 8.1.1

benkelukas avatar Jul 18 '22 13:07 benkelukas

Hey @benkelukas, can you reproduce the issue on https://psalm.dev ?

psalm-github-bot[bot] avatar Jul 18 '22 13:07 psalm-github-bot[bot]

If I get this right, you can either use composer to install the tool OR use an extension in PHP, correct?

So yeah, when you use the extension, the classes are not included and you don't get types from reflection due to the lack of support in the extension (I suppose).

This could be solved by adding a stub for Protobuf in Psalm to let it know what are the types. This should not be very hard to do if the types are documented in the actual composer classes. What would need to be done is take all those class, empty method contents and just put all that in a file that would serve as a stub.

Are you up for it?

orklah avatar Jul 18 '22 16:07 orklah

@orklah Not sure if that really is the issue, the \Google\Protobuf\Timestamp class comes from the "google/protobuf": "^3.21" composer package, that is always installed, regardless if the protobuf extension is installed or not.

Problem is, when I install the protobuf extension, Psalm no longer parses the Google\Protobuf\Timestamp when I use it in my code (index.php in the example repo) and reports that it cannot infer type for the variable $t which contains an instance of Google\Protobuf\Timestamp.

When I uninstall the protobuf extension, Psalm no longer reports that error and infers the types correctly.

Hope I understood You correctly, please correct me if not :)

Thanks

benkelukas avatar Jul 19 '22 06:07 benkelukas

I think you could drop the composer require for google/protobuf when installing the extension because the API is actually included in the extension itself.

According to the doc: Both implementations provide the same runtime APIs and share the same generated code. Which means when you provide both, I'm guessing the extension declares every symbol so composer is never called to retrieve the php classes in vendor. Unfortunately, using reflection on the extension doesn't provide the same details as when psalm has the classes with documentation, hence the need for a stub.

You should be able to check what I'm saying by running psalm with --debug-by-line and checking if Psalm actually retrieve classes in vendor when you have the extension

orklah avatar Jul 19 '22 18:07 orklah

I see, yes, I've run both cases with --debug-by-line and with the extension installed, the Google\Protobuf\* classes are not retrieved from vendor, the log says Using reflection to load metadata for ...

I can work on those stubs, will submit PR when ready, thanks! :)

benkelukas avatar Jul 20 '22 05:07 benkelukas