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

Signature inference for attributes inherited from generic dataclasses #1780

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

brentyi
Copy link

@brentyi brentyi commented May 24, 2021

Thanks for the great library!

Just patched an issue where dataclasses that also happen to be generics aren't being recognized when climbing the MRO. See example in test file.

@brentyi
Copy link
Author

brentyi commented May 24, 2021

Actually, this is a little more complicated than I first imagined. Just pushed another test; this one fails:

from typing import Generic, TypeVar
T = TypeVar("T")

@dataclass
class Z:
    z: int # not recognized

@dataclass
class Y(Z, Generic[T]):
    y: T

@dataclass
class X(Y[int]):
    name: str
    foo = 3
    price: float
    quantity: int = 0.0

When I add print(cls) to get_signatures() here, the superclass is getting parsed as a DataclassWrapper inside a Decoratee inside a GenericClass. Maybe this is expected? But not sure how to deal with it:

<GenericClass: TypingClassWithGenerics(Generic<LazyG>[S{<TypeVar: T>}])[S{<ClassValue: <Class: int@164-240>>}]>
<GenericClass: <ClassValue: <Class: object@83-109>>[S{<ClassValue: <Class: int@164-240>>}]>
<GenericClass: Decoratee(DataclassWrapper(<ClassValue: <Class: Z@6-8>>))[S{<ClassValue: <Class: int@164-240>>}]>
<GenericClass: DataclassWrapper(<ClassValue: <Class: Y@9-11>>)[S{<ClassValue: <Class: int@164-240>>}]>
DataclassWrapper(<ClassValue: <Class: X@12-17>>)

Could use some help because this would be a nice thing to have working. 🙂

@davidhalter
Copy link
Owner

This is definitely not an easy one. In general it's a pretty bad idea to use isinstance in Jedi. It's also in general a code smell, but here it will probably break in some places.

Nevertheless I of course appreciate your effort! The failing tests are usually half of the work.

I feel like the way to solve this is probably to make it a DataclassWrapper after it was converted to a GenericClass and not the other way around. It seems like the data class wrapper is applied before the generic class wrapper - which looks like a bug. I'm not sure how this is even possible, but it looks like it is. This is probably an issue for some other decorators as well. So, thanks for bringing it up.

@brentyi
Copy link
Author

brentyi commented May 26, 2021

@davidhalter thanks for the quick response!

I spent a bit more time looking at this and pushed a slightly less specific fix, which just knows to iterate into ValueWrapper and LazyValueWrapper objects. If there's something much more fundamental that needs to be changed I may be underqualified 😅

On the GenericClass wrapper being applied after the DataclassWrapper: my intuition for what's going on is extremely cloudy, but I think this makes sense because the generic wrapper is applied from Y[int] in the base class list, as opposed to the original class Y(Generic[T]) declaration.

@brentyi brentyi changed the title Signature inference for attributes inherited from generic dataclasses Signature inference for generic dataclasses Jul 29, 2021
@brentyi brentyi changed the title Signature inference for generic dataclasses Signature inference for attributes inherited from generic dataclasses Jul 29, 2021
@brentyi
Copy link
Author

brentyi commented Jan 24, 2022

@davidhalter any chance you could take a second look at this PR? It'd still be super helpful for myself + some collaborators. (though I totally understand if you'd prefer not to merge!)

The current solution relies on an isinstance() call, but it doesn't seem like it should be any less robust than the existing code.

@davidhalter
Copy link
Owner

davidhalter commented Jan 26, 2022

@brentyi Sorry, but I'm choosing not to merge it, because I'm not sure I understand all its implications. isinstance is really not the way Jedi works internally, everything can be wrapped. So even if this is working for you, someone else is going to report bugs, because of this.

So I'm not at all going to close this, because I can probably still use 90% of this and merge it at some point, but for now I won't. The issue mentioned above needs to be addressed first.

@brentyi
Copy link
Author

brentyi commented Jan 26, 2022

Sounds sensible; appreciate the update!

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.

2 participants