-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[red-knot] Add MRO resolution for classes (take 2) #14027
Conversation
9bb38e5
to
723169d
Compare
5327372
to
b8325fc
Compare
|
b8325fc
to
c36f055
Compare
3e75482
to
d1ccf88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great and I like the simplification. I would be interested in a few more cycle tests to make sure that indirect cycles are correctly handled too.
CodSpeed Performance ReportMerging #14027 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not finished reviewing yet, but submitting some comments now rather than waiting because you're actively working on the branch :)
Overall, this is awesome!
crates/red_knot_python_semantic/resources/mdtest/scopes/moduletype_attrs.md
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Some comments, but nothing blocking besides the cycle handling.
abf1496
to
53fa1f9
Compare
crates/red_knot_python_semantic/resources/mdtest/stubs/class.md
Outdated
Show resolved
Hide resolved
7a6002a
to
fc44949
Compare
# TODO: can we avoid emitting the errors for these? | ||
# The classes have cyclic superclasses, | ||
# but are not themselves cyclic... | ||
class Baz(Bar, BarCycle): ... # error: [cyclic-class-def] | ||
class Spam(Baz): ... # error: [cyclic-class-def] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty unhappy about the cascading errors here. I might work on a followup PR to see if I can avoid them.
Unfortunately a solution doesn't seem trivial (at least from what I can see). Also, this situation should really be very rare (I don't see any plausible reason why anybody would try to make a class inherit from itself, even indirectly, in real code). So I think this is okay, if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a reason why they would want to do that but I remember that I at least accidentally did just that more than once in my early career because I was unaware that it leads to a cycle.
ff08396
to
ae91fc7
Compare
This is great! |
This reverts commit 7c724d4.
ae91fc7
to
44f7403
Compare
79aded2
to
a9d379b
Compare
Thanks both for the excellent review comments! |
🎉 |
* main: (39 commits) Also remove trailing comma while fixing C409 and C419 (astral-sh#14097) Re-enable clippy `useless-format` (astral-sh#14095) Derive message formats macro support to string (astral-sh#14093) Avoid cloning `Name` when looking up function and class types (astral-sh#14092) Replace `format!` without parameters with `.to_string()` (astral-sh#14090) [red-knot] Do not panic when encountering string annotations (astral-sh#14091) [red-knot] Add MRO resolution for classes (astral-sh#14027) [red-knot] Remove `Type::None` (astral-sh#14024) Cached inference of all definitions in an unpacking (astral-sh#13979) Update dependency uuid to v11 (astral-sh#14084) Update Rust crate notify to v7 (astral-sh#14083) Update cloudflare/wrangler-action action to v3.11.0 (astral-sh#14080) Update dependency mdformat-mkdocs to v3.1.1 (astral-sh#14081) Update pre-commit dependencies (astral-sh#14082) Update dependency ruff to v0.7.2 (astral-sh#14077) Update NPM Development dependencies (astral-sh#14078) Update Rust crate thiserror to v1.0.67 (astral-sh#14076) Update Rust crate syn to v2.0.87 (astral-sh#14075) Update Rust crate serde to v1.0.214 (astral-sh#14074) Update Rust crate pep440_rs to v0.7.2 (astral-sh#14073) ...
A second attempt at implementing MRO resolution (see #13722 for the first attempt). Unlike the first attempt, this version does not attempt to handle union types in a class's bases list (which would potentially mean that you would have to consider multiple possible MROs for any given class).
Summary
A Python class's "Method Resolution Order" ("MRO") is the order in which superclasses of that class are traversed by the Python runtime when searching for an attribute (which includes methods) on that class. Accurately inferring a class's MRO is essential for a type checker if it is going to be able to accurately lookup the types of attributes and methods accessed on that class (or instances of that class).
For simple classes, the MRO (which is accessible at runtime via the
__mro__
attribute on a class) is simple:For more complex classes that use multiple inheritance, things can get a bit more complicated, however:
And for some classes, Python realises that it cannot determine which order the superclasses should be positioned in order to create the MRO at class creation time:
The algorithm Python uses at runtime to determine what a class's MRO should be is known as the C3 linearisation algorithm. An in-depth description of the motivations and details of the algorithm can be found in this article in the Python docs. The article is quite old, however, and the algorithm given at the bottom of the page is written in Python 2. As part of working on this PR, I translated the algorithm first into Python 3 (see this gist), and then into Rust (the
c3_merge
function inmro.rs
in this PR). In order for us to correctly infer a class's MRO, we need our own implementation of the C3 linearisation algorithm.As well as implementing the C3 linearisation algorithm in Rust, I also had to make some changes to account for the fact that a class might have a dynamic type in its bases: in our current model, we have three dynamic types, which are
Unknown
,Any
andTodo
. This PR takes the simple approach of deciding that the MRO ofAny
is[Any, object]
, the MRO ofUnknown
is[Unknown, object]
, and the MRO ofTodo
is[Todo, object]
; other than that, they are not treated particularly specially by the C3 linearisation algorithm. Other than simplicity, this has a few advantages:object
.Dynamic types will have to be treated specially when it comes to attribute and method access from these types; however, that is for a separate PR.
Implementation strategy
The implementation is encapsulated in a new
red_knot_python_semantic
submodule,types/mro.rs
.ClassType::try_mro
attempts to resolve a class's MRO, and returns an error if it cannot;ClassType::mro
is a wrapper aroundClassType::try_mro
that resolves the MRO to[<class in question>, Unknown, builtins.object]
if no MRO for the class could be resolved.It's necessary for us to emit a diagnostic if we can determine that a particular list of bases will (or could) cause a
TypeError
to be raised at runtime due to an unresolvable MRO. However, we can't do this while creating theClassType
and storing it inself.types.declarations
ininfer.rs
, as we need to know the bases of the class in order to determine its MRO, and some of the bases may be deferred. This PR therefore iterates over all classes in a certain scope after all types (including deferred types) have been inferred, as part ofTypeInferenceBuilder::infer_region_scope
. For types that will (or could) raise an exception due to an invalid MRO, we infer the MRO as being[<class in question>, Unknown, object]
as well as emitting the diagnostic.We also emit diagnostics for classes that inherit from bases which would be too complicated for us to resolve an MRO for: anything except a class-literal,
Any
,Unknown
orTodo
is rejected.I deleted the
ClassType::bases()
method, because:ClassType::bases()
intypes.rs
orinfer.rs
anymore now (they all should have been iterating over the MRO all along!).bases()
method was something of a footgun: it only gave you the slice of the class's explicit bases, and ignored the fact that a class will implicitly haveobject
in its bases list at runtime if it has no explicit bases.Test Plan
Lots of mdtests added. Several tests taken from https://docs.python.org/3/howto/mro.html#python-2-3-mro.