Rewrite type aliases#791
Conversation
paracycle
force-pushed
the
uk-type-alias-rewriting
branch
4 times, most recently
from
d1a833a to
1823880
Compare
Morriar
left a comment
Morriar
left a comment
There was a problem hiding this comment.
Do we have any plans for type aliases used in inline annotations?
x = foo #: myTypeAlias| end | ||
|
|
||
| #: (String ruby, file: String) -> [Prism::Node, Array[Prism::Comment]] | ||
| def parse_ruby_with_comments(ruby, file:) |
There was a problem hiding this comment.
Why can't we use parse_ruby(ruby, file:, comments: true)?
There was a problem hiding this comment.
Ah yeah, I meant to note this, but forgot.
I need the comments for the whole file to be returned, so that the rewriter can access all comments, and not just the ones that are attached to nodes. That need arises since type alias comments are not supposed to be attached to nodes.
The parse_ruby method was not returning comments, and instead of changing the return type of that method and changing all the call sites, I decided to add another method with different return type.
There was a problem hiding this comment.
Which comments are not attached to node?
I tried this:
require "prism"
result = Prism.parse("# foo")
result.attach_comments!
result.value # => ProgramNode
result.value.comments.first.slice # => "# foo"So it looks like the nodes are properly attached?
There was a problem hiding this comment.
Sure, but what I am saying is that we expect to find #: type_alias comments in arbitrary locations in the document. They don't have to be attached to any particular node that we are looking for, they might even end up being attached to a local variable assignment that is happening inside a class body.
That is why I opted to process type aliases from the global list of all comments nodes in the document, as opposed to fishing them out from the nodes they happen to be attached to.
There was a problem hiding this comment.
Oh I see, you don't want to have to visit the tree to figure where the comment is?
I think I'm fine with changing the signature of parse_ruby and changing the callsites instead of duplicating 👍
We could also remove the conditional on the comments: keyword parameter.
Let's release with a version change though.
There was a problem hiding this comment.
Done in a separate commit.
| rbs_type = decls.first | ||
| sorbet_type = RBI::RBS::TypeTranslator.translate(rbs_type.type) | ||
|
|
||
| alias_name = ::RBS::TypeName.new( |
There was a problem hiding this comment.
Could we reuse the logic from RBI::RBS::TypeTranslator#translate_type_alias?
There was a problem hiding this comment.
Yeah, I do want to use it, but I am not sure how. There is a TypeAlias for a type-alias type (i.e. type alias reference like myAlias) which only has a name and which RBI::RBS::TypeTranslator#translate_type_alias handles.
What we have here is a TypeAlias for a type-alias declaration (i.e. type alias declaration like type myAlias = Integer), which has a name and a type, which this needs to handle. I am not sure how I can use the logic for the previous one in this context, but let me think about it.
There was a problem hiding this comment.
I think the cleanest way to do this would be to modify rbi and create a new public helper function that takes a name. It could be used both here and in TypeTranslator. Since it'd require a version bump in rbi I'll hold off on it for now but let me know if you want it done.
Those should just work since the |
paracycle
force-pushed
the
uk-type-alias-rewriting
branch
from
1823880 to
90ff825
Compare
KaanOzkan
force-pushed
the
uk-type-alias-rewriting
branch
from
90ff825 to
2509a94
Compare
KaanOzkan
force-pushed
the
uk-type-alias-rewriting
branch
from
2509a94 to
5fbfe87
Compare
KaanOzkan
left a comment
KaanOzkan
left a comment
There was a problem hiding this comment.
Do we have any plans for type aliases used in inline annotations?
x = foo #: myTypeAlias
Spoom doesn't translate RBS assertions to their RBI equivalents so we're skipping this for now.
| rbs_type = decls.first | ||
| sorbet_type = RBI::RBS::TypeTranslator.translate(rbs_type.type) | ||
|
|
||
| alias_name = ::RBS::TypeName.new( |
There was a problem hiding this comment.
I think the cleanest way to do this would be to modify rbi and create a new public helper function that takes a name. It could be used both here and in TypeTranslator. Since it'd require a version bump in rbi I'll hold off on it for now but let me know if you want it done.
| end | ||
|
|
||
| #: (String ruby, file: String) -> [Prism::Node, Array[Prism::Comment]] | ||
| def parse_ruby_with_comments(ruby, file:) |
There was a problem hiding this comment.
Done in a separate commit.
4 participants
Resolves #809