[ty] Skip redundancy checks for sub-union-elements#22071
[ty] Skip redundancy checks for sub-union-elements#22071MichaReiser wants to merge 4 commits intomainfrom
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
CodSpeed Performance ReportMerging #22071 will improve performance by 32.09%Comparing Summary
Benchmarks breakdown
Footnotes
|
|
The speed-ups in the ty-benchmark are between 0-6% |
|
Not sure how to tell whether there are any new mypy primer hits or if it's just the usual non-deterministic cases |
It might be worth seeing if you can reproduce the prefect errors locally. I don't recall seeing those ones before. |
|
Yeah, they look suspicious. |
The optimization in PR astral-sh#22071 skips redundancy checks between elements of a sub-union when adding it to a UnionBuilder, assuming the sub-union is already simplified. However, unions created via `from_elements_cycle_recovery` are built with `cycle_recovery=true` (which skips redundancy checks) but `recursively_defined=No` (the default). When such a union is later added to a normal builder: - `!self.cycle_recovery` = true - `self.recursively_defined == union.recursively_defined()` = true (both No) - The optimization kicks in and skips redundancy checks - But the elements were never checked against each other! This caused type inference errors where redundant types remained in unions, leading to `invalid-argument-type` errors for generic type parameters. The fix: Set `recursively_defined=Yes` for unions built during cycle_recovery, so the optimization condition fails when adding them to normal builders, ensuring proper redundancy checks are performed.
7b6d0a9 to
cf2259d
Compare
14dc667 to
54347a5
Compare
|
I don't see getting time to look at this anytime soon, so I'll close the PR but I'll open a follow up issue, because the improvement on seems promising. |
Summary
When adding a sub-union to a union, don't compare its elements to each other, as they are already simplified.
While the
is_redundant_withcalls are mostly cached, reducing the impact of this change, other calls likeis_subtype_ofaren't cached and can be skipped with this change.It turned out that this change is slightly trickier than I thought because we build unions in different modes, and the optimization is only correct if both unions use the same mode.