Fix cext support for google-protobuf

.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:
 
@@ -25,6 +26,8 @@ Compatibility:
 * Fix `rb_str_locktmp()` and `rb_str_unlocktmp()` to raise `FrozenError` when string argument is frozen (#3752, @andrykonchin).
 * Fix Struct setters to raise `FrozenError` when a struct is frozen (#3850, @andrykonchin).
 * Fix `Struct#initialize` when mixed positional and keyword arguments (#3855, @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
 

src/main/c/cext/exception.c

@@ -61,6 +61,11 @@ VALUE rb_errinfo(void) {
   return RUBY_CEXT_INVOKE("rb_errinfo");
 }
 
+void rb_error_frozen_object(VALUE frozen_obj) {
+    RUBY_CEXT_INVOKE_NO_WRAP("rb_error_frozen_object", frozen_obj);
+    UNREACHABLE;
+}
+
 void rb_syserr_fail(int eno, const char *message) {
   VALUE messageValue = (message == NULL) ? Qnil : rb_str_new_cstr(message);
   polyglot_invoke(RUBY_CEXT, "rb_syserr_fail", eno, rb_tr_unwrap(messageValue));

src/main/java/org/truffleruby/core/exception/CoreExceptions.java

@@ -304,8 +304,14 @@ public RubyException frozenError(Object object, Node currentNode) {
                 object);
     }
 
+    public RubyException frozenError(Object object, String name, Node currentNode) {
+        String string = inspectFrozenObject(object);
+        return frozenError(StringUtils.format("can't modify frozen %s: %s", name, string), currentNode,
+                object);
+    }
+
     @TruffleBoundary
-    public RubyException frozenError(String message, Node currentNode, Object receiver) {
+    private RubyException frozenError(String message, Node currentNode, Object receiver) {
         RubyClass exceptionClass = context.getCoreLibrary().frozenErrorClass;
         RubyString errorMessage = StringOperations.createUTF8String(context, language, message);
         final Backtrace backtrace = context.getCallStack().getBacktrace(currentNode);

src/main/java/org/truffleruby/core/module/ModuleFields.java

@@ -290,9 +290,9 @@ public void checkFrozen(RubyContext context, Node currentNode) {
             throw new RaiseException(
                     context,
                     context.getCoreExceptions().frozenError(
-                            StringUtils.format("can't modify frozen %s", name),
-                            currentNode,
-                            receiver));
+                            receiver,
+                            name,
+                            currentNode));
         }
     }
 

src/main/java/org/truffleruby/core/support/TypeNodes.java

@@ -161,6 +161,15 @@ Object freeze(Object self,
         }
     }
 
+    @Primitive(name = "frozen?")
+    public abstract static class IsFrozenPrimitive extends PrimitiveArrayArgumentsNode {
+        @Specialization
+        boolean isFrozen(Object self,
+                @Cached IsFrozenNode isFrozenNode) {
+            return isFrozenNode.execute(self);
+        }
+    }
+
     @Primitive(name = "immediate_value?")
     public abstract static class IsImmediateValueNode extends PrimitiveArrayArgumentsNode {
 

tool/generate-cext-trampoline.rb

@@ -13,6 +13,7 @@
 NO_RETURN_FUNCTIONS = %w[
   ruby_malloc_size_overflow
   rb_error_arity
+  rb_error_frozen_object
   rb_iter_break
   rb_iter_break_value
   rb_f_notimplement