phptools-docs icon indicating copy to clipboard operation
phptools-docs copied to clipboard

PHPStan generic arguments not working

Open mindplay-dk opened this issue 1 year ago • 3 comments

The generics by example article on phpstan.org includes the following example of generic return types:

class Container
{
	/**
	 * @template T of object
	 * @return ($name is class-string<T> ? T : object)
	 */
	public function get(string $name): object
	{
		// ...
	}
}

This doesn't work as expected.

Here is an example with the types reported by PHPStan:

<?php

class Container
{
	/**
	 * @template T of object
	 * @param string $name component name
	 * @return ($name is class-string<T> ? T : mixed)
	 */
	public function get(string $name): mixed
	{
		// ...
	}
}

class MyService {}

function hello(NotMyService $service) {
	// ...
}

$c = new Container();

$service = $c->get(MyService::class);

\PHPStan\dumpType($service); // 👈 MyService 👍

$stuff = $c->get("stuff");

\PHPStan\dumpType($stuff); // 👈 mixed 👍

The same example returns different hover types (and type-checks) with your extension:

$service = $c->get(MyService::class); // 👈 mixed|T 👎

$stuff = $c->get("stuff"); // 👈 mixed|T 👎

Is there any particular reason you're inventing your own type inference and type-checking in you product?

PHPStan is open source and MIT licensed and extremely good at inference and type-checking. 🙂

mindplay-dk avatar Apr 06 '24 10:04 mindplay-dk

Thank you for reporting this misbehavior, the detailed test case, and your suggestions. I appreciate it.

This is an excellent question. There are a few technical reasons why our PHP extension (former PHP Tools for Visual Studio) is not based on PHPStan. I'm listing a few of them because it's worth summarizing this somewhere .. maybe I'll put it on our blog.

  • We invented/implemented this type-inferring engine about 16 years ago, long before PHPStan, for our commercial clients of a different product. Based on the theory of language compilers and practical implementations from C/C++ languages.
  • PHPStan runs on PHP runtime, while our product is based on the C#/.NET platform.
  • Our engine needs to handle not only PHPStan special tags and semantics (used by <20% of our user base), but also Psalm, WordPress annotations, MediaWiki annotations, PhpStorm annotations and metadata, and tons of custom and user-specific "weird" annotations.
  • We need continuous type-inferring and analysis of incomplete syntax trees in <50 milliseconds.
  • We need code completion and type-inferring based on "heuristics".
  • Our engine is built for extreme performances, multithreaded, where we're able to analyze the entire frameworks (i.e. Laravel) in 3 to 12 seconds with a low memory footprint.
  • We need to integrate the type-inferring with the rest of IDE - refactorings, mouse hovers, inlay hints, code actions, diagnostics, debug inlines, debug tooltips, debug completions,, AI suggestions, .... and we need it in-process and fast.

Anyways; back to the issue. The generic arguments are (in general) supported. We'll implement this kind of conditional return semantic as well. Currently, we support the following scenario (using @param class-string<T> $name).

       /**
	 * @template T
	 * @param string|class-string<T> $name
	 * @return T|object
	 */
	public function get($name) {}

jakubmisek avatar Apr 06 '24 13:04 jakubmisek

Cool, you clearly head plenty of good reasons to go this route. 😄

Unfortunately, the example you showed here does not work as expected:

$lol = $container->get("lol"); // lol|object

$service = $container->get(CacheProvider::class); // CacheProvider|object

It does not work with PHPStan either. (I use PHPStan in CI, so it decides what's "correct".)

Do you know what's more popular with your customers, PHPStan, Psalm or Phan?

It's frustrating there are so many conflicting standards - must make things very difficult for you. For what it's worth, there was talk in the FIG forum last week of reviving PSR-5 and standardizing the doc-blocks. This could take years though...

mindplay-dk avatar Apr 06 '24 20:04 mindplay-dk

Good point - we don't validate whether the string is an existing class name - will be done.

More popular used to be Psalm. The most popular is "nothing specific".

We'll try to be as close to phpstan as possible. Standardizing doc blocks would be a way to go, Still, we need to support tons of weird legacy code :)

jakubmisek avatar Apr 08 '24 13:04 jakubmisek

Implemented in 1.46.15428 (pre-release). Full release will be prepared in about a week.

Thanks!

jakubmisek avatar May 14 '24 16:05 jakubmisek

With v1.46.15428, the example I posted still doesn't work.

Your example now sort of works, but it yields a return type of MyType|object, which isn't exactly correct.

Also, as mentioned, that syntax doesn't work with PHPStan.

A conditional return-type is what's required here - so that, if $name is a class-string, it should return that type, or else the type is mixed.

By the way, I'm not married to PHPStan - completely open to using something else, if it offers a way to do this. But PHPStan does appear to have the most advanced type-hinting syntax? I haven't seen any other type-hinting tool that will let me accurately type-hint this return-type. (?)

It really would help if there were any actual standards for this stuff. Getting something that works both with an IDE and in CI is sort of a jungle right now...

mindplay-dk avatar May 15 '24 08:05 mindplay-dk

With v1.46.15428, the example I posted still doesn't work.

Your example now sort of works, but it yields a return type of MyType|object, which isn't exactly correct.

Also, as mentioned, that syntax doesn't work with PHPStan.

A conditional return-type is what's required here - so that, if $name is a class-string, it should return that type, or else the type is mixed.

By the way, I'm not married to PHPStan - completely open to using something else, if it offers a way to do this. But PHPStan does appear to have the most advanced type-hinting syntax? I haven't seen any other type-hinting tool that will let me accurately type-hint this return-type. (?)

It really would help if there were any actual standards for this stuff. Getting something that works both with an IDE and in CI is sort of a jungle right now...

This is expected behavior, conditional return types are often too complex to to accurately infer the data type, so union type will be returned. Our main goal is for the LSP to be able to suggest code, so sometimes it's better to have extra rather than missing. The conditional return type is actually quite complex because it requires the LSP to remember the actual values of variables, which is very complicated and affects code suggestion performance.

ging-dev avatar May 15 '24 12:05 ging-dev

Thank you for the comments. There might be some cases we can do better. As a fallback, as @ging-dev mentioned, the LS rather uses union of possible types so user is still getting some code completion.

Our test for this issue is below. I'd be happy to add/implement more special cases here.

image

We're also happy with the PHPStan/Psalm syntaxes. There is no reason to (re)invent something that is already there and well documented.

jakubmisek avatar May 16 '24 09:05 jakubmisek

Our main goal is for the LSP to be able to suggest code, so sometimes it's better to have extra rather than missing.

As a fallback, as @ging-dev mentioned, the LS rather uses union of possible types so user is still getting some code completion.

I'm not sure I follow.

Do you prioritize getting more auto-completions over getting stricter type checks?

This would seem to imply that some of those auto-completions would be wrong - what's the usefulness of getting more auto-completions if the additional ones are just wrong, and could have been avoided with stricter type-checking?

I understand that not everything is provable, some things may not calculable in a reasonable time frame, and so on - but in my example, the correct (narrower) type is locally calculable, is it not?

(PHPStan is able to do it, and afaik has similar performance constraints.)

mindplay-dk avatar May 18 '24 13:05 mindplay-dk