[ty] Make inferred specializations line up with source types more better#23715
Merged
[ty] Make inferred specializations line up with source types more better#23715
Conversation
Typing conformance resultsNo changes detected ✅ |
e460047 to
d8a2559
Compare
Memory usage reportSummary
Significant changesClick to expand detailed breakdownflake8
trio
sphinx
prefect
|
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
0 | 1 | 97 |
unresolved-attribute |
0 | 0 | 44 |
invalid-await |
0 | 40 | 0 |
unsupported-operator |
0 | 0 | 17 |
type-assertion-failure |
0 | 0 | 14 |
invalid-assignment |
0 | 0 | 5 |
invalid-return-type |
0 | 1 | 2 |
not-iterable |
0 | 0 | 2 |
| Total | 0 | 42 | 181 |
Member
Author
|
I've looked through the mypy_primer and ecosystem results. The additions are due to continuing flakiness. Everything else is an expected change in the ordering of a union type to better match the source order. |
Member
Do you know what's up with the non-flaky removed |
Member
Author
There are several diagnostics on that line, and I think it's a problem with the report diff logic. If you look just above the removed diagnostic, you'll see a "changed" one that is using the wrong diagnostic as the "before". It seems like there might have been two copies of the diagnostic for the |
Member
Author
|
I've confirmed that there are two copies of the
feature branch: I've also run the feature branch 50 times each for both the |
Member
|
Thank you for checking ❤️ |
AlexWaygood
approved these changes
Mar 4, 2026
Member
AlexWaygood
left a comment
There was a problem hiding this comment.
Really great PR writeup that made reviewing very easy -- thank you!
carljm
added a commit
that referenced
this pull request
Mar 5, 2026
* main: Update conformance suite commit hash (#23746) conformance.py: Collapse the summary paragraph when nothing changed (#23745) [ty] Make inferred specializations line up with source types more better (#23715) Bump 0.15.5 (#23743) [ty] Render all changed diagnostics in conformance.py (#23613) [ty] Split deferred checks out of `types/infer/builder.rs` (#23740) Discover markdown files by default in preview mode (#23434) [ty] Use `HasOptionalDefinition` for `except` handlers (#23739) [ty] Fix precedence of `all` selector in TOML configurations (#23723) [ty] Make `all` selector case sensitive (#23713) [ty] Add a diagnostic if a `TypeVar` is used to specialize a `ParamSpec`, or vice versa (#23738) [ty] Override home directory in ty tests (#23724) [ty] More type-variable default validation (#23639) [ty] Validate bare ParamSpec usage in type annotations, and support stringified ParamSpecs as the first argument to `Callable` (#23625) [ty] Add `all` selector to ty.json's `schema` (#23721) [ty] Add quotes to related issues links (#23720) [ty] Fix panic on incomplete except handlers (#23708)
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.
I discovered this while looking into some unexpected ecosystem results on #22578. It took a couple of days of wrestling with pi to discover the issue and reduce it to the following example:
We are trying to infer a specialization of
dictwhen we pass in thelist[tuple[...]]type an argument. This matches against the constructor overload that takes in anIterator[tuple[_KT, _VT]]. All good so far.As part of doing that, we create the constraint set
(Any | str ≤ _KT) ∧ (Any | str ≤ _VT), which has the following structure:A few interesting things have happened:
Iteratoris covariant; contra × co = contraAny | str, and_KTappears before_VTin the definition ofdict.Any | str ≤ _KTbecomes(Any ≤ _KT) ∧ (str ≤ _KT)Any ≤ _KT,str ≤ _KT,Any ≤ _VT,str ≤ _VT. (The1/1etc in the graph rendering is thesource_orderthat we have assigned to each constraint.)Any ≤ _KT) appears closest to the leaf nodes. (This is on purpose for efficiency reasons)Everything described so far is correct and expected behavior.
Next we have to find a solution for this constraint set. To do that, we find every path from the root to the
alwaysterminal, remembering all of the constraints that we encounter along that path. In this case, there is only one path:Those constraints are still in reverse order! But we're tracking a
source_orderfor each constraint, and we sort bysource_orderbefore turning this list of constraints into a solution. That should mean that we build up the unions asAny | str.So why were we seeing
str | Anyinstead? If you print out thesource_orders that we actually see when we get to the sort, we have:The
Anyconstraints have copied theirsource_orders from the correspondingstrconstraints!What has happened is that some purposeful behavior has engaged too aggressively. When we look at a path that represents a solution, we want to know all of the constraints that are true for that path — not just the ones that are explicitly mentioned in the BDD. We have a
SequentMaptype that records the relationships between constraints. Among other things, it records implications: "when X is true, Y is true as well". As we build up potential paths, each time we add one of the constraints from the BDD, we immediately check the sequent map to see if we now know any other derived facts to be true. If so, we add them to the path. And importantly, we want those derived facts to appear "near" their "origin" constraint — so we give the derived constraint the samesource_orderas the BDD constraint we just added.In this case, the first constraint we encounter in the BDD is
str ≤ _VT (4). The sequent map tells us thatstr ≤ _VTimpliesAny ≤ _VT, so we addAny ≤ _VTas a derived constraint, which inheritssource_order=4from its origin constraint. Next we encounterAny ≤ _VTas an origin constraint (since it appears in the BDD). But since it's already in the path, we don't add it as a duplicate or update itssource_order.The fix is straightforward: origin
source_orders should take precedence over inherited derivedsource_orders.