From c09b0ea518d98df14c3b2833451dd87e5bb8d72e Mon Sep 17 00:00:00 2001 From: Dirceu Pereira Tiegs Date: Wed, 22 Feb 2023 16:26:34 -0500 Subject: [PATCH 1/5] protobuf: Fix case where descriptor is nil Fixes https://github.com/Shopify/tapioca/issues/1410 --- lib/tapioca/dsl/compilers/protobuf.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/tapioca/dsl/compilers/protobuf.rb b/lib/tapioca/dsl/compilers/protobuf.rb index a97dd5512..dc78af316 100644 --- a/lib/tapioca/dsl/compilers/protobuf.rb +++ b/lib/tapioca/dsl/compilers/protobuf.rb @@ -91,6 +91,7 @@ def decorate create_type_members(klass, "Key", "Value") else descriptor = T.unsafe(constant).descriptor + next if descriptor.nil? case descriptor when Google::Protobuf::EnumDescriptor @@ -145,7 +146,7 @@ def decorate klass.create_method("initialize", parameters: [kwargs_parameter], return_type: "void") end else - raise TypeError, "Unexpected descriptor class: #{descriptor.class.name}" + raise TypeError, "Unexpected descriptor class `#{descriptor.class.name}` for constant `#{constant}`" end end end From f88740704fda1c9a4484ae6186db173d03bd8032 Mon Sep 17 00:00:00 2001 From: Dirceu Pereira Tiegs Date: Wed, 22 Feb 2023 18:35:13 -0500 Subject: [PATCH 2/5] dsl: Use Protobuf::AbstractMessage descendants ... but not Google::Protobuf::AbstractMessage itself, as the RBI for that class is already being created by `tapioca gem`. --- lib/tapioca/dsl/compilers/protobuf.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/tapioca/dsl/compilers/protobuf.rb b/lib/tapioca/dsl/compilers/protobuf.rb index dc78af316..69445140a 100644 --- a/lib/tapioca/dsl/compilers/protobuf.rb +++ b/lib/tapioca/dsl/compilers/protobuf.rb @@ -91,7 +91,6 @@ def decorate create_type_members(klass, "Key", "Value") else descriptor = T.unsafe(constant).descriptor - next if descriptor.nil? case descriptor when Google::Protobuf::EnumDescriptor @@ -163,7 +162,13 @@ def gather_constants T.cast(desc, Google::Protobuf::EnumDescriptor).enummodule end - results = T.cast(ObjectSpace.each_object(marker).to_a, T::Array[Module]).concat(enum_modules) + abstract_message_const = ::Google::Protobuf.const_get(:AbstractMessage) + abstract_message_descendants = descendants_of(abstract_message_const) + + results = T.cast( + ObjectSpace.each_object(marker).to_a, + T::Array[Module], + ).concat(enum_modules).concat(abstract_message_descendants) - [abstract_message_const] results.any? ? results + [Google::Protobuf::RepeatedField, Google::Protobuf::Map] : [] end end From 620ce9e204587cdc5c8ec97a34b0ecaf6863cc0c Mon Sep 17 00:00:00 2001 From: Dirceu Pereira Tiegs Date: Thu, 23 Feb 2023 08:14:30 -0500 Subject: [PATCH 3/5] Fix logic for gather_constants and add test Co-authored-by: Ufuk Kayserilioglu --- lib/tapioca/dsl/compilers/protobuf.rb | 16 ++++++++++------ spec/tapioca/dsl/compilers/protobuf_spec.rb | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/lib/tapioca/dsl/compilers/protobuf.rb b/lib/tapioca/dsl/compilers/protobuf.rb index 69445140a..68936bcc6 100644 --- a/lib/tapioca/dsl/compilers/protobuf.rb +++ b/lib/tapioca/dsl/compilers/protobuf.rb @@ -162,13 +162,17 @@ def gather_constants T.cast(desc, Google::Protobuf::EnumDescriptor).enummodule end - abstract_message_const = ::Google::Protobuf.const_get(:AbstractMessage) - abstract_message_descendants = descendants_of(abstract_message_const) + results = if Google::Protobuf.const_defined?(:AbstractMessage) + abstract_message_const = ::Google::Protobuf.const_get(:AbstractMessage) + descendants_of(abstract_message_const) - [abstract_message_const] + else + T.cast( + ObjectSpace.each_object(marker).to_a, + T::Array[Module], + ) + end - results = T.cast( - ObjectSpace.each_object(marker).to_a, - T::Array[Module], - ).concat(enum_modules).concat(abstract_message_descendants) - [abstract_message_const] + results = results.concat(enum_modules) results.any? ? results + [Google::Protobuf::RepeatedField, Google::Protobuf::Map] : [] end end diff --git a/spec/tapioca/dsl/compilers/protobuf_spec.rb b/spec/tapioca/dsl/compilers/protobuf_spec.rb index 91ca06bb2..83c23e6a6 100644 --- a/spec/tapioca/dsl/compilers/protobuf_spec.rb +++ b/spec/tapioca/dsl/compilers/protobuf_spec.rb @@ -36,6 +36,24 @@ class ProtobufSpec < ::DslSpec assert_includes(gathered_constants, "Google::Protobuf::Map") assert_includes(gathered_constants, "Google::Protobuf::RepeatedField") end + + it "skips AbstractMessage" do + add_ruby_file("content.rb", <<~RUBY) + Google::Protobuf::DescriptorPool.generated_pool.build do + add_file("cart.proto", :syntax => :proto3) do + add_message "MyCart" do + optional :shop_id, :int32, 1 + optional :customer_id, :int32, 2 + end + end + end + + Cart = Google::Protobuf::DescriptorPool.generated_pool.lookup("MyCart").msgclass + RUBY + + assert_equal(["Cart"], gathered_constants.reject { |constant| constant.start_with?("Google::Protobuf") }) + refute_includes(gathered_constants, "Google::Protobuf::AbstractMessage") + end end describe "decorate" do From 7d09bf16d55e0dec83639dd5fed144fe11aa446b Mon Sep 17 00:00:00 2001 From: Dirceu Pereira Tiegs Date: Thu, 23 Feb 2023 08:44:10 -0500 Subject: [PATCH 4/5] Move the assertion to an existing test --- spec/tapioca/dsl/compilers/protobuf_spec.rb | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/spec/tapioca/dsl/compilers/protobuf_spec.rb b/spec/tapioca/dsl/compilers/protobuf_spec.rb index 83c23e6a6..87de9a36a 100644 --- a/spec/tapioca/dsl/compilers/protobuf_spec.rb +++ b/spec/tapioca/dsl/compilers/protobuf_spec.rb @@ -35,23 +35,6 @@ class ProtobufSpec < ::DslSpec assert_equal(["Cart"], gathered_constants.reject { |constant| constant.start_with?("Google::Protobuf") }) assert_includes(gathered_constants, "Google::Protobuf::Map") assert_includes(gathered_constants, "Google::Protobuf::RepeatedField") - end - - it "skips AbstractMessage" do - add_ruby_file("content.rb", <<~RUBY) - Google::Protobuf::DescriptorPool.generated_pool.build do - add_file("cart.proto", :syntax => :proto3) do - add_message "MyCart" do - optional :shop_id, :int32, 1 - optional :customer_id, :int32, 2 - end - end - end - - Cart = Google::Protobuf::DescriptorPool.generated_pool.lookup("MyCart").msgclass - RUBY - - assert_equal(["Cart"], gathered_constants.reject { |constant| constant.start_with?("Google::Protobuf") }) refute_includes(gathered_constants, "Google::Protobuf::AbstractMessage") end end From 7bdee83287836da6e3706b87c89ccfa0f5109ef6 Mon Sep 17 00:00:00 2001 From: Dirceu Pereira Tiegs Date: Thu, 23 Feb 2023 11:00:52 -0500 Subject: [PATCH 5/5] Avoid raising when dealing with an unexpected descriptor --- lib/tapioca/dsl/compilers/protobuf.rb | 4 +++- spec/tapioca/dsl/compilers/protobuf_spec.rb | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/tapioca/dsl/compilers/protobuf.rb b/lib/tapioca/dsl/compilers/protobuf.rb index 68936bcc6..457f676d5 100644 --- a/lib/tapioca/dsl/compilers/protobuf.rb +++ b/lib/tapioca/dsl/compilers/protobuf.rb @@ -145,7 +145,9 @@ def decorate klass.create_method("initialize", parameters: [kwargs_parameter], return_type: "void") end else - raise TypeError, "Unexpected descriptor class `#{descriptor.class.name}` for constant `#{constant}`" + add_error(<<~MSG.strip) + Unexpected descriptor class `#{descriptor.class.name}` for `#{constant}` + MSG end end end diff --git a/spec/tapioca/dsl/compilers/protobuf_spec.rb b/spec/tapioca/dsl/compilers/protobuf_spec.rb index 87de9a36a..0b5c56967 100644 --- a/spec/tapioca/dsl/compilers/protobuf_spec.rb +++ b/spec/tapioca/dsl/compilers/protobuf_spec.rb @@ -501,6 +501,23 @@ def clear_phone_number; end def contact_info; end RBI end + + it "shows an error for an unexpected descriptor class" do + expect_dsl_compiler_errors! + + add_ruby_file("protobuf.rb", <<~RUBY) + Cart = Class.new(::Google::Protobuf.const_get(:AbstractMessage)) + RUBY + + rbi_output = rbi_for(:Cart) + + assert_equal(<<~RBI, rbi_output) + # typed: strong + + class Cart; end + RBI + assert_equal(["Unexpected descriptor class `NilClass` for `Cart`"], generated_errors) + end end end end