Include uninstantiated generic subclasses in virtual metaclass dispatch (#11134)#16998
Include uninstantiated generic subclasses in virtual metaclass dispatch (#11134)#16998stakach wants to merge 2 commits into
Conversation
…ch (crystal-lang#11134) `Bar.as(Foo.class).foo` segfaulted when `class Bar(T) < Foo` had no instantiation. The dispatch table generated for `Foo+.class.foo` had branches for every concrete metaclass subtype but skipped `Bar` (the uninstantiated generic), so the matching call fell through the `else` into an LLVM `unreachable`. `Type::VirtualType` learns `subtypes_including_generic`, which yields uninstantiated `GenericType` subclasses only when no instantiations exist (instantiations carry the dispatch otherwise, and including the uninstantiated form when instantiations exist trips up class methods that reference the type parameter — c.f. spec crystal-lang#9621). `VirtualMetaclassType#each_concrete_type` and `VirtualTypeLookup#lookup_matches` use the new method only for virtual *metaclass* dispatch. Instance virtual dispatch keeps the existing `subtypes` behaviour so this change is scoped to the actual bug.
straight-shoota
left a comment
There was a problem hiding this comment.
issue: This patch is for a semantic bug, so it should introduce semantic specs that reproduce the bug, not just a codegen spec.
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
| end | ||
|
|
||
| def collect_subtypes(type, subtypes) | ||
| subtypes << type unless type.is_a?(GenericType) || type.unbound? |
There was a problem hiding this comment.
thought: I'm wondering why GenericType#unbound? does not already return true. A non-instantiated generic type should be unbound.
This affects the existing implementation, but would also have an effect on the patch (and maybe result in a different fix as a consequence).
There was a problem hiding this comment.
So it looks like unbound? means that the type expression contains unresolved type parameters.
GenericType is a metaclass that can be used as a type expression directly, even without an instantiation.
A call like Array.additive_identity with Array(T) being a GenericType is entirely valid. The type parameter T is technically unresolved, but that doesn't matter if it's not used. We're just using Array, but it's still considered a GenericType.
Fixes #11134.
The bug
The dispatch table generated for
Foo+.class.foohas branches forBazand instantiated forms ofBar, but no branch forBaritself (the uninstantiated generic). The call's type-id falls through every arm and lands on theunreachableLLVM emits for theelse, which is undefined behaviour.Root cause
Type#collect_subtypesfiltersGenericTypeout of a virtual type's subtypes:That's correct for instance virtual dispatch —
Bar(T)itself isn't instantiable — but for metaclass dispatch the uninstantiatedBarIS a callable class value (Bar.foois legal Crystal), so the metaclass dispatch table needs an arm for it.Fix
Three coordinated changes, all narrowly scoped to metaclass dispatch:
Type::VirtualType(types.cr) — addsubtypes_including_generic/collect_subtypes_including_generic. They yield uninstantiatedGenericTypesubclasses 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 keeps the regression scope minimal.VirtualMetaclassType#each_concrete_type(types.cr) — use the new method when iterating concrete metaclass types. This adds the missingBar.metaclassarm toFoo+.class's match-fun table.VirtualTypeLookup#lookup_matches(method_lookup.cr) — same swap whenvirtual_metaclass?, so the lookup phase agrees with codegen. Instance virtual dispatch keeps the existingsubtypesbehaviour, so this change has no effect on non-metaclass paths.Tests
spec/compiler/codegen/class_spec.cr— newrun-based regression for Dispatch over virtual metaclasses ignores uninstantiated generic subclasses #11134 (return value2, exercising both the uninstantiated genericBarand the non-generic siblingBazso the dispatch has multiple arms). Verified to segfault on master and return2here.Note
This was originally part of #16982 (which also covered #16947 and #10831). Per review feedback, that PR is being split. #10831's symmetric
common_descendentis a separate, independent change. #16947's reproducer could not be reliably demonstrated on master so no regression test is included for it — though the same code paths plausibly help.