Show where mutated chilled strings were allocated

[Feature #20205] The warning now suggests running with --debug-frozen-string-literal: ``` test.rb:3: warning: literal string will be frozen in the future (run with --debug-frozen-string-literal for more information) ``` When using --debug-frozen-string-literal, the location where the string was created is shown: ``` test.rb:3: warning: literal string will be frozen in the future test.rb:1: info: the string was created here ``` When resurrecting strings and debug mode is not enabled, the overhead is a simple FL_TEST_RAW. When mutating chilled strings and deprecation warnings are not enabled, the overhead is a simple warning category enabled check. Co-authored-by: Jean Boussier <[email protected]> Co-authored-by: Nobuyoshi Nakada <[email protected]> Co-authored-by: Jean Boussier <[email protected]>
github · Oct 21, 2024 · 257f78f · 257f78f
1 parent 75ef89c
commit 257f78f
Show file tree

Hide file tree

Showing 9 changed files with 144 additions and 77 deletions.
diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb
@@ -4726,7 +4726,7 @@ def test(klass, args)
 }
 
 # Chilled string setivar trigger warning
-assert_equal 'literal string will be frozen in the future', %q{
+assert_match(/literal string will be frozen in the future/, %q{
   Warning[:deprecated] = true
   $VERBOSE = true
   $warning = "no-warning"
@@ -4754,7 +4754,7 @@ def setivar!(str)
 
   setivar!("chilled") # Emit warning
   $warning
-}
+})
 
 # arity=-2 cfuncs
 assert_equal '["", "1/2", [0, [:ok, 1]]]', %q{

diff --git a/compile.c b/compile.c
@@ -4873,10 +4873,8 @@ static_literal_value(const NODE *node, rb_iseq_t *iseq)
       case NODE_FILE:
       case NODE_STR:
         if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) {
-            VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX((int)nd_line(node)));
-            VALUE lit = rb_str_dup(get_string_value(node));
-            rb_ivar_set(lit, id_debug_created_info, rb_ary_freeze(debug_info));
-            return rb_str_freeze(lit);
+            VALUE lit = get_string_value(node);
+            return rb_str_with_debug_created_info(lit, rb_iseq_path(iseq), (int)nd_line(node));
         }
         else {
             return get_string_value(node);
@@ -10927,28 +10925,25 @@ iseq_compile_each0(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const no
         debugp_param("nd_lit", get_string_value(node));
         if (!popped) {
             VALUE lit = get_string_value(node);
-            switch (ISEQ_COMPILE_DATA(iseq)->option->frozen_string_literal) {
+            const rb_compile_option_t *option = ISEQ_COMPILE_DATA(iseq)->option;
+            if ((option->debug_frozen_string_literal || RTEST(ruby_debug)) &&
+                option->frozen_string_literal != ISEQ_FROZEN_STRING_LITERAL_DISABLED) {
+                lit = rb_str_with_debug_created_info(lit, rb_iseq_path(iseq), line);
+            }
+            switch (option->frozen_string_literal) {
               case ISEQ_FROZEN_STRING_LITERAL_UNSET:
                 ADD_INSN1(ret, node, putchilledstring, lit);
-                RB_OBJ_WRITTEN(iseq, Qundef, lit);
                 break;
               case ISEQ_FROZEN_STRING_LITERAL_DISABLED:
                 ADD_INSN1(ret, node, putstring, lit);
-                RB_OBJ_WRITTEN(iseq, Qundef, lit);
                 break;
               case ISEQ_FROZEN_STRING_LITERAL_ENABLED:
-                if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) {
-                    VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX(line));
-                    lit = rb_str_dup(lit);
-                    rb_ivar_set(lit, id_debug_created_info, rb_ary_freeze(debug_info));
-                    lit = rb_str_freeze(lit);
-                }
                 ADD_INSN1(ret, node, putobject, lit);
-                RB_OBJ_WRITTEN(iseq, Qundef, lit);
                 break;
               default:
                 rb_bug("invalid frozen_string_literal");
             }
+            RB_OBJ_WRITTEN(iseq, Qundef, lit);
         }
         break;
       }

diff --git a/error.c b/error.c
@@ -3973,29 +3973,72 @@ inspect_frozen_obj(VALUE obj, VALUE mesg, int recur)
     return mesg;
 }
 
+static VALUE
+get_created_info(VALUE obj, int *pline)
+{
+    VALUE info = rb_attr_get(obj, id_debug_created_info);
+
+    if (NIL_P(info)) return Qnil;
+
+    VALUE path = rb_ary_entry(info, 0);
+    VALUE line = rb_ary_entry(info, 1);
+    if (NIL_P(path)) return Qnil;
+    *pline = NUM2INT(line);
+    return StringValue(path);
+}
+
 void
 rb_error_frozen_object(VALUE frozen_obj)
 {
     rb_yjit_lazy_push_frame(GET_EC()->cfp->pc);
 
-    VALUE debug_info;
-    const ID created_info = id_debug_created_info;
     VALUE mesg = rb_sprintf("can't modify frozen %"PRIsVALUE": ",
                             CLASS_OF(frozen_obj));
     VALUE exc = rb_exc_new_str(rb_eFrozenError, mesg);
 
     rb_ivar_set(exc, id_recv, frozen_obj);
     rb_exec_recursive(inspect_frozen_obj, frozen_obj, mesg);
 
-    if (!NIL_P(debug_info = rb_attr_get(frozen_obj, created_info))) {
-        VALUE path = rb_ary_entry(debug_info, 0);
-        VALUE line = rb_ary_entry(debug_info, 1);
-
-        rb_str_catf(mesg, ", created at %"PRIsVALUE":%"PRIsVALUE, path, line);
+    int created_line;
+    VALUE created_path = get_created_info(frozen_obj, &created_line);
+    if (!NIL_P(created_path)) {
+        rb_str_catf(mesg, ", created at %"PRIsVALUE":%d", created_path, created_line);
     }
     rb_exc_raise(exc);
 }
 
+void
+rb_warn_unchilled(VALUE obj)
+{
+    rb_warning_category_t category = RB_WARN_CATEGORY_DEPRECATED;
+    if (!NIL_P(ruby_verbose) && rb_warning_category_enabled_p(category)) {
+        int line;
+        VALUE file = rb_source_location(&line);
+        VALUE mesg = NIL_P(file) ? rb_str_new(0, 0) : rb_str_dup(file);
+
+        if (!NIL_P(file)) {
+            if (line) rb_str_catf(mesg, ":%d", line);
+            rb_str_cat2(mesg, ": ");
+        }
+        rb_str_cat2(mesg, "warning: literal string will be frozen in the future");
+
+        VALUE str = obj;
+        if (STR_SHARED_P(str)) {
+            str = RSTRING(obj)->as.heap.aux.shared;
+        }
+        VALUE created = get_created_info(str, &line);
+        if (NIL_P(created)) {
+            rb_str_cat2(mesg, " (run with --debug-frozen-string-literal for more information)\n");
+        } else {
+            rb_str_cat2(mesg, "\n");
+            rb_str_append(mesg, created);
+            if (line) rb_str_catf(mesg, ":%d", line);
+            rb_str_cat2(mesg, ": info: the string was created here\n");
+        }
+        rb_warn_category(mesg, rb_warning_category_to_name(category));
+    }
+}
+
 #undef rb_check_frozen
 void
 rb_check_frozen(VALUE obj)

diff --git a/internal/string.h b/internal/string.h
@@ -57,6 +57,10 @@ VALUE rb_str_upto_each(VALUE, VALUE, int, int (*each)(VALUE, VALUE), VALUE);
 size_t rb_str_size_as_embedded(VALUE);
 bool rb_str_reembeddable_p(VALUE);
 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);
 
 static inline bool STR_EMBED_P(VALUE str);
 static inline bool STR_SHARED_P(VALUE str);
@@ -124,7 +128,7 @@ static inline void
 CHILLED_STRING_MUTATED(VALUE str)
 {
     FL_UNSET_RAW(str, STR_CHILLED);
-    rb_category_warn(RB_WARN_CATEGORY_DEPRECATED, "literal string will be frozen in the future");
+    rb_warn_unchilled(str);
 }
 
 static inline void

diff --git a/prism_compile.c b/prism_compile.c
@@ -319,10 +319,7 @@ parse_static_literal_string(rb_iseq_t *iseq, const pm_scope_node_t *scope_node,
 
     if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) {
         int line_number = pm_node_line_number(scope_node->parser, node);
-        VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX(line_number));
-        value = rb_str_dup(value);
-        rb_ivar_set(value, id_debug_created_info, rb_ary_freeze(debug_info));
-        rb_str_freeze(value);
+        value = rb_str_with_debug_created_info(value, rb_iseq_path(iseq), line_number);
     }
 
     return value;
@@ -726,9 +723,7 @@ static VALUE
 pm_static_literal_string(rb_iseq_t *iseq, VALUE string, int line_number)
 {
     if (ISEQ_COMPILE_DATA(iseq)->option->debug_frozen_string_literal || RTEST(ruby_debug)) {
-        VALUE debug_info = rb_ary_new_from_args(2, rb_iseq_path(iseq), INT2FIX(line_number));
-        rb_ivar_set(string, id_debug_created_info, rb_ary_freeze(debug_info));
-        return rb_str_freeze(string);
+        return rb_str_with_debug_created_info(string, rb_iseq_path(iseq), line_number);
     }
     else {
         return rb_fstring(string);

diff --git a/spec/ruby/command_line/fixtures/debug_info.rb b/spec/ruby/command_line/fixtures/debug_info.rb
@@ -1,4 +1,3 @@
-# frozen_string_literal: true
 a = 'string'
 b = a
 c = b

diff --git a/spec/ruby/command_line/frozen_strings_spec.rb b/spec/ruby/command_line/frozen_strings_spec.rb
@@ -57,9 +57,18 @@
 
 describe "The --debug flag produces" do
   it "debugging info on attempted frozen string modification" do
-    error_str = ruby_exe(fixture(__FILE__, 'debug_info.rb'), options: '--debug',  args: "2>&1")
+    error_str = ruby_exe(fixture(__FILE__, 'debug_info.rb'), options: '--enable-frozen-string-literal --debug',  args: "2>&1")
     error_str.should include("can't modify frozen String")
     error_str.should include("created at")
-    error_str.should include("command_line/fixtures/debug_info.rb:2")
+    error_str.should include("command_line/fixtures/debug_info.rb:1")
+  end
+
+  guard -> { ruby_version_is "3.4" and !"test".frozen? } do
+    it "debugging info on mutating chilled string" do
+      error_str = ruby_exe(fixture(__FILE__, 'debug_info.rb'), options: '-w --debug',  args: "2>&1")
+      error_str.should include("literal string will be frozen in the future")
+      error_str.should include("the string was created here")
+      error_str.should include("command_line/fixtures/debug_info.rb:1")
+    end
   end
 end
diff --git a/string.c b/string.c
@@ -1811,41 +1811,9 @@ ec_str_alloc_heap(struct rb_execution_context_struct *ec, VALUE klass)
 }
 
 static inline VALUE
-str_duplicate_setup(VALUE klass, VALUE str, VALUE dup)
+str_duplicate_setup_encoding(VALUE str, VALUE dup, VALUE flags)
 {
-    const VALUE flag_mask =
-        ENC_CODERANGE_MASK | ENCODING_MASK |
-        FL_FREEZE
-        ;
-    VALUE flags = FL_TEST_RAW(str, flag_mask);
     int encidx = 0;
-    if (STR_EMBED_P(str)) {
-        long len = RSTRING_LEN(str);
-
-        RUBY_ASSERT(STR_EMBED_P(dup));
-        RUBY_ASSERT(str_embed_capa(dup) >= len + 1);
-        MEMCPY(RSTRING(dup)->as.embed.ary, RSTRING(str)->as.embed.ary, char, len + 1);
-    }
-    else {
-        VALUE root = str;
-        if (FL_TEST_RAW(str, STR_SHARED)) {
-            root = RSTRING(str)->as.heap.aux.shared;
-        }
-        else if (UNLIKELY(!(flags & FL_FREEZE))) {
-            root = str = str_new_frozen(klass, str);
-            flags = FL_TEST_RAW(str, flag_mask);
-        }
-        RUBY_ASSERT(!STR_SHARED_P(root));
-        RUBY_ASSERT(RB_OBJ_FROZEN_RAW(root));
-
-        RSTRING(dup)->as.heap.ptr = RSTRING_PTR(str);
-        FL_SET(root, STR_SHARED_ROOT);
-        RB_OBJ_WRITE(dup, &RSTRING(dup)->as.heap.aux.shared, root);
-        flags |= RSTRING_NOEMBED | STR_SHARED;
-    }
-
-    STR_SET_LEN(dup, RSTRING_LEN(str));
-
     if ((flags & ENCODING_MASK) == (ENCODING_INLINE_MAX<<ENCODING_SHIFT)) {
         encidx = rb_enc_get_index(str);
         flags &= ~ENCODING_MASK;
@@ -1855,18 +1823,54 @@ str_duplicate_setup(VALUE klass, VALUE str, VALUE dup)
     return dup;
 }
 
+static const VALUE flag_mask = ENC_CODERANGE_MASK | ENCODING_MASK | FL_FREEZE;
+
 static inline VALUE
-ec_str_duplicate(struct rb_execution_context_struct *ec, VALUE klass, VALUE str)
+str_duplicate_setup_embed(VALUE klass, VALUE str, VALUE dup)
+{
+    VALUE flags = FL_TEST_RAW(str, flag_mask);
+    long len = RSTRING_LEN(str);
+
+    RUBY_ASSERT(STR_EMBED_P(dup));
+    RUBY_ASSERT(str_embed_capa(dup) >= len + 1);
+    MEMCPY(RSTRING(dup)->as.embed.ary, RSTRING(str)->as.embed.ary, char, len + 1);
+    STR_SET_LEN(dup, RSTRING_LEN(str));
+    return str_duplicate_setup_encoding(str, dup, flags);
+}
+
+static inline VALUE
+str_duplicate_setup_heap(VALUE klass, VALUE str, VALUE dup)
+{
+    VALUE flags = FL_TEST_RAW(str, flag_mask);
+    VALUE root = str;
+    if (FL_TEST_RAW(str, STR_SHARED)) {
+        root = RSTRING(str)->as.heap.aux.shared;
+    }
+    else if (UNLIKELY(!(flags & FL_FREEZE))) {
+        root = str = str_new_frozen(klass, str);
+        flags = FL_TEST_RAW(str, flag_mask);
+    }
+    RUBY_ASSERT(!STR_SHARED_P(root));
+    RUBY_ASSERT(RB_OBJ_FROZEN_RAW(root));
+
+    RSTRING(dup)->as.heap.ptr = RSTRING_PTR(str);
+    FL_SET(root, STR_SHARED_ROOT);
+    RB_OBJ_WRITE(dup, &RSTRING(dup)->as.heap.aux.shared, root);
+    flags |= RSTRING_NOEMBED | STR_SHARED;
+
+    STR_SET_LEN(dup, RSTRING_LEN(str));
+    return str_duplicate_setup_encoding(str, dup, flags);
+}
+
+static inline VALUE
+str_duplicate_setup(VALUE klass, VALUE str, VALUE dup)
 {
-    VALUE dup;
     if (STR_EMBED_P(str)) {
-        dup = ec_str_alloc_embed(ec, klass, RSTRING_LEN(str) + TERM_LEN(str));
+        return str_duplicate_setup_embed(klass, str, dup);
     }
     else {
-        dup = ec_str_alloc_heap(ec, klass);
+        return str_duplicate_setup_heap(klass, str, dup);
     }
-
-    return str_duplicate_setup(klass, str, dup);
 }
 
 static inline VALUE
@@ -1912,13 +1916,32 @@ VALUE
 rb_ec_str_resurrect(struct rb_execution_context_struct *ec, VALUE str, bool chilled)
 {
     RUBY_DTRACE_CREATE_HOOK(STRING, RSTRING_LEN(str));
-    VALUE new_str = ec_str_duplicate(ec, rb_cString, str);
+    VALUE new_str, klass = rb_cString;
+
+    if (!(chilled && RTEST(rb_ivar_defined(str, id_debug_created_info))) && STR_EMBED_P(str)) {
+        new_str = ec_str_alloc_embed(ec, klass, RSTRING_LEN(str) + TERM_LEN(str));
+        str_duplicate_setup_embed(klass, str, new_str);
+    }
+    else {
+        new_str = ec_str_alloc_heap(ec, klass);
+        str_duplicate_setup_heap(klass, str, new_str);
+    }
     if (chilled) {
         STR_CHILL_RAW(new_str);
     }
     return new_str;
 }
 
+VALUE
+rb_str_with_debug_created_info(VALUE str, VALUE path, int line)
+{
+    VALUE debug_info = rb_ary_new_from_args(2, path, INT2FIX(line));
+    if (OBJ_FROZEN_RAW(str)) str = rb_str_dup(str);
+    rb_ivar_set(str, id_debug_created_info, rb_ary_freeze(debug_info));
+    STR_CHILL_RAW(str);
+    return rb_str_freeze(str);
+}
+
 /*
  *
  *  call-seq:

diff --git a/test/ruby/test_rubyoptions.rb b/test/ruby/test_rubyoptions.rb
@@ -1262,9 +1262,8 @@ def test_frozen_string_literal_debug_chilled_strings
     code = <<~RUBY
       "foo" << "bar"
     RUBY
-    warning = ["-:1: warning: literal string will be frozen in the future"]
-    assert_in_out_err(["-W:deprecated"], code, [], warning)
-    assert_in_out_err(["-W:deprecated", "--debug-frozen-string-literal"], code, [], warning)
+    assert_in_out_err(["-W:deprecated"], code, [], ["-:1: warning: literal string will be frozen in the future (run with --debug-frozen-string-literal for more information)"])
+    assert_in_out_err(["-W:deprecated", "--debug-frozen-string-literal"], code, [], ["-:1: warning: literal string will be frozen in the future", "-:1: info: the string was created here"])
     assert_in_out_err(["-W:deprecated", "--disable-frozen-string-literal", "--debug-frozen-string-literal"], code, [], [])
     assert_in_out_err(["-W:deprecated", "--enable-frozen-string-literal", "--debug-frozen-string-literal"], code, [], ["-:1:in '<main>': can't modify frozen String: \"foo\", created at -:1 (FrozenError)"])
   end