I think it's good if we make Rails internals "frozen string" friendly. Though I'd prefer if we used .dup instead of String.new. I feel like I'm having Java flashbacks. 😉

Fair enough :) I meant to comment on the different approaches (String.new vs dup vs StringIO vs += in place of <<). I'll switch my changes to dup.

Incidentally, the encoding of the empty string generated by String.new without argument is not UTF-8 but ASCII-8BIT.

String.new.encoding     # => #<Encoding:ASCII-8BIT>
String.new("").encoding # => #<Encoding:UTF-8>
"".dup.encoding         # => #<Encoding:UTF-8>
"".encoding             # => #<Encoding:UTF-8>

I also think that it is preferable to use the String#dup rather than String.new so that this difference does not cause unexpected problems.
(There is no problem because String.new without argument did not exist in this PR.)

Thanks for that note @koic, good to know :)

I've gone ahead and made changes to ActiveModel, ActiveRecord, ActionMailer and ActionView. ActionJob didn't need any changes. For ActionCable, ActionPack and Railties, there are underlying dependencies that need patching (redis, rack, and thor, respectively), so I'll look into those tomorrow.

If anyone with particular focus/expertise on the libraries I have patched would like to review, please go ahead!

Also, @tenderlove: I updated my Builder patch to use dup as well 👍

Can we add the magic comment to the files that were fixed?

@rafaelfranca we could, but that's not going to help stop bad uses of frozen string literals appear in all the other files. We could add the comment to all current files, but still, any new files that get added to Rails would need to have it as well - which may be acceptable?

Using either pragmater or RUBYOPT helps avoid regressions more reliably, I feel. Certainly, if all dependencies are frozen-string-literal friendly, then RUBYOPT is the simpler approach, but I'm a little wary of that given the number of dependencies Rails (and its test suite) relies upon.

@pat thanks for this!

I think the comment is important for these files in particular because otherwise "".dup will allocate two strings when the global option isn't set. Even so, we should consider whether any of these are particularly hot -- the magic comment can't help us for 2.2.

@rafaelfranca perhaps we should consider just adding the comment everywhere, and enabling the relevant rubocop rule. Once we (read: @pat 😉) have done the work to find all the strings that need a dup, the rest are all neutral at best, or otherwise a free [trivial] performance gain.

Bonus thought: even with the magic comment in every file, that wouldn't (AFAIK) cover eval calls, of which we have a few -- though most won't contain strings.

Probably narrow enough that we can ignore enforcement for now, until we can go full RUBYOPT on our test suite.

@rafaelfranca perhaps we should consider just adding the comment everywhere, and enabling the relevant rubocop rule. Once we (read: @pat 😉) have done the work to find all the strings that need a dup, the rest are all neutral at best, or otherwise a free [trivial] performance gain.

+1. On that note, are there actual benchmarks that show the frozen string default helping (especially for a real app)? Last thing I saw was a modest speed increase for micro benchmarks. But the last tests I saw were probably 2 years ago when we were discussing the feature on ruby-core.

I still think we should move forward with frozen string literals (because I like them), I'm just genuinely curious about real performance.

perhaps we should consider just adding the comment everywhere, and enabling the relevant rubocop rule

That is the idea. I did that already in arel.