Freeze all instances of Regexp, except subclass instances

* Instances of a subclass of Regexp are not frozen for compatibility.
* [Feature ruby#8948]
* Use a less confusing example in test_regexp2,
  the ivar was named @encoding but had no effect on Regexp#encoding.

Author: eregon

NEWS.md

@@ -42,6 +42,12 @@ Note: We're only listing outstanding class updates.
     * `Array#pack` accepts a new format `R` and `r` for unpacking unsigned
       and signed LEB128 encoded integers. [[Feature #21785]]
 
+* Regexp
+
+    * All instances of `Regexp` are now frozen, not just literals.
+      Subclasses of `Regexp` are not frozen for compatibility.
+      [[Feature #8948]]
+
 * Set
 
     * A deprecated behavior, `Set#to_set`, `Range#to_set`, and
@@ -140,6 +146,7 @@ A lot of work has gone into making Ractors more stable, performant, and usable.
 ## JIT
 
 [Feature #6012]: https://bugs.ruby-lang.org/issues/6012
+[Feature #8948]: https://bugs.ruby-lang.org/issues/8948
 [Feature #15330]: https://bugs.ruby-lang.org/issues/15330
 [Feature #21390]: https://bugs.ruby-lang.org/issues/21390
 [Feature #21785]: https://bugs.ruby-lang.org/issues/21785

depend

@@ -8862,6 +8862,7 @@ marshal.$(OBJEXT): $(top_srcdir)/internal/hash.h
 marshal.$(OBJEXT): $(top_srcdir)/internal/imemo.h
 marshal.$(OBJEXT): $(top_srcdir)/internal/numeric.h
 marshal.$(OBJEXT): $(top_srcdir)/internal/object.h
+marshal.$(OBJEXT): $(top_srcdir)/internal/re.h
 marshal.$(OBJEXT): $(top_srcdir)/internal/sanitizers.h
 marshal.$(OBJEXT): $(top_srcdir)/internal/serial.h
 marshal.$(OBJEXT): $(top_srcdir)/internal/set_table.h

internal/re.h

@@ -12,6 +12,7 @@
 #include "ruby/ruby.h"          /* for VALUE */
 
 /* re.c */
+VALUE rb_reg_s_alloc(VALUE klass);
 VALUE rb_reg_compile(VALUE str, int options, const char *sourcefile, int sourceline);
 VALUE rb_reg_check_preprocess(VALUE);
 long rb_reg_search0(VALUE, VALUE, long, int, int, VALUE *);

marshal.c

@@ -30,6 +30,7 @@
 #include "internal/hash.h"
 #include "internal/numeric.h"
 #include "internal/object.h"
+#include "internal/re.h"
 #include "internal/struct.h"
 #include "internal/symbol.h"
 #include "internal/util.h"
@@ -1731,7 +1732,10 @@ r_ivar_encoding(VALUE obj, struct load_arg *arg, VALUE sym, VALUE val)
     int idx = sym2encidx(sym, val);
     if (idx >= 0) {
         if (rb_enc_capable(obj)) {
-            rb_enc_associate_index(obj, idx);
+            // Check if needed to avoid rb_check_frozen() check for Regexps
+            if (rb_enc_get_index(obj) != idx) {
+                rb_enc_associate_index(obj, idx);
+            }
         }
         else {
             rb_raise(rb_eArgError, "%"PRIsVALUE" is not enc_capable", obj);
@@ -1854,17 +1858,17 @@ append_extmod(VALUE obj, VALUE extmod)
         override_ivar_error(type, str); \
     } while (0)
 
-static VALUE r_object_for(struct load_arg *arg, bool partial, int *ivp, VALUE extmod, int type);
+static VALUE r_object_for(struct load_arg *arg, bool partial, int *ivp, VALUE klass, VALUE extmod, int type);
 
 static VALUE
 r_object0(struct load_arg *arg, bool partial, int *ivp, VALUE extmod)
 {
     int type = r_byte(arg);
-    return r_object_for(arg, partial, ivp, extmod, type);
+    return r_object_for(arg, partial, ivp, 0, extmod, type);
 }
 
 static VALUE
-r_object_for(struct load_arg *arg, bool partial, int *ivp, VALUE extmod, int type)
+r_object_for(struct load_arg *arg, bool partial, int *ivp, VALUE klass, VALUE extmod, int type)
 {
     VALUE (*hash_new_with_size)(st_index_t) = rb_hash_new_with_size;
     VALUE v = Qnil;
@@ -1940,12 +1944,12 @@ r_object_for(struct load_arg *arg, bool partial, int *ivp, VALUE extmod, int typ
             }
             type = r_byte(arg);
             if ((c == rb_cHash) &&
-                /* Hack for compare_by_identify */
+                /* Hack for compare_by_identity */
                 (type == TYPE_HASH || type == TYPE_HASH_DEF)) {
                 hash_new_with_size = rb_ident_hash_new_with_size;
                 goto type_hash;
             }
-            v = r_object_for(arg, partial, 0, extmod, type);
+            v = r_object_for(arg, partial, 0, c, extmod, type);
             if (RB_SPECIAL_CONST_P(v) || RB_TYPE_P(v, T_OBJECT) || RB_TYPE_P(v, T_CLASS)) {
                 goto format_error;
             }
@@ -2082,7 +2086,10 @@ r_object_for(struct load_arg *arg, bool partial, int *ivp, VALUE extmod, int typ
                 }
                 rb_str_set_len(str, dst - ptr);
             }
-            VALUE regexp = rb_reg_new_str(str, options);
+            if (!klass) {
+                klass = rb_cRegexp;
+            }
+            VALUE regexp = rb_reg_init_str(rb_reg_s_alloc(klass), str, options);
             r_copy_ivar(regexp, str);
 
             v = r_entry0(regexp, idx, arg);

re.c

@@ -3368,6 +3368,9 @@ rb_reg_initialize(VALUE obj, const char *s, long len, rb_encoding *enc,
                           options & ARG_REG_OPTION_MASK, err,
                           sourcefile, sourceline);
     if (!re->ptr) return -1;
+    if (RBASIC_CLASS(obj) == rb_cRegexp) {
+        OBJ_FREEZE(obj);
+    }
     RB_GC_GUARD(unescaped);
     return 0;
 }
@@ -3407,7 +3410,7 @@ rb_reg_initialize_str(VALUE obj, VALUE str, int options, onig_errmsg_buffer err,
     return ret;
 }
 
-static VALUE
+VALUE
 rb_reg_s_alloc(VALUE klass)
 {
     NEWOBJ_OF(re, struct RRegexp, klass, T_REGEXP | (RGENGC_WB_PROTECTED_REGEXP ? FL_WB_PROTECTED : 0), sizeof(struct RRegexp), 0);
@@ -3460,9 +3463,7 @@ rb_reg_init_str_enc(VALUE re, VALUE s, rb_encoding *enc, int options)
 VALUE
 rb_reg_new_ary(VALUE ary, int opt)
 {
-    VALUE re = rb_reg_new_str(rb_reg_preprocess_dregexp(ary, opt), opt);
-    rb_obj_freeze(re);
-    return re;
+    return rb_reg_new_str(rb_reg_preprocess_dregexp(ary, opt), opt);
 }
 
 VALUE
@@ -3496,7 +3497,6 @@ rb_reg_compile(VALUE str, int options, const char *sourcefile, int sourceline)
         rb_set_errinfo(rb_reg_error_desc(str, options, err));
         return Qnil;
     }
-    rb_obj_freeze(re);
     return re;
 }
 
@@ -4033,6 +4033,9 @@ reg_copy(VALUE copy, VALUE orig)
     RREGEXP_PTR(copy)->timelimit = RREGEXP_PTR(orig)->timelimit;
     rb_enc_copy(copy, orig);
     FL_SET_RAW(copy, FL_TEST_RAW(orig, KCODE_FIXED|REG_ENCODING_NONE));
+    if (RBASIC_CLASS(copy) == rb_cRegexp) {
+        OBJ_FREEZE(copy);
+    }
 
     return copy;
 }
@@ -4115,6 +4118,9 @@ rb_reg_initialize_m(int argc, VALUE *argv, VALUE self)
     }
 
     set_timeout(&RREGEXP_PTR(self)->timelimit, args.timeout);
+    if (RBASIC_CLASS(self) == rb_cRegexp) {
+        OBJ_FREEZE(self);
+    }
 
     return self;
 }

spec/ruby/core/marshal/dump_spec.rb

@@ -473,14 +473,26 @@ def _dump(level)
       Marshal.dump(//im).should == "\x04\bI/\x00\x05\x06:\x06EF"
     end
 
-    it "dumps a Regexp with instance variables" do
-      o = Regexp.new("")
+    it "dumps a Regexp subclass with instance variables" do
+      o = UserRegexp.new("")
       o.instance_variable_set(:@ivar, :ivar)
-      Marshal.dump(o).should == "\x04\bI/\x00\x00\a:\x06EF:\n@ivar:\tivar"
+      Marshal.dump(o).should == "\x04\bIC:\x0FUserRegexp/\x00\x00\a:\x06EF:\n@ivar:\tivar"
     end
 
-    it "dumps an extended Regexp" do
-      Marshal.dump(Regexp.new("").extend(Meths)).should == "\x04\bIe:\nMeths/\x00\x00\x06:\x06EF"
+    it "dumps an extended Regexp subclass" do
+      Marshal.dump(UserRegexp.new("").extend(Meths)).should == "\x04\bIe:\nMethsC:\x0FUserRegexp/\x00\x00\x06:\x06EF"
+    end
+
+    ruby_version_is ""..."4.1" do
+      it "dumps a Regexp with instance variables" do
+        o = Regexp.new("")
+        o.instance_variable_set(:@ivar, :ivar)
+        Marshal.dump(o).should == "\x04\bI/\x00\x00\a:\x06EF:\n@ivar:\tivar"
+      end
+
+      it "dumps an extended Regexp" do
+        Marshal.dump(Regexp.new("").extend(Meths)).should == "\x04\bIe:\nMeths/\x00\x00\x06:\x06EF"
+      end
     end
 
     it "dumps a Regexp subclass" do

spec/ruby/core/marshal/shared/load.rb

@@ -901,14 +901,52 @@ def io.binmode; raise "binmode"; end
   end
 
   describe "for a Regexp" do
-    it "loads an extended Regexp" do
-      obj = /[a-z]/.dup.extend(Meths, MethsMore)
-      new_obj = Marshal.send(@method, "\004\be:\nMethse:\016MethsMore/\n[a-z]\000")
+    ruby_version_is "4.1" do
+      it "raises FrozenError for an extended Regexp" do
+        -> {
+          Marshal.send(@method, "\004\be:\nMethse:\016MethsMore/\n[a-z]\000")
+        }.should raise_error(FrozenError)
+      end
+
+      it "raises FrozenError when regexp has instance variables" do
+        -> {
+          Marshal.send(@method, "\x04\bI/\nhello\x00\a:\x06EF:\x11@regexp_ivar[\x06i/")
+        }.should raise_error(FrozenError)
+      end
+    end
+
+    ruby_version_is ""..."4.1" do
+      it "loads an extended Regexp" do
+        obj = /[a-z]/.dup.extend(Meths, MethsMore)
+        new_obj = Marshal.send(@method, "\004\be:\nMethse:\016MethsMore/\n[a-z]\000")
+
+        new_obj.should == obj
+        new_obj_metaclass_ancestors = class << new_obj; ancestors; end
+        new_obj_metaclass_ancestors[@num_self_class, 3].should ==
+          [Meths, MethsMore, Regexp]
+      end
+
+      it "restore the regexp instance variables" do
+        obj = Regexp.new("hello")
+        obj.instance_variable_set(:@regexp_ivar, [42])
+
+        new_obj = Marshal.send(@method, "\x04\bI/\nhello\x00\a:\x06EF:\x11@regexp_ivar[\x06i/")
+        new_obj.instance_variables.should == [:@regexp_ivar]
+        new_obj.instance_variable_get(:@regexp_ivar).should == [42]
+      end
+    end
+
+    it "loads a Regexp subclass instance variables" do
+      obj = UserRegexp.new('abc')
+      obj.instance_variable_set(:@noise, 'much')
+
+      new_obj = Marshal.send(@method, Marshal.dump(obj))
 
       new_obj.should == obj
+      new_obj.instance_variable_get(:@noise).should == 'much'
       new_obj_metaclass_ancestors = class << new_obj; ancestors; end
-      new_obj_metaclass_ancestors[@num_self_class, 3].should ==
-        [Meths, MethsMore, Regexp]
+      new_obj_metaclass_ancestors[@num_self_class, 2].should ==
+        [UserRegexp, Regexp]
     end
 
     it "loads a Regexp subclass instance variables when it is extended with a module" do
@@ -924,15 +962,6 @@ def io.binmode; raise "binmode"; end
         [Meths, UserRegexp, Regexp]
     end
 
-    it "restore the regexp instance variables" do
-      obj = Regexp.new("hello")
-      obj.instance_variable_set(:@regexp_ivar, [42])
-
-      new_obj = Marshal.send(@method, "\x04\bI/\nhello\x00\a:\x06EF:\x11@regexp_ivar[\x06i/")
-      new_obj.instance_variables.should == [:@regexp_ivar]
-      new_obj.instance_variable_get(:@regexp_ivar).should == [42]
-    end
-
     it "preserves Regexp encoding" do
       source_object = Regexp.new("a".encode("utf-32le"))
       regexp = Marshal.send(@method, Marshal.dump(source_object))

spec/ruby/core/regexp/initialize_spec.rb

@@ -9,7 +9,21 @@
     -> { //.send(:initialize, "") }.should raise_error(FrozenError)
   end
 
-  it "raises a TypeError on an initialized non-literal Regexp" do
-    -> { Regexp.new("").send(:initialize, "") }.should raise_error(TypeError)
+  ruby_version_is "4.1" do
+    it "raises a FrozenError on an initialized non-literal Regexp" do
+      regexp = Regexp.new("")
+      -> { regexp.send(:initialize, "") }.should raise_error(FrozenError)
+    end
+  end
+
+  ruby_version_is ""..."4.1" do
+    it "raises a TypeError on an initialized non-literal Regexp" do
+      -> { Regexp.new("").send(:initialize, "") }.should raise_error(TypeError)
+    end
+  end
+
+  it "raises a TypeError on an initialized non-literal Regexp subclass" do
+    r = Class.new(Regexp).new("")
+    -> { r.send(:initialize, "") }.should raise_error(TypeError)
   end
 end

spec/ruby/core/regexp/shared/new.rb

@@ -5,6 +5,12 @@
     Regexp.send(@method, '').is_a?(Regexp).should == true
   end
 
+  ruby_version_is "4.1" do
+    it "is frozen" do
+      Regexp.send(@method, '').should.frozen?
+    end
+  end
+
   it "works by default for subclasses with overridden #initialize" do
     class RegexpSpecsSubclass < Regexp
       def initialize(*args)

spec/ruby/core/string/match_spec.rb

@@ -137,9 +137,17 @@ def obj.method_missing(*args) "." end
   end
 
   it "calls match on the regular expression" do
-    regexp = /./.dup
-    regexp.should_receive(:match).and_return(:foo)
-    'hello'.match(regexp).should == :foo
+    # Can't use regexp.should_receive(:match).and_return(:foo) since regexps are frozen
+    ScratchPad.clear
+    regexp = Class.new(Regexp) {
+      def match(*args)
+        ScratchPad.record [:match, *args]
+        super(*args)
+      end
+    }.new('.')
+
+    'hello'.match(regexp)
+    ScratchPad.recorded.should == [:match, 'hello']
   end
 end