Service fetched by interface has type of its concrete implementation alias, which is against LSP

[Wirone]

We encountered issue today (PHPStan 1.11.6 with bleeding edge + Symfony extension 1.4.5) that I consider violating the Liskov Substitution Principle. Consider simplified code:

services:
  # Interface
  Foo\ClientFactory: '@Foo\ApplicationClientFactory'

  # Implementation
  Foo\ApplicationClientFactory: ~

then, when we do something like:

/** @var \Foo\ClientFactory $factory */
$factory = $container->get(\Foo\ClientFactory::class);

we get an error:

PHPDoc tag @var with type Foo\ClientFactory is not subtype of type Foo\ApplicationClientFactory.

which does not make sense. We fetch ClientFactory from DI container, it's aliased to ApplicationClientFactory implementation that satisfies the interface, so of course interface is not a subtype, it's super type.

I don't want more specific type here, as the whole idea of interface is interchangeability. In the code, when I fetch ClientFactory (interface) I don't care what implementation comes from DI container, I only want this particular contract, because I don't want to encounter problems when DI alias is changed to other implementation. In our case ApplicationClientFactory has more methods that implemented ClientFactory interface, I don't want developers to be able to use these methods because PHPStan allows it (as it knows the aliased service), rather the other way around - I would expect that static analysis reports usage of methods not defined in the interface.

I believe it's a bug and PHPStan should not narrow the type here. Similar here, I believe I should be allowed to restrict the contract to interface instead of relying on extended implementation.

FYI: we don't use @var when we don't need, in this case it's only for IDE which does not have autocompletion for ClientFactory fetched from the container (since get()'s return type is ?object). We use dependency injection mostly, but unfortunately in legacy part of the code there is a lot of direct interaction with DI container.

Originally posted by @Wirone in #350 (comment)

[ondrejmirtes]

Hey, this is a tough one. We can't simply change it because there might be people out today relying on concrete implementations being returned when asked for an interface. So it'd be a BC break.

We usually pride ourselves of precise type inference. This one is done by https://github.com/phpstan/phpstan-symfony/blob/1.4.x/src/Type/Symfony/ServiceDynamicReturnTypeExtension.php.

It's great that PHPStan can know the exact service types and use them in code. It's also great that it forces "only useful" @var annotations - meaning it's going to tell you the annotation is not narrowing down the type. I stand firmly behind both of these features.

On one hand, I understand what you ask for. On the other hand, I don't see an easy solution for you.

I'm not even sure if Symfony has some special handling for class names or not. With a config like this:

services:
  # Interface
  Foo\ClientFactory: '@Foo\ApplicationClientFactory'

  # Implementation
  Foo\ApplicationClientFactory: ~

Would Symfony throw an exception if Foo\ApplicationClientFactory did not implement Foo\ClientFactory interface? Or are all of these just arbitrary names for services that happen to be same as class/interface names?

[ondrejmirtes]

Also please note you're getting this error:

 PHPDoc tag @var with type Foo\ClientFactory is not subtype of type Foo\ApplicationClientFactory.

Only because you have enabled reportWrongPhpDocTypeInVarTag: https://phpstan.org/config-reference#reportwrongphpdoctypeinvartag

Without this setting you'd only get this error against native types, like: https://phpstan.org/r/1c76b925-aecf-4b2c-95c7-84cfd1317fb1

[Wirone]

Thank you for the quick and descriptive response.

Hey, this is a tough one. We can't simply change it because there might be people out today relying on concrete implementations being returned when asked for an interface. So it'd be a BC break.

Is it possible to achieve this with some configuration flag / feature toggle?

Would Symfony throw an exception if Foo\ApplicationClientFactory did not implement Foo\ClientFactory interface? Or are all of these just arbitrary names for services that happen to be same as class/interface names?

I was pretty sure Symfony's cache:clear command would fail on it, but it does not (at least not in 6.4 we use) 😩. I've changed the aliased service to random class that does not implement the interface, and cache was compiled successfully. @nicolas-grekas is it something that should be addressed on Symfony's side or maybe was improved in 7.x?

[nicolas-grekas]

The role of the DI component is not to validate your config. The PHP engines does that.
You might want to use the lint:container command for that.

[ondrejmirtes]

One more point I want to make here - you can completely avoid this problem if you stop using DI container as a service locator (which is an anti-pattern). If you really want to have clean code, you should ask for ClientFactory in the constructor instead. PHPStan is not going to complain about that, and it's going to keep ClientFactory as the type.

[Wirone]

Thanks @nicolas-grekas for fast response. Yeah, the lint:container command reports such mismatch:

$ bin/console lint:container

 [ERROR] Invalid definition for service "Foo\Bar": argument 2 of "Foo\Bar::__construct()" accepts "Foo\ClientFactory", "Foo\Baz" passed.

Thankfully we have this in our CI setup so we won't let such invalid DI definitions sneak into our codebase 🙂.

One more point I want to make here - you can completely avoid this problem if you stop using DI container as a service locator (which is an anti-pattern). If you really want to have clean code, you should ask for ClientFactory in the constructor instead. PHPStan is not going to complain about that, and it's going to keep ClientFactory as the type.

@ondrejmirtes I completely agree and am aware of it, but as I stated in the issue - this is legacy part of our huge app and it's impossible to get rid of direct container usage just like that. We work on it step by step, but there's too much of a work around that.

[ondrejmirtes]

Invalid definition for service

That doesn't actually sound like the error I'd expect.

What does lint:container say about these situations?

services:
  # Interface
  Foo\NonexistentClass: '@Foo\ApplicationClientFactory'

And:

services:
  # Interface
  Foo\InterfaceThatExistsButApplicationClientFactoryDoesNotImplementIt: '@Foo\ApplicationClientFactory'

[Wirone]

There is not any error for both, as service name can be anything, FQNs are just used for convenience (if I remember correctly service name is used as a fallback when class is not provided explicitly). The error would be encountered if Foo\NonexistentClass or Foo\InterfaceThatExistsButApplicationClientFactoryDoesNotImplementIt would have been used as arguments for other service definition and Foo\ApplicationClientFactory service would not match that service's signature. Exactly what happened for Foo\Bar above, which expected Foo\ClientFactory but service alias was resolved to the service based on class that does not implement such interface. IMHO "Invalid definition for service" is correct here, even though the root cause lays in the invalid alias for one of the dependencies of that service.

[ondrejmirtes]

So PHPStan can't assume anything about names or class hierarchy here. The DIC is simply not designed for that.

You can name your services anything. It'd be wrong to infer service fetched by "ClientFactory" name to implement "ClientFactory" interface and not go more specific with the service type.

You can override what phpstan-symfony does here with this extension: https://apiref.phpstan.org/1.11.x/PHPStan.Type.ExpressionTypeResolverExtension.html

But instead what I'd recommend is some kind of wrapper class for your container where you can write your own getByClass method with a simple generic signature that will make PHPStan infer the type you want. From class-string<T> in a parameter to T in the return type.

[Wirone]

So PHPStan can't assume anything about names or class hierarchy here. The DIC is simply not designed for that.

Hmmmm, I am not sure about that. Container's approach aside, PHPStan's extension gets service's name and resolves the type using the cached container. I believe PHPStan could verify if service's name is a FQN of an interface, and if the resolved type implements this interface, then interface could be returned instead of more specific type of an implementation. Are there any technical difficulties to achieve this?

[ondrejmirtes]

PHPStan could technically give a meaning to something that doesn't have said meaning, but why would it though? It'd just cause issues being reported by people that expect the opposite to happen.

I gave you some ideas for solutions on your side already. But this isn't in phpstan-symfony's interest to change.

Anyway, this is likely my last response on this issue, I already spent a lot of effort and mental energy on it so don't expect any more from me. Thanks for understanding.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.