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

Jump to bottom

Add flatten_scopes rewriter #108

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add flatten_scopes rewriter #108

Show file tree
Hide file tree
Changes from all commits
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
171 changes: 171 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,171 @@
# typed: true
# frozen_string_literal: true

require "test_helper"

module RBI
class FlattenScopesTest < Minitest::Test
def test_flatten_scopes_with_empty_scopes
rbi = RBI::Parser.parse_string(<<~RBI)
module A
class B
class C
module D; end
end
end
end

module E; end
RBI

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::Parser.parse_string(<<~RBI)
module A
A1 = 42

class B
class C
C1 = 42

module D; end
end
end
end

module E
E1 = 42
end
RBI

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::Parser.parse_string(<<~RBI)
module A
class B
class C
module D; end

class << self
def m1; end
end
end
end
end

module E; end
RBI

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

def test_flatten_scopes_with_t_struct
rbi = RBI::Parser.parse_string(<<~RBI)
module A
class B < T::Struct
const :foo, String

class C
module D; end
end
end
end

module E; end
RBI

rbi.flatten_scopes!

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

class A::B < T::Struct
const :foo, String
end

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

def test_flatten_scopes_with_t_enum
rbi = RBI::Parser.parse_string(<<~RBI)
module A
class B
class C
class D < T::Enum
enums do
Foo = new
Bar = new
end
end
end
end
end

module E; end
RBI

rbi.flatten_scopes!

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

class A::B::C::D < T::Enum
enums do
Foo = new
Bar = new
end
end
RBI
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add one more test showing the expected behaviour when the scopes are duplicated:

Suggested change
end
end
def test_flatten_duplicated_scopes
rbi = RBI::Parser.parse_string(<<~RBI)
module A
class B; end
end
module A
class C; end
end
module A
class D; end
end
RBI
rbi.flatten_scopes!
assert_equal(<<~RBI, rbi.string)
module A; end
module A; end
module A; end
class A::B; end
class A::C; end
class A::D; end
RBI
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should be smarter with this using an Index but I believe we can do this later if needed.

end
end