[ty] Defer base inference for functional type(...) classes#22792
Merged
charliermarsh merged 18 commits intomainfrom Feb 14, 2026
Merged
[ty] Defer base inference for functional type(...) classes#22792charliermarsh merged 18 commits intomainfrom
type(...) classes#22792charliermarsh merged 18 commits intomainfrom
Conversation
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Typing conformance resultsNo changes detected ✅ |
|
charliermarsh
force-pushed
the
charlie/defer-bases
branch
2 times, most recently
from
e5ac3bf to
9c3e5ec
Compare
AlexWaygood
reviewed
Jan 26, 2026
Member
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Looks to be along the right lines! A few comments
Comment on lines
5134
to
5137
| DynamicClassAnchor::ScopeOffset { .. } => { | ||
| // For dangling calls, the bases were already inferred as part of the scope. | ||
| infer_complete_scope_types(db, scope).expression_type(bases_arg) | ||
| } |
Member
There was a problem hiding this comment.
infer_complete_scope_types is somewhat expensive, and more likely to lead to cycles than definition_expression_type. I also don't think it's necessary to lazily infer the bases for a dangling type() call, as it's impossible for a dangling type() call to refer recursively back to the class that the dangling call creates -- I think it's only necessary to defer inference of bases when the type() call is immediately assigned to a variable. So for DynamicClassAnchor::ScopeOffset{}, I would be inclined to store the inferred types of the bases directly on the enum variant, in the same way that store the NamedTupleSpec directly on the DynamicNamedTupleAnchor::ScopeOffset{} variant
Comment on lines
731
to
744
| /// These mirror the relevant variants from `MroErrorKind` for static classes. | ||
| #[derive(Debug, Clone, PartialEq, Eq, get_size2::GetSize, salsa::Update)] | ||
| pub(crate) enum DynamicMroErrorKind<'db> { | ||
| /// The class inherits from one or more invalid bases. | ||
| /// | ||
| /// Similar to `StaticMroErrorKind::InvalidBases`, this records the indices | ||
| /// and types of bases that could not be converted to valid class bases. | ||
| InvalidBases(Box<[(usize, Type<'db>)]>), | ||
|
|
||
| /// A cycle was encountered resolving the class' bases. | ||
| InheritanceCycle, | ||
|
|
||
| /// The class has duplicate bases in its bases tuple. | ||
| DuplicateBases(Box<[ClassBase<'db>]>), |
Member
There was a problem hiding this comment.
it seems like the only variant that exists on StaticMroErrorKind but not this one is now Pep695ClassWithGenericInheritance. Which makes me wonder if it's really still worth having two enums, or if they could be combined?
Hmm, but maybe there's still enough annoying differences in the wrapped data that it's worth having the two enums stay different
Comment on lines
6656
to
6667
| // For assigned `type()` calls, the bases are inferred via deferred inference after the | ||
| // class is created. For dangling calls, `bases_type` is already available from immediate | ||
| // inference. | ||
| let bases_type = | ||
| bases_type.unwrap_or_else(|| self.infer_expression(bases_arg, TypeContext::default())); | ||
|
|
||
| // Validate bases and collect disjoint bases for diagnostics. | ||
| let mut disjoint_bases = self.validate_dynamic_type_bases(bases_arg, bases_type, &name); | ||
|
|
||
| // Check for MRO errors. | ||
| match dynamic_class.try_mro(db) { | ||
| Err(error) => match error.reason() { |
Member
There was a problem hiding this comment.
same comment as #22586 (comment) -- immediately asking the types of the bases to be resolved like this seems like it will immediately trigger Salsa's cycle handling if we are inferring the type of a type() call where the bases refer recursively to the type being defined. For non-dangling type() calls, we should postpone these checks until we've inferred types for the rest of the scope, in the same way that we do for static classes in check_class_definitions()
carljm
reviewed
Jan 26, 2026
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Was partway through a review, but since Alex just submitted a review, I'll submit what I have to avoid multiple rounds of revision. Still some things I don't quite understand here.
Comment on lines
6339
to
6351
| @@ -6340,6 +6340,15 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { | |||
| self.infer_newtype_assignment_deferred(arguments); | |||
| return; | |||
| } | |||
| if known_class == Some(KnownClass::Type) { | |||
| // Infer the `bases` argument for three-argument `type()` calls. | |||
| // This is deferred to break cycles for self-referential definitions | |||
| // like `X = type("X", (tuple["X | None"],), {})`. | |||
| if arguments.args.len() >= 2 { | |||
| self.infer_expression(&arguments.args[1], TypeContext::default()); | |||
| } | |||
| return; | |||
| } | |||
| // class is created. For dangling calls, `bases_type` is already available from immediate | ||
| // inference. | ||
| let bases_type = | ||
| bases_type.unwrap_or_else(|| self.infer_expression(bases_arg, TypeContext::default())); |
Contributor
There was a problem hiding this comment.
As discussed above, I think this addition is a plausible-looking no-op. We've only "deferred" anything from a few lines above in the same method, which doesn't actually do anything.
Comment on lines
6343
to
6351
| if known_class == Some(KnownClass::Type) { | ||
| // Infer the `bases` argument for three-argument `type()` calls. | ||
| // This is deferred to break cycles for self-referential definitions | ||
| // like `X = type("X", (tuple["X | None"],), {})`. | ||
| if arguments.args.len() >= 2 { | ||
| self.infer_expression(&arguments.args[1], TypeContext::default()); | ||
| } | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
This new clause doesn't seem to do anything. Tests all pass if it is removed.
As far as I can tell, this PR doesn't actually use the "deferred inference" infrastructure at all -- using that requires at some point skipping inference of some expression and inserting a definition into self.deferred, and I don't see anywhere we do that.
I think this is related to the comments below about how we are never actually deferring inference of the bases arg. (And it seems like we don't need to? Which I don't yet fully understand.)
charliermarsh
force-pushed
the
charlie/defer-bases
branch
from
9c3e5ec to
bbe9333
Compare
AlexWaygood
reviewed
Jan 26, 2026
Comment on lines
5050
to
5060
| #[salsa::tracked(returns(deref), cycle_initial=|_, _, _| Box::default(), heap_size=ruff_memory_usage::heap_size)] | ||
| pub(crate) fn explicit_bases(self, db: &'db dyn Db) -> Box<[Type<'db>]> { | ||
| // For dangling calls, bases are stored directly on the anchor. | ||
| if let DynamicClassAnchor::ScopeOffset { explicit_bases, .. } = self.anchor(db) { | ||
| return explicit_bases; | ||
| } | ||
|
|
||
| // For assigned calls, we need to use deferred inference. | ||
| let DynamicClassAnchor::Definition(definition) = self.anchor(db) else { | ||
| unreachable!("handled above") | ||
| }; |
Member
There was a problem hiding this comment.
The pattern I used in the NamedTuple PR was to use an inner cached function inside NamedTuple::spec(). The advantage of that is that we only call the cached function in the case where we actually deferred inference of the class's bases (if DynamicNamedTupleAnchor is DynamicNamedTupleAnchor::TypingDefinition()).
ruff/crates/ty_python_semantic/src/types/class.rs
Lines 5730 to 5759 in 73399e5
If self.anchor() is DynamicClassAnchor::ScopeOffset, there should be no need to call a cached function, which is bad for our memory usage. The explicit bases are already store directly on the enum variant.
charliermarsh
force-pushed
the
charlie/defer-bases
branch
from
41b2318 to
6c58a81
Compare
Member
Author
|
(I pushed some improvements but need to spend a little more time with the PR before I take it out of draft.) |
charliermarsh
force-pushed
the
charlie/defer-bases
branch
from
6c58a81 to
eeec94b
Compare
charliermarsh
force-pushed
the
charlie/defer-bases
branch
from
eeec94b to
ba879d0
Compare
Member
Author
|
I'll open this back up for review. |
carljm
approved these changes
Feb 13, 2026
Contributor
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Looks good, thank you! Just nits.
Comment on lines
+141
to
+146
| // When there's a cycle, return a minimal MRO with just the class itself and object. | ||
| // This breaks the cycle and allows type checking to continue. | ||
| Ok(Mro::from([ | ||
| ClassBase::Class(ClassType::NonGeneric(self_.into())), | ||
| ClassBase::object(db), | ||
| ])) |
Contributor
There was a problem hiding this comment.
This seems reasonable in general -- are there bad effects if we do this for static class cycle initial also?
Comment on lines
332
to
335
| // Use a placeholder class literal for try_from_type (the subclass parameter is only | ||
| // used for NamedTuple subclasses, which doesn't apply here). | ||
| let placeholder_class: ClassLiteral<'db> = | ||
| KnownClass::Object.try_to_class_literal(db).unwrap().into(); |
Contributor
There was a problem hiding this comment.
We do this three different places in this diff, and even with the comment it's pretty opaque what's going on here unless you actually go read try_from_type.
I think there are a couple things here:
-
The comment says
NamedTuple"doesn't apply here", but why not? We should add a test for what happens when you try to create a dynamic class inheriting fromtyping.NamedTuple. Would be fine if we just error on it, since an error is what you get at runtime -- but since its a weird edge case, we should test it to make sure it doesn't panic. -
If we have multiple places where we need "
ClassBase.try_from_typebut without a concrete subclass, error if you try to inheritNamedTuple" (which seems like probably the right answer here), then we should just support that directly in thetry_from_typeAPI, even if just by making thesubclassarg optional.
| } | ||
| ClassBase::Dynamic(_) => return class_base, | ||
| // Check for special bases that are not allowed for dynamic classes. | ||
| // Dynamic classes can't be generic, protocols, TypedDicts, or enums. |
Contributor
There was a problem hiding this comment.
Does NamedTuple also belong in this list?
Member
Author
There was a problem hiding this comment.
This code was actually just moved (from above) so I'll make a note to look into it separately.
Member
Author
There was a problem hiding this comment.
Actually did end up supporting this.
charliermarsh
force-pushed
the
charlie/defer-bases
branch
from
ba879d0 to
99be8f5
Compare
charliermarsh
force-pushed
the
charlie/defer-bases
branch
from
99be8f5 to
42afc32
Compare
Memory usage reportMemory usage unchanged ✅ |
charliermarsh
force-pushed
the
charlie/defer-bases
branch
from
42afc32 to
f68b713
Compare
carljm
added a commit
that referenced
this pull request
Feb 14, 2026
* main: (209 commits) [ty] Defer base inference for functional `type(...)` classes (#22792) flake8-executable: allow global flags in uv shebangs (EXE003) (#22582) [ty] Add `replace-imports-with-any` option (#23122) Update html comments in mdtests (#23269) Apply ruff formatting to mdtests (#22935) [ty] Exclude test-related symbols from non-first-party packages in auto-import completions [ty] Refactor `CursorTest` helper to support site-packages [ty] Qualify inlay hint edit symbol when possibly referencing another variable (#23265) [ty] Avoid `UnionBuilder` overhead when creating a new union from the filtered elements of an existing union (#22352) [ty] Refactor TypedDict key assignment validation (#23262) [ty] Improve Python environment path documentation (#23256) [ty] loop control flow analysis using loop header definitions Prepare for 0.15.1 (#23253) Remove docker-run-action (#23254) [ty] Allow discovering dependencies in system Python environments (#22994) Ensure pending suppression diagnostics are reported (#23242) [`isort`] support for configurable import section heading comments (#23151) [ty] Fix method calls on subclasses of `Any` (#23248) [ty] Fix bound method access on `None` (#23246) Make range suppression test snapshot actually useful (#23251) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes astral-sh/ty#2564.