ruby-core@ruby-lang.org archive (unofficial mirror)
 help / color / mirror / Atom feed
* [ruby-core:71785] [Ruby trunk - Bug #11759] [Open] URI breaks with frozen strings
       [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
@ 2015-12-02  0:01 ` mperham
  2015-12-02  1:18 ` [ruby-core:71786] [Ruby trunk - Bug #11759] " me
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: mperham @ 2015-12-02  0:01 UTC (permalink / raw)
  To: ruby-core

Issue #11759 has been reported by Mike Perham.

----------------------------------------
Bug #11759: URI breaks with frozen strings
https://bugs.ruby-lang.org/issues/11759

* Author: Mike Perham
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 2.3.0-preview1
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
It appears URI uses String mutation and breaks frozen string mode.

~~~
$ RUBYOPT="--enable-frozen-string-literal" bundle exec rake
/Users/mike/.rubies/ruby-2.3.0-preview1/lib/ruby/2.3.0/uri/generic.rb:1344:in `to_s': can't modify frozen String (RuntimeError)
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:257:in `normalize_uri'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:198:in `add_remote'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `block in initialize'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `reverse_each'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `initialize'
~~~



-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71786] [Ruby trunk - Bug #11759] URI breaks with frozen strings
       [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
  2015-12-02  0:01 ` [ruby-core:71785] [Ruby trunk - Bug #11759] [Open] URI breaks with frozen strings mperham
@ 2015-12-02  1:18 ` me
  2015-12-02  1:30 ` [ruby-core:71787] " me
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: me @ 2015-12-02  1:18 UTC (permalink / raw)
  To: ruby-core

Issue #11759 has been updated by David Celis.

File 0001-Do-not-mutate-strings-in-URI-to_s.patch added

I've attached a patch that will avoid string mutation in URI#to_s, though I'm not sure it's optimal to generate so many new strings in this way. I'm happy to edit as necessary

----------------------------------------
Bug #11759: URI breaks with frozen strings
https://bugs.ruby-lang.org/issues/11759#change-55194

* Author: Mike Perham
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 2.3.0-preview1
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
It appears URI uses String mutation and breaks frozen string mode.

~~~
$ RUBYOPT="--enable-frozen-string-literal" bundle exec rake
/Users/mike/.rubies/ruby-2.3.0-preview1/lib/ruby/2.3.0/uri/generic.rb:1344:in `to_s': can't modify frozen String (RuntimeError)
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:257:in `normalize_uri'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:198:in `add_remote'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `block in initialize'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `reverse_each'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `initialize'
~~~

---Files--------------------------------
0001-Do-not-mutate-strings-in-URI-to_s.patch (1.87 KB)


-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71787] [Ruby trunk - Bug #11759] URI breaks with frozen strings
       [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
  2015-12-02  0:01 ` [ruby-core:71785] [Ruby trunk - Bug #11759] [Open] URI breaks with frozen strings mperham
  2015-12-02  1:18 ` [ruby-core:71786] [Ruby trunk - Bug #11759] " me
@ 2015-12-02  1:30 ` me
  2015-12-02  1:47   ` [ruby-core:71788] " Eric Wong
  2015-12-02  3:56 ` [ruby-core:71790] " me
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 14+ messages in thread
From: me @ 2015-12-02  1:30 UTC (permalink / raw)
  To: ruby-core

Issue #11759 has been updated by David Celis.

File 0001-Do-not-mutate-strings-in-URI-to_s.patch added

I just realized that the patch contained many cosmetic updates, so I reverted them to maintain the existing style. Sorry about that.

----------------------------------------
Bug #11759: URI breaks with frozen strings
https://bugs.ruby-lang.org/issues/11759#change-55195

* Author: Mike Perham
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 2.3.0-preview1
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
It appears URI uses String mutation and breaks frozen string mode.

~~~
$ RUBYOPT="--enable-frozen-string-literal" bundle exec rake
/Users/mike/.rubies/ruby-2.3.0-preview1/lib/ruby/2.3.0/uri/generic.rb:1344:in `to_s': can't modify frozen String (RuntimeError)
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:257:in `normalize_uri'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:198:in `add_remote'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `block in initialize'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `reverse_each'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `initialize'
~~~

---Files--------------------------------
0001-Do-not-mutate-strings-in-URI-to_s.patch (1.87 KB)
0001-Do-not-mutate-strings-in-URI-to_s.patch (1.76 KB)


-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71788] Re: [Ruby trunk - Bug #11759] URI breaks with frozen strings
  2015-12-02  1:30 ` [ruby-core:71787] " me
@ 2015-12-02  1:47   ` Eric Wong
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Wong @ 2015-12-02  1:47 UTC (permalink / raw)
  To: Ruby developers

Hi David, a new problem is to_s now returns a frozen string.
This has the potential to break many existing callers of to_s.

Perhaps the following (untested) one-liner is enough?

--- a/lib/uri/generic.rb
+++ b/lib/uri/generic.rb
@@ -1339,7 +1339,7 @@ def normalize!
     # Constructs String from URI
     #
     def to_s
-      str = ''
+      str = ''.freeze.dup
       if @scheme
         str << @scheme
         str << ':'.freeze

I added the extra 'freeze' instead of just calling 'dup' to avoid
allocating the extra object when frozen string literals are not
enabled.

But yeah, problems like this is why I remain against frozen string
literals for 3.0 (but I'm alright with the opt-in magic comment).

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71790] [Ruby trunk - Bug #11759] URI breaks with frozen strings
       [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
                   ` (2 preceding siblings ...)
  2015-12-02  1:30 ` [ruby-core:71787] " me
@ 2015-12-02  3:56 ` me
  2015-12-02  4:10 ` [ruby-core:71791] " akr
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: me @ 2015-12-02  3:56 UTC (permalink / raw)
  To: ruby-core

Issue #11759 has been updated by David Celis.


Hey Eric, thanks for the alternative! I was under the impression that we should assume strings to be frozen in this mode but if that's not the case I'd be happy to update the attached patch.

----------------------------------------
Bug #11759: URI breaks with frozen strings
https://bugs.ruby-lang.org/issues/11759#change-55199

* Author: Mike Perham
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 2.3.0-preview1
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
It appears URI uses String mutation and breaks frozen string mode.

~~~
$ RUBYOPT="--enable-frozen-string-literal" bundle exec rake
/Users/mike/.rubies/ruby-2.3.0-preview1/lib/ruby/2.3.0/uri/generic.rb:1344:in `to_s': can't modify frozen String (RuntimeError)
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:257:in `normalize_uri'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:198:in `add_remote'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `block in initialize'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `reverse_each'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `initialize'
~~~

---Files--------------------------------
0001-Do-not-mutate-strings-in-URI-to_s.patch (1.76 KB)


-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71791] [Ruby trunk - Bug #11759] URI breaks with frozen strings
       [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
                   ` (3 preceding siblings ...)
  2015-12-02  3:56 ` [ruby-core:71790] " me
@ 2015-12-02  4:10 ` akr
  2015-12-02  7:11 ` [ruby-core:71797] " colin
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 14+ messages in thread
From: akr @ 2015-12-02  4:10 UTC (permalink / raw)
  To: ruby-core

Issue #11759 has been updated by Akira Tanaka.


Don't mix optimization and incompatible change.
Incompatible change should be separated to another feature ticket.

Don't assume global "mode" for frozen-string-literal: true/false.
This comment is specified for each file.
So, what library uses frozen-string-literal:true doesn't mean
applications uses frozen-string-literal:true.


----------------------------------------
Bug #11759: URI breaks with frozen strings
https://bugs.ruby-lang.org/issues/11759#change-55200

* Author: Mike Perham
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 2.3.0-preview1
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
It appears URI uses String mutation and breaks frozen string mode.

~~~
$ RUBYOPT="--enable-frozen-string-literal" bundle exec rake
/Users/mike/.rubies/ruby-2.3.0-preview1/lib/ruby/2.3.0/uri/generic.rb:1344:in `to_s': can't modify frozen String (RuntimeError)
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:257:in `normalize_uri'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:198:in `add_remote'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `block in initialize'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `reverse_each'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `initialize'
~~~

---Files--------------------------------
0001-Do-not-mutate-strings-in-URI-to_s.patch (1.76 KB)


-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71797] [Ruby trunk - Bug #11759] URI breaks with frozen strings
       [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
                   ` (4 preceding siblings ...)
  2015-12-02  4:10 ` [ruby-core:71791] " akr
@ 2015-12-02  7:11 ` colin
  2015-12-02 21:36   ` [ruby-core:71804] " Eric Wong
  2015-12-03  5:20 ` [ruby-core:71812] " colin
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 14+ messages in thread
From: colin @ 2015-12-02  7:11 UTC (permalink / raw)
  To: ruby-core

Issue #11759 has been updated by Colin Kelley.

File 11759.patch added

Isn't it sufficient to initialize the string buffer with String.new?

I've attached a patch that also includes the magic comment to indicate that this file has been converted.


----------------------------------------
Bug #11759: URI breaks with frozen strings
https://bugs.ruby-lang.org/issues/11759#change-55205

* Author: Mike Perham
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 2.3.0-preview1
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
It appears URI uses String mutation and breaks frozen string mode.

~~~
$ RUBYOPT="--enable-frozen-string-literal" bundle exec rake
/Users/mike/.rubies/ruby-2.3.0-preview1/lib/ruby/2.3.0/uri/generic.rb:1344:in `to_s': can't modify frozen String (RuntimeError)
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:257:in `normalize_uri'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:198:in `add_remote'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `block in initialize'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `reverse_each'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `initialize'
~~~

---Files--------------------------------
0001-Do-not-mutate-strings-in-URI-to_s.patch (1.76 KB)
11759.patch (610 Bytes)


-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71804] Re: [Ruby trunk - Bug #11759] URI breaks with frozen strings
  2015-12-02  7:11 ` [ruby-core:71797] " colin
@ 2015-12-02 21:36   ` Eric Wong
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Wong @ 2015-12-02 21:36 UTC (permalink / raw)
  To: Ruby developers

colin@invoca.com wrote:
> Isn't it sufficient to initialize the string buffer with String.new?

Yes, but I prefer to avoid String.new because the constant lookup
requires an inline cache lookup + storage entry in the iseq.

Here's their respective disassembly code:

''.freeze.dup
== disasm: #<ISeq:<compiled>@<compiled>>================================
0000 trace            1                                               (   1)
0002 opt_str_freeze   ""
0004 opt_send_without_block <callinfo!mid:dup, argc:0, ARGS_SIMPLE>, <callcache>
0007 leave


String.new
== disasm: #<ISeq:<compiled>@<compiled>>================================
0000 trace            1                                               (   1)
0002 getinlinecache   9, <is:0>
0005 getconstant      :String
0007 setinlinecache   <is:0>
0009 opt_send_without_block <callinfo!mid:new, argc:0, ARGS_SIMPLE>, <callcache>
0012 leave

But maybe String.new is slightly faster; but I normally
prefer smaller code unless something is called in a tight loop.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71812] [Ruby trunk - Bug #11759] URI breaks with frozen strings
       [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
                   ` (5 preceding siblings ...)
  2015-12-02  7:11 ` [ruby-core:71797] " colin
@ 2015-12-03  5:20 ` colin
  2015-12-03 23:41   ` [ruby-core:71820] " Eric Wong
  2015-12-05 16:20 ` [ruby-core:71846] " colin
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 14+ messages in thread
From: colin @ 2015-12-03  5:20 UTC (permalink / raw)
  To: ruby-core

Issue #11759 has been updated by Colin Kelley.


> Yes, but I prefer to avoid String.new because the constant lookup
> requires an inline cache lookup + storage entry in the iseq.

Compared to the surrounding code in the full method, would the extra constant lookup make a measurable difference in code size?

> But maybe String.new is slightly faster; but I normally
> prefer smaller code unless something is called in a tight loop.

If it's the same speed or faster, I would vote for `String.new` because to me it makes the intention the most clear. This seems ugly:

~~~
''.freeze.dup
~~~

because the `.freeze` is temporary for Ruby 2.1 and 2.2, right?  When would this copy of generic.rb ever be run with Ruby versions earlier than 2.3?  Assuming it will just be used for Ruby 2.3 and later, the magic comment included in the patch will implicitly freeze the string literal.  Hence this would also be sufficient (and in my opinion, nearly as clear in intention as `String.new`):

~~~
''.dup
~~~

----------------------------------------
Bug #11759: URI breaks with frozen strings
https://bugs.ruby-lang.org/issues/11759#change-55216

* Author: Mike Perham
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 2.3.0-preview1
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
It appears URI uses String mutation and breaks frozen string mode.

~~~
$ RUBYOPT="--enable-frozen-string-literal" bundle exec rake
/Users/mike/.rubies/ruby-2.3.0-preview1/lib/ruby/2.3.0/uri/generic.rb:1344:in `to_s': can't modify frozen String (RuntimeError)
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:257:in `normalize_uri'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:198:in `add_remote'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `block in initialize'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `reverse_each'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `initialize'
~~~

---Files--------------------------------
0001-Do-not-mutate-strings-in-URI-to_s.patch (1.76 KB)
11759.patch (610 Bytes)


-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71820] Re: [Ruby trunk - Bug #11759] URI breaks with frozen strings
  2015-12-03  5:20 ` [ruby-core:71812] " colin
@ 2015-12-03 23:41   ` Eric Wong
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Wong @ 2015-12-03 23:41 UTC (permalink / raw)
  To: Ruby developers

colin@invoca.com wrote:
> Compared to the surrounding code in the full method, would the extra
> constant lookup make a measurable difference in code size?

64 bytes on ruby 2.3.0dev (2015-12-03 trunk 52872) [x86_64-linux].

require 'objspace'
iseq = RubyVM::InstructionSequence
p ObjectSpace.memsize_of(iseq.compile("str = String.new"))
p ObjectSpace.memsize_of(iseq.compile("str = ''.dup"))
p ObjectSpace.memsize_of(iseq.compile("str = ''.freeze.dup"))

Outputs:
480
416
416

> because the `.freeze` is temporary for Ruby 2.1 and 2.2, right?  When
> would this copy of generic.rb ever be run with Ruby versions earlier
> than 2.3?

No, we shouldn't worry about performance with older versions of Ruby
with the stdlib.

> Assuming it will just be used for Ruby 2.3 and later, the
> magic comment included in the patch will implicitly freeze the string
> literal.  Hence this would also be sufficient (and in my opinion,
> nearly as clear in intention as `String.new`):
> 
> ~~~
> ''.dup
> ~~~

I prefer that if we go for "frozen_string_literal: true" in the comment.

I've also added this test for to_s.  However, checking more closely,
the "path" accessor also gets frozen changes because we call
set_path with literal strings.

There may be code out in the wild which relies on "path" being mutable.

--- a/test/uri/test_generic.rb
+++ b/test/uri/test_generic.rb
@@ -14,6 +14,13 @@ def uri_to_ary(uri)
     uri.class.component.collect {|c| uri.send(c)}
   end
 
+  def test_to_s
+    exp = 'http://example.com/'.freeze
+    str = URI(exp).to_s
+    assert_equal exp, str
+    refute_predicate str, :frozen?, '[ruby-core:71785] [Bug #11759]'
+  end
+
   def test_parse
     # 0
     assert_kind_of(URI::HTTP, @base_url)

...  So perhaps at least one additional change is needed:

--- a/lib/uri/generic.rb
+++ b/lib/uri/generic.rb
@@ -786,7 +786,7 @@ def check_path(v)
     # see also URI::Generic.path=
     #
     def set_path(v)
-      @path = v
+      @path = v.frozen? ? v.dup : v
     end
     protected :set_path
 

All these frozen literal changes will require going every single
method and and call site with a very fine tooth comb....

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71846] [Ruby trunk - Bug #11759] URI breaks with frozen strings
       [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
                   ` (6 preceding siblings ...)
  2015-12-03  5:20 ` [ruby-core:71812] " colin
@ 2015-12-05 16:20 ` colin
  2015-12-07  7:19 ` [ruby-core:71879] " matz
  2015-12-07 15:59 ` [ruby-core:71910] " colin
  9 siblings, 0 replies; 14+ messages in thread
From: colin @ 2015-12-05 16:20 UTC (permalink / raw)
  To: ruby-core

Issue #11759 has been updated by Colin Kelley.

File 11759-rev2.patch added

> Outputs:
> 480
> 416
> 416

That is more significant than I thought. `''.dup` wins.

> No, we shouldn't worry about performance with older versions of Ruby
> with the stdlib.

Good to have that confirmed.  Then we should definitely leave the magic comment at the top for these reasons:

a) It documents which files have been visited in the process of converting to immutable string literals.
b) It allows us to immediately remove the clutter of `.freeze` at the end of string literals

I've attached an updated patch with the above changes.

> There may be code out in the wild which relies on "path" being mutable.

I don't think we have to support that.  Callers should not expect to be able to mutate object internals retrieved by accessors.  If they try and a "can't modify frozen String" exception is raised, they can fix _their_ code to `dup` the value.

----------------------------------------
Bug #11759: URI breaks with frozen strings
https://bugs.ruby-lang.org/issues/11759#change-55252

* Author: Mike Perham
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 2.3.0-preview1
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
It appears URI uses String mutation and breaks frozen string mode.

~~~
$ RUBYOPT="--enable-frozen-string-literal" bundle exec rake
/Users/mike/.rubies/ruby-2.3.0-preview1/lib/ruby/2.3.0/uri/generic.rb:1344:in `to_s': can't modify frozen String (RuntimeError)
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:257:in `normalize_uri'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:198:in `add_remote'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `block in initialize'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `reverse_each'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `initialize'
~~~

---Files--------------------------------
0001-Do-not-mutate-strings-in-URI-to_s.patch (1.76 KB)
11759.patch (610 Bytes)
11759-rev2.patch (3.21 KB)


-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71879] [Ruby trunk - Bug #11759] URI breaks with frozen strings
       [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
                   ` (7 preceding siblings ...)
  2015-12-05 16:20 ` [ruby-core:71846] " colin
@ 2015-12-07  7:19 ` matz
  2015-12-07 15:59 ` [ruby-core:71910] " colin
  9 siblings, 0 replies; 14+ messages in thread
From: matz @ 2015-12-07  7:19 UTC (permalink / raw)
  To: ruby-core

Issue #11759 has been updated by Yukihiro Matsumoto.


I prefer `String.new()` to `"".dup` because the former describes intention (of creating a buffer).
In fact, my best choice is introducing `String#+` that returns a mutable copy of a string.

For the size of byte code and performance, we can expect future optimization.

Matz.


----------------------------------------
Bug #11759: URI breaks with frozen strings
https://bugs.ruby-lang.org/issues/11759#change-55282

* Author: Mike Perham
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 2.3.0-preview1
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
It appears URI uses String mutation and breaks frozen string mode.

~~~
$ RUBYOPT="--enable-frozen-string-literal" bundle exec rake
/Users/mike/.rubies/ruby-2.3.0-preview1/lib/ruby/2.3.0/uri/generic.rb:1344:in `to_s': can't modify frozen String (RuntimeError)
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:257:in `normalize_uri'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:198:in `add_remote'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `block in initialize'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `reverse_each'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `initialize'
~~~

---Files--------------------------------
0001-Do-not-mutate-strings-in-URI-to_s.patch (1.76 KB)
11759.patch (610 Bytes)
11759-rev2.patch (3.21 KB)


-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71910] [Ruby trunk - Bug #11759] URI breaks with frozen strings
       [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
                   ` (8 preceding siblings ...)
  2015-12-07  7:19 ` [ruby-core:71879] " matz
@ 2015-12-07 15:59 ` colin
  2015-12-08 21:35   ` [ruby-core:71957] " Eric Wong
  9 siblings, 1 reply; 14+ messages in thread
From: colin @ 2015-12-07 15:59 UTC (permalink / raw)
  To: ruby-core

Issue #11759 has been updated by Colin Kelley.

File 11759-rev3.patch added

> I prefer String.new() to "".dup because the former describes intention (of creating a buffer).

Ok. I've attached a rev3 patch that uses `String.new` instead of `''.dup`.

> In fact, my best choice is introducing String#+ that returns a mutable copy of a string.

How would that be different from the current binary string operator `String#+` that does string concatenation?

----------------------------------------
Bug #11759: URI breaks with frozen strings
https://bugs.ruby-lang.org/issues/11759#change-55316

* Author: Mike Perham
* Status: Open
* Priority: Normal
* Assignee: 
* ruby -v: 2.3.0-preview1
* Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
----------------------------------------
It appears URI uses String mutation and breaks frozen string mode.

~~~
$ RUBYOPT="--enable-frozen-string-literal" bundle exec rake
/Users/mike/.rubies/ruby-2.3.0-preview1/lib/ruby/2.3.0/uri/generic.rb:1344:in `to_s': can't modify frozen String (RuntimeError)
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:257:in `normalize_uri'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:198:in `add_remote'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `block in initialize'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `reverse_each'
/Users/mike/.gem/ruby/2.3.0/gems/bundler-1.10.6/lib/bundler/source/rubygems.rb:25:in `initialize'
~~~

---Files--------------------------------
0001-Do-not-mutate-strings-in-URI-to_s.patch (1.76 KB)
11759.patch (610 Bytes)
11759-rev2.patch (3.21 KB)
11759-rev3.patch (3.21 KB)


-- 
https://bugs.ruby-lang.org/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [ruby-core:71957] Re: [Ruby trunk - Bug #11759] URI breaks with frozen strings
  2015-12-07 15:59 ` [ruby-core:71910] " colin
@ 2015-12-08 21:35   ` Eric Wong
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Wong @ 2015-12-08 21:35 UTC (permalink / raw)
  To: ruby-core

colin@invoca.com wrote:
> > I prefer String.new() to "".dup because the former describes intention (of creating a buffer).
> 
> Ok. I've attached a rev3 patch that uses `String.new` instead of `''.dup`.

Thanks.  Committed as r52981

> > In fact, my best choice is introducing String#+ that returns a mutable copy of a string.
> 
> How would that be different from the current binary string operator `String#+` that does string concatenation?

It actually calls the "+@" method as implemented in r52917 / [Feature #11782]
But according to [ruby-core:71924], maybe it's not a good idea...

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-12-08 21:03 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <redmine.issue-11759.20151202000114@ruby-lang.org>
2015-12-02  0:01 ` [ruby-core:71785] [Ruby trunk - Bug #11759] [Open] URI breaks with frozen strings mperham
2015-12-02  1:18 ` [ruby-core:71786] [Ruby trunk - Bug #11759] " me
2015-12-02  1:30 ` [ruby-core:71787] " me
2015-12-02  1:47   ` [ruby-core:71788] " Eric Wong
2015-12-02  3:56 ` [ruby-core:71790] " me
2015-12-02  4:10 ` [ruby-core:71791] " akr
2015-12-02  7:11 ` [ruby-core:71797] " colin
2015-12-02 21:36   ` [ruby-core:71804] " Eric Wong
2015-12-03  5:20 ` [ruby-core:71812] " colin
2015-12-03 23:41   ` [ruby-core:71820] " Eric Wong
2015-12-05 16:20 ` [ruby-core:71846] " colin
2015-12-07  7:19 ` [ruby-core:71879] " matz
2015-12-07 15:59 ` [ruby-core:71910] " colin
2015-12-08 21:35   ` [ruby-core:71957] " Eric Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).