diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 342ac6fb3562..aa8cbc554912 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 15aff6ebce48..7ef44077fcb9 100644 --- a/CHANGELOG.md +++ b/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: diff --git a/lib/cext/include/truffleruby/truffleruby-abi-version.h b/lib/cext/include/truffleruby/truffleruby-abi-version.h index 24a6960dddf0..5602c03ad4f1 100644 --- a/lib/cext/include/truffleruby/truffleruby-abi-version.h +++ b/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 diff --git a/lib/truffle/truffle/cext.rb b/lib/truffle/truffle/cext.rb index aeb84e4613f0..3d92f880e853 100644 --- a/lib/truffle/truffle/cext.rb +++ b/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}" diff --git a/spec/ruby/core/exception/frozen_error_spec.rb b/spec/ruby/core/exception/frozen_error_spec.rb index 979ec2ff98a3..e2b010f56eec 100644 --- a/spec/ruby/core/exception/frozen_error_spec.rb +++ b/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 diff --git a/spec/ruby/language/def_spec.rb b/spec/ruby/language/def_spec.rb index eb44331bb5f4..a7ec62298d23 100644 --- a/spec/ruby/language/def_spec.rb +++ b/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 diff --git a/spec/ruby/optional/capi/exception_spec.rb b/spec/ruby/optional/capi/exception_spec.rb index 5bb60608b224..5bc8e26c6241 100644 --- a/spec/ruby/optional/capi/exception_spec.rb +++ b/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) diff --git a/spec/ruby/optional/capi/ext/exception_spec.c b/spec/ruby/optional/capi/ext/exception_spec.c index 0e8347ab0d68..e0d96b55e9c4 100644 --- a/spec/ruby/optional/capi/ext/exception_spec.c +++ b/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); diff --git a/spec/ruby/optional/capi/ext/object_spec.c b/spec/ruby/optional/capi/ext/object_spec.c index eab0eb75342a..995bc38fcf8d 100644 --- a/spec/ruby/optional/capi/ext/object_spec.c +++ b/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 diff --git a/spec/ruby/optional/capi/object_spec.rb b/spec/ruby/optional/capi/object_spec.rb index 27faecbb49f5..8b4d8a9bba0e 100644 --- a/spec/ruby/optional/capi/object_spec.rb +++ b/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 diff --git a/spec/tags/language/def_tags.txt b/spec/tags/language/def_tags.txt deleted file mode 100644 index 7461260ad73a..000000000000 --- a/spec/tags/language/def_tags.txt +++ /dev/null @@ -1 +0,0 @@ -fails:A singleton method definition raises FrozenError with the correct class name diff --git a/src/main/c/cext/exception.c b/src/main/c/cext/exception.c index 0d8a07051654..6c5352cc8919 100644 --- a/src/main/c/cext/exception.c +++ b/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)); diff --git a/src/main/java/org/truffleruby/core/exception/CoreExceptions.java b/src/main/java/org/truffleruby/core/exception/CoreExceptions.java index d26a42975bd1..3bbb14935a70 100644 --- a/src/main/java/org/truffleruby/core/exception/CoreExceptions.java +++ b/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); diff --git a/src/main/java/org/truffleruby/core/module/ModuleFields.java b/src/main/java/org/truffleruby/core/module/ModuleFields.java index 198815b1af69..0f5f3d5c2567 100644 --- a/src/main/java/org/truffleruby/core/module/ModuleFields.java +++ b/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)); } } diff --git a/src/main/java/org/truffleruby/core/support/TypeNodes.java b/src/main/java/org/truffleruby/core/support/TypeNodes.java index 7f987e73168b..44653a574cd6 100644 --- a/src/main/java/org/truffleruby/core/support/TypeNodes.java +++ b/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 { diff --git a/tool/generate-cext-trampoline.rb b/tool/generate-cext-trampoline.rb index 4923a680be34..0bb7295027d4 100755 --- a/tool/generate-cext-trampoline.rb +++ b/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