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

Always fully qualify types for RBIs and avoid using attributes #1767

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

vinistock
Copy link
Contributor

There were a couple of things I missed in #1716.

Tapioca always uses compact class/module style (e.g.: class Foo::Bar) and never nests definitions inside a namespace. Because of that, all of the types we use must be fully qualified or else they will cause a typechecking error.

For example

class Prism::CallNode
  sig { returns(T.nilable(Location)) } # => Location does not exist. We need to use Prism::Location
  def message_loc; end
end

Also, Tapioca won't properly mix the signatures if we use attr_reader. We should always use regular method definitions in RBIs.

I tested this on Ruby LSP and the RBI generated after these changes is fully valid.

@@ -2,79 +2,79 @@

module Prism
class ParseResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vinistock do you want to un-nest this? i.e., make it class Prism::ParseResult`?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. It'll make it easier to spot future issues.

I also removed the # typed: true comments from the top of the files, because Tapioca adds those automatically - and we were ending up with duplicates.

@kddnewton kddnewton merged commit 09ca9ba into ruby:main Nov 3, 2023
46 checks passed
@paracycle
Copy link
Collaborator

Also, Tapioca won't properly mix the signatures if we use attr_reader. We should always use regular method definitions in RBIs.

I think this is actually a bug in RBI wrt its logic when doing a merge. It should treat attr_xxx defined methods and readers/writers on the same footing and merge their signatures properly.

@vinistock
Copy link
Contributor Author

That's a good point. I created Shopify/tapioca#1709 to investigate.

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.

3 participants