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

egiurleo · 2022-06-27T14:58:35Z

Closes #890

Motivation

This is the culmination of all the dynamic mixin work!

Currently, when RBIs are generated for mixins, tapioca doesn't take into account whether a mixin was performed in the current gem or in another gem. This results in strange scenarios where one gem will re-open a class to perform a mixin, and then that mixin is reflected in another gem's RBI. We should only generate RBI for mixins that are performed in the current gem.

Implementation

This PR can be read commit-by-commit.

The first commit refactors the mixin tracker to remove the mixin_locations_for method, which is no longer used, and modify the constants_with_mixin method to keep track of mixin locations.
The second commit moves the constant_name_from_singleton_class method into Runtime::Reflection so it can be shared with the Module extension coming from the mixin tracker.
The second commit modifies the mixins pipeline listener to use the mixin_locations_for method to skip any mixins that weren't performed in the current gem.

Tests

Modified existing tests to remove mixins that should be filtered out. Un-commented two lines in a cli test that now pass because of these changes!

Tophatting 🎩

Can generate gem RBIs on Shopify core with these changes.

egiurleo · 2022-06-27T15:59:04Z

lib/tapioca/gem/listeners/foreign_constants.rb

@@ -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 do |_, location_info|


The changes in this file are a result of the refactor to constants_with_mixin. Functionality is not changed.

Should this be:

Suggested change

Runtime::Trackers::Mixin.constants_with_mixin(mixin).each do |_, location_info|

Runtime::Trackers::Mixin.constants_with_mixin(mixin).each_value do |location_info|

instead?

egiurleo · 2022-06-27T16:08:03Z

lib/tapioca/gem/listeners/mixins.rb

+              # Some extend mixins are actually includes or prepend on the singleton class.
+              # If an extend mixin is not found in the tracker, these lines search for a prepend
+              # or include on the singleton class and use the resulting file as the mixin location.
+              if mixin_location.nil? && mixin_type == Runtime::Trackers::Mixin::Type::Extend


Sometimes, extension mixins are actually tracked in the mixin tracker as prepends or includes to the singleton class. If the mixin is not found under the Runtime::Trackers::Mixin::Type::Extend type, these lines search the mixin tracker for a prepend or include on the singleton class.

This feels a little bit hacky to me...

This is something we can rectify at mixin registration time, I guess. If a mixin is being prepended/included into a singleton class, then we can record that as an extend on the attached class at that time.

egiurleo · 2022-06-27T16:08:30Z

spec/tapioca/cli/gem_spec.rb

          # actionpack RBI should have nothing in it about `TypedParameters`
-          # refute_includes(actionpack_rbi, "TypedParameters")
+          refute_includes(actionpack_rbi, "TypedParameters")


This assertion passes now 🎉

spec/tapioca/gem/pipeline_spec.rb

Morriar · 2022-06-27T19:27:47Z

lib/tapioca/gem/listeners/mixins.rb

          mods
            .select do |mod|
              name = @pipeline.name_of(mod)

              name && !filtered_mixin?(name)
            end
            .map do |mod|
+              mixin_location = mixin_location(constant, mod, mixin_type)
+
+              # Some extend mixins are actually includes or prepend on the singleton class.


Suggested change

# Some extend mixins are actually includes or prepend on the singleton class.

# Some extend mixins are actually includes or prepends on the singleton class.

Morriar · 2022-06-27T19:31:05Z

lib/tapioca/gem/listeners/mixins.rb

+            location: String,
+          ).returns(T::Boolean)
+        end
+        def mixed_in_by_gem?(location)


The method is mixed_in_by_gem? but works with a location only, which doesn't seem strictly related to mixins, should we rename it location_in_gem??

Or maybe mixed_in_by_gem? should take a constant?

Morriar · 2022-06-27T19:33:00Z

lib/tapioca/gem/listeners/mixins.rb

+        sig do
+          params(
+            location: String,
+          ).returns(T::Boolean)
+        end


Let's inline signatures < 120 chars please 🙏

paracycle · 2022-06-28T13:56:32Z

lib/tapioca/gem/listeners/foreign_constants.rb

@@ -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 do |_, location_info|


Should this be:

Suggested change

Runtime::Trackers::Mixin.constants_with_mixin(mixin).each do |_, location_info|

Runtime::Trackers::Mixin.constants_with_mixin(mixin).each_value do |location_info|

instead?

paracycle · 2022-06-28T13:59:43Z

lib/tapioca/gem/listeners/mixins.rb

+          ).returns(T.nilable(String))
+        end
+        def mixin_location(constant, mixin, mixin_type)
+          T.must(Runtime::Trackers::Mixin.constants_with_mixin(mixin)[mixin_type])[constant]


Can we use dig instead?

Suggested change

T.must(Runtime::Trackers::Mixin.constants_with_mixin(mixin)[mixin_type])[constant]

Runtime::Trackers::Mixin.constants_with_mixin(mixin).dig(mixin_type, constant)

paracycle · 2022-06-28T14:02:05Z

lib/tapioca/gem/listeners/mixins.rb

+          T.must(mixin_locations[Runtime::Trackers::Mixin::Type::Include])[singleton_class] ||
+            T.must(mixin_locations[Runtime::Trackers::Mixin::Type::Prepend])[singleton_class]


Again, dig would be much nicer:

Suggested change

T.must(mixin_locations[Runtime::Trackers::Mixin::Type::Include])[singleton_class] ||

T.must(mixin_locations[Runtime::Trackers::Mixin::Type::Prepend])[singleton_class]

mixin_locations.dig(Runtime::Trackers::Mixin::Type::Include, singleton_class) ||

mixin_locations.dig(Runtime::Trackers::Mixin::Type::Prepend, singleton_class)

spec/tapioca/gem/pipeline_spec.rb

paracycle · 2022-06-28T14:36:08Z

lib/tapioca/gem/listeners/mixins.rb

+              # Some extend mixins are actually includes or prepend on the singleton class.
+              # If an extend mixin is not found in the tracker, these lines search for a prepend
+              # or include on the singleton class and use the resulting file as the mixin location.
+              if mixin_location.nil? && mixin_type == Runtime::Trackers::Mixin::Type::Extend


This is something we can rectify at mixin registration time, I guess. If a mixin is being prepended/included into a singleton class, then we can record that as an extend on the attached class at that time.

Refactor the Trackers::Mixin#constants_with_mixin method to keep track of the type of each mixin. This will be used to filter mixins that were not performed in the current gem.

Uses `Trackers::Mixin` to find the location of each mixin and filter out any that did not occur in the current gem to avoid generating unnecessary RBI.

egiurleo · 2022-06-28T20:38:15Z

lib/tapioca/runtime/reflection.rb

@@ -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)


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.

egiurleo · 2022-06-28T20:38:28Z

lib/tapioca/gem/listeners/foreign_constants.rb

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


This method was moved into Runtime::Reflection

lib/tapioca/gem/listeners/mixins.rb

egiurleo · 2022-06-28T20:41:25Z

lib/tapioca/runtime/trackers/mixin.rb

+    # 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)


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.

egiurleo · 2022-06-28T20:41:53Z

spec/tapioca/gem/pipeline_spec.rb

-        .gsub(/^\s+include ::Minitest::Expectations\s/, "")
-        .gsub(/^\s+include ::JSON::Ext::Generator::GeneratorMethods::Object\s/, "")
-        .gsub(/^\s+include ::PP::ObjectMixin\s/, "")


@paracycle Is this gsub you were talking about?

egiurleo marked this pull request as ready for review June 27, 2022 15:27

egiurleo force-pushed the emily/mixin-filtering branch from b9aca82 to 50ada09 Compare June 27, 2022 16:07

egiurleo commented Jun 27, 2022

View reviewed changes

egiurleo requested a review from a team June 27, 2022 16:09

Morriar reviewed Jun 27, 2022

View reviewed changes

paracycle reviewed Jun 28, 2022

View reviewed changes

egiurleo added 3 commits June 28, 2022 12:50

Add mixin_type to constants_with_mixin method

f1d07cc

Refactor the Trackers::Mixin#constants_with_mixin method to keep track of the type of each mixin. This will be used to filter mixins that were not performed in the current gem.

Move #contant_name_from_singleton_class into Runtime::Reflection

0ea9bde

Filter out mixins that are not performed in current gem

445d9ef

Uses `Trackers::Mixin` to find the location of each mixin and filter out any that did not occur in the current gem to avoid generating unnecessary RBI.

egiurleo force-pushed the emily/mixin-filtering branch from 50ada09 to 445d9ef Compare June 28, 2022 20:37

egiurleo commented Jun 28, 2022

View reviewed changes

egiurleo requested review from paracycle and Morriar June 28, 2022 20:42

Morriar approved these changes Jun 29, 2022

View reviewed changes

paracycle added the feature label Jun 29, 2022

paracycle approved these changes Jun 29, 2022

View reviewed changes

Morriar merged commit b61b074 into main Jun 29, 2022

Morriar deleted the emily/mixin-filtering branch June 29, 2022 14:40

shopify-shipit bot temporarily deployed to production July 7, 2022 17:53 Inactive

shopify-shipit bot temporarily deployed to 0-9-stable August 19, 2022 20:37 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

egiurleo commented Jun 27, 2022 •

edited

Loading

egiurleo Jun 27, 2022

paracycle Jun 28, 2022

egiurleo Jun 27, 2022

paracycle Jun 28, 2022

egiurleo Jun 27, 2022

Morriar Jun 27, 2022

Morriar Jun 27, 2022

Morriar Jun 27, 2022

paracycle Jun 28, 2022

paracycle Jun 28, 2022

paracycle Jun 28, 2022

paracycle Jun 28, 2022

egiurleo Jun 28, 2022

egiurleo Jun 28, 2022

egiurleo Jun 28, 2022

egiurleo Jun 28, 2022

	Runtime::Trackers::Mixin.constants_with_mixin(mixin).each do \|_, location_info\|
	Runtime::Trackers::Mixin.constants_with_mixin(mixin).each_value do \|location_info\|

	# Some extend mixins are actually includes or prepend on the singleton class.
	# Some extend mixins are actually includes or prepends on the singleton class.

	T.must(Runtime::Trackers::Mixin.constants_with_mixin(mixin)[mixin_type])[constant]
	Runtime::Trackers::Mixin.constants_with_mixin(mixin).dig(mixin_type, constant)

		T.must(mixin_locations[Runtime::Trackers::Mixin::Type::Include])[singleton_class] \|\|
		T.must(mixin_locations[Runtime::Trackers::Mixin::Type::Prepend])[singleton_class]

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

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

Conversation

egiurleo commented Jun 27, 2022 • edited Loading

Motivation

Implementation

Tests

Tophatting 🎩

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egiurleo commented Jun 27, 2022 •

edited

Loading