Project

General

Profile

Bug #15241

net/pop fix to use mutable strings was too eager

Added by eventualbuddha (Brian Donovan) about 1 year ago. Updated 3 months ago.

Status:
Rejected
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:89509]

Description

See [[https://github.com/ruby/ruby/commit/3993fbb5f6bfdae0bce040988d7e2dd632247cdc#commitcomment-30987842|this thread on Github]]. I believe the changes made in Backport #14416 for 2.5.2 (on both trunk and 2.5.x) over-eagerly applied mutability to literal strings. In particular, the strings returned by inspect have no real need to be mutable as far as I can tell.

History

#1

Updated by eventualbuddha (Brian Donovan) about 1 year ago

  • ruby -v changed from 2.5w to 2.5.2

Updated by normalperson (Eric Wong) about 1 year ago

 me@brian-donovan.com wrote:
 > https://bugs.ruby-lang.org/issues/15241
 > See
 > [[https://github.com/ruby/ruby/commit/3993fbb5f6bfdae0bce040988d7e2dd632247cdc#commitcomment-30987842|this
 > thread on Github]]. I believe the changes made in Backport
 > #14416 for 2.5.2 (on both trunk and 2.5.x) over-eagerly
 > applied mutability to literal strings. In particular, the
 > strings returned by `inspect` have no real need to be mutable
 > as far as I can tell.

 We cannot make assumptions about how people use strings returned
 from .inspect.  Such code exists and we should not break it:

 warn(foo.inspect << " #{__FILE__}:#{__LINE__}")

 Disclaimer: I do not speak for the rest of ruby-core.

 This worked in 2.2 and earlier, and we should be maintaining
 compatibility with such code.  If we have .inspect method
 returning frozen strings, I consider it a bug.

 I hate that we allocate extra objects and lose performance;
 but losing compatibility and breaking existing code is worse.

Updated by Eregon (Benoit Daloze) about 1 year ago

I think it's very bad style to modify the return value of #inspect, and some objects do already return frozen strings for #inspect.
So I think it would be a fair enough to change as it seems to have very low risk, i.e.,
it would only break code which is questionable in the first place (and should use + or interpolation instead).

OTOH, it's likely irrelevant for performance, and could likely be optimized if needed anyway.
So I think there is not much to gain either way, but conceptually it's wrong for user code to assume the result #inspect is always mutable.

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

  • Status changed from Open to Rejected

I don't think that this should be considered a bug. Going from immutable to mutable should not break things, while going from mutable to immutable can. It is true that the using String#+ can decrease performance slightly, but that should not be considered a bug.

Also available in: Atom PDF