Mark strings returned by Symbol#to_s as chilled (#12065)

casperisfine · etiennebarrie · byroot · web-flow · commit 6deeec5d459e · 2024-11-13T09:20:00.000-05:00
* Use FL_USER0 for ELTS_SHARED

This makes space in RString for two bits for chilled strings.

* Mark strings returned by `Symbol#to_s` as chilled

[Feature #20350]

`STR_CHILLED` now spans on two user flags. If one bit is set it
marks a chilled string literal, if it's the other it marks a
`Symbol#to_s` chilled string.

Since it's not possible, and doesn't make much sense to include
debug info when `--debug-frozen-string-literal` is set, we can't
include allocation source, but we can safely include the symbol
name in the warning message, making it much easier to find the source
of the issue.

Co-Authored-By: Étienne Barrié &lt;etienne.barrie@gmail.com&gt;

---------

Co-authored-by: Étienne Barrié &lt;etienne.barrie@gmail.com&gt;
Co-authored-by: Jean Boussier &lt;jean.boussier@gmail.com&gt;
diff --git a/array.c b/array.c
@@ -42,12 +42,12 @@ VALUE rb_cArray_empty_frozen;
 
 /* Flags of RArray
  *
+ * 0:   RARRAY_SHARED_FLAG (equal to ELTS_SHARED)
+ *          The array is shared. The buffer this array points to is owned by
+ *          another array (the shared root).
  * 1:   RARRAY_EMBED_FLAG
  *          The array is embedded (its contents follow the header, rather than
  *          being on a separately allocated buffer).
- * 2:   RARRAY_SHARED_FLAG (equal to ELTS_SHARED)
- *          The array is shared. The buffer this array points to is owned by
- *          another array (the shared root).
  * 3-9: RARRAY_EMBED_LEN
  *          The length of the array when RARRAY_EMBED_FLAG is set.
  * 12:  RARRAY_SHARED_ROOT_FLAG
diff --git a/compile.c b/compile.c
@@ -8950,7 +8950,7 @@ compile_builtin_attr(rb_iseq_t *iseq, const NODE *node)
 
         if (!SYMBOL_P(symbol)) goto non_symbol_arg;
 
-        string = rb_sym_to_s(symbol);
+        string = rb_sym2str(symbol);
         if (strcmp(RSTRING_PTR(string), "leaf") == 0) {
             ISEQ_BODY(iseq)->builtin_attrs |= BUILTIN_ATTR_LEAF;
         }
diff --git a/error.c b/error.c
@@ -4016,7 +4016,7 @@ rb_error_frozen_object(VALUE frozen_obj)
 }
 
 void
-rb_warn_unchilled(VALUE obj)
+rb_warn_unchilled_literal(VALUE obj)
 {
     rb_warning_category_t category = RB_WARN_CATEGORY_DEPRECATED;
     if (!NIL_P(ruby_verbose) && rb_warning_category_enabled_p(category)) {
@@ -4047,6 +4047,15 @@ rb_warn_unchilled(VALUE obj)
     }
 }
 
+void
+rb_warn_unchilled_symbol_to_s(VALUE obj)
+{
+    rb_category_warn(
+        RB_WARN_CATEGORY_DEPRECATED,
+        "warning: string returned by :%s.to_s will be frozen in the future", RSTRING_PTR(obj)
+    );
+}
+
 #undef rb_check_frozen
 void
 rb_check_frozen(VALUE obj)
diff --git a/include/ruby/internal/abi.h b/include/ruby/internal/abi.h
@@ -24,7 +24,7 @@
  * In released versions of Ruby, this number is not defined since teeny
  * versions of Ruby should guarantee ABI compatibility.
  */
-#define RUBY_ABI_VERSION 0
+#define RUBY_ABI_VERSION 1
 
 /* Windows does not support weak symbols so ruby_abi_version will not exist
  * in the shared library. */
diff --git a/include/ruby/internal/core/rarray.h b/include/ruby/internal/core/rarray.h
@@ -80,6 +80,8 @@
  * here is at least incomplete.
  */
 enum ruby_rarray_flags {
+    /* RUBY_FL_USER0 is for ELTS_SHARED */
+
     /**
      * This flag  has something to do  with memory footprint.  If  the array is
      * "small"  enough, ruby  tries to  be creative  to abuse  padding bits  of
@@ -99,8 +101,6 @@ enum ruby_rarray_flags {
      */
     RARRAY_EMBED_FLAG      = RUBY_FL_USER1,
 
-    /* RUBY_FL_USER2 is for ELTS_SHARED */
-
     /**
      * When an array employs embedded strategy (see ::RARRAY_EMBED_FLAG), these
      * bits  are used  to store  the number  of elements  actually filled  into
diff --git a/include/ruby/internal/fl_type.h b/include/ruby/internal/fl_type.h
@@ -370,7 +370,7 @@ ruby_fl_type {
      * 3rd parties.  It must be an implementation detail that they should never
      * know.  Might better be hidden.
      */
-    RUBY_ELTS_SHARED  = RUBY_FL_USER2,
+    RUBY_ELTS_SHARED  = RUBY_FL_USER0,
 
     /**
      * This flag has something to do with an object's class.  There are kind of
diff --git a/include/ruby/internal/intern/error.h b/include/ruby/internal/intern/error.h
@@ -258,7 +258,7 @@ rb_check_frozen_inline(VALUE obj)
 
     /* ref: internal CHILLED_STRING_P()
        This is an implementation detail subject to change. */
-    if (RB_UNLIKELY(RB_TYPE_P(obj, T_STRING) && FL_TEST_RAW(obj, RUBY_FL_USER3))) {
+    if (RB_UNLIKELY(RB_TYPE_P(obj, T_STRING) && FL_TEST_RAW(obj, RUBY_FL_USER2 | RUBY_FL_USER3))) { // STR_CHILLED
         rb_str_modify(obj);
     }
 }
diff --git a/internal/string.h b/internal/string.h
@@ -15,9 +15,11 @@
 #include "ruby/encoding.h"      /* for rb_encoding */
 #include "ruby/ruby.h"          /* for VALUE */
 
-#define STR_NOEMBED      FL_USER1
-#define STR_SHARED       FL_USER2 /* = ELTS_SHARED */
-#define STR_CHILLED      FL_USER3
+#define STR_SHARED                  FL_USER0 /* = ELTS_SHARED */
+#define STR_NOEMBED                 FL_USER1
+#define STR_CHILLED                 (FL_USER2 | FL_USER3)
+#define STR_CHILLED_LITERAL         FL_USER2
+#define STR_CHILLED_SYMBOL_TO_S     FL_USER3
 
 enum ruby_rstring_private_flags {
     RSTRING_CHILLED = STR_CHILLED,
@@ -60,7 +62,8 @@ VALUE rb_str_upto_endless_each(VALUE, int (*each)(VALUE, VALUE), VALUE);
 VALUE rb_str_with_debug_created_info(VALUE, VALUE, int);
 
 /* error.c */
-void rb_warn_unchilled(VALUE str);
+void rb_warn_unchilled_literal(VALUE str);
+void rb_warn_unchilled_symbol_to_s(VALUE str);
 
 static inline bool STR_EMBED_P(VALUE str);
 static inline bool STR_SHARED_P(VALUE str);
@@ -127,14 +130,18 @@ CHILLED_STRING_P(VALUE obj)
 static inline void
 CHILLED_STRING_MUTATED(VALUE str)
 {
+    VALUE chilled_reason = RB_FL_TEST_RAW(str, STR_CHILLED);
     FL_UNSET_RAW(str, STR_CHILLED);
-    rb_warn_unchilled(str);
-}
-
-static inline void
-STR_CHILL_RAW(VALUE str)
-{
-    FL_SET_RAW(str, STR_CHILLED);
+    switch (chilled_reason) {
+      case STR_CHILLED_SYMBOL_TO_S:
+        rb_warn_unchilled_symbol_to_s(str);
+        break;
+      case STR_CHILLED_LITERAL:
+        rb_warn_unchilled_literal(str);
+        break;
+      default:
+        rb_bug("RString was chilled for multiple reasons");
+    }
 }
 
 static inline bool
diff --git a/lib/ruby_vm/rjit/c_type.rb b/lib/ruby_vm/rjit/c_type.rb
@@ -31,8 +31,8 @@ module Immediate
       def self.new(fiddle_type)
         name = Fiddle.constants.find do |const|
           const.start_with?('TYPE_') && Fiddle.const_get(const) == fiddle_type.abs
-        end&.to_s
-        name.delete_prefix!('TYPE_')
+        end&.name
+        name = name.delete_prefix('TYPE_')
         if fiddle_type.negative?
           name.prepend('U')
         end
diff --git a/object.c b/object.c
@@ -500,10 +500,12 @@ rb_obj_clone_setup(VALUE obj, VALUE clone, VALUE kwfreeze)
       case Qnil:
         rb_funcall(clone, id_init_clone, 1, obj);
         RBASIC(clone)->flags |= RBASIC(obj)->flags & FL_FREEZE;
-        if (CHILLED_STRING_P(obj)) {
-            STR_CHILL_RAW(clone);
+
+        if (RB_TYPE_P(obj, T_STRING)) {
+            FL_SET_RAW(clone, FL_TEST_RAW(obj, STR_CHILLED));
         }
-        else if (RB_OBJ_FROZEN(obj)) {
+
+        if (RB_OBJ_FROZEN(obj)) {
             rb_shape_t * next_shape = rb_shape_transition_shape_frozen(clone);
             if (!rb_shape_obj_too_complex(clone) && next_shape->type == SHAPE_OBJ_TOO_COMPLEX) {
                 rb_evict_ivars_to_hash(clone);
diff --git a/prism_compile.c b/prism_compile.c
@@ -3382,7 +3382,7 @@ pm_compile_builtin_attr(rb_iseq_t *iseq, const pm_scope_node_t *scope_node, cons
         }
 
         VALUE symbol = pm_static_literal_value(iseq, argument, scope_node);
-        VALUE string = rb_sym_to_s(symbol);
+        VALUE string = rb_sym2str(symbol);
 
         if (strcmp(RSTRING_PTR(string), "leaf") == 0) {
             ISEQ_BODY(iseq)->builtin_attrs |= BUILTIN_ATTR_LEAF;
diff --git a/spec/ruby/core/string/chilled_string_spec.rb b/spec/ruby/core/string/chilled_string_spec.rb
@@ -2,69 +2,141 @@
 
 describe "chilled String" do
   guard -> { ruby_version_is "3.4" and !"test".equal?("test") } do
-    describe "#frozen?" do
-      it "returns false" do
-        "chilled".frozen?.should == false
+    describe "chilled string literals" do
+
+      describe "#frozen?" do
+        it "returns false" do
+          "chilled".frozen?.should == false
+        end
       end
-    end
 
-    describe "#-@" do
-      it "returns a different instance" do
-        input = "chilled"
-        interned = (-input)
-        interned.frozen?.should == true
-        interned.object_id.should_not == input.object_id
+      describe "#-@" do
+        it "returns a different instance" do
+          input = "chilled"
+          interned = (-input)
+          interned.frozen?.should == true
+          interned.object_id.should_not == input.object_id
+        end
       end
-    end
 
-    describe "#+@" do
-      it "returns a different instance" do
-        input = "chilled"
-        duped = (+input)
-        duped.frozen?.should == false
-        duped.object_id.should_not == input.object_id
+      describe "#+@" do
+        it "returns a different instance" do
+          input = "chilled"
+          duped = (+input)
+          duped.frozen?.should == false
+          duped.object_id.should_not == input.object_id
+        end
       end
-    end
 
-    describe "#clone" do
-      it "preserves chilled status" do
-        input = "chilled".clone
-        -> {
-          input << "-mutated"
-        }.should complain(/literal string will be frozen in the future/)
-        input.should == "chilled-mutated"
+      describe "#clone" do
+        it "preserves chilled status" do
+          input = "chilled".clone
+          -> {
+            input << "-mutated"
+          }.should complain(/literal string will be frozen in the future/)
+          input.should == "chilled-mutated"
+        end
+      end
+
+      describe "mutation" do
+        it "emits a warning" do
+          input = "chilled"
+          -> {
+            input << "-mutated"
+          }.should complain(/literal string will be frozen in the future/)
+          input.should == "chilled-mutated"
+        end
+
+        it "emits a warning on singleton_class creation" do
+          -> {
+            "chilled".singleton_class
+          }.should complain(/literal string will be frozen in the future/)
+        end
+
+        it "emits a warning on instance variable assignment" do
+          -> {
+            "chilled".instance_variable_set(:@ivar, 42)
+          }.should complain(/literal string will be frozen in the future/)
+        end
+
+        it "raises FrozenError after the string was explicitly frozen" do
+          input = "chilled"
+          input.freeze
+          -> {
+            -> {
+            input << "mutated"
+            }.should raise_error(FrozenError)
+          }.should_not complain(/literal string will be frozen in the future/)
+        end
       end
     end
 
-    describe "mutation" do
-      it "emits a warning" do
-        input = "chilled"
-        -> {
-          input << "-mutated"
-        }.should complain(/literal string will be frozen in the future/)
-        input.should == "chilled-mutated"
+    describe "chilled strings returned by Symbol#to_s" do
+
+      describe "#frozen?" do
+        it "returns false" do
+          :chilled.to_s.frozen?.should == false
+        end
       end
 
-      it "emits a warning on singleton_class creation" do
-        -> {
-          "chilled".singleton_class
-        }.should complain(/literal string will be frozen in the future/)
+      describe "#-@" do
+        it "returns a different instance" do
+          input = :chilled.to_s
+          interned = (-input)
+          interned.frozen?.should == true
+          interned.object_id.should_not == input.object_id
+        end
       end
 
-      it "emits a warning on instance variable assignment" do
-        -> {
-          "chilled".instance_variable_set(:@ivar, 42)
-        }.should complain(/literal string will be frozen in the future/)
+      describe "#+@" do
+        it "returns a different instance" do
+          input = :chilled.to_s
+          duped = (+input)
+          duped.frozen?.should == false
+          duped.object_id.should_not == input.object_id
+        end
+      end
+
+      describe "#clone" do
+        it "preserves chilled status" do
+          input = :chilled.to_s.clone
+          -> {
+            input << "-mutated"
+          }.should complain(/string returned by :chilled\.to_s will be frozen in the future/)
+          input.should == "chilled-mutated"
+        end
       end
 
-      it "raises FrozenError after the string was explicitly frozen" do
-        input = "chilled"
-        input.freeze
-        -> {
+      describe "mutation" do
+        it "emits a warning" do
+          input = :chilled.to_s
+          -> {
+            input << "-mutated"
+          }.should complain(/string returned by :chilled\.to_s will be frozen in the future/)
+          input.should == "chilled-mutated"
+        end
+
+        it "emits a warning on singleton_class creation" do
+          -> {
+            :chilled.to_s.singleton_class
+          }.should complain(/string returned by :chilled\.to_s will be frozen in the future/)
+        end
+
+        it "emits a warning on instance variable assignment" do
+          -> {
+            :chilled.to_s.instance_variable_set(:@ivar, 42)
+          }.should complain(/string returned by :chilled\.to_s will be frozen in the future/)
+        end
+
+        it "raises FrozenError after the string was explicitly frozen" do
+          input = :chilled.to_s
+          input.freeze
           -> {
-          input << "mutated"
-          }.should raise_error(FrozenError)
-        }.should_not complain(/literal string will be frozen in the future/)
+            -> {
+            input << "mutated"
+            }.should raise_error(FrozenError)
+          }.should_not complain(/string returned by :chilled\.to_s will be frozen in the future/)
+        end
       end
     end
   end
diff --git a/string.c b/string.c
diff --git a/variable.c b/variable.c
diff --git a/yjit/src/cruby_bindings.inc.rs b/yjit/src/cruby_bindings.inc.rs

Original file line number	Diff line number	Diff line change
`@@ -8950,7 +8950,7 @@ compile_builtin_attr(rb_iseq_t iseq, const NODE node)`
`8950`	`8950`
`8951`	`8951`	`if (!SYMBOL_P(symbol)) goto non_symbol_arg;`
`8952`	`8952`
`8953`		`- string = rb_sym_to_s(symbol);`
	`8953`	`+ string = rb_sym2str(symbol);`
`8954`	`8954`	`if (strcmp(RSTRING_PTR(string), "leaf") == 0) {`
`8955`	`8955`	`ISEQ_BODY(iseq)->builtin_attrs \|= BUILTIN_ATTR_LEAF;`
`8956`	`8956`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4016,7 +4016,7 @@ rb_error_frozen_object(VALUE frozen_obj)`
`4016`	`4016`	`}`
`4017`	`4017`
`4018`	`4018`	`void`
`4019`		`-rb_warn_unchilled(VALUE obj)`
	`4019`	`+rb_warn_unchilled_literal(VALUE obj)`
`4020`	`4020`	`{`
`4021`	`4021`	`rb_warning_category_t category = RB_WARN_CATEGORY_DEPRECATED;`
`4022`	`4022`	`if (!NIL_P(ruby_verbose) && rb_warning_category_enabled_p(category)) {`
`@@ -4047,6 +4047,15 @@ rb_warn_unchilled(VALUE obj)`
`4047`	`4047`	`}`
`4048`	`4048`	`}`
`4049`	`4049`
	`4050`	`+void`
	`4051`	`+rb_warn_unchilled_symbol_to_s(VALUE obj)`
	`4052`	`+{`
	`4053`	`+ rb_category_warn(`
	`4054`	`+ RB_WARN_CATEGORY_DEPRECATED,`
	`4055`	`+ "warning: string returned by :%s.to_s will be frozen in the future", RSTRING_PTR(obj)`
	`4056`	`+ );`
	`4057`	`+}`
	`4058`	`+`
`4050`	`4059`	`#undef rb_check_frozen`
`4051`	`4060`	`void`
`4052`	`4061`	`rb_check_frozen(VALUE obj)`
Original file line number	Diff line number	Diff line change
`@@ -258,7 +258,7 @@ rb_check_frozen_inline(VALUE obj)`
`258`	`258`
`259`	`259`	`/* ref: internal CHILLED_STRING_P()`
`260`	`260`	`This is an implementation detail subject to change. */`
`261`		`- if (RB_UNLIKELY(RB_TYPE_P(obj, T_STRING) && FL_TEST_RAW(obj, RUBY_FL_USER3))) {`
	`261`	`+ if (RB_UNLIKELY(RB_TYPE_P(obj, T_STRING) && FL_TEST_RAW(obj, RUBY_FL_USER2 \| RUBY_FL_USER3))) { // STR_CHILLED`
`262`	`262`	`rb_str_modify(obj);`
`263`	`263`	`}`
`264`	`264`	`}`