Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to bind Annotated to instances of origin #174

Open
pstatix opened this issue Jan 5, 2021 · 6 comments
Open

Unable to bind Annotated to instances of origin #174

pstatix opened this issue Jan 5, 2021 · 6 comments

Comments

@pstatix
Copy link

pstatix commented Jan 5, 2021

Originally stemming from interest in #133 , I too thought to replicate Guice's @Named convention. In doing so, I originally used the typing.NewType (prior to having even seen the aforementioned issue) and succeeded. However, instantiating a type for each simple alias, I thought typing.Annotated (albeit, using typing_extensions.Annotated) would be more idiomatic.

Given a simple example:

import pathlib
import injector

ConfigFile = injector.Annotated[pathlib.WindowsPath, 'config']

cfg_file = pathlib.Path.home() / 'settings.cfg'


class AppModule(injector.Module):

    def configure(self, binder):
        binder.bind(ConfigFile, to=cfg_file) # Guice uses annotatedWith here

class App:

    @injector.inject
    def __init__(self, cfg: ConfigFile):
        self.config = cfg

app = injector.Injector(AppModule).get(App)

...throws...

Traceback (most recent call last):
  File "E:\Programming\test\test_injector\test.py", line 20, in <module>
    app = injector.Injector(AppModule).get(App)
  File "C:\Python37\lib\site-packages\injector\__init__.py", line 913, in __init__
    self.binder.install(module)
  File "C:\Python37\lib\site-packages\injector\__init__.py", line 577, in install
    instance(self)
  File "C:\Python37\lib\site-packages\injector\__init__.py", line 860, in __call__
    self.configure(binder)
  File "E:\Programming\test\test_injector\test.py", line 12, in configure
    binder.bind(ConfigFile, to=cfg_file) # Guice uses annotatedWith here
  File "C:\Python37\lib\site-packages\injector\__init__.py", line 479, in bind
    self._bindings[interface] = self.create_binding(interface, to, scope)
  File "C:\Python37\lib\site-packages\injector\__init__.py", line 582, in create_binding
    provider = self.provider_for(interface, to)
  File "C:\Python37\lib\site-packages\injector\__init__.py", line 644, in provider_for
    raise UnknownProvider('couldn\'t determine provider for %r to %r' % (interface, to))
injector.UnknownProvider: couldn't determine provider for typing_extensions.Annotated[pathlib.WindowsPath, 'config'] to WindowsPath('C:/Users/Paul/settings.cfg')

My belief is that provider_for does not adequately check to see if the interface is an _AnnotatedAlias or not. The origin variable resolves to the base interface correctly, in this case pathlib.WindowsPath, but since base_type is typing_extenstions.Annotated, not check is made given that case.

@jstasiak
Copy link
Collaborator

jstasiak commented Jan 5, 2021

Ah yeah, I haven't considered handling typing.Annotated in this context yet and it's totally unsupported.

It's probably worth exploring what possible uses of Annotated could there be other than this one (a kinda-new-type opaque to Injector), but this seems like a reasonable use case.

@pstatix
Copy link
Author

pstatix commented Jan 5, 2021

One can, however, use typing.Annotated interfaces so long as the @provider decoration is used in the injector.Module:

import pathlib
import injector

ConfigFile = injector.Annotated[pathlib.WindowsPath, 'config']

cfg_file = pathlib.Path.home() / 'settings.cfg'


class AppModule(injector.Module):

    @singleton
    @prodiver
    def provides_config_file(self) -> ConfigFile:
        return cfg_file

class App:

    @injector.inject
    def __init__(self, cfg: ConfigFile):
        self.config = cfg

app = injector.Injector(AppModule).get(App)
print(app.config)

# C:\Users\<me>\settings.cfg

However, for simply annotating multiple instances of the same type (e.g. a file interface [app config, user config, network config, etc.]), using the bind() API would be idiomatic and closer to the Guice semantics.

@jstasiak
Copy link
Collaborator

jstasiak commented Jan 5, 2021

Oh, really? That's interesting, those should have the same effect. Also I have a feeling that due to Annotated-stripping in the Injector internals this won't distinguish between typing.Annotated[pathlib.Path] and plain pathlib.Path. I seem to be correct:

% cat asd.py
import pathlib
import injector

ConfigFile = injector.Annotated[pathlib.Path, 'config']
OtherConfigFile = injector.Annotated[pathlib.Path, 'other_config']

cfg_file = pathlib.Path.home() / 'settings.cfg'
other_cfg_file = pathlib.Path.home() / 'settings.cfg' / 'other'


class AppModule(injector.Module):

    @injector.singleton
    @injector.provider
    def provides_config_file(self) -> ConfigFile:
        return cfg_file

    @injector.singleton
    @injector.provider
    def provides_other_config_file(self) -> OtherConfigFile:
        return other_cfg_file

class App:
    @injector.inject
    def __init__(self, cfg: ConfigFile, other_cfg: OtherConfigFile) -> None:
        self.cfg = cfg
        self.other_cfg = other_cfg

app = injector.Injector(AppModule).get(App)
print(app.cfg)
print(app.other_cfg)
% python asd.py
/Users/user/settings.cfg/other
/Users/user/settings.cfg/other

With this in mind I'm tempted to add a big warning to the documentation to inform people about this for the time being.

Bonus: Injector(...).get(ConfigFile) will fail with the same error you provided in the original post.

(edit: originally both provided methods returned ConfigFile, this was a mistake, the example is still correct)

@pstatix
Copy link
Author

pstatix commented Jan 5, 2021

Correct. Using typing.NewType however results in "aligned" semantics to @Named (which one would think is easily achievable with typing.Annotated):

import typing
import pathlib
import injector

UserFile = typing.NewType('UserFile', pathlib.WindowsPath)
ConfigFile = typing.NewType('ConfigFile', pathlib.WindowsPath)

usr_file = pathlib.Path.home() / 'user.dat'
cfg_file = pathlib.Path.home() / 'settings.cfg'


class AppModule(injector.Module):

    def configure(self, binder):
        binder.bind(UserFile, to=usr_file)
        binder.bind(ConfigFile, to=cfg_file)

class App:

    @injector.inject
    def __init__(self, cfg: ConfigFile, usr: UserFile):
        self.user = usr
        self.config = cfg

app = injector.Injector(AppModule).get(App)
print(app.config)
print(app.user)

# C:\Users\<me>\settings.cfg
# C:\Users\<me>\user.dat

I believe such integration is achievable still in the provider_for method as base_type maintains the reference to the _AnnotatedAlias. The _is_specialized method could also be extended since it already has conditionals for annotations.

@jstasiak
Copy link
Collaborator

jstasiak commented Jan 5, 2021

Yes, absolutely. I'd like to consider other options before allowing arbitrary, opaque Annotated instances in context like this, mixing with Inject/NoInject etc., the design space here is not that small possibly.

@strangemonad
Copy link

strangemonad commented Mar 28, 2023

FYI, calling out a technically nit that I mentioned in #133 about NewType vs type aliases (#133 (comment)). I don't think it's right to equate the behavior of @named and NewType. NewType ends up being a different key because it's actually a new type so it would break cases where a caller wants instances of the same type but annotated differently.

For instance, in the example above (I know it was just meant as a motivating example) the binder.bind(UserFile, to=usr_file) passes type checking because to:Any when it really shouldn't since you're binding a pathlib.Path to a UserFile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants