Use our internal RBI backend for DSL generators #388

Morriar · 2021-07-14T23:48:43Z

Motivation

Use our RBI generation framework instead of Parlour.

Implementation

Added a RBI::Builder to make an interface compatible with our current generators and minimize the diffs.

Tests

See automated tests.

This was also tested by regenerating all Core DSL RBIs with success. Typecheking is passing and diff is minimal.

One interesting side effect is the time required to generate all the RBIs:

Before: 3m 31s
After: 2m 23s

Fixes #346.

Morriar · 2021-07-15T00:01:38Z

lib/tapioca/compilers/dsl/state_machines.rb

        def decorate(root, constant)
          return if constant.state_machines.empty?

-          root.path(constant) do |klass|
+          root.create_path(T.unsafe(constant)) do |klass|


Tree#create_path expects a Module but we get a StateMachines::ClassMethods for argument constant which doesn't seem to be a module.

One notable problem here is that the redefinition of decorate does not follow Liskov substitution principle. We do some shady thing (https://github.com/Shopify/tapioca/blob/master/lib/tapioca/compilers/dsl/base.rb#L28-L36) to avoid detection but this is weird design.

We should refactor this part to offer a correct/safe interface.

Morriar · 2021-07-15T00:04:22Z

lib/tapioca/rbi/builder.rb

+
+      sig { params(node: Node).returns(Node) }
+      def create_node(node)
+        cached = nodes_cache[node.to_s]


To avoid recreating the scopes between two generators, if we already created it in a previous generator we return it and append to it.

Morriar · 2021-07-15T00:05:46Z

spec/tapioca/cli_spec.rb

@@ -451,47 +452,50 @@ def title=(title); end
      assert_path_exists("#{outdir}/namespace/comment.rbi")
      refute_path_exists("#{outdir}/user.rbi")

-      assert_equal(<<~CONTENTS.chomp, File.read("#{outdir}/baz/role.rbi"))
+      assert_equal(<<~CONTENTS, File.read("#{outdir}/baz/role.rbi"))


No need to chomp since this PR fixes #346.

Morriar · 2021-07-15T00:07:12Z

spec/tapioca/compilers/dsl/active_record_enum_spec.rb

        class Conversation
          include EnumMethodsModule

+          sig { returns(T::Hash[T.any(String, Symbol), Integer]) }


Like for gem RBIs, all methods are sorted before the subconstants

Morriar · 2021-07-15T00:08:11Z

spec/tapioca/compilers/dsl/protobuf_spec.rb

        class Cart
+          sig { params(customer_id: Integer, shop_id: Integer).void }


Like in gems RBIs, initialize methods are always sorted first.

Morriar · 2021-07-15T00:10:12Z

spec/tapioca/compilers/symbol_table_compiler_spec.rb

@@ -268,10 +268,10 @@ def to_bar; end

        class String
          include ::Comparable
-          include ::JSON::Ext::Generator::GeneratorMethods::String


This order changes because we removed Parlour has a dependency.

Morriar · 2021-07-15T00:13:10Z

spec/tapioca/compilers/dsl/url_helpers_spec.rb

          include ::ActionDispatch::Routing::UrlFor
+          include ::ActionDispatch::Routing::PolymorphicRoutes


This is now in the same order we generate them:

Not sure why Parlour was changing the order?

Nice! Thankfully, the order didn't matter here.

Morriar · 2021-07-15T00:14:57Z

spec/tapioca/compilers/dsl/url_helpers_spec.rb

+
+        class ActionDispatch::IntegrationTest
+          include GeneratedUrlHelpersModule
+          include GeneratedPathHelpersModule


Now in the same order we want them to be generated:

KaanOzkan

Great speedup 🏃

KaanOzkan · 2021-07-15T15:39:09Z

spec/tapioca/compilers/dsl/active_record_enum_spec.rb

        class Conversation
          include EnumMethodsModule

+          sig { returns(T::Hash[T.any(String, Symbol), Integer]) }
+          def self.statuses; end


Usage of both class << self and def self. in the RBIs is inconsistent. Do we prefer it over sticking with def self. everywhere?

I'm planning to make this configurable in a follow up.

My thinking is to always create them as def self.foo then let the use decide if they should be printed as such or inside a class << self.

For now I aimed at the smaller possible diff in generated files.

paracycle

Looks great! I just have one question.

spec/tapioca/cli_spec.rb

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

…elf block Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Morriar · 2021-07-20T16:12:20Z

Pushed a few changed:

Rebased on master and fixed merge conflicts
Removed the class << self nesting
Added a last commit to always sort def self.x singleton methods after the non singleton ones

Morriar requested a review from a team July 14, 2021 23:48

Morriar commented Jul 15, 2021

View reviewed changes

KaanOzkan approved these changes Jul 15, 2021

View reviewed changes

paracycle reviewed Jul 19, 2021

View reviewed changes

spec/tapioca/cli_spec.rb Outdated Show resolved Hide resolved

Morriar added 19 commits July 20, 2021 11:27

Load all RBI dependencies at once

bb80a60

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com

Generate DSL RBIs using the new RBI framework

625e35c

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate action_controller_helpers generator and tests

e11caed

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate action_mailer generator and tests

98cf780

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate active_job controller and tests

0f0a269

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate active_record_associations generator and tests

c4d98c1

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate active_record_columns controller and tests

70a9d5c

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate active_record_enum generator and tests

843c4c2

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate active_record_scope generator and tests

79b991f

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate active_record_typed_store generator and tests

c9e0196

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate active_resource generator and tests

eea2bf9

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate active_support_concern generator and tests

81b3d57

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate active_support_current_attributes generator and tests

45899e8

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate config generator and tests

c730b27

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate frozen_record generator and tests

e1f0646

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate identity_cache generator and tests

2a8b3d3

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate protobuf generator and spec

1e6d669

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate sidekiq_worker generator and spec

72a62bb

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate smart_properties generator and tests

6d447f5

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Morriar added 6 commits July 20, 2021 12:04

Migrate state_machines generator and tests

905df38

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate url_helpers generator and tests

5aeec75

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Migrate DSL generation in tests to new RBI framework

f377032

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com

Remove dependency to Parlour

7585051

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Update CLI tests

f414579

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Always sort methods before singleton methods when not in a class << s…

daf867f

…elf block Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>

Morriar force-pushed the at-rbi-dsl branch from 76e713e to daf867f Compare July 20, 2021 16:10

Morriar requested a review from paracycle July 20, 2021 16:12

paracycle approved these changes Jul 20, 2021

View reviewed changes

Morriar merged commit af07710 into master Jul 20, 2021

Morriar deleted the at-rbi-dsl branch July 20, 2021 16:29

shopify-shipit bot temporarily deployed to production September 7, 2021 16:35 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use our internal RBI backend for DSL generators #388

Use our internal RBI backend for DSL generators #388

Morriar commented Jul 14, 2021 •

edited

Loading

Morriar Jul 15, 2021

Morriar Jul 15, 2021

Morriar Jul 15, 2021

Morriar Jul 15, 2021 •

edited

Loading

Morriar Jul 15, 2021

Morriar Jul 15, 2021

Morriar Jul 15, 2021

paracycle Jul 19, 2021

Morriar Jul 15, 2021

KaanOzkan left a comment

KaanOzkan Jul 15, 2021

Morriar Jul 15, 2021

paracycle left a comment

Morriar commented Jul 20, 2021

		class Cart
		sig { params(customer_id: Integer, shop_id: Integer).void }

		include ::ActionDispatch::Routing::UrlFor
		include ::ActionDispatch::Routing::PolymorphicRoutes

Use our internal RBI backend for DSL generators #388

Use our internal RBI backend for DSL generators #388

Conversation

Morriar commented Jul 14, 2021 • edited Loading

Motivation

Implementation

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Morriar Jul 15, 2021 • edited Loading

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

KaanOzkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paracycle left a comment

Choose a reason for hiding this comment

Morriar commented Jul 20, 2021

Morriar commented Jul 14, 2021 •

edited

Loading

Morriar Jul 15, 2021 •

edited

Loading