nette/di

Invalid type 'Nette\DI\?T' when accessing `Container#getByType()` from DI config

Closed this issue · 23 comments

dakur commented

Version: 3.0.11, bug present as of 3.0.10

Bug Description

If you use Container#getByType() inside of NEON configuration, it fails on resolving its return type – from Helpers#getReturnTypeAnnotation() it is returned Nette\DI\?T.

More generally, I believe (have not tried) accessing any class with method having generic return type will fail.

Steps To Reproduce

In nette/web-project, create this class:

namespace App;
final class FakeContainer
{
	/**
	 * copied from Container#getByType()
	 * @template T
	 * @param class-string<T> $type
	 * @return ?T
	 */
	public function getService(string $type)
	{
		return null;
	}
}

In services.neon, use it:

services:
	- App\FakeContainer::getService(HomepagePresenter::class)

image

Expected Behavior

Run, not to fail with type check.

dakur commented

Note that if you change @return ?T to @return T, it fails with different error. I think that is related.

image

milo commented

What if you declare it as following?

public function getService(string $type): object
{}
dakur commented

That doesn't seem to help..

Return type of App\FakeContainer::getService() is not expected to be nullable/union/intersection/built-in, 'object' given

image

This seems like expected behavior, generics are not supported.

dg commented

DI doesn't support generics and I don't plan to implement it, in fact I plan to remove support for @return annotation completely in version 4. But you can specify the type directly in the config:

services:
	- factory: App\FakeContainer::getService(HomepagePresenter::class)
	  type: HomepagePresenter
mabar commented

Is there any reason to not support object?

dg commented

It is always object.

mabar commented

I mean it's bit confusing from the user's point of view that object is not allowed return type of method. But in reality it's okay and we just have to declare more specific type in neon because library is not able to infer it from generics itself. Some hint like "I don't understand object / Foo&Bar / ... type, could you change the type to some specific class or add the type to service definition?" would be helpful

dg commented

It's not that it doesn't understand it, it understands and doesn't allow it. So return type is not expected to be nullable/union/intersection/built-in type.

dakur commented

@dg Thank you, but the suggestion did not help for the example code – it fails with Return type of App\FakeContainer::getService() is not expected to be nullable/union/intersection/built-in, 'object' given Invalid App\?T* (sorry, the long message was for forced object return type)

It didn't help in real code either as I already have type already set there. So maybe there is also bug in type not making any effect?

For illustration, I attach real code:

pages.filesystem:
	type: League\Flysystem\Filesystem
	factory: @outerContainer::getByType(PageFilesystemProvider)::getPageFilesystem()
	autowired: false

Just an explanation of our use-case: we have app and model container – app is outer, model inner. App can use services from inner (model) container, but model container needs some bindings to the app world – e.g. translator, identity etc. – thus this @outerContainer::getByType() calls.

I understand that such usage of DI can be a bit marginal/edge-case (🤷‍♂️❓), but I still think it reveals some bug(s) in DI resolver/type checking.


* The message actually changed from Nette\DI\ServiceCreationException: Service (App\FakeContainer::getService()): Invalid type 'App\?T' (reported in the original post) to Nette\InvalidArgumentException: Invalid type 'App\?T' so it must failed at different place.

dg commented

Yeah, I finally get it, the problem is the throwing of the exception.

So it should work that if the return type is 'object', it will ignore the @return annotation and 'object' too, right?

dakur commented

So it should work that if the return type is 'object', it will ignore the @return annotation and 'object' too, right?

Not pretty sure. I don't have such knowledge of DI to tell if this is solution to the root cause, sorry. I'm not even sure I know what is the root cause actually.

But yeah, the problem is that it throws exception instead of returning HomepagePresenter/PageFilesystemProvider instance.

dg commented

Try v3.0-dev, now is should work.

final class FakeContainer
{
	/**
	 * @template T
	 * @param class-string<T> $type
	 * @return ?T             
	 */
	public function getService(string $type): ?object  // @return ?T is ignored now
	{
	}
}
dakur commented

@dg I'm sorry, but it's still the same. From the referenced commit, I don't even understand how it should help as there was just added one failing scenario.

But if you plan remove generic annotation from Container#getByType() in v4 (especially the @return ?T line), that helps, so I can wait with update until it is published.

dakur commented

@dg I hope I can describe it better now. The problem is that if I call Container#getByType() in NEON configuration, it fails with Invalid type 'Nette\DI\?T' instead of returning the requested service. The cause is that Container#getByType() has generic @return annotation.

You said you plan to remove it, is that still true? I can still see it in master and I'm stuck because of this bug on 3.0.9.

dg commented

Try to create some minimal project, I really don't understand what is not working for you.

Btw what is the hash in Container#getByType()?

dakur commented

Here you go: https://github.com/dakur/nette-di-274

Clone it, composer install, run webserver and open in browser. You see the message. If you remove return annotation from Container#getByType(), it starts working.

(I run it on php 8.1, don't know about 8.2)

Btw what is the hash in Container#getByType()?

What do you mean by hash?

dg commented

I mean # in Container#getByType() Does it have any special meaning?

dakur commented

I think it's a way how to refer to a class method without associating it with static or non-static usage. But I could write Container::getByType() as well.

dg commented

Oh, I've never seen that before. I see :: as way how to refer to a class method without associating it with static or non-static usage. Then it's just a matter of context.

dg commented

Oh, I see what's going on. The problem is the calling of another method, when DI tries to evaluate over which object is called

services:
	- App\FakeContainer::getService(HomepagePresenter::class)::anotherMethod()

It should be working now in both 3.0-dev and 3.1-dev.

mabar commented

Evaluation is done to call the method properly, right?

It would be nice to have PHP-like syntax for constants/properties/(static) methods in neon.
e.g. @Nette\Http\IRequest->getUrl()->getBasePath() instead of @Nette\Http\IRequest::getUrl()::getBasePath.
DI would not have to evaluate these calls.

(sorry for asking for complicated changes)

dakur commented

Looks like it works, thank you!