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

Generic classes narrowed by @overloaded __init__ #680

Closed
srittau opened this issue Oct 14, 2019 · 8 comments · Fixed by python/mypy#7860
Closed

Generic classes narrowed by @overloaded __init__ #680

srittau opened this issue Oct 14, 2019 · 8 comments · Fixed by python/mypy#7860

Comments

@srittau
Copy link
Collaborator

srittau commented Oct 14, 2019

Quite often in typeshed we have the case that a generic class can be narrowed by the arguments provided to __init__. One example are file classes that are generic over AnyStr, where a mode argument narrows the type. It would be useful if this could be described somehow in typing, possibly by extending the @overload decorator or with some other mechanism.

@srittau
Copy link
Collaborator Author

srittau commented Oct 14, 2019

One idea:

class MyFile(Generic[AnyStr]):
    @overload(narrow_generic_0=str)
    def __init__(self, mode: Literal["r", "w"] = ...) -> None: ...
    @overload(narrow_generic_0=bytes)
    def __init__(self, mode: Literal["rb", "wb", "b"]) -> None: ...
    @overload  # AnyStr is not narrowed
    def __init__(self, mode: str) -> None: ...

@JelleZijlstra
Copy link
Member

I think this is doable by overloading __new__, like we implemented recently for subprocess.Popen.

@srittau
Copy link
Collaborator Author

srittau commented Oct 14, 2019

That's a nice trick for stubs! I think something like this would be still be useful, though:

  • For implementation files where the __new__ trick can't be used.
  • As a more obvious/non-hacky alternative.
  • To make stubs better reflect reality (by not including __new__).

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Oct 14, 2019

I think this is doable by overloading __new__, like we implemented recently for subprocess.Popen.

Yes, this is however rather a workaround than a real fix. This can cause troubles if an actual class uses __init__ at runtime. Recently there was a bug report caused by this, see python/typeshed#3363. Also allowing annotations on self and overloading in them would be useful for other things (e.g. supporting mixins and methods with signatures that depend on type argument). Also I don't think we need any special syntax/rules for this. We can just allow:

class MyFile(Generic[AnyStr]):
    @overload
    def __init__(self: MyFile[str], mode: Literal["r", "w"] = ...) -> None: ...
    @overload
    def __init__(self: MyFile[bytes], mode: Literal["rb", "wb", "b"]) -> None: ...
    @overload
    def __init__(self, mode: str) -> None: ...

From mypy point of view we would need to improve how bind_self() works plus few other things, I assume this would be similar for other type checkers.

UPDATE: mypy implementation may be actually more tricky and would need some special-casing in e.g. class_callable().

@ilevkivskyi
Copy link
Member

Just to add an example for how annotations on self can be used to support mixins:

class Base:
    def base_meth(self) -> None: ...

class Mixin:
    def mix_meth(self: Base) -> None:
        # Here `self` is an intersection of current class and annotation.
        self.base_meth()  # OK

class Good(Base, Mixin): ...
class Bad(Mixin): ...

Good().mix_meth()  # OK
Bad().mix_meth()  # Error from `bind_self`

In principle we can make this work in mypy, but more advanced types for mixins may be available only if we add intersection types (at least for proper classes and their unions, similar to what is allowed in Type[...]).

ilevkivskyi added a commit to ilevkivskyi/mypy that referenced this issue Nov 5, 2019
Fixes python#3625
Fixes python#5305
Fixes python#5320
Fixes python#5868
Fixes python#7191
Fixes python#7778
Fixes python/typing#680

So, lately I was noticing many issues that would be fixed by (partially) moving the check for self-type from definition site to call site.

This morning I found that we actually have such function `check_self_arg()` that is applied at call site, but it is almost not used. After more reading of the code I found that all the patterns for self-types that I wanted to support should either already work, or work with minimal modifications. Finally, I discovered that the root cause of many of the problems is the fact that `bind_self()` uses wrong direction for type inference! All these years it expected actual argument type to be _supertype_ of the formal one.

After fixing this bug, it turned out it was easy to support following patterns for explicit self-types:
* Structured match on generic self-types
* Restricted methods in generic classes (methods that one is allowed to call only for some values or type arguments)
* Methods overloaded on self-type
* (Important case of the above) overloaded `__init__` for generic classes
* Mixin classes (using protocols)
* Private class-level decorators (a bit hacky)
* Precise types for alternative constructors (mostly already worked)

This PR cuts few corners, but it is ready for review (I left some TODOs). Note I also add some docs, I am not sure this is really needed, but probably good to have.
@srittau
Copy link
Collaborator Author

srittau commented Dec 4, 2019

One thing where this change is quite useful are my partial SQLalchemy stubs. Previously columns needed to be explicitly annotated:

class Foo(ORMBase):
    name: Column[str] = Column(String, primary_key=True)

Now using overloads this is no necessary anymore:

class Foo(ORMBase):
    name = Column(String, primary_key=True)

Needs six overloads per type and doesn't cover all corner cases, but still very useful.

@ilevkivskyi
Copy link
Member

Have you thought about contributing whatever you have on SQLAlchemy to https://github.com/dropbox/sqlalchemy-stubs/? I think it would be great to have a single package with better coverage.

@ilevkivskyi
Copy link
Member

@srittau ^^

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

Successfully merging a pull request may close this issue.

3 participants