Use `getattr` for `collections.abc._check_methods`

Ideas
1

_check_methods is the helper for the various __subclasshook__ in collections.abc. It’s becoming more and more important with the spread of typed python and type checkers as they’re much happier with types getting refined via isinstance than they are with more ad-hoc structural checks.

_check_methods is implemented by walking the MRO manually in Python in order to check for the requested attributes being present and non-None:

def _check_methods(C, *methods):
    mro = C.__mro__
    for method in methods:
        for B in mro:
            if method in B.__dict__:
                if B.__dict__[method] is None:
                    return NotImplemented
                break
        else:
            return NotImplemented
    return True

this seems equivalent to:

def _check_methods(C, *methods):
    for method in methods:
        if getattr(C, method, None) is None:
            raise NotImplemented
    return True

but because getattr walks the MRO in C (and can use the MRO cache?) that runs in about 60% the time of the original, and for the ABCs with a single attribute (Iterable, Callable, Container,…) a further 20% (from the original) can be shaved off by removing the outer loop:

def _check_method(C, method):
    if getattr(C, method, None) is None:
        raise NotImplemented
    return True

Am I missing something? Or was it implemented this way because it was simpler and abc wasn’t really worth thinking too much about at the time? Or was getattr significantly sped up since? This implementation style is from the original PEP 3119 so it dates back pretty much to Python 3.0 and hasn’t been revisited since (except for consolidating the implementation under a single helper, and some of the hooks used to use any rather than an explicit loop)

2

Metaclass magic with custom __getattr__ or similar can very much make the two non-equivalent.

2 Likes
4

Arg good point, I assumed but did not check that protocol resolution (what cpython actually does at runtime) would follow the metaclass’ directives, and after checking it indeed does not.

And although it looks like a metaclass check would not completely invalidate the optimization it would eat into it quite significantly, and would preclude dropping the existing code.

5

Generally, after making your change you should run the tests to see what would break :‍)

One of the tests in test_collections checks isinstance(None, Callable), which points to the metaclass issue (type(None) is callable, like any type, but None itself isn’t).

1 Like
6

I guess raise instead of return is just a typo?