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: A class that derives from a dataclass should be labelled a dataclass #239

Closed
has2k1 opened this issue Feb 14, 2024 · 2 comments
Closed
Assignees

Comments

@has2k1
Copy link
Contributor

has2k1 commented Feb 14, 2024

Here is a failing test.

code = '''
from dataclasses import dataclass

@dataclass
class Base:
    pass

class Derived(Base):
    pass
'''

with temporary_visited_module(code) as module:
    obj = module["Derived"]
    assert "dataclass" in obj.labels

For reference, the parallel test in core python is.

from dataclasses import dataclass, is_dataclass

@dataclass
class Base:
    pass

class Derived(Base):
    pass

assert is_dataclass(Derived)
@pawamoy
Copy link
Member

pawamoy commented Feb 14, 2024

Hmmm, this one is problematic. Consider the following example:

from dataclasses import dataclass

@dataclass
class A:
    a: int

class B(A):
    b: int

@dataclass
class C(B):
    c: int

print(inspect.signature(A))
print(inspect.signature(B))
print(inspect.signature(C))

Running it will print:

(a: int) -> None
(a: int) -> None
(a: int, c: int) -> None

It means that, to build the __init__ method ourselves during static analysis, we have to list attributes from parent classes that are dataclasses, excluding ones that are not. We tell them apart thanks to the dataclass label. If we mark B as a dataclass, then we will wrongly add b as a parameter to C's __init__ method.

If we want to support this, I suppose we will have to use something else than the dataclass label to tell classes apart. Maybe something internal, stored in our extra metadata... Not sure yet. It seems to be what dataclasses itself does:

def is_dataclass(obj):
    """Returns True if obj is a dataclass or an instance of a
    dataclass."""
    cls = obj if isinstance(obj, type) else type(obj)
    return hasattr(cls, _FIELDS)

I find this confusing IMO, because in the above example, B has a _FIELDS attribute only because it inherits it from A, but it actually does not benefit from dataclass transformations, so it's a bit far-fetched to call it a dataclass.

Any other idea?

EDIT: Well I guess we can check the decorators everytime instead of the labels.

Dynamic transformations at import time are not fun for static analysis... I like dataclasses less now, they're a can of worms in this context 😂

@has2k1
Copy link
Contributor Author

has2k1 commented Feb 14, 2024

Yes checking the decorators. I did not know about it. Thanks for that one, I will change my "is_dataclass" checks to use that.

pawamoy added a commit that referenced this issue Mar 5, 2024
Instead of generating parameters on the fly by (wrongly) checking attributes of the class,
we always load a Griffe extension that re-creates `__init__` methods and their parameters.

Issue-33: #233
Issue-34: #234
Issue-38: #238
Issue-39: #239
PR-240: #240
@pawamoy pawamoy closed this as completed Mar 5, 2024
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