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

[Bug] utilpepget.get_hint_pep_args returns incorrect value for typing.Callables #137

Closed
tlambert03 opened this issue Jun 5, 2022 · 5 comments

Comments

@tlambert03
Copy link
Contributor

typing.get_args special cases both Annotated and callable classes:

def get_args(tp):
    if isinstance(tp, _AnnotatedAlias):
        return (tp.__origin__,) + tp.__metadata__
    if isinstance(tp, (_GenericAlias, GenericAlias)):
        res = tp.__args__
        if _should_unflatten_callable_args(tp, res):
            res = (list(res[:-1]), res[-1])
        return res
    if isinstance(tp, types.UnionType):
        return tp.__args__
    return ()

but utilpepget.get_hint_pep_args only checks for __args__, which leads to the following behavior:

In [11]: hint = Callable[[], int]

In [12]: get_args(hint)
Out[12]: ([], <class 'int'>)

In [13]: get_hint_pep_args(hint)
Out[13]: (<class 'int'>,)  # we lost the "takes no arguments" info

In [14]: hint = Callable[[int, str], int]

In [15]: get_args(hint)
Out[15]: ([<class 'int'>, <class 'str'>], <class 'int'>)

In [16]: get_hint_pep_args(hint)
Out[16]: (<class 'int'>, <class 'str'>, <class 'int'>)  # flat list

Is there any reason not to try to use typing.get_args when available? (Not entirely sure about all the potential back-porting and other compatibility issues here)

running into this in #136

@leycec
Copy link
Member

leycec commented Jun 7, 2022

Curse you yet again, typing.get_args()! Curse you to heck and back!

...ahem. So, I may have gotten a bit carried away there. But I did so for a justifiably valid reason. And that reason is this: the typing module is basically broken at runtime, because it was never intended to be reliably used at runtime. The typing module exists and defines various public attributes with well-defined names that mostly stay the same across Python versions – but that's as much as can be depended upon, really.

Nothing that typing defines can be trusted at runtime. This especially includes the public functions that typing defines like get_args(). We intentionally avoid deferring to those functions, because those functions are poorly tested, badly implemented, and blatantly broken under common edge cases. For example...

The Empty Tuple Type Hint

So, there's this totally Bizarro World type hint called the "empty tuple type hint" specified with magic syntactic sugar like tuple[()] and typing.Tuple[()]. Yeah. That's ugly stuff, I know. Tell me about it! don't please don't

But that's also the only means of annotating a fixed-length tuple of length 0. Since there are valid reasons for wanting to do that, PEP 484 explicitly supports that:

The empty tuple can be typed as Tuple[()].

Here's where things get ugly.

# PEP 484-style empty tuple type hint. This makes sense.
>>> from typing import Tuple
>>> tuple0_pep484 = Tuple[()]
>>> tuple0_pep484.__args__
((),)

# So, @beartype correctly infers the arguments of an
# PEP 484-style empty tuple type hint to be the empty tuple.
# This makes sense, too.
>>> from beartype._util.hint.pep.utilpepget import get_hint_pep_args
>>> get_hint_pep_args(tuple0_pep484)
((),)

# typing.get_args() also correctly infers the same arguments.
# So far, so good.
>>> from typing import get_args
>>> get_args(tuple0_pep484)
((),)

# And things take a sudden turn for the worse. Here's a
# PEP 585-style empty tuple type hint. This hint incorrectly
# destroys its arguments, because the C-based implementation
# of the "types.GenericAlias" superclass is broken. Not kidding.
>>> tuple0_pep585 = Tuple[()]
>>> tuple0_pep585.__args__
()

# But things get better for a bit. @beartype internally detects
# this edge case and restores portability by pretending that
# PEP 484- and 585-style empty tuple type hints have the
# same arguments.
>>> get_hint_pep_args(tuple0_pep585)
((),)

# Then things get worse again. typing.get_args() doesn't
# know that "types.GenericAlias" is broken, so it silently
# trusts "types.GenericAlias" by returning its broken
# "__args__" tuple sight unseen.
>>> get_args(tuple0_pep585)
()

But Wait, More Badness

Oh – and there's yet another reason to avoid typing.get_args() here. That getter creates and returns inefficient mutable non-memoizable lists rather than efficient immutable memoizable tuples when handling Callable[...] type hints: e.g.,

def get_args(tp):
    ...
            res = (list(res[:-1]), res[-1])  # <-- this is bad, bro

@beartype internally memoizes everything it does for space and time efficiency. We can't do that if we start passing lists around. Ergo, we still need to handle this ourselves. I sigh expansively. 😮‍💨

And... that's just two mind-bending edge cases. Python 3.7's typing doesn't even define get_args(), so we'd need to backport a newer version – but there's no point, because even newer versions are broken in the aforementioned edge cases.

Please God Just Make Callables Work, Already

I feel your growing migraine and kindly sympathize as only a fellow runtime typing enthusiast can.

@beartype currently fails to deeply type-check Callable[...] type hints – mostly because I'm lazy. Also, it's non-trivial to do that while preserving O(1) constant-time constraints. So, I never even knew that both PEP 484- and 585-style Callable[...] type hints insanely destroy their first arguments:

>>> from typing import Callable
>>> Callable[[], int].__args__
(int,)
>>> from collections.abc import Callable as CallableABC
>>> CallableABC[[], int].__args__
(int,)

I sigh wearily with exhaustion exceeding the finite bounds of string theory's eleventh dimension.

The only saving grace here is that it's trivial for @beartype to explicitly detect and handle that edge case (e.g., with the same hacky logic that typing.get_args() itself employs here). For great justice, @beartype will solve this for you and all humanity early this week! ⏳

Thank You So Much, @tlambert03

Allow me to conclude by effusively thanking you for all your tremendous volunteerism, @tlambert03. You've been a huge help over the past few days. The QA community applauds your valiant bravery. This is rough stuff. I really appreciate you toughing this out on the front lines with me. Your herculean efforts will be vindicated when I merge your awe-inspiring PR later this week.

Together, we will vanquish the darkness by the power of type hint arithmetic.

@leycec
Copy link
Member

leycec commented Jun 7, 2022

Hmm... In reflection, it's probably preferable for get_hint_pep_args() to preserve Callable[...].__args__ flattened tuples like it currently does (e.g., as (<class 'int'>, <class 'str'>, <class 'int'>)) rather than de-flattening those tuples into a deeply nested data structure (e.g., as ([<class 'int'>, <class 'str'>], <class 'int'>)). The former's substantially faster and saves quite a bit more space – which is, of course, our running obsession here. 😵‍💫

Sure, flattened tuples are a bit oddball from the semantic perspective – but they're quite a bit easier to deal with from the pragmatic perspective. Does that make sense? Is there something I'm fundamentally missing that requires Callable[...] arguments to be de-flattened for type hint arithmetic to "just" work – like, say, ellipses (e.g., Callable[..., str]), typing.ParamSpec[...], or typing.Concatenate[...]? I'd be happy to ignore the latter two for now. And maybe the ellipses edge case sorta handles itself? I'm probably fundamentally missing something. </sigh>

That said...

typing.Annotated Exists

I absolutely agree that our get_hint_pep_args() getter needs to special-case PEP 593-style typing.Annotated[...] arguments for type hint arithmetic to "just" work. We don't want to special-case those hints everywhere. So, it's best if get_hint_pep_args() silently makes that happen on your behalf. Right?

Right! I shall immediately make it so tomorrow. 🌈

leycec added a commit that referenced this issue Jun 7, 2022
This commit is the first in a commit chain improving our private
`beartype._util.hint.pep.utilpepget.get_hint_pep_args()` getter to
**deflatten** (i.e., restore to a facsimile of their original form)
`Callable[...]` type hints passed to that getter, en-route to resolving
issue #137 kindly submitted by Harvard microscopist and all-around
typing math guru @tlambert03 (Talley Lambert), required as a mandatory
dependency of pull request #136 also kindly submitted by @tlambert03.
Specifically, this commit lightly refactors that getter in preparation
for subsequent surgery as well as detailing internal improvements
needed elsewhere for this to fully behave as expected. (*Long-tallied talons!*)
@leycec
Copy link
Member

leycec commented Jun 7, 2022

So. It begins.

@leycec
Copy link
Member

leycec commented Jun 8, 2022

Actually... I hope you don't mind if I close this issue out on you. The current behaviour of get_hint_pep_args() is (mostly) working as intended, which is surprising even to me.

My eternal gratitude for all your extraordinary outpouring of enthusiasm for this. So excited, peeps! 😄

@leycec leycec closed this as completed Jun 8, 2022
@tlambert03
Copy link
Contributor Author

I hope you don't mind if I close this issue out on you.

yep, no problem!

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

No branches or pull requests

2 participants