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] Type variables bound by forward references (e.g., TypeVar(..., bounds='{UndefinedType}')) raise erroneous type-checking exceptions 😭 #367

Closed
iamrecursion opened this issue Apr 15, 2024 · 7 comments

Comments

@iamrecursion
Copy link

Well I truly seem to be into the weeds recently, and I am not surprised that the bear will not follow. After all, the poor thing might not see its prey amongst these troublesome plants.

My sincere apologies to @leycec for yet another terriffic terrible bug report.

To help our future __selves__

The first piece of strangeness that I am seeing is some misbehavior when it comes to the combination of from __future__ import annotations and certain __dunder__ methods.

For the bear's perusal I present the following code:

from beartype import beartype
from typing_extensions import Self

@beartype
class MyClass:
    attribute: int
    
    def __init__(self, attr: int) -> None:
        self.attribute = attr
    
    def add(self, other: int) -> Self:
        self.__class__(self.attribute + other)
    
    def __add__(self, other: int) -> Self:
        self.__class__(self.attribute + other)

Now, as one might expect, this all works properly! The class gets defined properly and decoration all works.

Now, add some magic at the top of the file from __future__ import annotations. Suddenly it all explodes! In particular, the __add__ dunder method being annotated by Self is the problem.

We get this wonderful but—at least in this case—nonsensical error message from the Bearhugger Broadcasting Service:

BeartypeDecorHintPep673Exception: Method __main__.MyClass.__add__() return PEP 673 type hint "typing.Self" invalid outside @beartype-decorated class. PEP 673 type hints are valid only inside classes decorated by @beartype. If this hint annotates a method decorated by @beartype, instead decorate the class declaring this method by @beartype: e.g.,

    # Instead of decorating methods by @beartype like this...
    class BadClassIsBad(object):
        @beartype
        def awful_method_is_awful(self: Self) -> Self:
            return self

    # ...decorate classes by @beartype instead - like this!
    @beartype
    class GoodClassIsGood(object):
        def wonderful_method_is_wonderful(self: Self) -> Self:
            return self

This has been a message of the Bearhugger Broadcasting Service.

After all, I have done what the bear's news team suggests, but I am still being warned, alas!

If it helps, forward-declaring an appropriate desugaring for Self using a TypeVar does actually work in this case!

from __future__ import annotations

from beartype import beartype
from typing_extensions import Self, TypeVar

OtherSelf = TypeVar("OtherSelf", bound="MyClass")

@beartype
class MyClass:
    attribute: int
    
    def __init__(self, attr: int) -> None:
        self.attribute = attr
    
    def add(self, other: int) -> Self:
        self.__class__(self.attribute + other)
    
    def __add__(self, other: int) -> OtherSelf:
        self.__class__(self.attribute + other)

Many Different Selfes (Yes I Know It Should Be "Selves" But Where's The Fun In That)

As seems to be the case with my forays into the darker reaches of typedom, this is not the end of the story.

Consider, if you will, the case of returning a tuple of Self instances from a function as shown in this little test case:

from beartype import beartype
from typing_extensions import Self, TypeVar

OtherSelf = TypeVar("OtherSelf", bound="MyClass")

@beartype
class MyClass:
    attribute: int
    
    def __init__(self, attr: int) -> None:
        self.attribute = attr
        
    def dup(self, other: int) -> tuple[Self, Self]:
        return self.__class__(other), self.__class__(other)

Yet again, I get the same error, but have done exactly as the bear asked! The kicker here is that we can see the error without the from __future__ import annotations. This issue is now firmly in our present!

Yet again, I can work around it somewhat by using my dastardly type variable! Replacing the dup signature with def dup(self, other: int) -> tuple[OtherSelf, OtherSelf]: works consistently as expected.

Oh God Is She Done????

Gratifyingly yes. After dumping renewed levels of typing horror upon the poor unsuspecting bear, I am at least done.

@iamrecursion
Copy link
Author

She lied (forgot about one part of the bug until it bit her in the ass three minutes later)! She was not done!

I Am Not Sure That Redefining One's Self is A Good Idea

Things get even stranger once you put these things in a notebook situation, where we have redefinitions of classes. Even without our dastardly future knowledge, we see further strangeness.

Consider the following example:

from beartype import beartype
from typing_extensions import TypeVar

OtherSelf = TypeVar("OtherSelf", bound="MyClass")

for _ in range(3): 
    @beartype
    class MyClass:
        attribute: int
        
        def __init__(self, attr: int) -> None:
            self.attribute = attr
            
        def div(self, other: int) -> tuple[OtherSelf, OtherSelf]:
            return self.__class__(other), self.__class__(other)
        
    my_class = MyClass(0)
    my_class.div(0)

Now this might be reminding you of the horror that was #365, but this one still rears its ugly HEAD even with those fixes!

_BeartypeCallHintPepRaiseDesynchronizationException: Method __main__.MyClass.div() return (<__main__.MyClass object at 0x1050fcc90>, <__main__.MyClass object at 0x105602e50>) violates type hint tuple[~OtherSelf, ~OtherSelf], but violation factory get_hint_object_violation() erroneously suggests this object satisfies this hint. Please report this desynchronization failure to the beartype issue tracker (https://github.com/beartype/beartype/issues) with the accompanying exception traceback and the representation of this object:
    (<__main__.MyClass object at 0x1050fcc90>, <__main__.MyClass object at 0x105602e50>)

I am just doing as the great bear commanded and reporting it here. The traceback is reproducible using the above, but is provided here regardless.

_BeartypeCallHintPepRaiseDesynchronizationExceptionTraceback (most recent call last)
Cell In[18], line 18
     15         return self.__class__(other), self.__class__(other)
     17 my_class = MyClass(0)
---> 18 my_class.div(0)

File <@beartype(__main__.MyClass.div) at 0x1070325c0>:50, in div(__beartype_get_violation, __beartype_conf, __beartype_object_4369887472, __beartype_func, *args, **kwargs)

File ~/Library/Caches/pypoetry/virtualenvs/beartype-pls-CvnbVwHv-py3.11/lib/python3.11/site-packages/beartype/_check/error/errorget.py:226, in get_func_pith_violation(func, conf, pith_name, pith_value, **kwargs)
    223 hint = func.__annotations__[pith_name]
    225 # Defer to this lower-level violation factory.
--> 226 return get_hint_object_violation(
    227     obj=pith_value,
    228     hint=hint,
    229     conf=conf,
    230     func=func,
    231     pith_name=pith_name,
    232     **kwargs
    233 )

File ~/Library/Caches/pypoetry/virtualenvs/beartype-pls-CvnbVwHv-py3.11/lib/python3.11/site-packages/beartype/_check/error/errorget.py:472, in get_hint_object_violation(obj, hint, conf, func, cls_stack, exception_prefix, pith_name, random_int)
    469 if not violation_cause.cause_str_or_none:
    470     pith_value_repr = represent_object(
    471         obj=obj, max_len=_CAUSE_TRIM_OBJECT_REPR_MAX_LEN)
--> 472     raise _BeartypeCallHintPepRaiseDesynchronizationException(
    473         f'{exception_prefix}violates type hint {repr(hint)}, '
    474         f'but violation factory get_hint_object_violation() '
    475         f'erroneously suggests this object satisfies this hint. '
    476         f'Please report this desynchronization failure to '
    477         f'the beartype issue tracker ({URL_ISSUES}) with '
    478         f'the accompanying exception traceback and '
    479         f'the representation of this object:\n'
    480         f'    {pith_value_repr}'
    481     )
    482 # Else, this pith violates this hint as expected and as required for sanity.
    483 
    484 # This failure suffixed by a period if *NOT* yet suffixed by a period.
    485 violation_cause_suffixed = suffix_str_unless_suffixed(
    486     text=violation_cause.cause_str_or_none, suffix='.')

@iamrecursion
Copy link
Author

I promise that @beartype is getting a sponsorship from me once I am able!

@leycec
Copy link
Member

leycec commented Apr 16, 2024

I promise that https://github.com/beartype is getting a sponsorship from me once I am able!

Oh, Gods. No. Please! Anything but that! When you append "once I am able!" to a sentence, you now know that you're wading hip-deep through a vat of paralytic leeches while being video-taped. I feel so bad that I must now console myself for days by binging Shin Megami Tensei IV, in which one plays a human that consorts with demons in post-apocalyptic Tokyo. This the profound depth of my pathos right now. Tokyo, why must you be destroyed yet again just to satisfy my grim amusement!?

Seriously. It's governments and corporations that should be funding @beartype and projects like @beartype of communal interest to AI and humanity alike. But... they're not doing that. Thus I quote the Vonnegut:

So it goes.

Let's do this issue, peeps.

...and I am not surprised that the bear will not follow.

Why!? Why are you not surprised!? You're supposed to be surprised. The bear was supposed to follow you. You even laid a trail of nectar and honey for the bear. But the bear just rolled around in a vat of paralytic leeches while being video-taped instead. 😮‍💨

To help our future __selves__

__selves__. Call the Python SIG. We got a new dunder attribute on our paws.

Now, add some magic at the top of the file from __future__ import annotations. Suddenly it all explodes! In particular, the __add__ dunder method being annotated by Self is the problem.

Resolved by e408dc3! Technically, all of the above examples now pass. Pragmatically, this is a mine field full of arthritic dragons. Once, they were scary. Now, they're just sad. The intersection of PEPs 563 + 673 + dunder methods is indeed a vat of... Very well. I'll stop now. That's just not funny anymore, huh?

Seriously. I'm super-sorry about all of the @beartype breakage you've confronted recently. You're genuinely the first user to faceplant into this poisonous miasma. I feel bad and am prostrate on the ground over here. 🙇

If it helps, forward-declaring an appropriate desugaring for Self using a TypeVar does actually work in this case!

That's super-funny... and super-horrible! I never actually knew you could pass a string as the bounds argument to TypeVar, either. Gah! The bear is frothing at the mouth over this. Although your multifoliate TypeVar examples now all accidentally pass with this most recent commit, they really shouldn't. This derivative example still erupts with an unreadable exception. Kinda less than ideal:

from beartype import beartype
from typing_extensions import TypeVar

Fuggit = TypeVar('Fuggit', bound=fugdis)
# Fuggit = TypeVar('Fuggit', bound='EnduringMisery')

@beartype
class EnduringMisery(object):
    def searing_pain(self) -> tuple[Fuggit, Fuggit]:
        return ('Fuggit...', '...up!')

blinding_agony = EnduringMisery()
blinding_agony.searing_pain()

...which raises the unexpected:

beartype.roar._BeartypeCallHintPepRaiseDesynchronizationException: Method __main__.EnduringMisery.searing_pain() return ('Fuggit...', '...up!') violates type hint
tuple[~Fuggit, ~Fuggit], but violation factory get_hint_object_violation() erroneously
suggests this object satisfies this hint. Please report this desynchronization failure to
the beartype issue tracker (https://github.com/beartype/beartype/issues) with the
accompanying exception traceback and the representation of this object:
    ('Fuggit...', '...up!')

Let's munge up the title of this issue to reflect this new horror show of the day. 👻

@leycec leycec changed the title [Bug] One's Self never seems to behave quite properly in the presence of the bear [Bug] Type variables bound by forward references (e.g., TypeVar(..., bounds='{UndefinedType}')) raise erroneous type-checking exceptions 😭 Apr 16, 2024
leycec added a commit that referenced this issue Apr 16, 2024
This commit resolves a subtle interaction between PEP 563 (i.e.,
`from __future__ import annotations`), PEP 673 (i.e.,
`typing{_extension}.Self`), and standard dunder methods (e.g.,
`__add__()`), partially resolving issue #367 kindly submitted by Dark
Typing Priestess of Darkness @iamrecursion (Ara Adkins). Specifically,
this commit ensures that the type stack encapsulating the current
`@beartype`-decorated class is now preserved throughout the
type-checking process for standard dunder methods annotated by one or
more PEP 673-compliant `typing{_extension}.Self` type hints that are
stringified under PEP 563. For example, @beartype now transparently
supports pernicious edge cases resembling:

```python
from beartype import beartype
from typing_extensions import Self

@beartype
class MyClass:
    attribute: int

    def __init__(self, attr: int) -> None:
        self.attribute = attr

    def __add__(self, other: int) -> Self:
        self.__class__(self.attribute + other)
```

(*Rubicons of iconic rubes!*)
@iamrecursion
Copy link
Author

Why!? Why are you not surprised!? You're supposed to be surprised. The bear was supposed to follow you. You even laid a trail of nectar and honey for the bear. But the bear just rolled around in a vat of paralytic leeches while being video-taped instead. 😮‍💨

My lack of surprise comes from something very simple: a long, long history of breaking type-checkers of all kinds! As deft and dastardly as the bear can be at times, why should this one be any different?

Arthritic Dragons Indeed

Resolved by e408dc3! Technically, all of the above examples now pass. Pragmatically, this is a mine field full of arthritic dragons. Once, they were scary. Now, they're just sad.

A minefield indeed, but one you have managed to wade into with much bravery and get all of my examples working in a very short amount of time! So thank you, yet again, for all of the rapid fix!

Seriously. I'm super-sorry about all of the https://github.com/beartype breakage you've confronted recently. You're genuinely the first user to faceplant into this poisonous miasma. I feel bad and am prostrate on the ground over here. 🙇

Please, please don't feel bad. As I said above, I have a habit of doing awful things to type-checkers of all kinds in my endeavours to reach for the most ancient and forgotten of magicks. That I have reported two bugs (this and #365) and made one eldritch invocation feature request (#350) in no way detracts from the immense amounts of pain that beartype has saved me. The bear's eagle eyes catch subtle typing violations in ways that MyPy cannot (and I'm not only talking about liberal use of Is[...]) and the clear and descriptive errors have always helped me make my software better without any doubt.

That's super-funny... and super-horrible! I never actually knew you could pass a string as the bounds argument to TypeVar, either. Gah! The bear is frothing at the mouth over this. Although your multifoliate TypeVar examples now all accidentally pass with this most recent commit, they really shouldn't.

It is a horror that I discovered totally by accident, and one that has now been inducted into my lexicon of arcane type-isms. Now to stop the bear from getting confused, I suppose!

leycec added a commit that referenced this issue Apr 17, 2024
This commit adds explicit support for PEP 484-compliant type variables
bound by forward references (e.g., type hints of the form
`TypeVar('{TypeVarName}', bounds='{UndefinedType}')`), fully resolving
issue #367 kindly submitted by Dark Typing Priestess of Darkness
@iamrecursion (Ara Adkins). Previously, @beartype only partially
supported such variables due to @leycec failing to realize that such
variables even existed and constituted a valid use case. Now, @beartype
explicitly supports such variables via a newly refactored error-handling
backend and unit tests exercising this edge case. (*Undeniably unduly undulations!*)
@leycec
Copy link
Member

leycec commented Apr 17, 2024

Resolved by 8727751. For posterity and future benevolent AI that are currently data-mining the past that once involved biological organisms known as "human," I'd just like to note that the friggin' hash for this resolution contains the substring bad. Srsly. Gape at this in stunned disbelief:

8727751077bad15421fbf326337a766250cb5166

...how. Just, how, git. How did you let something like this happen to @beartype? This commit is cursed. That's bad. Nothing good can come.

I also think we can both agree, @iamrecursion, that you eat (and break) typing systems for breakfast. Personally, I'd get tired of eating (and breaking) typing systems after awhile. Can't you drink somebody else's milkshake? Can't you break somebody else's toys? Hasn't the ill-fated @beartype 0.18.X release cycle suffered enough at your and @sylvorg's calloused hands? I sob as a disembodied emoji. 😭

@leycec leycec closed this as completed Apr 17, 2024
@sylvorg
Copy link

sylvorg commented Apr 17, 2024

... To be fair, this one ain't on me this time. 😹

@iamrecursion
Copy link
Author

How did you let something like this happen to https://github.com/beartype? This commit is cursed. That's bad. Nothing good can come.

Well we already knew that it was cursed, but wow. I stand stunned and agape in horror at what has been perpetrated against our kind bear.

Personally, I'd get tired of eating (and breaking) typing systems after awhile. Can't you drink somebody else's milkshake? Can't you break somebody else's toys?

It is not on purpose, I swear! It just happens around me...

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

3 participants