Closed dkraczkowski closed 11 months ago
Update: I've removed #type: ignore
from your code but it still resolves to Any
:
reveal_type(Injectable[IRepository])
# Any
Possibly relevant: https://github.com/python/mypy/issues/11501
Update 2: seems like some metaclass magic could do the trick:
class M(type):
def __getitem__(self, typ: Type[I]) -> I:
return typ
class Injectable(Generic[I], metaclass=M): pass
reveal_type(Injectable[X])
# Revealed type is "def () -> kink.inject.Injectable[1.IRepository]"
That still has some issues, trying to find a workaround for it.
Okay, I think I've finally found a way to do this:
from typing import Generic, TypeVar
from typing_extensions import reveal_type
import inspect
class Foo:
pass
T = TypeVar('T')
class M(type):
def __getitem__(self, item: T) -> T:
return item
class Injectable(Generic[T], metaclass=M):
pass
def foo(a: str, b: Injectable[Foo]):
pass
# mypy:
reveal_type(foo) # Revealed type is "def (a: builtins.str, b: test.Injectable[test.Foo]) -> Any"
# runtime:
print(Injectable[Foo]) # <class '__main__.Foo'>
print(inspect.signature(foo)) # (a: str, b: __main__.Foo)
I think I finally managed to make it work.
Some changes are required though:
I = TypeVar('I')
if TYPE_CHECKING:
# Give mypy a generic version of `Injectable[I]` that also inherits `I`.
# Do not implement `__getattr__` to prevent downgrading `Injectable[I]` to `I`.
class Injectable(I, Generic[I]):
pass
else:
# Give python a working version of `Injectable[I]` that actually returns `I`.
# This will convert `Injectable[I]` to `I` during runtime which is compatible with inject logic.
# (Taken from your PR)
class _Injectable:
def __getitem__(self, item: Type[I]) -> I: # FIXME: Shouldn't this be Type[I] -> Type[I]?
return item # type: ignore
Injectable: TypeAlias = _Injectable() # type: ignore[misc]
# Alternative version using metaclasses:
class M(type):
def __getitem__(self, item: Type[I]) -> Type[I]:
return item
class Injectable(metaclass=M):
pass
Corresponding plugin code:
from mypy.nodes import ARG_OPT
from mypy.plugin import FunctionContext, Plugin
from mypy.types import Instance
# Set this to contain full names of corresponding items
INJECT_FULLNAME = 'kink.inject.inject'
INJECTABLE_FULLNAME = 'kink.inject.Injectable'
class KinkPlugin(Plugin):
def get_function_hook(self, fullname: str):
if fullname == INJECT_FULLNAME:
return self._inject_hook
return super().get_function_hook(fullname)
def _inject_hook(self, ctx: FunctionContext):
try:
func = ctx.arg_types[0][0]
except IndexError:
# FIXME: This is not an `@inject` form, but `@inject()`.
# Do nothing since we don't have the function signature yet.
return ctx.default_return_type
for i, (arg_name, arg_type) in enumerate(zip(func.arg_names, func.arg_types)):
# Check if argument is of type `Injectable[T]`
if (
arg_name not in ('self', 'cls')
and isinstance(arg_type, Instance)
and arg_type.type.fullname == INJECTABLE_FULLNAME
):
# Mark as optional & replace with inner arg
func.arg_kinds[i] = ARG_OPT
# func.arg_types[i] = arg_type.args[0] # Not necessary since Injectable[T] implements T
return func
def plugin(version: str):
return KinkPlugin
This still doesn't work with @inject()
, only with @inject
, but this could possible be worked around by https://docs.python.org/3/library/typing.html#typing.overload
What do you think?
@dkraczkowski I just gave it some more thought, and I think there's a more universal solution for this.
The problem with my idea of Injectable
as a type hint is that it requires plugins to work properly. While we may succeed at making this work for mypy, other tools like pylance & pylint will still complain about the signature. Feels like the type hint might have been a bad idea...
However, I was thinking of adding a different way to mark injected values, but with the help of injected
function with a slightly different purpose than Injectable
: by using default value rather than type hint. Consider this example:
T = TypeVar('T')
def injected(typ: Type[T]) -> T:
# Stub for setting default values of injected arguments to make checkers happy.
# This eliminates "missing argument" errors.
# This is totally optional - most users will not use this, it's for those who want
# to make mypy & pylint work properly.
pass
# ...
class UserService:
@inject()
def register_new_user(self, username, repo: IRepository = injected(IRepository)):
user = User(username)
repo.save_user(user)
This passes mypy
and pylint
checks. This is also fully backwards-compatible - it preserves raising of exceptions like kink.errors.execution_error.ExecutionError: Cannot execute function without required parameters.
.
Some additional changes might be required to def inject(...): ...
signature to properly work with both parametrized & bare decorator forms by using @overload
: https://docs.python.org/3/library/typing.html#typing.overload
Pros:
Injectable[T]
Injectable[T]
approach (does not trigger Liskov violation when, for example, UserService
extends another class that defines register_new_user
as def register_new_user(self, username): ...
)Cons:
typing.Protocol
(or other abstract classes): Only concrete class can be given where "Type[IRepository]" is expected
. However, this is currently being addressed:
--disable-error-code=type-abstract
will be available in the next release of mypy
)@and3rson For some reason, GitHub didn't notify me about your comment o_0. What if we use your first solution with a minor tweak:
from typing import _SpecialForm
@_SpecialForm
def _Injectable(self, parameters):
# Stolen from `typing.Optional`
arg = _type_check(parameters, f"{self} requires a single type.")
return arg
Injectable: TypeAlias = _Injectable # type: ignore
def foo(x: Injectable[str]):
pass
I think this should sort out mypy and work how we want it too. From my perspective, assigning a default value is not the best idea (not from my perspective, maybe I would need to get used to it). Other explorations were fascinating (thank you for that).
I also had a bit of a problem myself sorting it out, so it is practical and not disturbing.
Ping me in the issue if I don't respond, Thanks ;)
OK. I have checked the solution and seems like IDE is not going to regonize it and mypy is complaining a lot. I will think how to solve it differently.
@dkraczkowski JFYI - I've been using service: IService = injected(IService)
notation for the past few days and had a lot of luck making both mypy & pylint happy with it. I'm still not sure if it's the best solution and I agree with you on that, but it seems like it's really very portable. However it works well only with @inject()
form, not @inject
. I think that if kink had @overload
definitions for callable and non-callable versions of inject
decorator, it would make injected()
work even better.
Some docs I've found:
Furthermore, having @overload
would bring better typing to callable and non-callable versions of inject
decorator, so I think you could consider using them to provide better static typing for the decorator itself irregardless of which way we decide to proceed with the original issue.
P.S. I'm not in any way pushing you to accept my proposal with injected
- I'm only sharing my findings so far just in case they are helpful in your research. :)
Thanks for the time & efforts you're putting into this! Kink has become our go-to library for dependency injection in one of our new healthcare projects and is going to production next week!
@and3rson Thank you for the links I will have a look in my spare time. I am really happy to see another projects in production using kink, this is really an awesome news and I wish you all the best with deployment ;).
A couple of things: 1: how can I help move this along? I have time I can use to noodle this, but its been dead for about a month in this conversation and I don't want to waste work if progress has been made in the mean time. 2: I actually end up putting a comment everywhere we inject to make it explicit that we are injecting, maybe the default parameter isn't a bad thing? 2a: I am pretty sure this behavior is intended to be optional, thus backwards compatible, and only necessary if you want to use it.
@dkraczkowski @and3rson Any progress on this? Willing to help but don't want to duplicate effort and need to get this fixed.
@adamlogan73 I am happy to share the effort, lately I did not had much time to look into issues that the community is facing.
Awesome! Thanks @dkraczkowski!
I did some testing and, for some reason, mypy fails to properly see
Injectable[X]
as it is, and instead infers it asAny
, thus preventing the plugin from spotting injected args:When I was doing some local testing few days ago, I made my own
Injectable
by stealing thetyping.Optional
as follows:...however this makes mypy complain that
Function "1.Injectable" is not valid as a type
- weird, considering thattyping.Optional
has same implementation and works fine.Any thoughts on that?