Breaking Change: freeze is now recursive, affecting all sub-messages, maps, and repeated fields.

haberman · copybara-github · commit 31313b16522b · 2024-01-04T12:54:10.000-08:00
PiperOrigin-RevId: 595779782
diff --git a/ruby/ext/google/protobuf_c/map.c b/ruby/ext/google/protobuf_c/map.c
@@ -563,30 +563,21 @@ VALUE Map_eq(VALUE _self, VALUE _other) {
  * Freezes the message object. We have to intercept this so we can pin the
  * Ruby object into memory so we don't forget it's frozen.
  */
-static VALUE Map_freeze(VALUE _self) {
+VALUE Map_freeze(VALUE _self) {
   Map* self = ruby_to_Map(_self);
-  if (!RB_OBJ_FROZEN(_self)) {
-    Arena_Pin(self->arena, _self);
-    RB_OBJ_FREEZE(_self);
-  }
-  return _self;
-}
 
-/*
- * Deep freezes the map and values recursively.
- * Internal use only.
- */
-VALUE Map_internal_deep_freeze(VALUE _self) {
-  Map* self = ruby_to_Map(_self);
-  Map_freeze(_self);
+  if (RB_OBJ_FROZEN(_self)) return _self;
+  Arena_Pin(self->arena, _self);
+  RB_OBJ_FREEZE(_self);
+
   if (self->value_type_info.type == kUpb_CType_Message) {
     size_t iter = kUpb_Map_Begin;
     upb_MessageValue key, val;
 
     while (upb_Map_Next(self->map, &key, &val, &iter)) {
       VALUE val_val =
           Convert_UpbToRuby(val, self->value_type_info, self->arena);
-      Message_internal_deep_freeze(val_val);
+      Message_freeze(val_val);
     }
   }
   return _self;
diff --git a/ruby/ext/google/protobuf_c/map.h b/ruby/ext/google/protobuf_c/map.h
@@ -39,6 +39,6 @@ extern VALUE cMap;
 void Map_register(VALUE module);
 
 // Recursively freeze map
-VALUE Map_internal_deep_freeze(VALUE _self);
+VALUE Map_freeze(VALUE _self);
 
 #endif  // RUBY_PROTOBUF_MAP_H_
diff --git a/ruby/ext/google/protobuf_c/message.c b/ruby/ext/google/protobuf_c/message.c
@@ -826,22 +826,12 @@ static VALUE Message_to_h(VALUE _self) {
  * Freezes the message object. We have to intercept this so we can pin the
  * Ruby object into memory so we don't forget it's frozen.
  */
-static VALUE Message_freeze(VALUE _self) {
+VALUE Message_freeze(VALUE _self) {
   Message* self = ruby_to_Message(_self);
-  if (!RB_OBJ_FROZEN(_self)) {
-    Arena_Pin(self->arena, _self);
-    RB_OBJ_FREEZE(_self);
-  }
-  return _self;
-}
 
-/*
- * Deep freezes the message object recursively.
- * Internal use only.
- */
-VALUE Message_internal_deep_freeze(VALUE _self) {
-  Message* self = ruby_to_Message(_self);
-  Message_freeze(_self);
+  if (RB_OBJ_FROZEN(_self)) return _self;
+  Arena_Pin(self->arena, _self);
+  RB_OBJ_FREEZE(_self);
 
   int n = upb_MessageDef_FieldCount(self->msgdef);
   for (int i = 0; i < n; i++) {
@@ -850,11 +840,11 @@ VALUE Message_internal_deep_freeze(VALUE _self) {
 
     if (field != Qnil) {
       if (upb_FieldDef_IsMap(f)) {
-        Map_internal_deep_freeze(field);
+        Map_freeze(field);
       } else if (upb_FieldDef_IsRepeated(f)) {
-        RepeatedField_internal_deep_freeze(field);
+        RepeatedField_freeze(field);
       } else if (upb_FieldDef_IsSubMessage(f)) {
-        Message_internal_deep_freeze(field);
+        Message_freeze(field);
       }
     }
   }
@@ -963,7 +953,7 @@ VALUE Message_decode_bytes(int size, const char* bytes, int options,
     rb_raise(cParseError, "Error occurred during parsing");
   }
   if (freeze) {
-    Message_internal_deep_freeze(msg_rb);
+    Message_freeze(msg_rb);
   }
   return msg_rb;
 }
diff --git a/ruby/ext/google/protobuf_c/message.h b/ruby/ext/google/protobuf_c/message.h
@@ -78,7 +78,7 @@ VALUE Message_decode_bytes(int size, const char* bytes, int options,
                            VALUE klass, bool freeze);
 
 // Recursively freeze message
-VALUE Message_internal_deep_freeze(VALUE _self);
+VALUE Message_freeze(VALUE _self);
 
 // Call at startup to register all types in this module.
 void Message_register(VALUE protobuf);
diff --git a/ruby/ext/google/protobuf_c/repeated_field.c b/ruby/ext/google/protobuf_c/repeated_field.c
@@ -478,29 +478,20 @@ VALUE RepeatedField_eq(VALUE _self, VALUE _other) {
  * Freezes the repeated field. We have to intercept this so we can pin the Ruby
  * object into memory so we don't forget it's frozen.
  */
-static VALUE RepeatedField_freeze(VALUE _self) {
+VALUE RepeatedField_freeze(VALUE _self) {
   RepeatedField* self = ruby_to_RepeatedField(_self);
-  if (!RB_OBJ_FROZEN(_self)) {
-    Arena_Pin(self->arena, _self);
-    RB_OBJ_FREEZE(_self);
-  }
-  return _self;
-}
 
-/*
- * Deep freezes the repeated field and values recursively.
- * Internal use only.
- */
-VALUE RepeatedField_internal_deep_freeze(VALUE _self) {
-  RepeatedField* self = ruby_to_RepeatedField(_self);
-  RepeatedField_freeze(_self);
+  if (RB_OBJ_FROZEN(_self)) return _self;
+  Arena_Pin(self->arena, _self);
+  RB_OBJ_FREEZE(_self);
+
   if (self->type_info.type == kUpb_CType_Message) {
     int size = upb_Array_Size(self->array);
     int i;
     for (i = 0; i < size; i++) {
       upb_MessageValue msgval = upb_Array_Get(self->array, i);
       VALUE val = Convert_UpbToRuby(msgval, self->type_info, self->arena);
-      Message_internal_deep_freeze(val);
+      Message_freeze(val);
     }
   }
   return _self;
diff --git a/ruby/ext/google/protobuf_c/repeated_field.h b/ruby/ext/google/protobuf_c/repeated_field.h
@@ -36,6 +36,6 @@ extern VALUE cRepeatedField;
 void RepeatedField_register(VALUE module);
 
 // Recursively freeze RepeatedField.
-VALUE RepeatedField_internal_deep_freeze(VALUE _self);
+VALUE RepeatedField_freeze(VALUE _self);
 
 #endif  // RUBY_PROTOBUF_REPEATED_FIELD_H_
diff --git a/ruby/lib/google/protobuf/ffi/descriptor.rb b/ruby/lib/google/protobuf/ffi/descriptor.rb
@@ -100,7 +100,7 @@ def options
           size_ptr = ::FFI::MemoryPointer.new(:size_t, 1)
           temporary_arena = Google::Protobuf::FFI.create_arena
           buffer = Google::Protobuf::FFI.message_options(self, size_ptr, temporary_arena)
-          Google::Protobuf::MessageOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).send(:internal_deep_freeze)
+          Google::Protobuf::MessageOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).freeze
         end
       end
 
@@ -138,7 +138,7 @@ def self.get_message(msg, descriptor, arena)
         message = OBJECT_CACHE.get(msg.address)
         if message.nil?
           message = descriptor.msgclass.send(:private_constructor, arena, msg: msg)
-          message.send :internal_deep_freeze if frozen?
+          message.freeze if frozen?
         end
         message
       end
diff --git a/ruby/lib/google/protobuf/ffi/enum_descriptor.rb b/ruby/lib/google/protobuf/ffi/enum_descriptor.rb
@@ -84,7 +84,7 @@ def options
           size_ptr = ::FFI::MemoryPointer.new(:size_t, 1)
           temporary_arena = Google::Protobuf::FFI.create_arena
           buffer = Google::Protobuf::FFI.enum_options(self, size_ptr, temporary_arena)
-          Google::Protobuf::EnumOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).send(:internal_deep_freeze)
+          Google::Protobuf::EnumOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).freeze
         end
       end
 
diff --git a/ruby/lib/google/protobuf/ffi/field_descriptor.rb b/ruby/lib/google/protobuf/ffi/field_descriptor.rb
@@ -211,7 +211,7 @@ def options
           size_ptr = ::FFI::MemoryPointer.new(:size_t, 1)
           temporary_arena = Google::Protobuf::FFI.create_arena
           buffer = Google::Protobuf::FFI.field_options(self, size_ptr, temporary_arena)
-          Google::Protobuf::FieldOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).send(:internal_deep_freeze)
+          Google::Protobuf::FieldOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).freeze
         end
       end
 
diff --git a/ruby/lib/google/protobuf/ffi/file_descriptor.rb b/ruby/lib/google/protobuf/ffi/file_descriptor.rb
@@ -51,7 +51,7 @@ def options
           size_ptr = ::FFI::MemoryPointer.new(:size_t, 1)
           temporary_arena = Google::Protobuf::FFI.create_arena
           buffer = Google::Protobuf::FFI.file_options(@file_def, size_ptr, temporary_arena)
-          Google::Protobuf::FileOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).send(:internal_deep_freeze)
+          Google::Protobuf::FileOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).freeze
         end
       end
     end
diff --git a/ruby/lib/google/protobuf/ffi/map.rb b/ruby/lib/google/protobuf/ffi/map.rb
@@ -155,6 +155,19 @@ def length
       end
       alias size length
 
+      def freeze
+        return self if frozen?
+        super
+        @arena.pin self
+        if value_type == :message
+          internal_iterator do |iterator|
+            value_message_value = Google::Protobuf::FFI.map_value(@map_ptr, iterator)
+            convert_upb_to_ruby(value_message_value, value_type, descriptor, arena).freeze
+          end
+        end
+        self
+      end
+
       ##
       # call-seq:
       #    Map.dup => new_map
@@ -269,17 +282,6 @@ def each &block
 
       include Google::Protobuf::Internal::Convert
 
-      def internal_deep_freeze
-        freeze
-        if value_type == :message
-          internal_iterator do |iterator|
-            value_message_value = Google::Protobuf::FFI.map_value(@map_ptr, iterator)
-            convert_upb_to_ruby(value_message_value, value_type, descriptor, arena).send :internal_deep_freeze
-          end
-        end
-        self
-      end
-
       def internal_iterator
         iter = ::FFI::MemoryPointer.new(:size_t, 1)
         iter.write(:size_t, Google::Protobuf::FFI::Upb_Map_Begin)
diff --git a/ruby/lib/google/protobuf/ffi/message.rb b/ruby/lib/google/protobuf/ffi/message.rb
@@ -59,8 +59,15 @@ def self.new(initial_value = nil)
           end
 
           def freeze
+            return self if frozen?
             super
             @arena.pin self
+            self.class.descriptor.each do |field_descriptor|
+              next if field_descriptor.has_presence? && !Google::Protobuf::FFI.get_message_has(@msg, field_descriptor)
+              if field_descriptor.map? or field_descriptor.repeated? or field_descriptor.sub_message?
+                get_field(field_descriptor).freeze
+              end
+            end
             self
           end
 
@@ -302,17 +309,6 @@ def self.encode_json(message, options = {})
 
           include Google::Protobuf::Internal::Convert
 
-          def internal_deep_freeze
-            freeze
-            self.class.descriptor.each do |field_descriptor|
-              next if field_descriptor.has_presence? && !Google::Protobuf::FFI.get_message_has(@msg, field_descriptor)
-              if field_descriptor.map? or field_descriptor.repeated? or field_descriptor.sub_message?
-                get_field(field_descriptor).send :internal_deep_freeze
-              end
-            end
-            self
-          end
-
           def self.setup_accessors!
             @descriptor.each do |field_descriptor|
               field_name = field_descriptor.name
@@ -639,7 +635,7 @@ def get_repeated_field(array, field)
             repeated_field = OBJECT_CACHE.get(array.address)
             if repeated_field.nil?
               repeated_field = RepeatedField.send(:construct_for_field, field, @arena, array: array)
-              repeated_field.send :internal_deep_freeze if frozen?
+              repeated_field.freeze if frozen?
             end
             repeated_field
           end
@@ -652,7 +648,7 @@ def get_map_field(map, field)
             map_field = OBJECT_CACHE.get(map.address)
             if map_field.nil?
               map_field = Google::Protobuf::Map.send(:construct_for_field, field, @arena, map: map)
-              map_field.send :internal_deep_freeze if frozen?
+              map_field.freeze if frozen?
             end
             map_field
           end
diff --git a/ruby/lib/google/protobuf/ffi/oneof_descriptor.rb b/ruby/lib/google/protobuf/ffi/oneof_descriptor.rb
@@ -58,7 +58,7 @@ def options
           size_ptr = ::FFI::MemoryPointer.new(:size_t, 1)
           temporary_arena = Google::Protobuf::FFI.create_arena
           buffer = Google::Protobuf::FFI.oneof_options(self, size_ptr, temporary_arena)
-          Google::Protobuf::OneofOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).send(:internal_deep_freeze)
+          Google::Protobuf::OneofOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).freeze
        end
       end
 
diff --git a/ruby/lib/google/protobuf/ffi/repeated_field.rb b/ruby/lib/google/protobuf/ffi/repeated_field.rb
@@ -174,6 +174,18 @@ def length
       end
       alias size :length
 
+      def freeze
+        return self if frozen?
+        super
+        @arena.pin self
+        if type == :message
+          each do |element|
+            element.freeze
+          end
+        end
+        self
+      end
+
       def dup
         instance = self.class.allocate
         instance.send(:initialize, type, descriptor: descriptor, arena: arena)
@@ -252,16 +264,6 @@ def concat(other)
 
       attr :name, :arena, :array, :type, :descriptor
 
-      def internal_deep_freeze
-        freeze
-        if type == :message
-          each do |element|
-            element.send :internal_deep_freeze
-          end
-        end
-        self
-      end
-
       def internal_push(*elements)
         elements.each do |element|
           append_msg_val convert_ruby_to_upb(element, arena, type, descriptor)
diff --git a/ruby/src/main/java/com/google/protobuf/jruby/RubyMap.java b/ruby/src/main/java/com/google/protobuf/jruby/RubyMap.java
@@ -372,11 +372,15 @@ public RubyHash toHash(ThreadContext context) {
     return RubyHash.newHash(context.runtime, mapForHash, context.nil);
   }
 
-  protected IRubyObject deepFreeze(ThreadContext context) {
+  @JRubyMethod
+  public IRubyObject freeze(ThreadContext context) {
+    if (isFrozen()) {
+      return this;
+    }
     setFrozen(true);
     if (valueType == FieldDescriptor.Type.MESSAGE) {
       for (IRubyObject key : table.keySet()) {
-        ((RubyMessage) table.get(key)).deepFreeze(context);
+        ((RubyMessage) table.get(key)).freeze(context);
       }
     }
     return this;
diff --git a/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java b/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java
@@ -664,7 +664,7 @@ public static IRubyObject decodeBytes(
               });
     }
     if (freeze) {
-      ret.deepFreeze(context);
+      ret.freeze(context);
     }
     return ret;
   }
@@ -814,16 +814,20 @@ public IRubyObject toHash(ThreadContext context) {
     return ret;
   }
 
-  protected IRubyObject deepFreeze(ThreadContext context) {
+  @JRubyMethod
+  public IRubyObject freeze(ThreadContext context) {
+    if (isFrozen()) {
+      return this;
+    }
     setFrozen(true);
     for (FieldDescriptor fdef : descriptor.getFields()) {
       if (fdef.isMapField()) {
-        ((RubyMap) fields.get(fdef)).deepFreeze(context);
+        ((RubyMap) fields.get(fdef)).freeze(context);
       } else if (fdef.isRepeated()) {
-        this.getRepeatedField(context, fdef).deepFreeze(context);
+        this.getRepeatedField(context, fdef).freeze(context);
       } else if (fields.containsKey(fdef)) {
         if (fdef.getType() == FieldDescriptor.Type.MESSAGE) {
-          ((RubyMessage) fields.get(fdef)).deepFreeze(context);
+          ((RubyMessage) fields.get(fdef)).freeze(context);
         }
       }
     }
diff --git a/ruby/src/main/java/com/google/protobuf/jruby/RubyRepeatedField.java b/ruby/src/main/java/com/google/protobuf/jruby/RubyRepeatedField.java
@@ -358,11 +358,15 @@ public IRubyObject inspect() {
     return storage.inspect();
   }
 
-  protected IRubyObject deepFreeze(ThreadContext context) {
+  @JRubyMethod
+  public IRubyObject freeze(ThreadContext context) {
+    if (isFrozen()) {
+      return this;
+    }
     setFrozen(true);
     if (fieldType == FieldDescriptor.Type.MESSAGE) {
       for (int i = 0; i < size(); i++) {
-        ((RubyMessage) storage.eltInternal(i)).deepFreeze(context);
+        ((RubyMessage) storage.eltInternal(i)).freeze(context);
       }
     }
     return this;
diff --git a/ruby/tests/basic.rb b/ruby/tests/basic.rb
