There is strictly no guarantee in the ContainerInterface that a service class is a subtype of the object id, even when the object id is a class-string. This PR adds this in the contract of the interface, which is a BC break to me (btw, it might force us to forbid some kind of service decoration to ensure this is true)

I agree with @stof here. We decided against using PHPdoc types that do not represent the 100% use-case, even if it improves the experience for 95% There is no guarantee that there is a direct relation between service ID and object type, even though it's a common convention that there is.

So 👎 from me.

There is strictly no guarantee in the ContainerInterface that a service class is a subtype of the object id, even when the object id is a class-string. This PR adds this in the contract of the interface, which is a BC break to me (btw, it might force us to forbid some kind of service decoration to ensure this is true)

Indeed, but I thought it would be okay since it's only PHPDoc.

But since it increase the DX a lot in test env, I thought it would be okay.

If we add a sentence, to explain is not 100% accurate, would it be okay?

Indeed, but I thought it would be okay since it's only PHPDoc.

Our phpdoc in our interfaces is considered as part of the defined contract (it is taken into account when deciding whether the change we do is a BC break or no)

If we add a sentence, to explain is not 100% accurate, would it be okay?

still -1 to me, because of the previous point.

okay, let's close it then

Oh, there was a race condition here. I close the PR at the same time of last comment from @nicolas-grekas. Let's re-open

Better than that: I'm fine if ppl that use a FQCN identifier without returning a compatible instance get a SA warning.

They would not. they might get a runtime error because the runtime type does not match what the SA thinks the type was. Static analysis won't report errors on the service definition file if not respecting that rule (as it does not even analyze those XML/Yaml files) and the original service definition might even respect the rule without the final one respecting it.

I don't think your interpretation of the BC policy is the one we're really sticking to. At least I don't agree with this argument when considering the current proposal. To me its totally fine and desired I'd say.

They would not. they might get a runtime error because the runtime type does not match what the SA thinks the type was. Static analysis won't report errors on the service definition file if not respecting that rule (as it does not even analyze those XML/Yaml files) and the original service definition might even respect the rule without the final one respecting it.

This is reverse thinking: the current declaration doesn't provide anything like that either, so the new one won't provide more guarantees. The DX improvement is obvious, the SA downside is none in practice.

The new phpdoc explicitly says "if the id looks like a class-string, the method MUST return an object of that type (or null)". That's definitely a stricter contract for the interface than saying "the method MUST return an object (or null)".

And phpdoc types have always been part of our BC policy (otherwise, adding native types would have been a BC break for callers of our methods due to narrowing the parameter types)

The BC policy is designed to allow people to upgrade seamlessly. Seamlessly "means without us breaking their app". Yes we consider types in phpdocs, as guides toward what should be supported. We also change them at will when needed, provided this doesn't create any execution-time side effect - which is the core criteria when BC is concerned.
Here, this is not a BC break: nothing changes. Also: no, we don't have to enforce phpdoc, so a runtime chek using eg service decoration wouldn't be required - because it's not our job to call something correctly. Types are just nicer if they help sort out not-working cases, but they're certainly not mandatory (we didn't have them before and we still delivered.)

So to me, DX clearly wins here.

@nicolas-grekas interface implementations have to respect the contract defined in the interface. That's where the BC break is when making the interface stricter (and with this PR, we would end up allowing to instantiate a Container that does not respect ContainerInterface depending on how services are configured)

The "allowing" part is not our business, same as one can wire a logger into a service argument that needs a serializer: "PHP" will throw, not "us". People are free to run whatever works for them. Declaring the intent is enough on our side, and will always be. Then no, no BC break here, because no exec-time side effect...

Implementations of this interface will get static analysis errors if they don't guarantee a correct return object type if a class FQCN is passed. That's a regression from the current state. Still, I don't think we consider static analysis errors a BC break (otherwise, any additional PHPdoc type is a BC break as it could lead to new static analysis errors).

However, this does change how we treat PHPdoc types. If we think this is OK, there are more types that we welcome from the phpstan/psalm stubs. We rejected these in the past, as they didn't represent the 100% truth (#47016). E.g. the stubbed form types which are a bigger elephpant in the room.

@nicolas-grekas the big difference is that in this case, the class that won't respect its interface is our class. So the faulty code would be ours.

We're not responsible for how people configure their containers, really.

Adding a decorator on a service that was registered using its class name as id (the default convention) is enough to make the service invalid regarding this new rule (the dev intent is probably to decorate based on the interface of the class rather than the class itself). And this does not require weird wiring of the container, just using one of its flagship features.

Sure, but still: the decorator has to act like the real objects, so the API will still be the same. SA tools won't be able to notice anyway, too many indirections. And if they do, ppl can decorate the common interface that the decorated and decorator must share.

@nicolas-grekas if the code is written against the common interface, things indeed work. But SA will tell you that the type you get is MyImplementation::class and won't complain if I pass it to a method that has MyImplementation as parameter type instead of the common interface (which will then fail when running it as the type is MyDecorator).

When installing the phpstan-symfony extension, phpstan will infer the type of $container->get() calls based on the actual configuration of the container (it inspects the dumped XML file in the cache), which solves the use case of @lyrixx regarding tests without introducing false positive for static analysis and without compromising the contract of the interface.

That's a very good point. All static analysers are using our XML dump of the container, which we make specifically for this use-case: mapping magic service ID strings to PHP object information.
I don't see a reason to merge a less ideal solution for a problem that is already fixed.

I don't know about the extension, but is the extension able to cope with scoped containers?
Because having the extension know about the main DI one doesn't really help DX, since this container shouldn't be used ANYWHERE (capital letter so that ppl that do so realize it's plain wrong 99,99% of the time.)

On the other end, scoped containers are a thing ppl can use at will. Yet how are they supported by the extension, DX wise?

@lyrixx maybe you'd have some hints on these questions?

@nicolas-grekas you seem to say that using a concrete class name as service id breaks IOC. but that's the default behavior of Symfony.

When using AutowireLocator you mean, sure. Still, it's a matter of vision. Your reasoning makes sense in a world where types are reified. Mine works in an erased types world. I prefer erased types: they're good enough to solve the purpose of types in a language like PHP.

I'm 👎 on this for the same reasons as stof and wouter (3 downvotes). This takes a too big shortcut for a too small benefit as this only matters in tests. Not worth the hack/wrong assumption IMHO

What do you think about changing phpdoc to rely on interfaces then?

https://psalm.dev/r/2c3a6cac5a

I don't know whether IDE will handle this properly, and why for now Psalm resolves class-string as interface-string too, but this will be the most safe approarch in my opinion

<?php

interface Container
{
    /**
     * @template T
     * @template V
     *
     * @param T $id
     *
     * @return T is interface-string<V> ? V : mixed
     */
     public function get(string $id): mixed;   
}

$container = container();

/** @psalm-trace $interfaceInstance */
$interfaceInstance = $container->get(\ArrayAccess::class); // expected - `\ArrayAccess`

/** @psalm-trace $classInstance */
$classInstance = $container->get(\stdClass::class); // expected - `mixed`

function container(): Container
{
   throw new \InvalidArgumentException('Not necessary');   
}

@chalasr

benefit as this only matters in tests.

You missed Service Locator

@lyrixx I rather use the lowest possible abstraction which is the PSR interface, as in the docs and every locator of our codebase

I don't think C|object will help much, as this would get normalized to object

I don't think C|object will help much, as this would get normalized to object

I think C|object is the answer to @lyrixx previous comment:

What could solve my issue is something not interpreted by SA but only by IDE.

C|object does

I updated the PR with @nicolas-grekas proposal. It seems good. Now I have autocomplete, and we don't mess with SA

Thank you all for the discussion!

Thank you @lyrixx.