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.

Sign up for GitHub

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

Add flatten_scopes rewriter #108

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+216 −0
Open

Add flatten_scopes rewriter #108

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73aee60
Add flatten_scopes rewriter
jeffcarbs Dec 1, 2021
f9949c0
Use RBI::Parser.parse_string for test setup
jeffcarbs Dec 2, 2021
9598683
Add tests for T::Struct and T::Enum
jeffcarbs Dec 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/rbi.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class Error < StandardError; end
require "rbi/rewriters/add_sig_templates"
require "rbi/rewriters/annotate"
require "rbi/rewriters/deannotate"
require "rbi/rewriters/flatten_scopes"
require "rbi/rewriters/merge_trees"
require "rbi/rewriters/nest_singleton_methods"
require "rbi/rewriters/nest_non_public_methods"
Expand Down
44 changes: 44 additions & 0 deletions lib/rbi/rewriters/flatten_scopes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# typed: strict
# frozen_string_literal: true

module RBI
module Rewriters
class FlattenScopes < Visitor
extend T::Sig

sig { override.params(node: T.nilable(Node)).void }
def visit(node)
return unless node

case node
when Tree
visit_all(node.nodes)

parent_tree = node.parent_tree
Copy link
Collaborator

Choose a reason for hiding this comment

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

My only concern with this is that if we have nested trees, we will end up moving the node to the wrong place which might get it processed again later:

For example consider this:

rbi = Tree.new do |tree1|
  tree1 << Class.new("Foo") do |cls1|
    cls1 << Tree.new do |tree2|
      tree2 << Class.new("Bar") do |cls2|
        cls2 << Method.new("bar")
      end
    end
  end
end

Running rbi.flatten_scopes! will give us something like this:

class Foo; end

class Foo::Foo::Bar
  def bar; end
end

I wonder if we should first find the root tree and put everything in it as we go instead of trying to find the parent one?

Or maybe the method should be flatten_scopes instead of flatten_scopes! and always return a new root Tree instead of modifying the current one?

return unless parent_tree

node.nodes.dup.each do |child|
next unless child.is_a?(Class) || child.is_a?(Module)
Morriar marked this conversation as resolved.
Show resolved Hide resolved

parent_scope = child.parent_scope
next unless parent_scope.is_a?(Class) || parent_scope.is_a?(Module)

child.detach
child.name = "#{parent_scope.name}::#{child.name}"
parent_tree << child
end
end
end
end
end

class Tree
extend T::Sig

sig { void }
def flatten_scopes!
visitor = Rewriters::FlattenScopes.new
visitor.visit(self)
end
end
end
102 changes: 102 additions & 0 deletions test/rbi/rewriters/flatten_scopes_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
# typed: true
# frozen_string_literal: true

require "test_helper"

module RBI
class FlattenScopesTest < Minitest::Test
def test_flatten_scopes_with_empty_scopes
rbi = RBI::Tree.new
scope1 = RBI::Module.new("A")
scope2 = RBI::Class.new("B")
scope3 = RBI::Class.new("C")
scope4 = RBI::Module.new("D")
scope5 = RBI::Module.new("E")
scope3 << scope4
scope2 << scope3
scope1 << scope2
rbi << scope1
rbi << scope5
Morriar marked this conversation as resolved.
Show resolved Hide resolved

rbi.flatten_scopes!

assert_equal(<<~RBI, rbi.string)
module A; end
module E; end
class A::B; end
class A::B::C; end
module A::B::C::D; end
RBI
end

def test_flatten_scopes_with_nonempty_scopes
rbi = RBI::Tree.new
scope1 = RBI::Module.new("A")
scope1 << RBI::Const.new("A1", "42")
scope2 = RBI::Class.new("B")
scope3 = RBI::Class.new("C")
scope3 << RBI::Const.new("C1", "42")
scope4 = RBI::Module.new("D")
scope5 = RBI::Module.new("E")
scope5 << RBI::Const.new("E1", "42")
scope3 << scope4
scope2 << scope3
scope1 << scope2
rbi << scope1
rbi << scope5
jeffcarbs marked this conversation as resolved.
Show resolved Hide resolved

rbi.flatten_scopes!

assert_equal(<<~RBI, rbi.string)
module A
A1 = 42
end

module E
E1 = 42
end

class A::B; end

class A::B::C
C1 = 42
end

module A::B::C::D; end
RBI
end

def test_flatten_scopes_with_singleton_classes
Morriar marked this conversation as resolved.
Show resolved Hide resolved
rbi = RBI::Tree.new
scope1 = RBI::Module.new("A")
scope2 = RBI::Class.new("B")
scope3 = RBI::Class.new("C")
scope3_singleton = RBI::SingletonClass.new
scope3_singleton << RBI::Method.new("m1")
scope4 = RBI::Module.new("D")
scope5 = RBI::Module.new("E")
scope3 << scope4
scope3 << scope3_singleton
scope2 << scope3
scope1 << scope2
rbi << scope1
rbi << scope5

rbi.flatten_scopes!

assert_equal(<<~RBI, rbi.string)
module A; end
module E; end
class A::B; end

class A::B::C
class << self
def m1; end
end
end

module A::B::C::D; end
RBI
end
end
end