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

Metaclass type/instance difficulties #3625

Closed
rtpg opened this issue Jun 28, 2017 · 14 comments · Fixed by #7860
Closed

Metaclass type/instance difficulties #3625

rtpg opened this issue Jun 28, 2017 · 14 comments · Fixed by #7860

Comments

@rtpg
Copy link
Contributor

rtpg commented Jun 28, 2017

Consider the following code from my hypothetical ORM that has three different ways of building an object

from typing import TypeVar, Type

T = TypeVar('T')


class ModelMeta(type):
    def build(cls: Type[T]) -> T:
        return cls()

    def build_other(cls):
        return cls()


class Model(metaclass=ModelMeta):

    @classmethod
    def new(cls: Type[T]) -> T:
       return cls()

I'm having trouble defining build in a way that type checks. In this example, I get an error on build about how Type[T] is not a supertype of the class ModelMeta.

build_other passes the type checker, but usages don't seem to catch anything (implicit Any perhaps?)

class Dog(Model):
    def __init__(self):
        print("Built Dog")


class Cat(Model):
    def __init__(self):
        print("Built Cat")


c: Cat = Dog.new()  # type check fails
c2: Cat = Dog.build()  # type check fails
c3: Cat = Dog.build_other()  # passes

Considering class methods work, it feels like metaclass methods should equally be able to "unwrap" the instance type for a class, but there might be subtleties I'm missing here.

My wish list here would be:

  • have the inverse of Type (something like Instance) so I could write something like def build(cls: T) -> Instance[T]. This would be (I think) a bit clearer.
  • Have an intersection type, with which I could declare something like
    def build(cls: (Type[T] & ModelMeta)) -> T. This would help with a lot of other problems as well. This seems like it would be a useful shortcut to writing subclasses
  • Have a magic hook in subtypes.is_subtype that would let me write a custom subtype check in some types

Full context: I'm writing some stubs for Django's ORM, and was hitting issues around the type of Model.objects.create. So in my case I end up needing a "class property" (hence doing things through the metaclass), which is why the simple new() call strategy hasn't been working for me.

I think this is related to #3438, but I'm having a hard time identifying the link.

@elazarg
Copy link
Contributor

elazarg commented Jun 28, 2017

Related to #3346 and some other discussions which I can't seem to find right now.

build_other is not checked at all, since it is not annotated. Also that cls() is not checked in your code, since Type[T] is considered instantiable with arbitrary arguments.

I think your first error message does not show up on master. However, it does show up if you constrain your variable "properly":

T = TypeVar('T', bound='Model')

The reason is that it is unsafe:

class ModelMeta(type):
    def build(cls: Type[T]) -> T: return cls()

class Model(metaclass=ModelMeta):  ...
class NonModel(metaclass=ModelMeta): ...

NonModel.build()  # error - this is not `Type[T]`

But I think @ilevkivskyi argued that this slight unsafety is useful, as in your case. If the metaclass was somehow denied of being used outside the file, it could be even possible to check at the point of definition, and not just access.

Intersection types can only help in the implementation of the function, but at some point the arguments should be checked to see that they are of Type[T] which is not guaranteed by the lookup.

Maybe a constraint on TypeVar could have helped:

T = TypeVar('T', metaclass='ModelMeta')

Or perhaps something like

class ModelMeta(type):
    def build(cls: ModelMeta[T]) -> T: ...

But I'm not sure exactly what should it mean. Maybe that's equivalent to your first wish. I'm just thinking out loud.

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Jun 28, 2017

Concerning this example:

T = TypeVar('T', bound='Model')

class ModelMeta(type):
    def build(cls: Type[T]) -> T: return cls()

class Model(metaclass=ModelMeta):  ...
class NonModel(metaclass=ModelMeta): ...

NonModel.build()  # error

I think this can be made safe if mypy will give the error at the point of use, not at the point of definition of build. This can be probably done in checkmember.bind_self.

Concerning the plugin hook, we support many plugin hooks, but I am not sure about his one. @JukkaL are there plans to support plugins for subtypes.is_subtype?

@elazarg
Copy link
Contributor

elazarg commented Jun 28, 2017

I'm not against the idea of checking on access, but it comes with some cost:

  1. When I tried to implement the check on access it was somewhat complex.
  2. There's also the problem of adding these checks to all accesses, just to support a corner case.
  3. Checking on access means not giving the library writer a warning about this usability problem, and we don't know whether it was intentional or not.

@JukkaL
Copy link
Collaborator

JukkaL commented Jun 28, 2017

Mypy could also reject the class NonModel due to metaclass conflict (because of build) instead of the call to NonModel.build().

There are currently no concrete plans to support subtyping plugin hooks, but the plugin system is still experimental and we can add all sorts of new hooks if they seem useful. @rtpg Feel free to create issues for any new plugin hooks that you would find useful, preferably with examples of how you'd use them.

@elazarg
Copy link
Contributor

elazarg commented Jun 28, 2017

Mypy could also reject the class NonModel due to metaclass conflict (because of build) instead of the call to NonModel.build()

Interesting... and it solves points (1) and (2) from my earlier comment.

@rtpg
Copy link
Contributor Author

rtpg commented Jun 29, 2017

I hadn't thought too deeply about adding bounds to the TypeVar, but while stepping through mypy code to figure out what was happening in the resolution, there is already a sort of "self" check in place to ensure that the type of self can properly unify with the type of the class... I imagine this could also handle meta-class checks similarly.

Though in my case, knowing that I will have at least a partial solution in #3346 helps me to solve most of my worries about the type here.

@rtpg
Copy link
Contributor Author

rtpg commented Jul 20, 2017

@elazarg I just pulled in master and tried to run the following code, but am still hitting some issues. Perhaps I'm missing a detail:

from typing import TypeVar, Type, Generic, cast

T = TypeVar('T')


class Manager(Generic[T]):
    def create(self) -> T:
        return cast(T,1)

   
class ModelMeta(type):

    @property
    def objects(cls: Type[T]) -> Manager[T]:
        return Manager()


class Model(metaclass=ModelMeta):
    pass

class Dog(Model):
    def __init__(self):
        print("Built Dog")


class Cat(Model):
    def __init__(self):
        print("Built Cat")


c: Cat = Dog.objects.create()
d: Dog = Dog.objects.create()

The error messages I get here are:

/Users/rtpg/tmp/mpy.py:31: error: Incompatible types in assignment (expression has type "Dog", variable has type "Cat")
/Users/rtpg/tmp/mpy.py:31: error: Invalid method type
/Users/rtpg/tmp/mpy.py:32: error: Invalid method type

The first one is the system "working as intended". But I'm not quite sure what to do about those method type error messages

@elazarg
Copy link
Contributor

elazarg commented Jul 20, 2017

with #3227 you get the following error:
tmp.py:14: error: The erased type of self 'Type[builtins.object]' is not a supertype of its class 'tmp.ModelMeta'
Which is more explanatory I believe. It's the same problem discussed in my first comment and hadn't been resolved yet.

@rtpg
Copy link
Contributor Author

rtpg commented Jul 21, 2017

ah sorry, I read your comment as saying that if I don't add bounds to the type var, then this could be setup.

This is a bit frustrating, because I feel like I'm so close to getting type checking on a pretty big part of the Django ORM but am hitting this issue, even though it feels like mypy "gets it".

I'm trying to figure out an alternative way to tackle this issue by having the type variable be on the Manager object instead.

from typing import TypeVar, Type, Generic, cast

Cls = TypeVar('Cls') #, bound=type)
T = TypeVar('T')


class Manager(Generic[Cls]):
    def create(self:Manager[Type[T]]) -> T:
        return cast(T,1)


class ModelMeta(type):

    @property
    def objects(cls: T) -> Manager[T]:
        return Manager()


class Model(metaclass=ModelMeta):
    pass

class Dog(Model):
    def __init__(self):
        print("Built Dog")


class Cat(Model):
    def __init__(self):
        print("Built Cat")


c: Cat = Dog.objects.create()
d: Dog = Dog.objects.create()

In this I also get the method failures, I imagine the same erased type issue is happening in this case as well?

@mitar
Copy link

mitar commented Dec 2, 2017

Another similar issue:

import typing

T = typing.TypeVar('T', bound='Foo')

class FooMeta(type):
    def __repr__(self: typing.Type[T]) -> str:
        return self.name

class Foo(metaclass=FooMeta):
    name = 'FooClass'
example.py:6: error: Self argument missing for a non-static method (or an invalid type for self)

@elazarg
Copy link
Contributor

elazarg commented Dec 2, 2017

Technically there are two issues in your example: the first is the discussed issue (there is no guarantee that self will be compatible with Type[Foo]).

The second issue is that there is no reason to assume that Type[Foo] will have an attribute name - and indeed it doesn't; type(Foo).name fails at runtime. I'm not sure why mypy doesn't complain about this error - looks like a bug to me.

@mitar
Copy link

mitar commented Dec 2, 2017

Ehm, Foo.name works, at least this is the idea here.

Type[T] -> Type[Foo] -> Foo class, so self is Foo which has name.

@elazarg
Copy link
Contributor

elazarg commented Dec 2, 2017

Indeed. Perhaps I'm confusing type(Foo) and Type[Foo] (the former being the metaclass itself) for no reason.

@ilevkivskyi
Copy link
Member

Note that this is closely related to #7191 (in the sense that the solution is the same, move consistency check from definition to use site, i.e. to bind_self).

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants