Repository navigation
fix support creation of union from generic class constructor #1134 - #5456
asukaminato0721 wants to merge 2 commits into
Conversation
|
This pull request has been imported. If you are a Meta employee, you can view this in D128397756. (Because this pull request was imported automatically, there will not be any future comments.) |
|
Diff from mypy_primer, showing the effect of this PR on open source code: ============================================================
SUMMARY
============================================================
Total: +5 new errors, -5 fixed errors
By preset: +1/-5 (default), +1/-5 (strict)
Projects with changes (9):
pip: +1 -0
rotki: +0 -1
openlibrary: +1 -1
koda-validate: +0 -1
spark: +1 -0
werkzeug: +0 -1
pytensor: +0 -1
dedupe: +1 -0
rich: +1 -0
============================================================
FULL DIFF DETAILS
------------------------------------------------------------
pip (https://github.com/pypa/pip)
+ ERROR src/pip/_vendor/rich/progress.py:504:44-82: Unused `# type: ignore` comment [unused-type-ignore]
rotki (https://github.com/rotki/rotki)
- ERROR rotkehlchen/api/services/transactions.py:495:36-499:14: `dict[CHAINS_WITH_TRANSACTIONS_TYPE, list[BTCAddress | ChecksumAddress | SolanaAddress | SubstrateAddress]]` is not assignable to variable `blockchain_addresses` with type `dict[CHAINS_WITH_TRANSACTIONS_TYPE, ListOfBlockchainAddresses]` [bad-assignment]
openlibrary (https://github.com/internetarchive/openlibrary)
- ERROR openlibrary/accounts/model.py:936:47-53: Expected argument after ** to be a mapping, got: dict[Unknown, Unknown] | int | str | Any in function `InternetArchiveAccount.__init__` [bad-unpacking]
+ ERROR openlibrary/accounts/model.py:936:47-53: Expected argument after ** to be a mapping, got: dict[str, Unknown] | int | str | Any in function `InternetArchiveAccount.__init__` [bad-unpacking]
koda-validate (https://github.com/keithasaurus/koda-validate)
- ERROR koda_validate/tuple.py:260:16-262:10: Returned type `NTupleValidator[tuple[T1] | tuple[T1, T2] | tuple[T1, T2, T3] | tuple[T1, T2, T3, T4] | tuple[T1, T2, T3, T4, T5] | tuple[T1, T2, T3, T4, T5, T6] | tuple[T1, T2, T3, T4, T5, T6, T7] | tuple[T1, T2, T3, T4, T5, T6, T7, T8]]` is not assignable to declared return type `NTupleValidator[tuple[T1]] | NTupleValidator[tuple[T1, T2]] | NTupleValidator[tuple[T1, T2, T3]] | NTupleValidator[tuple[T1, T2, T3, T4]] | NTupleValidator[tuple[T1, T2, T3, T4, T5]] | NTupleValidator[tuple[T1, T2, T3, T4, T5, T6]] | NTupleValidator[tuple[T1, T2, T3, T4, T5, T6, T7]] | NTupleValidator[tuple[T1, T2, T3, T4, T5, T6, T7, T8]]` [bad-return]
spark (https://github.com/apache/spark)
+ ERROR python/pyspark/pandas/namespace.py:353:34-60: Unused `# type: ignore` comment [unused-type-ignore]
werkzeug (https://github.com/pallets/werkzeug)
- ERROR src/werkzeug/wrappers/response.py:465:33-52: `list[bytes | str]` is not assignable to attribute `response` with type `Iterable[bytes] | Iterable[str]` [bad-assignment]
pytensor (https://github.com/pymc-devs/pytensor)
- ERROR pytensor/compile/rebuild.py:167:19-32: `list[SymbolicOutput | Variable[Unknown, Unknown]]` is not assignable to variable `outputs` with type `Sequence[SymbolicOutput] | Sequence[Variable[Unknown, Unknown]] | SymbolicOutput | Variable[Unknown, Unknown]` [bad-assignment]
dedupe (https://github.com/dedupeio/dedupe)
+ ERROR dedupe/api.py:551:47-73: Unused `# type: ignore` comment [unused-type-ignore]
rich (https://github.com/Textualize/rich)
+ ERROR rich/progress.py:504:44-82: Unused `# type: ignore` comment [unused-type-ignore] |
stroxler
left a comment
There was a problem hiding this comment.
Thanks!
A fresh round of AI review found a number of specific issues that could be introduced here. I'm also a little nervous that it's kind of append-only programming: it's taking an already complex bit of logic and strictly adding complexity to try to get good behavior.
I don't have the bandwidth to look at this in detail, but here's what my agent thought when I pressed about whether this approach could be made simpler. Note that the suggestion involves breaking this into smaller changes. What do you think?
If the sketch of a plan sounds reasonable to you, I'd recommend we close this PR in its current form and start with a refactor
Where the extra complexity comes from
We already have ConstructedInstance, containing the inferred type, hint-match information, specialization failures, and the three diagnostic collectors. But [construct_with_hint](https://github.com/meta/pyrefly/blob/6edcf94b30f4db093b1834dccb785a08011b7f6a/pyrefly/lib/alt/call.rs#L1205-L1279) immediately consumes that structured result through .take().
The new caller then reconstructs essentially the same information using:
- a
(Type, bool)return value; - three collectors for the unexpanded attempt;
- three more for each expanded attempt;
- another three for the accepted expansion group.
That’s not inherently necessary for union support—it’s largely compensating for publishing an attempt’s result too early.
A simpler organization
I’d organize construction around this flow:
Attempt construction → select across hints → optionally try argument expansions → publish the selected outcome.
Concretely:
- Have
construct_with_hintreturn an uncommitted structured outcome. ReuseConstructedInstance, with a clearly defined successful-match predicate that includes specialization failures. Keep the existing preference and fallback rules. - Accumulate expanded outcomes in that same representation. Only combine them when every argument expansion succeeds; otherwise retain the original fallback.
.take()happens at the outer boundary, rather than repeatedly into temporary destinations. - Give argument preparation an explicit ownership boundary. Expansion should reuse prepared argument information, not repeatedly infer arbitrary expressions. Preserve contextual checks for lambdas and other context-sensitive expressions—copying overloads’ wholesale context-free flattening would introduce a different problem.
The first two changes are straightforward simplification. The third is the substantive design work, and is where the performance and diagnostic problems belong.
What should remain separate
There is some unavoidable complexity: the rule is for every argument expansion, some hint member must accept it. That’s why independently union-valued arguments require considering mixed combinations.
I would reuse expansion mechanics, not turn constructors into synthetic overloads or unify the entire call-resolution machinery. Constructors still have their own __new__/__init__ behavior and contextual-specialization rules.
So my recommendation is a behavior-preserving constructor-outcome refactor first, then union distribution as a smaller semantic delta. Merely extracting this new loop into another helper would hide the complexity rather than remove it. A broader solver overhaul isn’t warranted for this feature.
Here were the original, specific issues the AI flagged before I pressed on structure. But I care more about structure than about a laundry list of edge cases
Nested-constructor performance regression (call.rs:1391–1414). Expansion repeatedly re-infers untouched nested arguments, compounding work recursively. An eight-level, init-only constructor case takes 0.87 seconds on base versus a timeout at 20 seconds on head. Cache/bound repeated argument inference while preserving contextual typing.
False unknown-lambda diagnostic (call.rs:1428). For A(lambda arg: None, value), expansion scans the lambda without context, then publishes its implicit-any-lambda diagnostic even though both accepted specializations supply valid context. Visible when that diagnostic is enabled.
Summary
Fixes #1134
Test Plan
add test according to the comments.