From 1376881e9afe6ff673f64afa791cf30f57147ee2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20Barri=C3=A9?= Date: Mon, 27 May 2024 11:22:39 +0200 Subject: [PATCH] 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 --- NEWS.md | 4 ++-- error.c | 5 ----- include/ruby/internal/intern/error.h | 10 ++++++++++ internal/string.h | 9 ++++++--- ractor.c | 18 ++---------------- spec/ruby/command_line/frozen_strings_spec.rb | 12 ++---------- spec/ruby/core/string/chilled_string_spec.rb | 12 +++++++----- string.c | 5 ++++- test/ruby/test_rubyoptions.rb | 3 --- test/ruby/test_string.rb | 7 +++---- yjit/bindgen/src/main.rs | 1 + yjit/src/codegen.rs | 2 +- yjit/src/cruby_bindings.inc.rs | 2 ++ 13 files changed, 40 insertions(+), 50 deletions(-) diff --git a/NEWS.md b/NEWS.md index 9ec9448d89..539fb84d92 100644 --- a/NEWS.md +++ b/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 - as if they were frozen. If they are mutated a deprecation warning is emitted. +* String literals in files without a `frozen_string_literal` comment now emit a deprecation warning + 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]] diff --git a/error.c b/error.c index 4398019b40..6d79f789d2 100644 --- a/error.c +++ b/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": ", diff --git a/include/ruby/internal/intern/error.h b/include/ruby/internal/intern/error.h index 11e147a121..c56db91b4f 100644 --- a/include/ruby/internal/intern/error.h +++ b/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); } diff --git a/internal/string.h b/internal/string.h index fb37f73114..3333b3afc3 100644 --- a/internal/string.h +++ b/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 | RUBY_FL_FREEZE); + FL_SET_RAW(str, STR_CHILLED); } static inline bool diff --git a/ractor.c b/ractor.c index 231a83db6f..d08a009d64 100644 --- a/ractor.c +++ b/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)) { - return false; - } - else if (!RB_TYPE_P(obj, T_DATA)) { + 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)) { - 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 (!frozen_shareable_p(obj, &made_shareable)) { if (made_shareable) { return traverse_skip; } diff --git a/spec/ruby/command_line/frozen_strings_spec.rb b/spec/ruby/command_line/frozen_strings_spec.rb index 334b98273b..06889755d2 100644 --- a/spec/ruby/command_line/frozen_strings_spec.rb +++ b/spec/ruby/command_line/frozen_strings_spec.rb @@ -42,16 +42,8 @@ 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 + 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 it "if file has frozen_string_literal:true comment produce same frozen strings each time" do diff --git a/spec/ruby/core/string/chilled_string_spec.rb b/spec/ruby/core/string/chilled_string_spec.rb index 8de4fc421b..9f81b1af6d 100644 --- a/spec/ruby/core/string/chilled_string_spec.rb +++ b/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 - "chilled".frozen?.should == true + it "returns false" do + "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 raise_error(FrozenError) + }.should_not complain(/literal string will be frozen in the future/) end end end diff --git a/string.c b/string.c index 9f7c163a81..d43a6391be 100644 --- a/string.c +++ b/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 { diff --git a/test/ruby/test_rubyoptions.rb b/test/ruby/test_rubyoptions.rb index 76be9152a7..d9b98be8ba 100644 --- a/test/ruby/test_rubyoptions.rb +++ b/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], diff --git a/test/ruby/test_string.rb b/test/ruby/test_string.rb index ebe85dac82..9bd86dfb78 100644 --- a/test/ruby/test_string.rb +++ b/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_predicate chilled_string, :frozen? + assert_not_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 diff --git a/yjit/bindgen/src/main.rs b/yjit/bindgen/src/main.rs index a7473c1bf6..94e821a05f 100644 --- a/yjit/bindgen/src/main.rs +++ b/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") diff --git a/yjit/src/codegen.rs b/yjit/src/codegen.rs index 747fa1769d..f426dd87ca 100644 --- a/yjit/src/codegen.rs +++ b/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"); diff --git a/yjit/src/cruby_bindings.inc.rs b/yjit/src/cruby_bindings.inc.rs index d53e630a4a..3a1de5f674 100644 --- a/yjit/src/cruby_bindings.inc.rs +++ b/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;