[Bug] Overly broad isinstanceable type detection raises false negatives

[patrick-kidger]

from beartype import beartype

class MetaFoo(type):
    def __instancecheck__(cls, other):
        return g()

class Foo(metaclass=MetaFoo):
    pass

@beartype
def f(x: Foo):
    pass

def g():
    return True

# beartype.roar.BeartypeDecorHintNonpepException: @beartyped __main__.f() parameter "x" <class '__main__.Foo'> uncheckable at runtime (i.e., not passable as second parameter to isinstance(), due to raising "name 'g' is not defined" from metaclass __instancecheck__() method).

It looks like beartype is calling isinstance on every type annotation eagerly, but it may not yet be valid to do so at decoration time.

[leycec]

Fascinating. You surmise correctly, as always:

It looks like beartype is calling isinstance on every type annotation eagerly...

Indeed, that is exactly what @beartype is doing. @beartype is doing this for a reasonable reason and should thus keep doing this — but more carefully to account for your very valid use case.

But why is @beartype doing this? Malicious types. The standard library is littered with all of these harmful classes that were intentionally designed to be incompatible with runtime type-checkers. They do this by (...waitforit) using a usually private metaclass whose __instancecheck__() method unconditionally raises an exception. We call these "non-isinstanceable types."

To raise human-readable exceptions, @beartype explicitly detects non-isinstanceable types at decoration time. And... we all see where this is going. For each type hint hint that is a class, @beartype detects whether that class is isinstanceable or not by calling isinstance(hint, object). If doing so either raises an exception or returns anything other than True, @beartype treats that class as non-instanceable by raising the exception you received above. </gulp>

Unfortunately, this isn't just an edge case. Since isinstanceable types are so common in the standard library, @beartype really does need to continue detecting them and preventing them from being used as type hints. Critically, this includes the private typing._SpecialForm metaclass that underlies most type hints:

class _SpecialForm(_Final, _NotIterable, _root=True):
    ...

    def __instancecheck__(self, obj):
        raise TypeError(f"{self} cannot be used with isinstance()")

    def __subclasscheck__(self, cls):
        raise TypeError(f"{self} cannot be used with issubclass()")

Unsurprisingly, typing._ProtocolMeta and typing._TypedDictMeta all behave similarly – albeit with differing exception messages. There's no meaningful commonality between those messages. So, substring matching is right out. But other grotesque things best unseen might still be in!

I See a QA Darkness

Yeah. Me three. But I'm pretty sure (but uncertain) that @beartype can have its cake and smear it all over its muzzle, too. @beartype can accommodate both isinstanceable type detection and your vital use case by cautiously reducing the scope of that detection. Specifically, we can:

• Consider only types whose __instancecheck__() methods raise a TypeError exception as non-isinstanceable. The typing._SpecialForm metaclass does that; yours doesn't. All other non-isinstanceable types would simply be treated as isinstanceable. This has the disadvantage of being:
  • Overly fragile. The typing module often changes in outlandish ways. But... tests will catch any future breakage, which we can then repair. It's not all despair and black eye mascara. 👩‍🎤
  • Overly narrow. This is the larger concern. There are probably other third-party packages also defining non-isinstanceable types. If we do this, @beartype might lose the ability to detect them. It is a conundrum for the ages.
• Consider all types whose __instancecheck__() methods raise an AttributeError exception as isinstanceable. Yours does that; the typing._SpecialForm metaclass doesn't do that. This has the disadvantage of being:
  • Overly narrow, yet again. There are probably more exception classes than just AttributeError that a metaclass in a partially-initialised module could raise. But... at least this specific issue would be resolved. Surely that means something!
• Try to detect whether the module defining the metaclass of the type hint being examined is partially-initialised or not. I'm kinda loathe to get into importlib shenanigans, thanks to non-portable implementation details. The Dark Side leads that way. Still... an option we do not want to choose. Gah!

I'm partial to the first option. All non-isinstanceable types declared by the typing module raise TypeError exceptions. That's a rather specific exception class connoting a rather specific type-checking failure. Any third-party non-isinstanceable types raising other classes of exceptions are bad; their badness isn't @beartype's fault, so we can't be held responsible.

tl;dr

Let's reduce the scope of non-isinstanceable types to only those types whose metaclass __instancecheck__() methods raise a TypeError. Raise the fist of accuracy, @beartype! ✊

[leycec]

This is definitely the worst of the cavalcade of issues you recently submitted, @patrick-kidger. Everything else is a feature request in disguise – but this is a genuine issue that's probably beating up somebody else's code, too.

Thankfully, resolving this is trivial. I think. I hope. Oh, Gods! Please let this be trivial! I'll tackle this headlong when vacuous free time permits. 😮‍💨

Thanks again for this tremendous outpouring of detailed issues. You rock the typing scene. @beartype is shuddering spasmodically under the massive weight of its oppressive failings, but we'll get there. Someday. Somehow. (Utopia is still in sight, everyone.)

[patrick-kidger]

Hahaha, sorry-not-sorry for the huge number of issues. :)

IIUC, this is about detecting the poorly-behaving typing.Foo hints? What about doing something like:

module = getattr(hint, "__module__", "")
metamodule = getattr(type(hint), "__module__", "")
return len({module, metamodule}.intersection({"types", "typing"})) > 0

The metamodule check handles stuff like list[int], which is either a typing._GenericAlias or a types.GenericAlias depending on Python version.

[leycec]

Smart. Tragically, some metaclasses defined by the typing and types modules do actually support isinstance() checks in the standard way – if I recall correctly (which I usually don't). Even if that isn't the case now, that certainly could be the case under any future Python release later. @beartype is fragile enough already. We're the bone china of the typing world here. 🍵

^{...actually, bone china is surprisingly robust relative to non-bone china, which is the whole point of bone china; my metaphor doesn't even make sense, despite sounding cool.}

Catching TypeError it is, then! \o/

[leycec]

Resolved by e643b91. Thanks so much for all the vigorous discussion, @patrick-kidger. Typing bros are the best bros. It is objectively true.

Now, let's see if we can knock out TypeGuard support at issue #221 as the last order of business for our upcoming beartype 0.13.0 release. The road to proper QA is fraught with hardship and woe.