[GR-19220] Fix cext support for google-protobuf (#3861)

PullRequest: truffleruby/4544

Author: andrykonchin

.github/workflows/ci.yml

@@ -242,7 +242,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        ruby: ['3.1', '3.2', '3.3', '3.4']
+        ruby: ['3.2', '3.3', '3.4']
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4

CHANGELOG.md

@@ -7,6 +7,7 @@ Bug fixes:
 
 * Fix `Range#cover?` on begin-less ranges and non-integer values (@nirvdrum, @rwstauner).
 * Fix `Time.new` with String argument and handle nanoseconds correctly (#3836, @andrykonchin).
+* Fix a possible case of infinite recursion when implementing `frozen?` in a native extension (@nirvdrum).
 
 Compatibility:
 
@@ -28,6 +29,8 @@ Compatibility:
 * Fix `Integer.sqrt` for large values (#3872, @tompng).
 * Fix `Data#inspect` when data contains a recursive attribute (#3847, @andrykonchin).
 * Fix `StringIO#{gets,readline}` when it is called with both separator and limit to truncate the separator if the limit is exceeded (#3856, @andrykonchin).
+* Implement `rb_error_frozen_object` for the google-protobuf gem (@nirvdrum).
+* Adjust a `FrozenError`'s message and add a receiver when a frozen module or class is modified (e.g. by defining or undefining an instance method or by defining a nested module (@andrykonchin).
 
 Performance:
 

lib/cext/include/truffleruby/truffleruby-abi-version.h

@@ -20,6 +20,6 @@
 // $RUBY_VERSION must be the same as TruffleRuby.LANGUAGE_VERSION.
 // $ABI_NUMBER starts at 1 and is incremented for every ABI-incompatible change.
 
-#define TRUFFLERUBY_ABI_VERSION "3.3.7.4"
+#define TRUFFLERUBY_ABI_VERSION "3.3.7.5"
 
 #endif

lib/truffle/truffle/cext.rb

@@ -477,7 +477,7 @@ def rb_obj_freeze(obj)
   end
 
   def rb_obj_frozen_p(object)
-    object.frozen?
+    Primitive.frozen?(object)
   end
 
   def rb_obj_id(object)
@@ -2374,6 +2374,19 @@ def rb_eval_cmd_kw(cmd, args, kw_splat)
     end
   end
 
+  def rb_error_frozen_object(object)
+    string = nil
+    recursion = Truffle::ThreadOperations.detect_recursion object do
+      string = object.inspect
+    end
+
+    if recursion
+      string = ' ...'
+    end
+
+    raise FrozenError.new("can't modify frozen #{Primitive.class(object)}: #{string}", receiver: object)
+  end
+
   def rb_tr_warn(message)
     location = caller_locations(1, 1)[0]
     message_with_prefix = "#{location.label}: warning: #{message}"

spec/ruby/core/exception/frozen_error_spec.rb

@@ -21,6 +21,20 @@ def o.x; end
   end
 end
 
+describe "FrozenError#message" do
+  it "includes a receiver" do
+    object = Object.new
+    object.freeze
+
+    -> {
+      def object.x; end
+    }.should raise_error(FrozenError, "can't modify frozen object: #{object.to_s}")
+
+    object = [].freeze
+    -> { object << nil }.should raise_error(FrozenError, "can't modify frozen Array: []")
+  end
+end
+
 describe "Modifying a frozen object" do
   context "#inspect is redefined and modifies the object" do
     it "returns ... instead of String representation of object" do

spec/ruby/language/def_spec.rb

@@ -97,7 +97,7 @@ def foo(a); end
         def foo; end
       end
     }.should raise_error(FrozenError) { |e|
-      e.message.should.start_with? "can't modify frozen module"
+      e.message.should == "can't modify frozen module: #{e.receiver.to_s}"
     }
 
     -> {
@@ -106,7 +106,7 @@ def foo; end
         def foo; end
       end
     }.should raise_error(FrozenError){ |e|
-      e.message.should.start_with? "can't modify frozen class"
+      e.message.should == "can't modify frozen class: #{e.receiver.to_s}"
     }
   end
 end
@@ -283,20 +283,20 @@ def obj.==(other)
   it "raises FrozenError with the correct class name" do
     obj = Object.new
     obj.freeze
-    -> { def obj.foo; end }.should raise_error(FrozenError){ |e|
-      e.message.should.start_with? "can't modify frozen object"
-    }
+    -> { def obj.foo; end }.should raise_error(FrozenError, "can't modify frozen object: #{obj.to_s}")
 
+    obj = Object.new
     c = obj.singleton_class
-    -> { def c.foo; end }.should raise_error(FrozenError){ |e|
-      e.message.should.start_with? "can't modify frozen Class"
-    }
+    c.singleton_class.freeze
+    -> { def c.foo; end }.should raise_error(FrozenError, "can't modify frozen Class: #{c.to_s}")
+
+    c = Class.new
+    c.freeze
+    -> { def c.foo; end }.should raise_error(FrozenError, "can't modify frozen Class: #{c.to_s}")
 
     m = Module.new
     m.freeze
-    -> { def m.foo; end }.should raise_error(FrozenError){ |e|
-      e.message.should.start_with? "can't modify frozen Module"
-    }
+    -> { def m.foo; end }.should raise_error(FrozenError, "can't modify frozen Module: #{m.to_s}")
   end
 end
 

spec/ruby/optional/capi/exception_spec.rb

@@ -100,6 +100,26 @@
     end
   end
 
+  describe "rb_error_frozen_object" do
+    it "raises a FrozenError regardless of the object's frozen state" do
+      # The type of the argument we supply doesn't matter. The choice here is arbitrary and we only change the type
+      # of the argument to ensure the exception messages are set correctly.
+      -> { @s.rb_error_frozen_object(Array.new) }.should raise_error(FrozenError, "can't modify frozen Array: []")
+      -> { @s.rb_error_frozen_object(Array.new.freeze) }.should raise_error(FrozenError, "can't modify frozen Array: []")
+    end
+
+    it "properly handles recursive rb_error_frozen_object calls" do
+      klass = Class.new(Object)
+      object = klass.new
+      s = @s
+      klass.define_method :inspect do
+        s.rb_error_frozen_object(object)
+      end
+
+      -> { @s.rb_error_frozen_object(object) }.should raise_error(FrozenError, "can't modify frozen #{klass}:  ...")
+    end
+  end
+
   describe "rb_syserr_new" do
     it "returns system error with default message when passed message is NULL" do
       exception = @s.rb_syserr_new(Errno::ENOENT::Errno, nil)

spec/ruby/optional/capi/ext/exception_spec.c

@@ -36,6 +36,11 @@ VALUE exception_spec_rb_set_errinfo(VALUE self, VALUE exc) {
   return Qnil;
 }
 
+VALUE exception_spec_rb_error_frozen_object(VALUE self, VALUE object) {
+    rb_error_frozen_object(object);
+    return Qnil;
+}
+
 VALUE exception_spec_rb_syserr_new(VALUE self, VALUE num, VALUE msg) {
   int n = NUM2INT(num);
   char *cstr = NULL;
@@ -66,6 +71,7 @@ void Init_exception_spec(void) {
   rb_define_method(cls, "rb_exc_new3", exception_spec_rb_exc_new3, 1);
   rb_define_method(cls, "rb_exc_raise", exception_spec_rb_exc_raise, 1);
   rb_define_method(cls, "rb_set_errinfo", exception_spec_rb_set_errinfo, 1);
+  rb_define_method(cls, "rb_error_frozen_object", exception_spec_rb_error_frozen_object, 1);
   rb_define_method(cls, "rb_syserr_new", exception_spec_rb_syserr_new, 2);
   rb_define_method(cls, "rb_syserr_new_str", exception_spec_rb_syserr_new_str, 2);
   rb_define_method(cls, "rb_make_exception", exception_spec_rb_make_exception, 1);

spec/ruby/optional/capi/ext/object_spec.c

@@ -383,6 +383,16 @@ static VALUE object_spec_custom_alloc_func_p(VALUE self, VALUE klass) {
   return allocator ? Qtrue : Qfalse;
 }
 
+static VALUE object_spec_redefine_frozen(VALUE self) {
+    // The purpose of this spec is to verify that `frozen?`
+    // and `RB_OBJ_FROZEN` do not mutually recurse infinitely.
+    if (RB_OBJ_FROZEN(self)) {
+        return Qtrue;
+    }
+
+    return Qfalse;
+}
+
 void Init_object_spec(void) {
   VALUE cls = rb_define_class("CApiObjectSpecs", rb_cObject);
   rb_define_method(cls, "FL_ABLE", object_spec_FL_ABLE, 1);
@@ -455,6 +465,9 @@ void Init_object_spec(void) {
   rb_define_method(cls, "custom_alloc_func?", object_spec_custom_alloc_func_p, 1);
   rb_define_method(cls, "not_implemented_method", rb_f_notimplement, -1);
   rb_define_method(cls, "rb_ivar_foreach", object_spec_rb_ivar_foreach, 1);
+
+  cls = rb_define_class("CApiObjectRedefinitionSpecs", rb_cObject);
+  rb_define_method(cls, "frozen?", object_spec_redefine_frozen, 0);
 }
 
 #ifdef __cplusplus

spec/ruby/optional/capi/object_spec.rb

@@ -691,6 +691,16 @@ def reach
     end
   end
 
+  describe "redefining frozen? works" do
+    it "allows an object to override frozen?" do
+      obj = CApiObjectRedefinitionSpecs.new
+
+      obj.frozen?.should == false
+      obj.freeze
+      obj.frozen?.should == true
+    end
+  end
+
   describe "rb_obj_taint" do
   end