Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile generic types in signatures using the correct constant #412

Merged

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Aug 3, 2021

Closes #323

Motivation

The current mechanism for compiling signature types is by invoking to_s on the specific constant used for the type. However, generics always return T.untyped for the to_s, so we need an alternative mechanism to get the actual constant name for compilation.

Note: constant.name also returns T.untyped.

Implementation

Basically, it boils down to this:

  1. If the parameter is not a generic, keep the old mechanism
  2. If the parameter is generic, list all constants inside the method owner and find the one for which it's value is equal the generic type. Then use the constant name for compiling

Tests

I added an extra type_template, since those also need special care when compiling.

@vinistock vinistock requested a review from a team August 3, 2021 17:26
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

I'd rather that we solve this in a more generic fashion by binding the constant value name or to_s to the name it is bound to, very early on. That way, we would be able to address return values or the use of type variables inside other generic types as well. (I assume the current implementation does not support something like sig { params(foo: T::Array[Elem]).void } either)

@vinistock
Copy link
Member Author

@paracycle how would you go about binding it? The return of type_member is not a Module, so I can't do anything like Module.instance_method(:name).bind(the_type).

@paracycle
Copy link
Member

@paracycle how would you go about binding it? The return of type_member is not a Module, so I can't do anything like Module.instance_method(:name).bind(the_type).

I see two ways to do that:

  1. In the GenericNamePatch we return the type_member and type_template as they have been constructed by the actual Sorbet call. Instead we can return instances of a subclass of TypeMember and TypeTemplate that has a name= method to bind the name to during processing in https://github.com/Shopify/tapioca/blob/uk-node-sorting/lib/tapioca/compilers/symbol_table/symbol_generator.rb/#L266-L271. Moreover, we can start storing those instances directly in the registry. This, IMO, is the better, cleaner way. The only downside to this is to make sure that we handle those special subclasses properly for RBI generation.
  2. The hacky way is to do a type_variable.singleton_class.define_method(:name) { constant_name } after this line.

@paracycle
Copy link
Member

paracycle commented Aug 3, 2021

Hmm, I see now that option 2 is not that straightforward since we store type_variable_id and not type_variable in the Hash. To be honest, I think we should switch those lookup hashes to be compare_by_identity and not have to deal with object ids ourselves, anyway.

@vinistock
Copy link
Member Author

@paracycle the latest commit includes the process you mentioned. Let me know if that's what you had in mind.

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

I think this is very close and the output is already much better. I see that we've already caught a T::Struct prop type as well. I left some comments to make this work even better.

Also, can we add a few test cases where the generic type variables are nested in some other type, like T::Hash[T::Array[Template], T::Set[Elem]] kind of thing?

lib/tapioca/compilers/symbol_table/symbol_generator.rb Outdated Show resolved Hide resolved
lib/tapioca/compilers/symbol_table/symbol_generator.rb Outdated Show resolved Hide resolved
lib/tapioca/compilers/symbol_table/symbol_generator.rb Outdated Show resolved Hide resolved
lib/tapioca/compilers/symbol_table/symbol_generator.rb Outdated Show resolved Hide resolved
lib/tapioca/sorbet_ext/generic_name_patch.rb Outdated Show resolved Hide resolved
lib/tapioca/sorbet_ext/generic_name_patch.rb Outdated Show resolved Hide resolved
lib/tapioca/sorbet_ext/generic_name_patch.rb Outdated Show resolved Hide resolved
lib/tapioca/sorbet_ext/generic_name_patch.rb Outdated Show resolved Hide resolved
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you!

@vinistock vinistock merged commit 1bcb8c6 into master Aug 9, 2021
@vinistock vinistock deleted the compile_generic_signatures_with_the_right_constant branch August 9, 2021 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Properly compile sigs with type_member types
3 participants