Skip to content

Filter out mixins that weren't performed in current gem #1012

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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Morriar merged 3 commits into from
Jun 29, 2022
Merged

Filter out mixins that weren't performed in current gem #1012

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
f1d07cc
Add mixin_type to constants_with_mixin method
egiurleo Jun 27, 2022
0ea9bde
Move #contant_name_from_singleton_class into Runtime::Reflection
egiurleo Jun 28, 2022
445d9ef
Filter out mixins that are not performed in current gem
egiurleo Jun 23, 2022
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
33 changes: 15 additions & 18 deletions lib/tapioca/gem/listeners/foreign_constants.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,25 @@ def on_scope(event)
# The way we identify these "foreign constants" is by asking the mixin tracker which
# constants have mixed in the current module that we are handling. We add all the
# constants that we discover to the pipeline to be processed.
Runtime::Trackers::Mixin.constants_with_mixin(mixin).each do |constant, location|
next unless mixed_in_by_gem?(location)
Runtime::Trackers::Mixin.constants_with_mixin(mixin).each_value do |location_info|
location_info.each do |constant, location|
next unless mixed_in_by_gem?(location)

name = @pipeline.name_of(constant)
name = @pipeline.name_of(constant)

# Calling Tapioca::Gem::Pipeline#name_of on a singleton class returns `nil`.
# To handle this case, use string parsing to get the name of the singleton class's
# base constant. Then, generate RBIs as if the base constant is extending the mixin,
# which is functionally equivalent to including or prepending to the singleton class.
if !name && constant.singleton_class?
name = constant_name_from_singleton_class(constant)
next unless name
# Calling Tapioca::Gem::Pipeline#name_of on a singleton class returns `nil`.
# To handle this case, use string parsing to get the name of the singleton class's
# base constant. Then, generate RBIs as if the base constant is extending the mixin,
# which is functionally equivalent to including or prepending to the singleton class.
if !name && constant.singleton_class?
name = constant_name_from_singleton_class(constant)
next unless name

constant = T.cast(constantize(name), Module)
end
constant = T.cast(constantize(name), Module)
end

@pipeline.push_foreign_constant(name, constant) if name
@pipeline.push_foreign_constant(name, constant) if name
end
end
end

Expand All @@ -53,11 +55,6 @@ def mixed_in_by_gem?(location)
@pipeline.gem.contains_path?(location)
end

sig { params(constant: Module).returns(T.nilable(String)) }
def constant_name_from_singleton_class(constant)
constant.to_s.match("#<Class:(.+)>")&.captures&.first
end
Comment on lines -56 to -59

@egiurleo egiurleo Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This method was moved into Runtime::Reflection


sig { override.params(event: NodeAdded).returns(T::Boolean) }
def ignore?(event)
event.is_a?(Tapioca::Gem::ForeignScopeNodeAdded)
Expand Down
28 changes: 24 additions & 4 deletions lib/tapioca/gem/listeners/mixins.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,26 +26,29 @@ def on_scope(event)
end

node = event.node
add_mixins(node, prepends.reverse, Runtime::Trackers::Mixin::Type::Prepend)
add_mixins(node, includes.reverse, Runtime::Trackers::Mixin::Type::Include)
add_mixins(node, extends.reverse, Runtime::Trackers::Mixin::Type::Extend)
add_mixins(node, constant, prepends.reverse, Runtime::Trackers::Mixin::Type::Prepend)
add_mixins(node, constant, includes.reverse, Runtime::Trackers::Mixin::Type::Include)
add_mixins(node, constant, extends.reverse, Runtime::Trackers::Mixin::Type::Extend)
end

sig do
params(
tree: RBI::Tree,
constant: Module,
mods: T::Array[Module],
mixin_type: Runtime::Trackers::Mixin::Type
).void
end
def add_mixins(tree, mods, mixin_type)
def add_mixins(tree, constant, mods, mixin_type)
mods
.select do |mod|
name = @pipeline.name_of(mod)

name && !filtered_mixin?(name)
end
.map do |mod|
next unless mixed_in_by_gem?(constant, mod, mixin_type)

name = @pipeline.name_of(mod)
@pipeline.push_symbol(name) if name

Expand All @@ -62,6 +65,23 @@ def add_mixins(tree, mods, mixin_type)
end
end

sig do
params(
constant: Module,
mixin: Module,
mixin_type: Runtime::Trackers::Mixin::Type
).returns(T::Boolean)
end
def mixed_in_by_gem?(constant, mixin, mixin_type)
mixin_location =
T.cast(
Runtime::Trackers::Mixin.constants_with_mixin(mixin).dig(mixin_type, constant),
T.nilable(String)
)

!!mixin_location && @pipeline.gem.contains_path?(mixin_location)
end
Comment thread
Morriar marked this conversation as resolved.

sig { params(mixin_name: String).returns(T::Boolean) }
def filtered_mixin?(mixin_name)
# filter T:: namespace mixins that aren't T::Props
Expand Down
11 changes: 11 additions & 0 deletions lib/tapioca/runtime/reflection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,17 @@ def required_from_location

required_location.absolute_path || ""
end

sig { params(constant: Module).returns(T.nilable(String)) }
def constant_name_from_singleton_class(constant)

@egiurleo egiurleo Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I moved these two methods into reflection because I had to use them in Trackers::Mixin to find the attached class based on a singleton class.

constant.to_s.match("#<Class:(.+)>")&.captures&.first
end

sig { params(constant: Module).returns(T.nilable(BasicObject)) }
def constant_from_singleton_class(constant)
constant_name = constant_name_from_singleton_class(constant)
constantize(constant_name) if constant_name
end
end
end
end
38 changes: 26 additions & 12 deletions lib/tapioca/runtime/trackers/mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,26 +28,18 @@ class Type < T::Enum
def self.register(constant, mixin, mixin_type)
location = Reflection.required_from_location

locs = mixin_locations_for(constant)
locs.fetch(mixin_type).store(mixin, location)

constants = constants_with_mixin(mixin)
constants[constant] = location
constants.fetch(mixin_type).store(constant, location)
end

sig { params(constant: Module).returns(T::Hash[Type, T::Hash[Module, String]]) }
def self.mixin_locations_for(constant)
@constants_to_mixin_locations[constant] ||= {
sig { params(mixin: Module).returns(T::Hash[Type, T::Hash[Module, String]]) }
def self.constants_with_mixin(mixin)
@mixins_to_constants[mixin] ||= {
Type::Prepend => {}.compare_by_identity,
Type::Include => {}.compare_by_identity,
Type::Extend => {}.compare_by_identity,
}
end

sig { params(mixin: Module).returns(T::Hash[Module, String]) }
def self.constants_with_mixin(mixin)
@mixins_to_constants[mixin] ||= {}.compare_by_identity
end
end
end
end
Expand All @@ -61,6 +53,9 @@ def prepend_features(constant)
self,
Tapioca::Runtime::Trackers::Mixin::Type::Prepend,
)

register_extend_on_attached_class(constant) if constant.singleton_class?

super
end

Expand All @@ -70,6 +65,9 @@ def append_features(constant)
self,
Tapioca::Runtime::Trackers::Mixin::Type::Include,
)

register_extend_on_attached_class(constant) if constant.singleton_class?

super
end

Expand All @@ -81,5 +79,21 @@ def extend_object(obj)
) if Module === obj
super
end

private

# Including or prepending on a singleton class is functionally equivalent to extending the
# attached class. Registering the mixin as an extend on the attached class ensures that
# this mixin can be found whether searching for an include/prepend on the singleton class
# or an extend on the attached class.
def register_extend_on_attached_class(constant)

@egiurleo egiurleo Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This functionality was moved from Listeners::Mixins so that an include/prepend on a singleton class can also be found as an extend on the attached class.

attached_class = Tapioca::Runtime::Reflection.constant_from_singleton_class(constant)

Tapioca::Runtime::Trackers::Mixin.register(
T.cast(attached_class, Module),
self,
Tapioca::Runtime::Trackers::Mixin::Type::Extend,
) if attached_class
end
end)
end
5 changes: 2 additions & 3 deletions spec/tapioca/cli/gem_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1096,10 +1096,9 @@ module TypedParameters
assert_includes(response.out, "Compiled actionpack")
assert_includes(response.out, "Compiled typed_parameters")

# TODO: Uncomment when addressing part 2 of tapioca#890
# actionpack_rbi = @project.read("sorbet/rbi/gems/[email protected]")
actionpack_rbi = @project.read("sorbet/rbi/gems/[email protected]")
# actionpack RBI should have nothing in it about `TypedParameters`
# refute_includes(actionpack_rbi, "TypedParameters")
refute_includes(actionpack_rbi, "TypedParameters")

@egiurleo egiurleo Jun 27, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This assertion passes now 馃帀


assert_project_file_equal("sorbet/rbi/gems/[email protected]", <<~RBI)
# typed: true
Expand Down
33 changes: 0 additions & 33 deletions spec/tapioca/gem/pipeline_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,16 +313,11 @@ def hello; end

object_output = template(<<~RBI)
class Object < ::BasicObject
include ::Kernel
Comment thread
egiurleo marked this conversation as resolved.

def hello; end
end
RBI

compiled = compile
.gsub(/^\s+include ::Minitest::Expectations\s/, "")
.gsub(/^\s+include ::JSON::Ext::Generator::GeneratorMethods::Object\s/, "")
.gsub(/^\s+include ::PP::ObjectMixin\s/, "")
Comment on lines -323 to -325

@egiurleo egiurleo Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@paracycle Is this gsub you were talking about?


assert_includes(compiled, basic_object_output)
assert_includes(compiled, object_output)
Expand Down Expand Up @@ -446,8 +441,6 @@ def bar; end
output = template(<<~RBI)
class Array
include ::Foo::Bar
include ::Enumerable
include ::JSON::Ext::Generator::GeneratorMethods::Array

def foo_int; end
end
Expand All @@ -460,8 +453,6 @@ def to_s; end
module Foo::Bar; end

class Hash
include ::Enumerable
include ::JSON::Ext::Generator::GeneratorMethods::Hash
extend ::Foo::Bar

def to_bar; end
Expand All @@ -472,10 +463,7 @@ def bar; end
end

class String
include ::Comparable
include ::JSON::Ext::Generator::GeneratorMethods::String
include ::Foo::Bar
extend ::JSON::Ext::Generator::GeneratorMethods::String::Extend

def to_foo(base = T.unsafe(nil)); end
end
Expand Down Expand Up @@ -519,8 +507,6 @@ class Array
output = template(<<~RBI)
class Array
include ::Foo::Bar
include ::Enumerable
include ::JSON::Ext::Generator::GeneratorMethods::Array
end

class Foo
Expand All @@ -531,16 +517,11 @@ def to_s; end
module Foo::Bar; end

class Hash
include ::Enumerable
include ::JSON::Ext::Generator::GeneratorMethods::Hash
extend ::Foo::Bar
end

class String
include ::Comparable
include ::JSON::Ext::Generator::GeneratorMethods::String
include ::Foo::Bar
extend ::JSON::Ext::Generator::GeneratorMethods::String::Extend
end
RBI

Expand Down Expand Up @@ -571,8 +552,6 @@ module Bar; end
output = template(<<~RBI)
class Array
include ::Foo::Bar
include ::Enumerable
include ::JSON::Ext::Generator::GeneratorMethods::Array
end

class Foo
Expand All @@ -583,16 +562,11 @@ def to_s; end
module Foo::Bar; end

class Hash
include ::Enumerable
include ::JSON::Ext::Generator::GeneratorMethods::Hash
extend ::Foo::Bar
end

class String
include ::Comparable
include ::JSON::Ext::Generator::GeneratorMethods::String
include ::Foo::Bar
extend ::JSON::Ext::Generator::GeneratorMethods::String::Extend
end
RBI

Expand Down Expand Up @@ -2431,9 +2405,6 @@ def self.name
output = template(<<~RBI)
class Foo
extend ::T::Props
extend ::T::Props::Plugin
extend ::T::Props::Optional
extend ::T::Props::WeakConstructor
extend ::T::Props::Constructor

class << self
Expand Down Expand Up @@ -2851,10 +2822,6 @@ def do_it; end
end

class Buzz
include ::T::Props
include ::T::Props::Plugin
include ::T::Props::Optional
include ::T::Props::WeakConstructor
include ::T::Props::Constructor
extend ::T::Props::ClassMethods
extend ::T::Props::Plugin::ClassMethods
Expand Down