Skip to content

Add erase_generic_types option to RBS translators#611

Open
dejmedus wants to merge 1 commit into
Alex/Options-objectfrom
jb-erase-generic-types
Open

Add erase_generic_types option to RBS translators#611
dejmedus wants to merge 1 commit into
Alex/Options-objectfrom
jb-erase-generic-types

Conversation

@dejmedus

@dejmedus dejmedus commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Previously, RBS::TypeTranslator and RBS::MethodTypeTranslator always translated generic types. However, as sorbet-runtime is unable to enforce them, they are superfluous in runtime contexts

This PR adds a default false erase_generic_types kwarg to both translator's constructors. When set to true generic types are dropped (Foo[Bar] to Foo) and type params are treated as T.anything

amomchilov
amomchilov reviewed Jun 9, 2026
View reviewed changes
Comment thread lib/rbi/rbs/method_type_translator.rb
Comment on lines -10 to -12
#: (Method, ::RBS::MethodType) -> Sig
def translate(method, type)
translator = new(method)

@amomchilov amomchilov Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This method is just a shim for backwards-compatibility, so there's no need to also add this option here.

Users who want the new API can just switch to .new.

Comment thread lib/rbi/rbs/method_type_translator.rb Outdated
Comment on lines +24 to +25
@result = Sig.new #: Sig
@type_translator = TypeTranslator.new #: TypeTranslator
@erase_generic_types = erase_generic_types

@amomchilov amomchilov Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Style nit, keep the ivars initialized from the params first and in-order

Suggested change
@result = Sig.new #: Sig
@type_translator = TypeTranslator.new #: TypeTranslator
@erase_generic_types = erase_generic_types
@erase_generic_types = erase_generic_types
@result = Sig.new #: Sig
@type_translator = TypeTranslator.new #: TypeTranslator

@dejmedus dejmedus Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like that!

Comment thread lib/rbi/rbs/type_translator.rb Outdated
Type.proc.params(arg0: Type.untyped).returns(Type.untyped)
when ::RBS::Types::Variable
Type.type_parameter(type.name)
@erase_generic_types ? Type.untyped : Type.type_parameter(type.name)

@amomchilov amomchilov Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rewrite into T.anything rather than T.untyped. It conveys a more specific specific intent.

Suggested change
@erase_generic_types ? Type.untyped : Type.type_parameter(type.name)
@erase_generic_types ? Type.anything : Type.type_parameter(type.name)

@dejmedus dejmedus Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good:) I wondered if anything would act differently from untyped since Sorbet seems to specifically use untyped, but outside of a snippet that couldn't come from statically correct RBS anyways it seems to be all good

Comment thread test/rbi/rbs/method_type_translator_test.rb Outdated
assert_equal(
[
SigParam.new("array", Type.simple("Array")),
SigParam.new("item", Type.untyped),

@amomchilov amomchilov Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will need to update the tests to match, e.g.

Suggested change
SigParam.new("item", Type.untyped),
SigParam.new("item", Type.anything),

Comment thread test/rbi/rbs/type_translator_test.rb Outdated
Comment on lines +105 to +108
def test_translate_variable
assert_equal(Type.type_parameter(:T), translate_variable(:T))
assert_equal(Type.type_parameter(:E), translate_variable(:E))
end

@amomchilov amomchilov Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh interesting, I guess this wasn't covered directly before, but indirectly via test/rbi/rbs/method_type_translator_test.rb

Comment thread test/rbi/rbs/type_translator_test.rb Outdated
Comment on lines +123 to +127
assert_equal(Type.class_of(Type.simple("Foo")), translate("singleton(Foo)", erase_generic_types: true))
assert_equal(
Type.class_of(Type.simple("Foo")),
translate("singleton(Foo)[Bar]", erase_generic_types: true),
)

@amomchilov amomchilov Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Notably, the expected value is the same in both cases. You can emphasize that by extracting the variable in common:

Suggested change
assert_equal(Type.class_of(Type.simple("Foo")), translate("singleton(Foo)", erase_generic_types: true))
assert_equal(
Type.class_of(Type.simple("Foo")),
translate("singleton(Foo)[Bar]", erase_generic_types: true),
)
expected = Type.class_of(Type.simple("Foo"))
assert_equal(expected, translate("singleton(Foo)", erase_generic_types: true))
assert_equal(
expected,
translate("singleton(Foo)[Bar]", erase_generic_types: true),
)

@dejmedus dejmedus Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I consolidated the tests a bit

@dejmedus dejmedus force-pushed the jb-erase-generic-types branch from 0556924 to 6680acb Compare June 16, 2026 02:44
dejmedus
dejmedus commented Jun 16, 2026
View reviewed changes
Comment thread lib/rbi/rbs/type_translator.rb
end

#: (String type_name) -> String
def erase_t_generic_type(type_name)

@dejmedus dejmedus Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Similarly to how we add the prefix to translate generic types, we probably want to strip prefixes and use the actual runtime type when erasing. Otherwise something like #: (String component_name, T::Array[Symbol] includes) -> void can error at runtime

@dejmedus dejmedus marked this pull request as ready for review June 16, 2026 03:49
@dejmedus dejmedus requested a review from a team as a code owner June 16, 2026 03:49
@dejmedus dejmedus requested a review from amomchilov June 16, 2026 03:50
@amomchilov amomchilov force-pushed the jb-erase-generic-types branch from 6680acb to 8d4e7bf Compare June 16, 2026 22:46
@amomchilov amomchilov changed the base branch from main to graphite-base/611 June 16, 2026 22:47
@amomchilov amomchilov changed the base branch from graphite-base/611 to Alex/Options-object June 16, 2026 22:47
@amomchilov

amomchilov commented Jun 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@amomchilov amomchilov mentioned this pull request Jun 16, 2026
Open
paracycle
paracycle reviewed Jun 18, 2026
View reviewed changes
Comment thread rbi/rbi.rbi Outdated
paracycle
paracycle reviewed Jun 18, 2026
View reviewed changes
Comment thread lib/rbi/rbs/type_translator.rb
Type.proc.params(arg0: Type.untyped).returns(Type.untyped)
when ::RBS::Types::Variable
Type.type_parameter(type.name)
@options.erase_generic_types? ? Type.anything : Type.type_parameter(type.name)

@paracycle paracycle Jun 18, 2026

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.

Inline with my other comment, I think these should always stay

@paracycle paracycle Jun 26, 2026

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.

Ok, I agree that we can replace method type variables with T.anyting, but nitpick: Can we make this a normal if/else. Usage of the ternary operator is really rare in Ruby:

Suggested change
@options.erase_generic_types? ? Type.anything : Type.type_parameter(type.name)
if @options.erase_generic_types?
Type.anything
else
Type.type_parameter(type.name)
end

paracycle
paracycle reviewed Jun 18, 2026
View reviewed changes
Comment thread lib/rbi/rbs/type_translator.rb Outdated
paracycle
paracycle reviewed Jun 18, 2026
View reviewed changes
Comment thread lib/rbi/rbs/type_translator.rb Outdated
paracycle
paracycle reviewed Jun 18, 2026
View reviewed changes
Comment thread lib/rbi/rbs/type_translator.rb
paracycle
paracycle reviewed Jun 18, 2026
View reviewed changes
Comment thread lib/rbi/rbs/type_translator.rb Outdated
paracycle
paracycle reviewed Jun 18, 2026
View reviewed changes
Comment thread lib/rbi/rbs/type_translator.rb
@dejmedus dejmedus changed the base branch from Alex/Options-object to graphite-base/611 June 23, 2026 21:24
@dejmedus
Add erase_generic_types option to RBS translators
a78fe18 
Previously, `RBS::TypeTranslator` and
`RBS::MethodTypeTranslator` always translated generic
types like `Foo[Bar]`. However, `sorbet-runtime` is
unable to enforce them, so they are superfluous in
runtime contexts

This commit adds a default `false` `erase_generic_types`
kwarg to both translators. When set to `true`
generic types are dropped and the simple type is
returned (`Foo[Bar]` to `Foo`)
@dejmedus dejmedus force-pushed the graphite-base/611 branch from 0b23987 to 4cd0e91 Compare June 26, 2026 00:14
@dejmedus dejmedus force-pushed the jb-erase-generic-types branch from 8d4e7bf to a78fe18 Compare June 26, 2026 00:14
@dejmedus dejmedus changed the base branch from graphite-base/611 to Alex/Options-object June 26, 2026 00:14
@dejmedus dejmedus requested a review from paracycle June 26, 2026 17:04
paracycle
paracycle approved these changes Jun 26, 2026
View reviewed changes

@paracycle paracycle 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.

Thanks, I have a small stylistic comment, otherwise, this looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paracycle paracycle paracycle approved these changes
@amomchilov amomchilov Awaiting requested review from amomchilov

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dejmedus @amomchilov @paracycle