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

typing.MutableMapping not a drop-in replacement for collections.abc.MutableMapping #203

Closed
bdarnell opened this issue Apr 17, 2016 · 9 comments · Fixed by #283 or #287
Closed

typing.MutableMapping not a drop-in replacement for collections.abc.MutableMapping #203

bdarnell opened this issue Apr 17, 2016 · 9 comments · Fixed by #283 or #287
Milestone

Comments

@bdarnell
Copy link

The only difference between typing.MutableMapping and its collections.abc counterpart is supposed to be the support for generic type parameters. However, it looks like it's also missing some other methods:

import collections.abc
import typing

def define_user_dict(base_class):
    class MyUserDict(base_class):
        def __getitem__(self, k):
            return None
        def __setitem__(self, k, v):
            pass
        def __delitem__(self, k):
            pass
        def __iter__(self):
            return iter(())
        def __len__(self):
            return 0
    return MyUserDict

UserDict1 = define_user_dict(collections.abc.MutableMapping)
UserDict2 = define_user_dict(typing.MutableMapping[str, str])

UserDict1().update() # this one works
UserDict2().update() # this one fails
$ python3.5 /tmp/mappingtest.py
Traceback (most recent call last):
  File "/tmp/mappingtest.py", line 22, in <module>
    UserDict2().update()
AttributeError: 'MyUserDict' object has no attribute 'update'

Tested on python 3.5.1, and on python 3.4 with typing 3.5.1.0 installed.

Is changing my base class from collections.abc.MutableMapping to typing.MutableMapping the right way to tell the type checker what types my user dict will work with? Should I be using both base classes together? Is there a way to tell the type checker that MyUserDict is a typing.MutableMapping[str, str] while keeping collections.abc.MutableMapping as the runtime base class?

Additionally, isinstance() checking is funky: isinstance(dict(), UserDict2) returns true, so I can't use isinstance() to distinguish my dict type from any other.

@bintoro
Copy link
Contributor

bintoro commented Apr 17, 2016

The reason for this is that the typing ABCs don't really inherit from their collections.abc counterparts. They just proxy issubclass calls to the collections classes.

Incidentally, I just today proposed changing this in a comment to #136.

The branch is here: https://github.com/bintoro/typing/tree/extra-as-base

Additionally, isinstance() checking is funky: isinstance(dict(), UserDict2) returns true, so I can't use isinstance() to distinguish my dict type from any other.

I raised the same issue in #202. This is also fixed in the above branch.

@gvanrossum
Copy link
Member

Agreed this is a problem. But I'm not sure I am going to get to this before Python 3.5.2 goes out -- not that that's about to be released, but I need to prioritize and I don't think that this (or the other patches proposed by @bintoro) is more important than the work on typeshed and mypy, and we do have a few mypy releases planned (May 5 and May 26).

Does combining both base classes work at all? If so it would be a reasonable work-around.

@bdarnell
Copy link
Author

It's not a priority for me.

The three different outcomes are:

  • collections.abc.MutableMapping alone: Everything works, but the code is less precisely typed than it could be. I'm sticking with this one for now.
  • typing.MutableMapping alone: Things break because methods like update() are missing. If it made it farther it would also see isinstance() problems.
  • Both base classes combined: Things mostly work but isinstance() is misbehaving and causing errors in some cases.

@gvanrossum gvanrossum modified the milestone: 3.5.3 Aug 26, 2016
gvanrossum pushed a commit that referenced this issue Sep 27, 2016
This PR:

- Fixes #136
- Fixes #133
- Partially addresses #203 (fixes the isinstance part, and multiple inheritance, still typing.Something is not a drop-in replacement for collections.abc.Something in terms of implementation).
- Also fixes http://bugs.python.org/issue26075, http://bugs.python.org/issue25830, and http://bugs.python.org/issue26477
- Makes almost everything up to 10x faster.
- Is aimed to be a minimalistic change. I only removed issubclass tests from test_typing and main changes to typing are __new__ and __getitem__.

The idea is to make most things not classes. Now _ForwardRef(), TypeVar(), Union[], Tuple[], Callable[] are not classes (i.e. new class objects are almost never created by typing).

Using isinstance() or issubclass() rises TypeError for almost everything. There are exceptions:

    Unsubscripted generics are still OK, e.g. issubclass({}, typing.Mapping). This is done to (a) not break existing code by addition of type information; (b) to allow using typing classes as a replacement for collections.abc in class and instance checks. Finally, there is an agreement that a generic without parameters assumes Any, and Any means fallback to dynamic typing.
    isinstance(lambda x: x, typing.Callable) is also OK. Although Callable is not a generic class, when unsubscribed, it could be also used as a replacement for collections.abc.Callable.
    The first rule for generics makes isinstance([], typing.List) possible, for consistency I also allowed isinstance((), typing.Tuple).

Finally, generics should be classes, to allow subclassing, but now the outcome of __getitem__ on classes is cached. I use an extended version of functools.lru_cache that allows fallback to non-cached version for unhashable arguments.
@gvanrossum
Copy link
Member

Sorry, #283 only partially fixed this.

@gvanrossum gvanrossum reopened this Sep 27, 2016
@gvanrossum
Copy link
Member

@ilevkivskyi Could you look into this more? The test from the first post still fails.

@ilevkivskyi
Copy link
Member

@gvanrossum The first question is whether we really want typing.MutableMapping to work in terms of implementation. It looks like specifying two bases works:

class MD(collections.abc.MutableMapping, typing.MutableMapping[str, str]):
     def __getitem__(self, k):
         return None
     def __setitem__(self, k, v):
         pass
     def __delitem__(self, k):
         pass
     def __iter__(self):
         return iter(())
     def __len__(self):
         return 0

>>> MD().update()  # This works
>>> isinstance(MD(), collections.abc.MutableMapping)
True
>>> isinstance(MD(), typing.MutableMapping)
True
>>> isinstance(MD(), typing.Dict)
False
>>> isinstance(MD(), collections.abc.Set)
False
# All other possible instance checks also work fine

But if you think that we should include the implementation (i.e. include collections.abc.MutableMapping in __mro__, not only in __subclasscheck__)
then a possible solution would be similar to proposed in #207 (patch bases at generic class creation and define __subclasshook__).

@gvanrossum
Copy link
Member

gvanrossum commented Sep 28, 2016 via email

@bintoro
Copy link
Contributor

bintoro commented Sep 28, 2016

It should be pretty straightforward to take the parts from #207 that established functional equivalence between typing.X and collections.abc.X and plug them into the current version.

@ilevkivskyi
Copy link
Member

Here is a PR as proposed by @bintoro #287

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