Filter out mixins that weren't performed in current gem by egiurleo · Pull Request #1012 · Shopify/tapioca

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

          # 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

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

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


This assertion passes now 🎉

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

            .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

+            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

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


Let's inline signatures < 120 chars please 🙏

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

          # 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

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

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

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

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

      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

-        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

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

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

-        .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 Morriar and paracycle 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

	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]

Uh oh!

Conversation

egiurleo commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Implementation

Tests

Tophatting 🎩

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

egiurleo commented Jun 27, 2022 •

edited

Loading