Fix DSL generation for Google::Protobuf::Struct

[dirceu]

Motivation

protobuf version 3.23.0 introduced changes that make tapioca dsl generate broken RBI in sorbet/rbi/dsl/google/protobuf/struct.rbi:

--- a/sorbet/rbi/dsl/google/protobuf/struct.rbi
+++ b/sorbet/rbi/dsl/google/protobuf/struct.rbi
@@ -5,19 +5,15 @@
 # Please instead update this file by running `bin/tapioca dsl Google::Protobuf::Struct`.

 class Google::Protobuf::Struct
-  sig do
-    params(
-      fields: T.nilable(T.any(Google::Protobuf::Map[String, Google::Protobuf::Value], T::Hash[String, Google::Protobuf::Value]))
-    ).void
-  end
-  def initialize(fields: Google::Protobuf::Map.new(:string, :message, Google::Protobuf::Value)); end
+  sig { params(fields: T.nilable(T.any(Google::Protobuf::RepeatedField[], T::Array[]))).void }
+  def initialize(fields: Google::Protobuf::RepeatedField.new(:message, )); end

   sig { void }
   def clear_fields; end

-  sig { returns(Google::Protobuf::Map[String, Google::Protobuf::Value]) }
+  sig { returns(Google::Protobuf::RepeatedField[]) }
   def fields; end

-  sig { params(value: Google::Protobuf::Map[String, Google::Protobuf::Value]).void }
+  sig { params(value: Google::Protobuf::RepeatedField[]).void }
   def fields=(value); end
 end

This RBI change, in turn, causes typechecking errors:

sorbet/rbi/dsl/google/protobuf/struct.rbi:8: Wrong number of type parameters for Google::Protobuf::RepeatedField. Expected: 1, got: 0 https://srb.help/7010
     8 |  sig { params(fields: T.nilable(T.any(Google::Protobuf::RepeatedField[], T::Array[]))).void }
                                                                              ^^

sorbet/rbi/dsl/google/protobuf/struct.rbi:8: Wrong number of type parameters for Array. Expected: 1, got: 0 https://srb.help/7010
     8 |  sig { params(fields: T.nilable(T.any(Google::Protobuf::RepeatedField[], T::Array[]))).void }
                                                                                          ^^

sorbet/rbi/dsl/google/protobuf/struct.rbi:14: Wrong number of type parameters for Google::Protobuf::RepeatedField. Expected: 1, got: 0 https://srb.help/7010
    14 |  sig { returns(Google::Protobuf::RepeatedField[]) }
                                                       ^^

sorbet/rbi/dsl/google/protobuf/struct.rbi:17: Wrong number of type parameters for Google::Protobuf::RepeatedField. Expected: 1, got: 0 https://srb.help/7010
    17 |  sig { params(value: Google::Protobuf::RepeatedField[]).void }

When debugging, I found that type_of(descriptor) is now returning nil for Google::Protobuf::Struct.

Implementation

The initial fix (first commit) was making sure that type_of(descriptor) will always return a String value as described in its signature—"T.untyped", if descriptor.subtype.msgclass.name.nil?. That fixes all typechecking errors, but it does that generating a signature that seems wrong:

def initialize(fields: Google::Protobuf::RepeatedField.new(:message, T.untyped)); end

This is not actually valid; running Google::Protobuf::RepeatedField.new(:message, T.untyped) or Google::Protobuf::RepeatedField.new(:message, nil) or variants like that all fail.

While pairing with @bitwise-aiden, we noticed that if we changed conditions inside #field_of to get to the same branch it used to get in the previous version, the RBI for Google::Protobuf::Struct doesn't change post update. It does create some changes in other DSL files, but nothing that seems to create typechecking errors.

Tests

I'm not quite sure how to reproduce this in a test. Is a test required for this, though, given how specific the issue is and how this is the only case that could be nil?

There's now a test that confirms the expected behaviour.

Extra

The changes that started in protocolbuffers/protobuf#12319 might affect our test setup in the future, as they extract the DSL to a separate package.

[dirceu]

I've tested this change against Shopify's Core monolith and against the repository of the service that originally reported the issue, and in both cases this fixes the problem.

Some tests are failing in specific CI pipelines due to CI issues (we're accidentally using the latest version of sorbet in those, which has changes we'll need to handle).

[KaanOzkan]

Don't have enough context but the fact that the generated type is same as before gives confidence.

[KaanOzkan]

Nice find

[KaanOzkan]

Did we test that the new implementation generates correct RBIs on the old google-protobuf version?

[dirceu]

Yes! 👍