Protobuf: Initializer, getter and setter for optional fields with message subtype should be nilable

[andrewn617]

Motivation

In protobuf, when an optional field has a message subtype it is nilable. Suppose I have some protobuf like:

Google::Protobuf::DescriptorPool.generated_pool.build do
  add_file("cart.proto", :syntax => :proto3) do
    add_message "Cart" do
      optional :customer_name, :string
      optional :notes,  :message, 2, "google.protobuf.StringValue"
    end
  end
end

Primitives like :string are not nilable. So the following are invalid:

Cart.new(customer_name: nil) # will error
Cart.new.customer_name = nil # will error
Cart.new.customer_name # returns ""

However, submessages are nilable:

cart = Cart.new(notes: nil)
cart.notes = nil
cart.notes # returns nil

Implementation

When assembling the Field struct, I check if the descriptor is an :optional and it is a :message. If it is, I wrap the type in T.nilable().

Tests

I updated the existing test to reflect this case.

[RayLLiu]

need to get ci passed. Note, this might have side effect on other components that using protobuf(a lot of rbi will be updated). I think making the not nil to nilable should be safe, but maybe worth to run some tests and give other team that use protobuf a heads up?

[andrewn617]

Sorry have been on a burst and then a vacation.

need to get ci passed.

I believe it was just an issue with the pipeline not the branch. Rerunning now.

I think making the not nil to nilable should be safe, but maybe worth to run some tests and give other team that use protobuf a heads up?

Not sure what the right move is here. The existing sigs are not accurate. But this is a move from a more strict sig to a less strict sig (from non-nilable to nilable) so I don't foresee it breaking anything. But I guess it could be an issue if people are relying on sorbet to make these non-nilable, but in that case they aren't really using protobuf properly so I am not sure what the remediation is. Maybe @paracycle has an idea?

[KaanOzkan]

Haven't verified nullability myself.

[andrewn617]

@KaanOzkan @paracycle Can we get this merged? This is causing pain for us so we are eager to get this change into a new version of the gem.

[KaanOzkan]

Applications that use this DSL generator may now see Method does not exist on NilClass component errors. If you're confident it won't be nil during runtime you can wrap the receiver with T.must to resolve them.