Skip to content

Fix virtual dispatch / type filtering for uninstantiated generic subclasses#16982

Closed
stakach wants to merge 2 commits into
crystal-lang:masterfrom
stakach:fix-virtual-uninstantiated-generic
Closed

Fix virtual dispatch / type filtering for uninstantiated generic subclasses#16982
stakach wants to merge 2 commits into
crystal-lang:masterfrom
stakach:fix-virtual-uninstantiated-generic

Conversation

@stakach

@stakach stakach commented May 19, 2026

Copy link
Copy Markdown
Contributor

Fixes #11134, #10831.

Two open bugs both trace back to the type lattice treating uninstantiated generic subclasses as outside the "virtual" view of their parent class:

The same code paths likely affect #16947 (Array(Foo) containing a SubGenericFoo(String)), but I couldn't reproduce its segfault on current master in any build mode, so I'm not claiming it as a fixed issue. The codegen change here plausibly helps it, but without a deterministic reproducer there's no regression test, so I'd rather leave it open and let it be closed only when someone can confirm.

Root cause

Type#collect_subtypes in src/compiler/crystal/types.cr filters out GenericType from a virtual type's subtypes:

subtypes << type unless type.is_a?(GenericType) || type.unbound?

That's right for instance virtual dispatch — Bar(T) itself isn't instantiable — but for metaclass dispatch the uninstantiated Bar IS a callable class value. And in the intersection logic, the absence of the relationship in both directions makes chained is_a? filters collapse.

Fix

Three coordinated changes:

  1. Type::VirtualType (types.cr): add subtypes_including_generic and collect_subtypes_including_generic. They yield uninstantiated GenericType subclasses only when no instantiations exist — if instantiations exist, those already carry the dispatch, and including the uninstantiated form would trip up class methods that reference the type parameter (e.g. def self.x; T.foo; end, the pattern in spec Error: can't instantiate abstract generic struct EnumType(Test) #9621). This is what kept the regression scope small.

  2. VirtualMetaclassType#each_concrete_type (types.cr) and VirtualTypeLookup#lookup_matches (method_lookup.cr): use the new method when iterating subtypes for a virtual metaclass. This adds the missing branch for Bar.metaclass in Foo+.class's dispatch table and ~match function. Instance virtual dispatch is unchanged.

  3. common_descendent(GenericClassType, GenericClassType) (type_intersect.cr): try common_descendent_instance_and_generic in both directions, so the relationship C(T) < B(T) flows in either order through chained is_a?.

Tests

The simple case of #10831 (x.is_a?(B) && x.is_a?(B(Int32))) was already fixed previously and continues to pass.

Comment thread spec/compiler/codegen/class_spec.cr Outdated
Comment thread spec/compiler/semantic/is_a_spec.cr Outdated
The crystal-lang#16947 codegen spec passes on master, so it is not a real
regression test. The full original reproducer cannot be reproduced
locally on master (in any build mode), so drop the test rather than
keep one that doesn't bisect.

The crystal-lang#10831 semantic spec used `assert_type` against
`is_a?(...) ? true : false`, which is statically `bool` regardless of
the filter result — so the spec passed even with the unsymmetric
`common_descendent`. Rewrite as a `run(...).to_b` check that actually
exercises runtime narrowing, and correct the misleading comment about
`NoReturn` (the expression incorrectly evaluated to `false`).
@stakach stakach requested a review from straight-shoota May 20, 2026 23:17

@straight-shoota straight-shoota left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like #10831 and #11134 are related, but independent bugs. We should fix them in separate PRs.

@stakach

stakach commented May 22, 2026

Copy link
Copy Markdown
Contributor Author

Splitting per @straight-shoota's review — see #16998 (virtual metaclass dispatch fix for #11134) and #16999 (symmetric common_descendent for #10831). Closing this PR; the split branches are off fresh master and contain only the changes relevant to each issue, with regression tests that have been verified to fail on master and pass with the fix.

@stakach stakach closed this May 22, 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:generics topic:compiler:semantic

Projects

None yet

2 participants