Adjust a FrozenError's message and add a receiver when a singleton method is defined on a frozen object

Author: andrykonchin

CHANGELOG.md

@@ -27,6 +27,7 @@ Compatibility:
 * 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/truffle/truffle/cext.rb

@@ -2375,7 +2375,16 @@ def rb_eval_cmd_kw(cmd, args, kw_splat)
   end
 
   def rb_error_frozen_object(object)
-    raise FrozenError.new("can't modify frozen #{Primitive.class(object)}", receiver: 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)

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

@@ -104,8 +104,19 @@
     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(Hash.new) }.should raise_error(FrozenError, "can't modify frozen Hash")
-      -> { @s.rb_error_frozen_object(Array.new.freeze) }.should raise_error(FrozenError, "can't modify frozen Array")
+      -> { @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
 

spec/tags/language/def_tags.txt

@@ -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/exception/CoreExceptions.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));
         }
     }