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

Separate RBS files to _internal directory to avoid exposing RBS #237

Merged

Conversation

pocke
Copy link
Contributor

@pocke pocke commented Mar 3, 2022

This PR moves RBS declarations to _internal directory to avoid exposing internal RBS.

Problem

The method definitions for Hash and Object in the RBSs are duplicated with ActiveSupport's RBS. For example: https://github.com/ruby/gem_rbs_collection/blob/6320a194c85afbf4ff332c3eb75fcae2a6518c92/gems/activesupport/6.0/activesupport-generated.rbs#L3298

RBS doesn't allow duplicated method definitions, so we cannot use meta-tags gem and ActiveSupport RBS at the same time.
For example:

$ cat Gemfile
source "https://rubygems.org"

gem "activesupport"
gem 'meta-tags'
gem 'rbs'

$ bundle install
# Install meta-tags RBS from sig/ directory and ActiveSupport RBS from ruby/gem_rbs_collection repo
$ bundle exec rbs collection init && bundle exec rbs collection install

$ rbs validate
Validating class/module definition: `::Array`...
bundler: failed to load command: rbs (/path/to/.rbenv/versions/trunk/lib/ruby/gems/3.2.0/bin/rbs)
/path/to/lib/rbs/definition_builder/method_builder.rb:36:in `block in validate!': .gem_rbs_collection/activesupport/6.0/activesupport-generated.rbs:4588:2...4588:27: ::Object#blank? has duplicated definitions in /path/to/meta-tags-2.16.0/sig/lib/_rails.rbs:8:2...8:24 (RBS::DuplicatedMethodDefinitionError)
        from /path/to/lib/rbs/definition_builder/method_builder.rb:34:in `each_value'
        from /path/to/lib/rbs/definition_builder/method_builder.rb:34:in `validate!'
        from /path/to/lib/rbs/definition_builder/method_builder.rb:139:in `build_instance'
        from /path/to/lib/rbs/definition_builder.rb:148:in `block (2 levels) in build_instance'
(snip)

Solution

Move the conflicted RBSs to _internal directory.

RBS ignores directories that start with _ if the directory is loaded as a library. So when you want to make a polyfill, you can use _internal directory. It is loaded when we develop meta-tags gem itself, but it is not loaded when we use meta-tags gem from other application/library.

I also moved the interfaces under MetaTags namespace.
The interfaces are necessary for exposed RBSs, so I didn't move them to _internal directory.
They are defined at the top level, so I moved them under the namespace to avoid conflicting other RBS.

Alternative Solution

We can also use the ActiveSupport's RBS from ruby/gem_rbs_collection repository instead of the polyfill. But I didn't recommend this way for now.

Because currently Steep supports rbs collection feature partially.
rbs collection is the library management system for RBS, like Bundler. You can install dependencies' RBSs with the command out of the box.
RBS v1 provides it as an experimental feature and it's GA on RBS v2. The latest Steep is not compatible with RBS v2.
So to use rbs collection, we need to use the experimental one, lack of some features, or use the master branch of Steep.

I think it is premature to use stably, so I recommend using the polyfill.


A lot of thanks to writing RBSs!

@kpumuk kpumuk merged commit 7c40235 into kpumuk:main Mar 3, 2022
@kpumuk
Copy link
Owner

kpumuk commented Mar 3, 2022

Thank you for the detailed investigation!

@pocke pocke deleted the Fix_RBS_files_to_avoid_to_expose_Rails_signature branch March 4, 2022 01:02
@natematykiewicz
Copy link

natematykiewicz commented Jul 7, 2022

I don't think the type signature for ::Object#presence is right.

class ::Object
  def presence: () -> String?
end

The definition of presence is:

class Object
  def presence
    self if present?
  end
end

https://github.com/rails/rails/blob/f95c0b7e96eb36bc3efc0c5beffbb9e84ea664e4/activesupport/lib/active_support/core_ext/object/blank.rb#L45-L48

So, presence returns the receiver or nil, not String or nil.

Note:
I'm very new to RBS, so I might be wrong here. I just looked at this PR, because Dependabot told me about meta-tags 2.17.0.

@kpumuk
Copy link
Owner

kpumuk commented Jul 7, 2022

Thanks @natematykiewicz

@natematykiewicz
Copy link

No problem!

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