Stop marking chilled strings as frozen

They were initially made frozen to avoid false positives for cases such
as:

    str = str.dup if str.frozen?

But this may cause bugs and is generally confusing for users.

[Feature #20205]

Co-authored-by: Jean Boussier <byroot@ruby-lang.org>

NEWS.md

@@ -7,8 +7,8 @@ Note that each entry is kept to a minimum, see links for details.
 
 ## Language changes
 
-* String literals in files without a `frozen_string_literal` comment now behave
+* String literals in files without a `frozen_string_literal` comment now emit a deprecation warning
-  as if they were frozen. If they are mutated a deprecation warning is emitted.
+  when they are mutated.
   These warnings can be enabled with `-W:deprecated` or by setting `Warning[:deprecated] = true`.
   To disable this change, you can run Ruby with the `--disable-frozen-string-literal`
   command line argument. [[Feature #20205]]

error.c

@@ -3884,11 +3884,6 @@ rb_error_frozen_object(VALUE frozen_obj)
 {
     rb_yjit_lazy_push_frame(GET_EC()->cfp->pc);
 
-    if (CHILLED_STRING_P(frozen_obj)) {
-        CHILLED_STRING_MUTATED(frozen_obj);
-        return;
-    }
-
     VALUE debug_info;
     const ID created_info = id_debug_created_info;
     VALUE mesg = rb_sprintf("can't modify frozen %"PRIsVALUE": ",

include/ruby/internal/intern/error.h

@@ -190,6 +190,7 @@ RBIMPL_ATTR_NONNULL(())
  */
 void rb_error_frozen(const char *what);
 
+RBIMPL_ATTR_NORETURN()
 /**
  * Identical  to  rb_error_frozen(),  except  it takes  arbitrary  Ruby  object
  * instead of C's string.
@@ -236,6 +237,9 @@ RBIMPL_ATTR_NORETURN()
  */
 void rb_error_arity(int argc, int min, int max);
 
+bool rb_str_chilled_p(VALUE str);
+void rb_str_modify(VALUE str);
+
 RBIMPL_SYMBOL_EXPORT_END()
 
 /**
@@ -248,12 +252,18 @@ RBIMPL_SYMBOL_EXPORT_END()
         if (RB_UNLIKELY(RB_OBJ_FROZEN(frozen_obj))) { \
             rb_error_frozen_object(frozen_obj); \
         } \
+        if (RB_UNLIKELY(rb_str_chilled_p(frozen_obj))) { \
+            rb_str_modify(frozen_obj); \
+        } \
     } while (0)
 
 /** @alias{rb_check_frozen} */
 static inline void
 rb_check_frozen_inline(VALUE obj)
 {
+    if (rb_str_chilled_p(obj)) {
+        rb_str_modify(obj);
+    }
     if (RB_UNLIKELY(RB_OBJ_FROZEN(obj))) {
         rb_error_frozen_object(obj);
     }

internal/string.h

@@ -19,6 +19,10 @@
 #define STR_SHARED       FL_USER2 /* = ELTS_SHARED */
 #define STR_CHILLED      FL_USER3
 
+enum ruby_rstring_private_flags {
+    RSTRING_CHILLED = STR_CHILLED,
+};
+
 #ifdef rb_fstring_cstr
 # undef rb_fstring_cstr
 #endif
@@ -118,15 +122,14 @@ CHILLED_STRING_P(VALUE obj)
 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");
-    FL_UNSET_RAW(str, STR_CHILLED | FL_FREEZE);
 }
 
 static inline void
 STR_CHILL_RAW(VALUE str)
 {
-    // Chilled strings are always also frozen
+    FL_SET_RAW(str, STR_CHILLED);
-    FL_SET_RAW(str, STR_CHILLED | RUBY_FL_FREEZE);
 }
 
 static inline bool

ractor.c

@@ -2984,10 +2984,7 @@ rb_obj_traverse(VALUE obj,
 static int
 frozen_shareable_p(VALUE obj, bool *made_shareable)
 {
-    if (CHILLED_STRING_P(obj)) {
+    if (!RB_TYPE_P(obj, T_DATA)) {
-        return false;
-    }
-    else if (!RB_TYPE_P(obj, T_DATA)) {
         return true;
     }
     else if (RTYPEDDATA_P(obj)) {
@@ -3016,18 +3013,7 @@ make_shareable_check_shareable(VALUE obj)
     if (rb_ractor_shareable_p(obj)) {
         return traverse_skip;
     }
-    else if (CHILLED_STRING_P(obj)) {
+    if (!frozen_shareable_p(obj, &made_shareable)) {
-        rb_funcall(obj, idFreeze, 0);
-
-        if (UNLIKELY(!RB_OBJ_FROZEN_RAW(obj))) {
-            rb_raise(rb_eRactorError, "#freeze does not freeze object correctly");
-        }
-
-        if (RB_OBJ_SHAREABLE_P(obj)) {
-            return traverse_skip;
-        }
-    }
-    else if (!frozen_shareable_p(obj, &made_shareable)) {
         if (made_shareable) {
             return traverse_skip;
         }

spec/ruby/command_line/frozen_strings_spec.rb

@@ -42,17 +42,9 @@ describe "With neither --enable-frozen-string-literal nor --disable-frozen-strin
     ruby_exe(fixture(__FILE__, "freeze_flag_one_literal.rb")).chomp.should == "false"
   end
 
-  ruby_version_is "3.4" do
-    it "if file has no frozen_string_literal comment produce different frozen strings each time" do
-      ruby_exe(fixture(__FILE__, "string_literal_raw.rb")).chomp.should == "frozen:true interned:false"
-    end
-  end
-
-  ruby_version_is ""..."3.4" do
   it "if file has no frozen_string_literal comment produce different mutable strings each time" do
     ruby_exe(fixture(__FILE__, "string_literal_raw.rb")).chomp.should == "frozen:false interned:false"
   end
-  end
 
   it "if file has frozen_string_literal:true comment produce same frozen strings each time" do
     ruby_exe(fixture(__FILE__, "string_literal_frozen_comment.rb")).chomp.should == "frozen:true interned:true"

spec/ruby/core/string/chilled_string_spec.rb

@@ -3,8 +3,8 @@ require_relative '../../spec_helper'
 describe "chilled String" do
   guard -> { ruby_version_is "3.4" and !"test".equal?("test") } do
     describe "#frozen?" do
-      it "returns true" do
+      it "returns false" do
-        "chilled".frozen?.should == true
+        "chilled".frozen?.should == false
       end
     end
 
@@ -45,7 +45,7 @@ describe "chilled String" do
         input.should == "chilled-mutated"
       end
 
-      it "emits a warning on singleton_class creaation" do
+      it "emits a warning on singleton_class creation" do
         -> {
           "chilled".singleton_class
         }.should complain(/literal string will be frozen in the future/)
@@ -57,12 +57,14 @@ describe "chilled String" do
         }.should complain(/literal string will be frozen in the future/)
       end
 
-      it "raises FrozenError after the string was explictly frozen" do
+      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

string.c

@@ -2453,6 +2453,9 @@ rb_check_lockedtmp(VALUE str)
 static inline void
 str_modifiable(VALUE str)
 {
+    if (CHILLED_STRING_P(str)) {
+        CHILLED_STRING_MUTATED(str);
+    }
     rb_check_lockedtmp(str);
     rb_check_frozen(str);
 }
@@ -3053,7 +3056,7 @@ rb_str_freeze(VALUE str)
 static VALUE
 str_uplus(VALUE str)
 {
-    if (OBJ_FROZEN(str)) {
+    if (OBJ_FROZEN(str) || CHILLED_STRING_P(str)) {
         return rb_str_dup(str);
     }
     else {

test/ruby/test_rubyoptions.rb

@@ -1208,15 +1208,12 @@ class TestRubyOptions < Test::Unit::TestCase
   end
 
   def test_frozen_string_literal_debug
-    default_frozen = eval("'test'").frozen?
-
     with_debug_pat = /created at/
     wo_debug_pat = /can\'t modify frozen String: "\w+" \(FrozenError\)\n\z/
     frozen = [
       ["--enable-frozen-string-literal", true],
       ["--disable-frozen-string-literal", false],
     ]
-    frozen << [nil, false] unless default_frozen
 
     debugs = [
       ["--debug-frozen-string-literal", true],

test/ruby/test_string.rb

@@ -3617,17 +3617,16 @@ CODE
   def test_chilled_string
     chilled_string = eval('"chilled"')
 
-    # Chilled strings pretend to be frozen
+    assert_not_predicate chilled_string, :frozen?
-    assert_predicate chilled_string, :frozen?
 
     assert_not_predicate chilled_string.dup, :frozen?
-    assert_predicate chilled_string.clone, :frozen?
+    assert_not_predicate chilled_string.clone, :frozen?
 
     # @+ treat the original string as frozen
     assert_not_predicate(+chilled_string, :frozen?)
     assert_not_same chilled_string, +chilled_string
 
-    # @- the original string as mutable
+    # @- treat the original string as mutable
     assert_predicate(-chilled_string, :frozen?)
     assert_not_same chilled_string, -chilled_string
   end

yjit/bindgen/src/main.rs

@@ -223,6 +223,7 @@ fn main() {
         .allowlist_function("rb_float_div")
 
         // From internal/string.h
+        .allowlist_type("ruby_rstring_private_flags")
         .allowlist_function("rb_ec_str_resurrect")
         .allowlist_function("rb_str_concat_literals")
         .allowlist_function("rb_obj_as_string_result")

yjit/src/codegen.rs

@@ -5611,7 +5611,7 @@ fn jit_rb_str_uplus(
     let recv_opnd = asm.stack_pop(1);
     let recv_opnd = asm.load(recv_opnd);
     let flags_opnd = asm.load(Opnd::mem(64, recv_opnd, RUBY_OFFSET_RBASIC_FLAGS));
-    asm.test(flags_opnd, Opnd::Imm(RUBY_FL_FREEZE as i64));
+    asm.test(flags_opnd, Opnd::Imm(RUBY_FL_FREEZE as i64 | RSTRING_CHILLED as i64));
 
     let ret_label = asm.new_label("stack_ret");
 

yjit/src/cruby_bindings.inc.rs

@@ -697,6 +697,8 @@ pub struct rb_call_data {
     pub ci: *const rb_callinfo,
     pub cc: *const rb_callcache,
 }
+pub const RSTRING_CHILLED: ruby_rstring_private_flags = 32768;
+pub type ruby_rstring_private_flags = u32;
 pub const RHASH_PASS_AS_KEYWORDS: ruby_rhash_flags = 8192;
 pub const RHASH_PROC_DEFAULT: ruby_rhash_flags = 16384;
 pub const RHASH_ST_TABLE_FLAG: ruby_rhash_flags = 32768;