[ty] Add support for dynamic dataclasses via make_dataclass#22586
[ty] Add support for dynamic dataclasses via make_dataclass#22586charliermarsh wants to merge 1 commit into
make_dataclass#22586Conversation
Typing conformance resultsNo changes detected ✅ |
|
0c2c95b to
c9c3caa
Compare
71f3816 to
929be6b
Compare
37f23f2 to
0bcbb7e
Compare
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-return-type |
1 | 4 | 4 |
invalid-argument-type |
1 | 2 | 3 |
invalid-assignment |
0 | 0 | 5 |
unused-ignore-comment |
4 | 0 | 0 |
unresolved-attribute |
0 | 3 | 0 |
| Total | 6 | 9 | 12 |
|
I'm a bit hesitant about this one. The typing spec doesn't say anything about this function AFAIK, and I don't remember seeing any user requests on the issue tracker for us to add special-cased support. Mypy_primer only shows two diagnostics going away in the ecosystem, on Meanwhile, this is quite a bit of new code, and the manual parsing of arguments in Do any other type checkers have special-cased support for this function? We do have to draw the line somewhere. I think I'd feel differently if we could share more of the call-expression-parsing logic with what we've already added for namedtuples (which definitely was necessary), but the schema Another -- much simpler -- way to avoid false positives with the objects returned by this function would be to just special-case the return type so that we infer |
|
AFAICT, Mypy and Pyright do not support it. But... I would still advocate for us to support it? We have all the infrastructure to do so! And almost all of the new code is in argument parsing -- which is very verbose, but at least fairly mechanical and testable? (E.g., it's much easier to know when we have that right, as opposed to something deep in type inference.) I think I don't agree that this is where we should draw the line, unless it's a feature that we can't support for reasons that I don't yet understand or would be uncovered by your review. |
|
Ditto and am totally fine to be overruled on it of course. I took it as a given that we'd support this given that it's in the type system overview; that may have been a mistake! |
Well, that issue just says that we don't have any tests for the function right now. But this PR adds a lot more than just test coverage 😆 |
| } | ||
|
|
||
| /// Look up an instance member by name (including superclasses). | ||
| pub(crate) fn instance_member(self, db: &'db dyn Db, name: &str) -> PlaceAndQualifiers<'db> { |
There was a problem hiding this comment.
It seems unfortunate that we have to repeat all those methods for every dynamic class literal. Can't we share more infrastructure?
b968cdf to
b66f74a
Compare
|
Can probably hold off on further review until #22627 is resolved, since it will have implications for this PR. |
|
(Marking as draft while we finish the recursive definition work upstream.) |
ccabd74 to
16f4349
Compare
dabdc49 to
74ec01d
Compare
74ec01d to
239ac3a
Compare
c41d60b to
c7ab85f
Compare
c7ab85f to
3e78ed6
Compare
|
I'm going to mark as draft because I think we should merge #22792 first. |
|
Going to close, I will re-open from a clean slate. |
Summary
Like #22327, but for dataclasses.