Fix generic type validation for T::Props and composite members#2673
Fix generic type validation for T::Props and composite members#2673dduugg wants to merge 3 commits into
Conversation
`GenericTypeRegistry::GenericType` overrides `valid?` to check the underlying constant rather than the generic clone `raw_type`, but `recursively_valid?` is inherited from `T::Types::Simple` and checks `raw_type` directly. Composite types (`T.all`, `T::Hash`, ...) and `T::Props`/`T::Struct` setters validate via `recursively_valid?`, so instances of the underlying type are wrongly rejected there. Delegate `recursively_valid?` to `valid?` so validation is consistent. Fixes Shopify#2130.
KaanOzkan
left a comment
There was a problem hiding this comment.
My agent found a case where sorbet-runtime takes a different code path for T::Types::Simple and bypasses valid? or recursively_valid? checks.
require "tapioca/internal"
module I
extend T::Generic
interface!
E = type_member
end
class C
include I
end
class S < T::Struct
const :x, I[Object]
end
S.new(x: C.new)results in TypeError: Parameter 'x': Can't set S.x to #<C:0x0000000127c91288> (instance of C) - need a I[::Object]
Maybe we shouldn't inherit from T::Types::Simple 🤔 , wdyt?
GenericType previously subclassed T::Types::Simple. sorbet-runtime's SetterFactory has an is_a?(T::Types::Simple) fast path that reads raw_type directly and bypasses valid?/recursively_valid?, so a bare generic prop member (e.g. const :x, I[Object]) rejected instances of the underlying constant. Subclassing T::Types::Base routes all validation through the valid? override.
|
Good catch, you're right, and I've taken your suggestion. The bare So instead of overriding Added a spec for your exact repro (bare generic member) alongside the composite one. |
| # Subclasses `T::Types::Base`, not `T::Types::Simple`: `Simple` fast paths | ||
| # (e.g. `T::Props::Private::SetterFactory`) check `raw_type` directly, but our | ||
| # `raw_type` is the clone, so they reject instances of the underlying constant. | ||
| # `Base` routes all validation through the `valid?` override below. |
There was a problem hiding this comment.
I think this comment can be improved by changing the language and making it more concise, or be removed completely.
| #: (T::Types::Base type) -> bool | ||
| private def subtype_of_single?(type) | ||
| true | ||
| end |
There was a problem hiding this comment.
I guess it's okay to allow everything while running tapioca dsl? There's a similar comment on create_generic_type below.
Motivation
Fixes #2130.
When a constant is typed with a Tapioca generic type (e.g. a generic interface
Foo[Bar]) as aT::Props/T::Structmember, instantiating or setting that member raises a spuriousTypeError, even though the value is an instance of the underlying constant. This happens whether the member is typed with the generic directly (const :x, Foo[Bar]) or nested inside a composite type (T::Hash[..., Foo[Bar]],T.all(Foo[Bar], ...)), and it blocksbin/tapioca dslfrom loading applications that use generic types in prop/struct members.Implementation
GenericTypeRegistry::GenericTypeoverridesvalid?to validate against the underlying constant (@underlying_type) instead of the generic clone it wraps asraw_type. Previously it subclassedT::Types::Simple, butSimplevalidates withobj.is_a?(raw_type)andsorbet-runtimehas fast paths keyed onis_a?(T::Types::Simple)that readraw_typedirectly and bypassvalid?/recursively_valid?entirely:T::Props::Private::SetterFactoryusessimple_non_nil_proc/simple_nilable_procforSimpletypes, checkingval.is_a?(raw_type)inline. This is what a bareconst :x, Foo[Bar](orT.nilable(Foo[Bar])) member hits.T.nilableunion construction takes aSimplePairUnionfast path forSimplemembers.Because
raw_typeis the clone rather than the underlying constant, every one of these rejects a valid instance. SubclassingT::Types::Baseinstead removes all of those fast paths, so all validation routes through thevalid?override.Baseis more minimal thanSimple, so the pieces it does not provide are added:name,build_type, andsubtype_of_single?(which returnstrue, matching the always-true<=we already define on the clone increate_generic_type).This complements #2657, which fixed the
T.let/T.castcoercion path but did not cover the setter/composite validation paths.Tests
Added tests to
spec/tapioca/runtime/generic_type_registry_spec.rbcovering both a bareT::Structmember (const :member, SampleGenericInterface[Object]) and a composite member (T::Hash[Symbol, SampleGenericInterface[Object]]), each constructed with an implementation instance. Both fail with aTypeErrorbefore this change and pass after.