Enable Frozen string literals

[tagliala]

The only method that may potentially be affected is
Money::Formatter#append_currency_symbol.

This method has been refactored to avoid any potential side effect
with string mutations

Additionally, a Lint/RedundantStringCoercion offense has been fixed

Close #1108

[yukideluxe]

@tagliala, thanks for the PR and the explanation! I've double checked and, indeed, the only method that could be affected is Money::Formatter#append_currency_symbol. That said, I don't like the fact that it works because we know the append_sign returns an interpolated string and that is never frozen (TIL, btw 😳).

I wonder if we should prevent unexpected consequences (eg. we want to use Money::Formatter#append_currency_symbol for other means in the future) and just rewrite that method to avoid the mutation. What do you think!?

Something like

    def append_currency_symbol(formatted_number)
      if rules[:with_currency]
        currency_part = 
          if rules[:html]
            "<span class=\"currency\">#{currency.to_s}</span>"
          elsif rules[:html_wrap]
            html_wrap(currency.to_s, "currency")
          else
            currency.to_s
          end

        "#{formatted_number} #{currency_part}"
      else
        formatted_number
      end
    end

[yukideluxe]

@tjschuck can you triple-check this one?! I am also inclined to remove the Code Climate check since Code Climate is going to be replaced anyways! what do you think?!

[tagliala]

Much better, thanks. I've rebased but now codeclimate complains. I would not split this method and passing the parameters around again. Let me know how do you want to proceed

[tjschuck]

The frozen_string_literal magic comment was introduced back in the Ruby 2.3 days, and a lot of stuff around strings has meaningfully changed since then. So I'm not entirely sure this is necessary, but it doesn't per-se seem harmful.

Anyway, I benchmarked Money#format with/without this change, since #1108 says that was the method that got the greatest improvement. It seems... fine, and again, not particularly harmful or offensive. I've gotten pretty good at ignoring these comments at the tops of files if they're there ¯\_(ツ)_/¯

First, the benchmark script, using benchmark-ips and benchmark-memory:

# Run twice -- first do
#   $ ruby bench.rb
# then do
#   $ WITH_FROZEN_COMMENT=true ruby bench.rb

require 'bundler/inline'

if ENV['WITH_FROZEN_COMMENT'] == 'true'
  gemfile do
    source "https://rubygems.org"
    gem "benchmark-ips"
    gem "benchmark-memory"

    gem "money", git: 'https://github.com/tagliala/money', branch: 'feature/frozen-string-literals'
  end
else
  gemfile do
    source "https://rubygems.org"
    gem "benchmark-ips"
    gem "benchmark-memory"

    gem "money", git: 'https://github.com/RubyMoney/money', branch: 'main'
  end
end

require 'benchmark/ips'
require 'benchmark/memory'

I18n.config.available_locales = :en
Money.locale_backend = :i18n
Money.rounding_mode = BigDecimal::ROUND_HALF_UP

m = Money.from_cents(123, "GBP")

Benchmark.ips do |x|
  x.report('with frozen string comment') do
    m.format
  end

  x.report('without frozen string comment') do
    m.format
  end

  x.hold! 'temp_results_ips'
  x.compare!
end

Benchmark.memory do |x|
  x.report('with frozen string comment') do
    m.format
  end

  x.report('without frozen string comment') do
    m.format
  end

  x.hold! 'temp_results_mem'
  x.compare!
end

I ran the above on all supported Rubies (3.1 – 3.4), plus 3.5.0-preview1 for good measure.

3.1

Comparison:
with frozen string comment:    70473.7 i/s
without frozen string comment:    69933.5 i/s - same-ish: difference falls within error

Comparison:
with frozen string comment:       5296 allocated
without frozen string comment:       5776 allocated - 1.09x more

3.2

Comparison:
with frozen string comment:    80144.5 i/s
without frozen string comment:    77333.0 i/s - same-ish: difference falls within error

Comparison:
with frozen string comment:       5200 allocated
without frozen string comment:       5720 allocated - 1.10x more

3.3

Comparison:
with frozen string comment:    81450.4 i/s
without frozen string comment:    75517.5 i/s - same-ish: difference falls within error

Comparison:
with frozen string comment:       5176 allocated
without frozen string comment:       5696 allocated - 1.10x more

3.4

Comparison:
with frozen string comment:    77199.8 i/s
without frozen string comment:    74524.5 i/s - same-ish: difference falls within error

Comparison:
with frozen string comment:       4144 allocated
without frozen string comment:       4664 allocated - 1.13x more

3.5.0-preview1

Comparison:
with frozen string comment:    78967.5 i/s
without frozen string comment:    74296.0 i/s - same-ish: difference falls within error

Comparison:
with frozen string comment:       4144 allocated
without frozen string comment:       4664 allocated - 1.13x more

---

So... sure, go for it!

[tjschuck]

And yeah, let's rip out Code Climate next. Fine to ignore the ❌ from it here.

[tjschuck]

Thanks @tagliala!

[tagliala]

So I'm not entirely sure this is necessary, but it doesn't per-se seem harmful.

The magic comment is still necessary because, as of now, no supported Ruby versions have frozen string literals enabled by default for all files.

Freezing string literals by default was originally planned for Ruby 3.0, but that change was postponed and has not happened yet. It may become the default in Ruby 4.0, but until then, the comment is needed

It is being discussed here: https://bugs.ruby-lang.org/issues/20205

---

Additional details:

Probably I did run

RUBYOPT='--enable=frozen-string-literal --debug=frozen-string-literal' bundle exec rake

last year to check that it weren't issues (at least covered by specs) before adding the magic comment, but I forgot to mention this

Ref: https://gist.github.com/fxn/bf4eed2505c76f4fca03ab48c43adc72