[ty] Refactor to support building constraint sets differently#23600
Merged
[ty] Refactor to support building constraint sets differently#23600
Conversation
Typing conformance resultsNo changes detected ✅ |
Memory usage reportMemory usage unchanged ✅ |
|
dcreager
commented
Feb 27, 2026
|
|
||
| fallback: R, | ||
|
|
||
| pub(crate) extra: Extra, |
Member
Author
There was a problem hiding this comment.
This lets us remove a data field from the TypeRelation enum, so that it can be trivially copyable without ConstraintSets needing to be as well.
Comment on lines
-192
to
+194
| SubtypingAssuming(ConstraintSet<'db>), | ||
| SubtypingAssuming, |
Comment on lines
+205
to
+210
| #[derive(Clone, Debug, Eq, Hash, PartialEq, get_size2::GetSize, salsa::Update)] | ||
| pub struct OwnedConstraintSet<'db> { | ||
| /// The BDD representing this constraint set | ||
| node: Node<'db>, | ||
| storage: ConstraintSetStorage<'db>, | ||
| } |
Member
Author
There was a problem hiding this comment.
This only exists to have something internable (for InternedConstraintSet) without having to make all of (eventual, in the later PR) fields of ConstraintSetBuilder implement Hash.
Comment on lines
+226
to
+227
| builder: &'c ConstraintSetBuilder<'db>, | ||
| _invariant: PhantomData<fn(&'c ()) -> &'c ()>, |
Member
Author
There was a problem hiding this comment.
These exist to provide compile- and runtime checks that we're only using ConstraintSets with the builders that created them. I'm considering making the builder field only exist in debug builds, but that would be a follow-on experiment.
Contributor
There was a problem hiding this comment.
In my C++ template metaprogramming days, I used to write "should not compile" tests. compile_fail in doctests can achieve something similar, but unfortunately it requires a few things to be pub instead of pub(crate), so I'm not sure if it's worth it. Certainly, adding lots of infrastructure just for this seems unnecessary.
/// ```compile_fail,E0521
/// use ty_python_semantic::types::constraints::{ConstraintSetBuilder, ConstraintSet};
///
/// fn wrong_constraint_set<'db>() {
/// let builder1 = ConstraintSetBuilder::new();
/// let constraints1 = ConstraintSet::always(&builder1);
///
/// let builder2 = ConstraintSetBuilder::new();
/// let _ = builder2.into_owned(|builder2| {
/// constraints1 // this is wrong
/// });
/// }
/// ```
Comment on lines
-120
to
+125
| pub(crate) fn has_relation_to_impl(self, db: &'db dyn Db, other: Self) -> ConstraintSet<'db> { | ||
| pub(crate) fn has_relation_to_impl<'c>( | ||
| self, | ||
| db: &'db dyn Db, | ||
| other: Self, | ||
| constraints: &'c ConstraintSetBuilder<'db>, | ||
| ) -> ConstraintSet<'db, 'c> { |
Member
Author
There was a problem hiding this comment.
This is a good example of the new method shape — we have to take in an additional ConstraintSetBuilder param, and the constraint set that is returned is linked to that builder via the 'c lifetime.
sharkdp
approved these changes
Feb 27, 2026
Contributor
sharkdp
left a comment
There was a problem hiding this comment.
Thank you very much for pulling it out as an additional refactoring. I think I'm going to merge this right away, both because it could easily accumulate conflicts, and because I'm working on a changeset that also affects has_relation_to and friends, so I might as well just immediately build on top of this.
One thought I had while scrolling through the diff: has_relation_to makes sense as a method on Type (and its sub-variants), but it doesn't need to be attached to it. As we thread more and more parameters through these function calls, I'm wondering if it would make sense to split out these type relations into their own module, and implement them on a TypeRelation struct (or similar) that holds on to those parameters? This would make refactorings like this one easier, but the downside would be that we might have to make some internals of Callable or similar available publicly(?).
sharkdp
reviewed
Feb 27, 2026
|
|
||
| pub(crate) fn into_owned( | ||
| self, | ||
| f: impl for<'c> FnOnce(&'c Self) -> ConstraintSet<'db, 'c>, |
Contributor
There was a problem hiding this comment.
Cool. I was aware this existed, but haven't used it myself yet.
carljm
added a commit
that referenced
this pull request
Feb 27, 2026
* main: [ty] Take myself out of the reviewer pool for the next few days (#23618) [ty] Fix bug where ty would think that a `Callable` with a variadic positional parameter could be a subtype of a `Callable` with a positional-or-keyword parameter (#23610) [`ruff`] Add fix for `none-not-at-end-of-union` (`RUF036`) (#22829) Bump cargo dist to 0.31 (#23614) [`pyflakes`] Fix false positive for names shadowing re-exports (`F811`) (#23356) [`fastapi`] Handle callable class dependencies with `__call__` method (`FAST003`) (#23553) [ty] Recurse into tuples and nested tuples when applying special-cased validation of `isinstance()` and `issubclass()` (#23607) Update typing conformance suite commit (#23606) [ty] Detect invalid uses of `@final` on non-methods (#23604) [ty] Move the type hierarchy request handlers to individual modules [ty] Wire up the type hierarchy implementation with the LSP [ty] Add routine for mapping from system path to vendored path [ty] Implement internal routines for providing the LSP "type hierarchy" feature [ty] Add some helper methods on `ClassLiteral` [ty] Move some module name helper routines to methods on `ModuleName` [ty] Bump version of `lsp-types` [ty] Refactor to support building constraint sets differently (#23600) [ty] Dataclass transform: neither frozen nor non-frozen (#23366) [ty] Add snapshot tests for advanced `invalid-assignment` scenarios (#23581) [ty] disallow negative narrowing on SubclassOf types (#23598)
Contributor
Not if the module is a submodule of In fact |
dcreager
added a commit
that referenced
this pull request
Mar 2, 2026
This PR updates how we manage the interning and memoization of our constraint set BDD implementation. Before we relied on Salsa interning and tracking. This had the nice property that everything was cached globally across the entire process, which in theory gives us space savings when the same constraint sets are created for different regions of source code. However, it made us very susceptible to changes in the ordering of salsa IDs. Salsa IDs are also assigned globally, in the order that interned structs or tracked method results are created. That means that if e.g. a particular constraint is created for two unrelated regions of code, there's no guarantee about how that constraint's salsa ID will compare to the IDs of the other constraints in the BDD. Since we were using constraint IDs to define our BDD variable ordering, this would sometimes give us different BDD structures for different (identical) invocations of ty. This was leading to a lot of nondeterminism in our CI jobs. We now rely on a new `ConstraintSetBuilder` type to define _local_ caching of BDD nodes and operations. The type was introduced as a no-op refactoring in #23600. This PR updates that type to actually take over responsiblity for the interning and memoization caches. This opens up some other potential optimizations, but I've kept this PR purposefully limited in scope — with one exception, we cache exactly the same things as before, just in a hand-roll `FxHashMap` instead of via a magic salsa macro. (That one exception is that we now intern typevars locally in the new builder, even though they are already salsa-interned globally. This is needed to give them a stable ID within the builder.) --------- Co-authored-by: Alex Waygood <alex.waygood@gmail.com>
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.
This is a pure refactoring PR that exists solely to set up #23538. That PR will update how we memoize the constraint sets that we build, with hand-rolled cached instead of relying on salsa interning.
To do that, we need a create a new
ConstraintSetBuildertype and thread it around through all of the varioushas_relation_tomethods and friends. That's a slog to review, so I've pulled that out into this separate PR to make reviewing easier.This PR is a pure refactoring. There should be no behavioral changes. In particular, the new
ConstraintSetBuildertype is currently empty! We're still using salsa interning at this stage of the migration, and so the builder doesn't need to hold onto any internal state. But don't worry, it will soon.