Bug #1550: String#lstrip! raises RuntimeError on Frozen String Despite Making No Changes

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 1.9.2dev (2009-05-28 trunk 23601) [i686-linux]

[runpaint (Run Paint Run Run)]

=begin
Calling String#lstrip! on a frozen string raises a RuntimeError even if the string was not changed. String#rstrip! doesn't raise an exception in this scenario. I believe that the latter behaviour is correct; #lstrip! should only raise a RuntimeError if the string would be changed.

$ rubybleed -ve '"ruby".freeze.lstrip!'
ruby 1.9.2dev (2009-05-28 trunk 23601) [i686-linux]
-e:1:in lstrip!': can't modify frozen string (RuntimeError) from -e:1:in '

$ rubybleed -ve '"ruby".freeze.rstrip!'
ruby 1.9.2dev (2009-05-28 trunk 23601) [i686-linux]

1.9.1 behaves the same as 1.9.2. 1.8.7 behaves correctly.
=end

[matz (Yukihiro Matsumoto)]

=begin
Hi,

In message "Re: [ruby-core:23657] [Bug #1550] String#lstrip! raises RuntimeError on Frozen String Despite Making No Changes"
on Mon, 1 Jun 2009 20:13:49 +0900, Run Paint Run Run [e-mail] writes:

|Calling String#lstrip! on a frozen string raises a RuntimeError even if the string was not changed. String#rstrip! doesn't raise an exception in this scenario. I believe that the latter behaviour is correct; #lstrip! should only raise a RuntimeError if the string would be changed.

I admit the inconsistency. There could be two policies:

(a) methods should raise RuntimeError only when actual changes are
made, because it should detect changes.

(b) methods should eagerly raise RuntimeError when they could
possibly make changes, to detect modify attempt to frozen
strings (that most likely caused by bugs) earlier.

Currently both policies mixed. You've suggested the former, I'd
rather prefer the latter. Any opinion?

						matz.

=end

[matz (Yukihiro Matsumoto)]

=begin
Hi,

In message "Re: [ruby-core:23663] Re: [Bug #1550] String#lstrip! raises RuntimeError on Frozen String Despite Making No Changes"
on Tue, 2 Jun 2009 07:14:11 +0900, James Gray [e-mail] writes:

|> I admit the inconsistency. There could be two policies:
|>
|> (a) methods should raise RuntimeError only when actual changes are
|> made, because it should detect changes.
|>
|> (b) methods should eagerly raise RuntimeError when they could
|> possibly make changes, to detect modify attempt to frozen
|> strings (that most likely caused by bugs) earlier.
|>
|> Currently both policies mixed. You've suggested the former, I'd
|> rather prefer the latter. Any opinion?
|
|I really feel (b) is the better choice. The fact that it worked is
|pretty irrelevant. It shouldn't have worked. Either you didn't need
|to call something like strip!() and you can remove it, or you
|sometimes do need to call it and you'll need to change the code not to
|work on frozen data. Either way, you need to change your code and the
|error shows that.

I have investigated the source and the following methods use the
policy (a):

sub/gsub
rstrip
chop
chomp

Other bang methods use the policy (b). I fixed the local copy.

						matz.

diff --git a/ChangeLog b/ChangeLog
index 4d0a55e..3d6da7b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+Tue Jun 2 07:44:43 2009 Yukihiro Matsumoto [e-mail]
+

• • string.c (rb_str_gsub_bang): modify check at the beginning.
• [ruby-core:23662] ref [ruby-core:23657]
• • string.c (rb_str_rstrip_bang): ditto. [ruby-core:23657]
• • string.c (rb_str_chop_bang): ditto.
• • string.c (rb_str_chomp_bang): ditto.

Mon Jun 1 11:21:29 2009 Nobuyoshi Nakada [e-mail]

* cont.c (cont_capture, fiber_store): reraise transferred error.

diff --git a/string.c b/string.c
index ef25fa5..74ee2a5 100644
--- a/string.c
+++ b/string.c
@@ -3744,7 +3744,6 @@ str_gsub(int argc, VALUE *argv, VALUE str, int bang)
val = rb_obj_as_string(val);
}
str_mod_check(str, sp, slen);

•  if (bang) str_frozen_check(str);
   if (val == dest) { 	/* paranoid check [ruby-dev:24827] */
   rb_raise(rb_eRuntimeError, "block should not cheat");
   }
  

@@ -3808,6 +3807,7 @@ str_gsub(int argc, VALUE *argv, VALUE str, int bang)
static VALUE
rb_str_gsub_bang(int argc, VALUE *argv, VALUE str)
{

• str_modify_keep_cr(str);
  return str_gsub(argc, argv, str, 1);
  }

@@ -6044,9 +6044,9 @@ chopped_length(VALUE str)
static VALUE
rb_str_chop_bang(VALUE str)
{

• str_modify_keep_cr(str);
  if (RSTRING_LEN(str) > 0) {
  long len;

• rb_str_modify(str);
  len = chopped_length(str);
  STR_SET_LEN(str, len);
  RSTRING_PTR(str)[len] = '\0';
  @@ -6100,6 +6100,7 @@ rb_str_chomp_bang(int argc, VALUE *argv, VALUE str)
  char *p, *pp, *e;
  long len, rslen;

• str_modify_keep_cr(str);
  len = RSTRING_LEN(str);
  if (len == 0) return Qnil;
  p = RSTRING_PTR(str);
  @@ -6108,7 +6109,6 @@ rb_str_chomp_bang(int argc, VALUE *argv, VALUE str)
  rs = rb_rs;
  if (rs == rb_default_rs) {
  smart_chomp:

•  str_modify_keep_cr(str);
   enc = rb_enc_get(str);
   if (rb_enc_mbminlen(enc) > 1) {
   pp = rb_enc_left_char_head(p, e-rb_enc_mbminlen(enc), e, enc);
  

@@ -6160,7 +6160,6 @@ rb_str_chomp_bang(int argc, VALUE *argv, VALUE str)
len--;
}
if (len < RSTRING_LEN(str)) {

•  str_modify_keep_cr(str);
   STR_SET_LEN(str, len);
   RSTRING_PTR(str)[len] = '\0';
   return str;
  

@@ -6182,7 +6181,6 @@ rb_str_chomp_bang(int argc, VALUE *argv, VALUE str)
memcmp(RSTRING_PTR(rs), pp, rslen) == 0)) {
if (rb_enc_left_char_head(p, pp, e, enc) != pp)
return Qnil;

• str_modify_keep_cr(str);
  if (ENC_CODERANGE(str) != ENC_CODERANGE_7BIT) {
  ENC_CODERANGE_CLEAR(str);
  }
  @@ -6301,6 +6299,7 @@ rb_str_rstrip_bang(VALUE str)
  rb_encoding *enc;
  char *s, *t, *e;

• str_modify_keep_cr(str);
  enc = STR_ENC_GET(str);
  rb_str_check_dummy_enc(enc);
  s = RSTRING_PTR(str);
  @@ -6324,7 +6323,6 @@ rb_str_rstrip_bang(VALUE str)
  if (t < e) {
  int len = t-RSTRING_PTR(str);

• str_modify_keep_cr(str);
  STR_SET_LEN(str, len);
  RSTRING_PTR(str)[len] = '\0';
  return str;

=end

[runpaint (Run Paint Run Run)]

=begin
On Mon, Jun 1, 2009 at 11:07 PM, Yukihiro Matsumoto [e-mail] wrote:

In message "Re: [ruby-core:23657] [Bug #1550] String#lstrip! raises RuntimeError on Frozen String Despite Making No Changes"
   on Mon, 1 Jun 2009 20:13:49 +0900, Run Paint Run Run [e-mail] writes:
[deletia]
Currently both policies mixed.  You've suggested the former, I'd
rather prefer the latter.  Any opinion?

I wasn't meaning to favour either approach; I was just under the
mistaken impression that a was the preferred way on the basis of
many RubySpec tests that specifically addressed this scenario to
prefer a. Thanks for fixing things so they are consistent. I'll
update the specs.

--
Run Paint Run Run

=end

[runpaint (Run Paint Run Run)]

=begin
On Mon, Jun 1, 2009 at 11:55 PM, Yukihiro Matsumoto [e-mail] wrote:
[deletia]

I have investigated the source and the following methods use the
policy (a):

 sub/gsub
 rstrip
 chop
 chomp

String#reverse! appears to belong on this list.

$ ruby -ve 'p "".freeze.reverse!'
ruby 1.9.2dev (2009-06-01 trunk 23614) [i686-linux]
""
Also, if you send :initialize to a frozen Array or Hash, a
RuntimeError is raised; String doesn't complain.

Moving on to Array:

$ ruby -ve '[1].freeze.concat []'
ruby 1.9.2dev (2009-06-01 trunk 23614) [i686-linux]

$ ruby -ve '[1].freeze.delete(0)'
ruby 1.9.2dev (2009-06-01 trunk 23614) [i686-linux]

$ ruby -ve '[1].freeze.flatten!'
ruby 1.9.2dev (2009-06-01 trunk 23614) [i686-linux]

$ ruby -ve '[1].freeze.insert(0)'
ruby 1.9.2dev (2009-06-01 trunk 23614) [i686-linux]

$ ruby -ve '[1,2].freeze.uniq!'
ruby 1.9.2dev (2009-06-01 trunk 23614) [i686-linux]

$ ruby -ve '[0].freeze.unshift'
ruby 1.9.2dev (2009-06-01 trunk 23614) [i686-linux]

--
Run Paint Run Run

=end

[matz (Yukihiro Matsumoto)]

• Status changed from Open to Closed
• % Done changed from 0 to 100

=begin
Applied in changeset r23620.
=end

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Wed, 3 Jun 2009 02:01:46 +0900,
Alex wrote in [ruby-core:23675]:

This change seems to break the build on my machine:

/home/alex/ruby/rbconfig.rb:184:in `gsub!': can't modify frozen string
(RuntimeError)

Could you show that rbconfig.rb?

--
Nobu Nakada

=end

[runpaint (Run Paint Run Run)]

=begin
matz,

Could you confirm that we can generalise this decision to all of the
core classes? i.e. calling a method on a frozen object that could
modify the receiver should always raise a RuntimeError. Specifically,
I'm interested in Array and Hash.

If your answer is in the affirmative, shall I start filing bug reports
for the non-compliant methods, or would you prefer that they just be
posted here?

--
Run Paint Run Run

=end

[matz (Yukihiro Matsumoto)]

=begin
Hi,

In message "Re: [ruby-core:23707] Re: [Bug #1550] String#lstrip! raises RuntimeError on Frozen String Despite Making No Changes"
on Fri, 5 Jun 2009 04:26:11 +0900, Run Paint Run Run [e-mail] writes:

|Could you confirm that we can generalise this decision to all of the
|core classes? i.e. calling a method on a frozen object that could
|modify the receiver should always raise a RuntimeError. Specifically,
|I'm interested in Array and Hash.
|
|If your answer is in the affirmative, shall I start filing bug reports
|for the non-compliant methods, or would you prefer that they just be
|posted here?

Affirmative. File the report per class.

						matz.

=end