biqqles / dataclassy

A fast and flexible reimplementation of data classes
https://pypi.org/project/dataclassy
Mozilla Public License 2.0
81 stars 9 forks source link

Inheriting from a dataclass using ABCMeta causes `AttributeError` #46

Closed fubuloubu closed 3 years ago

fubuloubu commented 3 years ago

Still debugging, but it appears to be caused by this line in this commit (which is present starting in v0.10.1)

https://github.com/biqqles/dataclassy/commit/0f34238b17ed0bd94ef2751ff827a89f44499c1a#diff-daf1e1a0a18a32d8c50faa743257d9e130ac95adeab1fddb2932525d147c7520R80


Error:

ImportError while loading conftest '~/work/ApeWorX/ape/tests/conftest.py'.
...
src/ape_accounts/accounts.py:34: in <module>
    class KeyfileAccount(AccountAPI):
~/.venv/python3.8/site-packages/dataclassy/dataclass.py:80: in __new__
    all_attrs = {a for b in bases for a in dir(b) if is_user_func(getattr(b, a))} | dict_.keys()
~/.venv/python3.8/site-packages/dataclassy/dataclass.py:80: in <setcomp>
    all_attrs = {a for b in bases for a in dir(b) if is_user_func(getattr(b, a))} | dict_.keys()
E   AttributeError: __abstractmethods__

I'm probably using it incorrectly, would be open for a better way to create subclass-able dataclass classes with abstract methods. It's a pretty integral part of how we're using it for our plugin system.


For reference, AccountAPI is a dataclassy.dataclass using abc.ABCMeta abstract metaclass and defining some abc.abstractmethods. Can check it out here: https://github.com/ApeWorX/ape/blob/687032e0b122a46fc73184fd7a0bea18e7b1a383/src/ape/api/accounts.py#L62

biqqles commented 3 years ago

Wow, bizarre bug. __abstractmethods__ is in dir(b) but somehow getattr fails? I don't understand how ABCMeta is doing that but I can try adding a hasattr call (or third argument to getattr) to try and prevent it. I'll see if I can get a MWE to test against.

fubuloubu commented 3 years ago

Wow, bizarre bug. __abstractmethods__ is in dir(b) but somehow getattr fails? I don't understand how ABCMeta is doing that but I can try adding a hasattr call (or third argument to getattr) to try and prevent it. I'll see if I can get a MWE to test against.

Thanks! Yeah, quite bizarre, and funny that I noticed it ~1hr after you released :laughing: due to a PR I added failing CI.

If I come up with MWE I'll post it here

biqqles commented 3 years ago

I tried copying your classes verbatim (except for type hints) and didn't get the error on CPython 3.6/3.7 and PyPy 3.6. What version of Python are you using?

Also, I'm very curious if swapping getattr(b, a) for getattr(b, a, None) on that line fixes it, whatever the cause is.

fubuloubu commented 3 years ago

I tried copying your classes verbatim (except for type hints) and didn't get the error on CPython 3.6/3.7 and PyPy 3.6. What version of Python are you using?

Also, I'm very curious if swapping getattr(b, a) for getattr(b, a, None) on that line fixes it, whatever the cause is.

Python 3.8

biqqles commented 3 years ago

Ah, just realised I could have read the stack trace!

biqqles commented 3 years ago

Sorry, not doing well on reading tonight. Copied KeyfileAccount too and I can reproduce. The getattr tweak fixes it. I'd still be interested in an MWE to add to the unit tests.

biqqles commented 3 years ago

If you can confirm that there are no other regressions caused by v0.10.1 I can release the fix in v0.10.2 asap!

biqqles commented 3 years ago

Simply subclassing at least twice from a class with ABCMeta as its metaclass seems to be the MWE.

fubuloubu commented 3 years ago

It looks like there is a regression

It tries to pass *args, **kwargs to __post_init__ if it exists. I didn't have them so it fails when I run my tests here: https://github.com/ApeWorX/ape/blob/1131c11f0d4b381ab46fd5a4134c8590732dd617/src/ape_test/providers.py#L15

biqqles commented 3 years ago

Oh that's caused by a stupid mistake where I ensure that __post_init__ will always run (new feature), will fix.

fubuloubu commented 3 years ago

Oh that's caused by a stupid mistake where I ensure that __post_init__ will always run (new feature), will fix.

p.s. thanks for being so responsive!

fubuloubu commented 3 years ago

I'm not sure what the issue is now, but that didn't seem to alleviate the *args, **kwargs passing issue for me. Will try to debug more when I get home

biqqles commented 3 years ago

Sorry, I think I see where the error is coming from, I'm just going to remove that "feature" for now as it's not as simple as I thought.

Edit: forgot to tag the issue but done.

fubuloubu commented 3 years ago

Sorry, I think I see where the error is coming from, I'm just going to remove that "feature" for now as it's not as simple as I thought.

Edit: forgot to tag the issue but done.

Tried that, but no change. It must be something else! Will keep hunting

biqqles commented 3 years ago

Is the problem that __init__ isn't running? __post_init__ getting sent *args, **kwargs from it is normal, shouldn't be any change there.

fubuloubu commented 3 years ago

yup, now I'm getting TypeError: __init__() got an unexpected keyword argument 'name' for a subclassed abstract-implementing class where name is defined as a field

biqqles commented 3 years ago

Oh dear, what class is this?

fubuloubu commented 3 years ago

https://github.com/ApeWorX/ape/blob/1131c11f0d4b381ab46fd5a4134c8590732dd617/src/ape/api/providers.py#L10

It's hidden behind a bunch of dynamic indirection, sorry to make debugging so difficult :grimacing:

fubuloubu commented 3 years ago

Well, I'm not really sure what to make of it, but the class that is causing the problem subclasses the abstract dataclass and another random class (Web3). I think due to the MRO it's getting confused badly (I have Web3 before ProviderAPI).

If I switch the two classes in MRO, the issue resolves itself.


https://github.com/ApeWorX/ape/blob/1131c11f0d4b381ab46fd5a4134c8590732dd617/src/ape_test/providers.py#L6

fubuloubu commented 3 years ago

Alright, my fix worked! Problem on my end!

Thanks for your help!

biqqles commented 3 years ago

Ah, that must have been caused by https://github.com/biqqles/dataclassy/commit/32d129b82264ea4ca0ec3c506a212a655924ed65, sorry. __init__ is no longer forcibly replaced by dataclassy if defined by the user (to match the behaviour with every other method). Changing the order of the bases is the right fix. Just let me know if this causes any more issues. Thanks for the reporting!