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

Fixed infinite recursion when pytypes tries to access .__orig_class__ on an instance of a @typechecked class with __getattr__/__getattribute__ #53

Merged
merged 4 commits into from
May 13, 2019

Conversation

lubieowoce
Copy link
Contributor

@lubieowoce lubieowoce commented Aug 31, 2018

Hi! I hit this problem yesterday and dived in to fix it because I was excited to use pytypes in my library :)
My environment is Python 3.5 running inside pipenv on Windows, and a fresh install of pytypes==1.0b5.post2+dirty.

Here's a simple case that should demonstrate the problematic behavior:

from pytypes import typechecked

@typechecked
class Bad:
    def __getattr__(self, attr: str):
        if attr == 'one':
            return 1
        else:
            raise AttributeError(attr)

bad = Bad()
res = bad.one  # causes infinite recursion into the wrapped `__getattr__`

If, like me, you keep mixing up __getattr__/__getattribute__,
__getattr__ is the one that acts like a fallback –
it's only called if other methods of attribute lookup failed:

- the attr wasn't in the instance dict
- there's a descriptor for the attr in the class dict but it raised AttributeError
- the attr wasn't in the class dict either

On an unwrapped class, bad.one would result in Thing.__getattr__(bad, 'one') and work fine.
When wrapped with @typechecked, it becomes be Thing.checker_tp<__getattr__>(bad, 'one'), where checker_tp<__getattr__> is the checker_tp (from pytypes.typechecker._typinspect_func) that wraps __getattr__.
Unfortunately, in its code, checker_tp<__getattr__> does this:

try:
    parent_class = args_kw[0].__orig_class__
except AttributeError:
    ...

We're in a method call, so args_kw[0] is self – our bad object. But bad.__orig_class__ – a dot-access on a nonexistent attribute – ends up calling checker_tp<__getattr__>(bad, '__orig_class__') again, which tries to get the __orig_class__ again, and so on until it hits a RecursionError.

The way I solve this in this pull request is simple –
Use object.__getattribute__(x, '__orig_class__') instead, thus bypassing a classes' __getattr__ and __getattribute__. I'm not familiar with pytypes' internals, So I don't know what consequences this could have; I imagine it might lead to unexpected behaviour on proxy-like classes.

Ideas for alternative solutions (FWIW from a person unfamiliar with the code):

  • Wrap __getattr__/__getattribute__, but special-case them somehow so that pytypes internals know they should use the unwrapped version instead. I'm pretty fuzzy on the details, but it seems plausible.

  • Don't wrap __getattr__/__getattribute__ at all.
    Unfortunately if the original __getattr[ibute]__ returns a method, an un-wrapped method would 'leak' out. But I guess this could happen with any method, so maybe it's not a solvable problem anyway.
    (I guess in that case pytypes could do identity checks with known methods from the class dict, and returning the appropriate wrapped method instead if possible. But that'd require wrapping __getattr[ibute]__, which we're trying to avoid.)

I hope this wasn't too long winded. Please tell me what you think about this solution!

@lubieowoce lubieowoce changed the title Fixed infinite recursion when pytypes tries to access __orig_class__ on an object of a @typecked class with __getattr[ibute]__ Fixed infinite recursion when pytypes tries to access .__orig_class__ on an instance of a @typecked class with __getattr[ibute]__ Aug 31, 2018
@lubieowoce lubieowoce changed the title Fixed infinite recursion when pytypes tries to access .__orig_class__ on an instance of a @typecked class with __getattr[ibute]__ Fixed infinite recursion when pytypes tries to access .__orig_class__ on an instance of a @typechecked class with __getattr[ibute]__ Aug 31, 2018
@lubieowoce lubieowoce changed the title Fixed infinite recursion when pytypes tries to access .__orig_class__ on an instance of a @typechecked class with __getattr[ibute]__ Fixed infinite recursion when pytypes tries to access .__orig_class__ on an instance of a @typechecked class with __getattr__/__getattribute__ Sep 1, 2018
@Stewori
Copy link
Owner

Stewori commented Sep 2, 2018

Hey, thanks for working on this. I came to the conclusion that Python allows so much overriding, monkeypatching, wrapping of all sorts of stuff that we probably never get rid of such issues completely.
Anyway, we can still try...
That said, it would be perfect to have a test that fails before the commit and passes afterwards. Link the commit in a comment in the test. If your commit further does not break any existing test I am fine with accepting. However it looks good to me, although I cannot fully retrace the use case (thats why a test would be nice). Additionally a test saves from regression, i.e. from breaking this fix.

(at least on my machine - Windows 10, Python 3.5)
@lubieowoce
Copy link
Contributor Author

Done! As you asked, the tests break before and work after (at least on my machine - is it possible to make Travis run the new tests on an older version?)

@Stewori
Copy link
Owner

Stewori commented May 13, 2019

I had to modify this slightly to fit into some changes I had made towards Python 3.7 support. Also moved your new function to a position where I find it belongs best.
That said, I observe that especially in the first 200 lines of type_util frequently dunder accesses occur, e.g. to __origin__, __extra__, __args__, __class__. Do you think a similar fix should be applied to these to prevent wrapping issues. Or would a better wrapping approach fix this issue in a broader sense, e.g. wrapt, c.f. #30.

@Stewori Stewori merged commit 5db5c49 into Stewori:master May 13, 2019
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

Successfully merging this pull request may close these issues.

None yet

2 participants