[ty] Add support for dynamic type() classes#22291
Conversation
|
(Not ready for review.) |
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
36229a1 to
8742a0b
Compare
|
I think the scrapy, black, and aiohttp-devtools changes are true positives. |
Merging this PR will not alter performance
Comparing Footnotes
|
bf6fb24 to
48d9181
Compare
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
unknown-rule |
0 | 156 | 0 |
invalid-return-type |
1 | 1 | 11 |
invalid-argument-type |
5 | 3 | 3 |
invalid-assignment |
0 | 2 | 5 |
unsupported-dynamic-base |
7 | 0 | 0 |
unresolved-attribute |
1 | 2 | 2 |
possibly-missing-attribute |
0 | 3 | 1 |
unused-ignore-comment |
3 | 0 | 0 |
invalid-await |
0 | 2 | 0 |
type-assertion-failure |
2 | 0 | 0 |
| Total | 19 | 169 | 22 |
6c6a3a5 to
b4e2567
Compare
|
I haven't looked too hard at this yet, but is adding a new We should also try to make our design here extensible for other classes-created-by-function-calls: functional namedtuples, functional typeddicts, and functional enums. |
|
For a previous attempt to make |
|
I can give that a try. I initially tried to add another variant to |
|
(The context around functional namedtuples, functional typeddicts, and functional enums is also very useful, thank you.) |
5744687 to
217dafb
Compare
There was a problem hiding this comment.
We should also emit a diagnostic (but don't currently on this branch) if you try to use an @final class as the base class for a dynamic class, e.g.
from typing import final
@final
class Base: ...
type("X", (Base,), {})This is maybe a counter-argument to https://github.com/astral-sh/ruff/pull/22291/files#r2678657516 -- if we looped through all dynamic class definitions in TypeInferenceBuilder::check_class_definitions as well as all the static class definitions, it might have been harder to forget to implement this check... but I'm guess there's a lot of stuff in that method that currently assumes that we have a StmtClassDef AST node, so it may be that what you have right now is the best design.
There was a problem hiding this comment.
I believe I did this in #22499. I can revisit the approach in that review (RE whether it's worth using check_class_definitions), if that works?
|
From my perspective, I think this is basically ready to go now! My remaining concerns are #22291 (comment) (which I'd like @MichaReiser's or @carljm's opinion on) and #22291 (comment). |
317f9ed to
b01743a
Compare
b01743a to
15c846c
Compare
dd8e67d to
873595a
Compare
80828b4 to
7403df1
Compare
|
I dropped support for "dangling" |
05c1afb to
5a177bb
Compare
AlexWaygood
left a comment
There was a problem hiding this comment.
LGTM! (But maybe give @carljm a chance to have another once-over if he wants to, before landing 😄)
6271d09 to
8df0d85
Compare
8df0d85 to
dd2fbb3
Compare
## Summary Addresses #22291 (comment).
## Summary This PR is intended to demonstrate how the pattern established in #22291 generalizes to other class "kinds". Closes astral-sh/ty#1049. --------- Co-authored-by: Alex Waygood <alex.waygood@gmail.com>
| /// [`unsupported-base`]: https://docs.astral.sh/ty/rules/unsupported-base | ||
| pub(crate) static UNSUPPORTED_DYNAMIC_BASE = { | ||
| summary: "detects dynamic class bases that are unsupported as ty could not feasibly calculate the class's MRO", | ||
| status: LintStatus::preview("1.0.0"), |
There was a problem hiding this comment.
It looks like claude loves to make this up as preview("1.0.0")
Summary
This PR adds support for dynamic classes created via
type(). The core of the change is thatClassLiteralis now an enum:And, in turn, various methods on
ClassLiterallikebody_scopenow returnOptionor similar (and callers must adjust to that change in signature).Over time, we can expand the enum to include functional namedtuples, etc. (I already have this working in a separate branch, and I believe it slots in well.)
(I'd love help with the names -- I think
StmtClassLiteralis kind of lame. MaybeDeclarativeClassLiteral?)Closes astral-sh/ty#740.