Skip to content

Fix non-commutative union reduction for 3+ types involving modules#16952

Open
stakach wants to merge 2 commits into
crystal-lang:masterfrom
stakach:fix-union-type-combine
Open

Fix non-commutative union reduction for 3+ types involving modules#16952
stakach wants to merge 2 commits into
crystal-lang:masterfrom
stakach:fix-union-type-combine

Conversation

@stakach
Copy link
Copy Markdown
Contributor

@stakach stakach commented May 15, 2026

Summary

Resolves #10788 and #15752 — both symptoms of the same root cause in Crystal::Program#type_combine.

#10788 example:

module A; end
class B; include A; end
class C; include A; end

{% puts Union(A, B, C) %} # => A         (correct)
{% puts Union(B, C, A) %} # => (A | C)   (wrong, should be A)
{% puts Union(C, B, A) %} # => (A | B)   (wrong, should be A)

#15752 example: Pointer(Bar0 | Bar1 | Bar2 | Foo) (where each BarN includes Foo) was failing to reduce to Pointer(Foo) for certain orderings, and an unrelated cast through that pointer corrupted instance variables on the read value.

Root cause

type_combine walks the input left-to-right and, for each new t2, scans existing accumulated types looking for a least-common-ancestor with each t1. When it finds one, it absorbs t1 into the ancestor — but uses .all?, which short-circuits on the first absorption. Any later t1 that also implements the same ancestor is left in all_types, so:

B | C | A → after B | C[B, C] → seeing A, absorbs B only → final [C, A] instead of [A].

Fix

Iterate over a snapshot of all_types (so we may keep mutating the live list) and absorb every matching t1, deduping the ancestor on insertion. One localized change in src/compiler/crystal/semantic/type_merge.cr#type_combine.

After the fix, all 12 permutations from the issue collapse to A, and the Pointer(Bar0 | Bar1 | Bar2 | Foo) case correctly reduces to Pointer(Foo).

Test changes

  • New context in spec/compiler/semantic/union_spec.cr with a helper assert_all_permutations_merge_to that verifies all permutations of a 3- or 4-type list merge to the same canonical type. Two new specs cover the module-and-sibling-classes shape from both issues.
  • spec/compiler/semantic/pointer_spec.cr's #15742 test was asserting the buggy element type (Pointer(Bar2 | Foo) instead of Pointer(Foo)) — its own inline comment even said should most certainly be just Foo*. Updated the assertion to the now-correct value and added #15752 to the spec title.

Verification

  • Original #10788 reproducer: all 12 macro orderings → A
  • #15752 reproducer: Pointer(Foo), and the through-cast no longer corrupts @a/@b
  • Full compiler suite: 13479 examples, 0 failures, 0 errors

@HertzDevil
Copy link
Copy Markdown
Contributor

Going from a linear scan to a quadratic one is a huge leap, and we would prefer not doing so unless it is known (or better yet, proven by a human) to be optimal, because quadratic scans over union types have in the past led to some drastic build time explosions.

@stakach stakach force-pushed the fix-union-type-combine branch from 60fa343 to 2430b38 Compare May 15, 2026 10:06
@stakach
Copy link
Copy Markdown
Contributor Author

stakach commented May 15, 2026

Reworked to address concerns about asymptotic regression. The original .all? short-circuited on the first absorption — the new version keeps that short-circuit and only does extra work in the specific situation that exposed the bug.

The refined algorithm:

  • Find an absorbing t1 for t2 — same linear-with-short-circuit scan as before.
  • If ancestor == t1.devirtualize (the common case — t2 was absorbed into an existing larger t1): replace t1 with its virtual form and break out. Same behaviour and same cost as the original.
  • If ancestor != t1.devirtualize (ancestor is t2, or a third common supertype): also do an O(K) reject! of any other entry in all_types that implements? ancestor. This is the only case the previous version missed — when adding a "container" type that more than one existing entry could fold into.

Net effect: identical work to the original on non-absorbing additions and on t2-absorbed-into-t1 additions; one extra O(K) sweep on t1-absorbed-into-t2 (or shared-supertype) additions, where the missed sibling-absorption was the root cause.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:lang:type-system topic:compiler:semantic labels May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:semantic topic:lang:type-system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unions of 3 or more types involving modules are not commutative

3 participants