Skip to content

Don't mint a type variable for a purely-mixed construction inference - #11973

Merged
danog merged 1 commit into
vimeo:6.xfrom
danog:fix_mixed_construction_argument_coercion
Sep 18, 2026
Merged

Don't mint a type variable for a purely-mixed construction inference#11973
danog merged 1 commit into
vimeo:6.xfrom
danog:fix_mixed_construction_argument_coercion

Conversation

@danog

@danog danog commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

PR #11875 mints a fresh type variable for every class template at a new site, so constraints recorded as the variable flows through the function accumulate as bounds and reconcile at the end. When the constructor arguments infer nothing more specific than mixed, though, there is no type to widen from, and deferring the argument checks to bound reconciliation silently drops them: a callable(Item) passed where a callable(TValue) is expected on a new Foo($mixedIterable) no longer raised MixedArgumentTypeCoercion, even though the equivalent non-new Foo still does (via CallAnalyzer::checkTemplateResult).

Pin the template to mixed in that case instead of minting a variable, restoring the diagnostic while leaving #11875's widening intact for constructions that infer a concrete type.

@danog danog added the release:fix The PR will be included in 'Fixes' section of the release notes label Sep 17, 2026
@danog
danog force-pushed the fix_mixed_construction_argument_coercion branch 3 times, most recently from c179d8e to 73ee2c5 Compare September 17, 2026 14:44
@danog @claude
Report coerced type-variable argument bounds instead of dropping them
6cc9a23 
PR vimeo#11875 mints a type variable for each class template at a `new` site and
defers the argument checks that flow through it to bound reconciliation at
the end of the function-like. Three things caused a contravariant callable
argument against such a variable to be silently accepted, so a
`callable(Item)` passed where a `callable(T)` is expected no longer raised
the coercion the equivalent non-`new` `Foo` does. Verified against the
Hack typechecker, which reports every one of these.

- ArgumentAnalyzer::resolveTypeVariablesInCallables resolved type variables
  in callable parameter positions to their bounds. That is right when the
  variable resolves to a concrete type (the callable comparison can then
  report a precise "expects callable(Foo)" error), but when it resolves to
  `mixed` the resulting `callable(mixed)` was skipped by the hasMixed() gate,
  dropping the coercion. Leave a variable that resolves to `mixed` in place
  so the comparison records a bound; keep resolving concrete ones.

- CallableTypeComparator::isParamContainedBy compared the parameters in
  already-contravariant order and *then* flipped the recorded bounds, so the
  correct `_0 <: Item` became `_0 >: Item`. Drop the flip.

- TypeVariableTracker reconciliation dropped every containment that only held
  through `mixed`. Report MixedArgumentTypeCoercion when the failing upper
  bound is an argument requirement (matching non-`new` `Foo`); other
  mixed-inferred bounds (e.g. a class-string construction) keep the loose
  gate.

Now `new Foo($mixedIterable)` with a narrower callback reports
MixedArgumentTypeCoercion; `new Box()` alone stays silent (the variable
solves); and `new Box(); $box->set(5); $box->each(fn(Item))` reports
IncompatibleTypeParameters — each matching Hack.

Co-Authored-By: Claude Opus 4.8 
Claude-Session: https://claude.ai/code/session_0164WmcdAJe6UCNVeJf6tnTj
@danog
danog force-pushed the fix_mixed_construction_argument_coercion branch from 73ee2c5 to 6cc9a23 Compare September 17, 2026 19:22
@danog
danog merged commit d84f392 into vimeo:6.x Sep 18, 2026
60 checks passed
danog added a commit to danog/psalm that referenced this pull request Sep 21, 2026
@danog @claude
Merge union-arm bounds the way Hack localizes union hints
fbd6960 
Returning `new It([0])` / `new It(["hello"])` (or an empty `new It()`)
where the declared return type is `It|It` recorded one upper
bound per arm on the same variable (`_0 <: string` AND `_0 <: int`) and
reconciled as impossible ("Type 0 should be a subtype of string").

Verified against HHVM: Hack localizes the declared `(It | It)`
to `It<(string | int)>` for a covariant It (Typing_union.union_list merges
same-class covariant members), so the variable just gains the upper bound
`string | int`; for an invariant It the arms stay separate and each
construction reconciles against its own member. Both forms are accepted.

UnionTypeComparator now collects the bounds each matching container arm
records for one input part and merges same-variable bounds across arms
into a single bound whose type is the union of the arm types (flagged
from_union_alternatives). ReturnAnalyzer keeps such a merged bound as a
plain upper bound rather than pinning it as an equality bound, since the
arms are alternatives. The staged approach of leaving per-arm bounds
unpinned did not survive a construction with a lower bound.

Hack conformance harness: fixtures may now carry `//// hhconfig:` header
lines (union type hints are gated behind union_intersection_type_hints).
Adds HHVM-verified fixtures for the type-variable tests added in vimeo#11972,
vimeo#11973, vimeo#11976, vimeo#11977, vimeo#11978 and for multipleReturnTypes,
multipleReturnTypesNotCovariant and emptyConstructionAgainstUnionKeyReturn.

Co-Authored-By: Claude Fable 5.1
danog added a commit to danog/psalm that referenced this pull request Sep 22, 2026
@danog
Merge pull request vimeo#11973 from danog/fix_mixed_construction_argu…
fed9640 
…ment_coercion

Don't mint a type variable for a purely-mixed construction inference
danog added a commit to danog/psalm that referenced this pull request Sep 22, 2026
@danog @claude
Merge union-arm bounds the way Hack localizes union hints
3953a49 
Returning `new It([0])` / `new It(["hello"])` (or an empty `new It()`)
where the declared return type is `It|It` recorded one upper
bound per arm on the same variable (`_0 <: string` AND `_0 <: int`) and
reconciled as impossible ("Type 0 should be a subtype of string").

Verified against HHVM: Hack localizes the declared `(It | It)`
to `It<(string | int)>` for a covariant It (Typing_union.union_list merges
same-class covariant members), so the variable just gains the upper bound
`string | int`; for an invariant It the arms stay separate and each
construction reconciles against its own member. Both forms are accepted.

UnionTypeComparator now collects the bounds each matching container arm
records for one input part and merges same-variable bounds across arms
into a single bound whose type is the union of the arm types (flagged
from_union_alternatives). ReturnAnalyzer keeps such a merged bound as a
plain upper bound rather than pinning it as an equality bound, since the
arms are alternatives. The staged approach of leaving per-arm bounds
unpinned did not survive a construction with a lower bound.

Hack conformance harness: fixtures may now carry `//// hhconfig:` header
lines (union type hints are gated behind union_intersection_type_hints).
Adds HHVM-verified fixtures for the type-variable tests added in vimeo#11972,
vimeo#11973, vimeo#11976, vimeo#11977, vimeo#11978 and for multipleReturnTypes,
multipleReturnTypesNotCovariant and emptyConstructionAgainstUnionKeyReturn.

Co-Authored-By: Claude Fable 5.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

release:fix The PR will be included in 'Fixes' section of the release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@danog