[BUG] Using InterpolationCompiler fails

[fnordfish]

What I tried to do

Using the InterpolationCompiler as described in https://github.com/ruby-i18n/i18n/blob/master/lib/i18n/backend/interpolation_compiler.rb#L13 stopped working with i18n v1.9.1

What I expected to happen

Should not lead to raising an exception

What actually happened

Raises TypeError "can't define singleton" as soon as a I18n.default_locale= is called.

Versions of i18n, rails, and anything else you think is necessary

• ruby v3.1.2, v3.0.3
• i18n v1.10.0, v1.9.1

---

$ ruby -v -r i18n -e'I18n::Backend::Simple.include(I18n::Backend::InterpolationCompiler); I18n.load_path << Dir[File.expand_path("locales") + "/*.yml"]; I18n.default_locale = :en'

ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-linux]
/usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:29:in `compile_if_an_interpolation': can't define singleton (TypeError)
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:29:in `instance_eval'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:29:in `compile_if_an_interpolation'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:117:in `block in compile_all_strings_in'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:116:in `each_value'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:116:in `compile_all_strings_in'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:118:in `block in compile_all_strings_in'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:116:in `each_value'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:116:in `compile_all_strings_in'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:118:in `block in compile_all_strings_in'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:116:in `each_value'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:116:in `compile_all_strings_in'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:110:in `store_translations'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/base.rb:232:in `block in load_file'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/base.rb:232:in `each'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/base.rb:232:in `load_file'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/base.rb:17:in `block in load_translations'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/base.rb:16:in `each'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/base.rb:16:in `load_translations'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/simple.rb:77:in `init_translations'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/backend/simple.rb:47:in `available_locales'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/config.rb:45:in `available_locales'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/config.rb:51:in `available_locales_set'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n.rb:351:in `locale_available?'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n.rb:357:in `enforce_available_locales!'
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n/config.rb:36:in `default_locale='
	from /usr/local/bundle/gems/i18n-1.10.0/lib/i18n.rb:74:in `default_locale='
	from -e:1:in `<main>'

Thanks! :heart:

[radar]

I am able to reproduce this bug (6 months late, it seems). I owe you a writeup.

Gemfile

source 'https://rubygems.org'

gem 'i18n', '1.10.0'

script.rb

require 'bundler'
Bundler.setup

require 'i18n'

I18n::Backend::Simple.include(I18n::Backend::InterpolationCompiler)
I18n.load_path << Dir[File.expand_path("locales") + "/*.yml"]
I18n.default_locale = :en

locales/en.yml

en:
  hello: "%{world}"

---

Run:

bundle install
ruby script.rb

See:

/Users/ryan.bigg/.asdf/installs/ruby/3.1.2/lib/ruby/gems/3.1.0/gems/i18n-1.10.0/lib/i18n/backend/interpolation_compiler.rb:29:in `compile_if_an_interpolation': can't define singleton (TypeError)

---

I was completely unaware that this module existed within i18n at all. That's probably not what you want to hear from the guy maintaining the project, but it's facts and I'd rather be straight with you there.

Looks like the code is attempting to define a method on the value from the translations -- that is to define a method on a string. Keep in mind that this is not defining a method on String, but defining a method on a string instead.

Small Ruby example:

a = "cow"
b = "duck"
def a.moo
  "Moo!"
end
a.moo # => "Moo!"
b.moo # => undefined method

Normally, not a problem. But, it is if the string is frozen:

a = "arctic cow".freeze
def a.moo
  "Moo!"
end
# => can't define singleton

---

You say that this is happening newly on 1.10.0, so what I can do is change my project's Gemfile to:

source 'https://rubygems.org'

gem 'pry'
gem 'i18n', path: "~/code/gems/i18n"

Then, over on i18n I can switch to whatever version I want.

git checkout v1.9.1

That's the version that's a "step" behind the v1.10.0 version, however I see the bug there too. I also see the bug in v1.9.0 (two "steps" back), but not in v1.8.11 (three "steps").

Right, so now we have a faulty version and a working version. Someone with a lot more time than me would run git diff between the two versions and eyeball the diff. Since I am someone who does not have a lot of free time (refer to the fact it took me 6 months to get to this issue...), I will use git bisect to find a commit that broke this.

git bisect good v1.8.11
git bisect bad v1.9.0

Git says:

status: waiting for both good and bad commits
status: waiting for bad commit, 1 good commit known
Bisecting: 22 revisions left to test after this (roughly 5 steps)

Run the script again. It works! Good commit.

git bisect good

Git says:

Bisecting: 10 revisions left to test after this (roughly 4 steps)
[d76337294819ef2e84f2c2830e7aff1a99132217] Merge pull request #600 from ooooooo-q/master

Script blows up this time. This is a commit after the breakage, but not the break itself (probably).

git bisect bad

Git says:

Bisecting: 5 revisions left to test after this (roughly 3 steps)
[be5a8e016f168f093d789382f3183aa470195f86] Merge pull request #591 from movermeyer/movermeyer/resolve_using_original_fallback_locale

Run the script again, and it works. Good, bad, good.

git bisect good

Bisecting: 3 revisions left to test after this (roughly 2 steps)
[0fda789ea745cd462658a8948ee085201aba5c6f] Symbolize and freeze keys when loading from YAML

This commit talks about freezing keys. And we saw above that freezing strings gave us the error. Could this be the one? We'll keep going.

Running the script blows things up.

git bisect bad

Bisecting: 0 revisions left to test after this (roughly 0 steps)
[8ac1724b51459295c339e2ed3c2e911cd8003715] Conditionally assert load_json returns symbolized data

The script now runs again fine. So we've gone: good, bad, good, bad, good. Only 5 steps!

git bisect good

0fda789ea745cd462658a8948ee085201aba5c6f is the first bad commit
commit 0fda789ea745cd462658a8948ee085201aba5c6f
Author: Paarth Madan <[email protected]>
Date:   Wed Nov 3 12:33:12 2021 -0400

    Symbolize and freeze keys when loading from YAML

 lib/i18n/backend/base.rb    | 2 +-
 test/backend/simple_test.rb | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

Ok, so we found the commit. Diff is pretty small:

commit 0fda789ea745cd462658a8948ee085201aba5c6f (refs/bisect/bad)
Author: Paarth Madan <[email protected]>
Date:   Wed Nov 3 12:33:12 2021 -0400

    Symbolize and freeze keys when loading from YAML

diff --git a/lib/i18n/backend/base.rb b/lib/i18n/backend/base.rb
index 07f9bbd..b78a6af 100644
--- a/lib/i18n/backend/base.rb
+++ b/lib/i18n/backend/base.rb
@@ -240,7 +240,7 @@ module I18n
         def load_yml(filename)
           begin
             if YAML.respond_to?(:unsafe_load_file) # Psych 4.0 way
-              [YAML.unsafe_load_file(filename), false]
+              [YAML.unsafe_load_file(filename, symbolize_names: true, freeze: true), true]
             else
               [YAML.load_file(filename), false]
             end
diff --git a/test/backend/simple_test.rb b/test/backend/simple_test.rb
index 5c5955c..23de486 100644
--- a/test/backend/simple_test.rb
+++ b/test/backend/simple_test.rb
@@ -92,7 +92,12 @@ class I18nBackendSimpleTest < I18n::TestCase

   test "simple load_yml: loads data from a YAML file" do
     data, _ = I18n.backend.send(:load_yml, "#{locales_dir}/en.yml")
-    assert_equal({ 'en' => { 'foo' => { 'bar' => 'baz' } } }, data)
+    if ::YAML.respond_to?(:unsafe_load_file)
+      assert_equal({ :en => { :foo => { :bar => 'baz' } } }, data)
+      assert_predicate data.dig(:en, :foo, :bar), :frozen?
+    else
+      assert_equal({ 'en' => { 'foo' => { 'bar' => 'baz' } } }, data)
+    end
   end

So the logic here has changed things to freeze strings that are loaded. See #583 and related issues for discussions.

---

I am unsure what course to recommend here. I can think of a few options or questions:

1. What performance gain were you seeing or expecting to get out of InterpolationCompiler?
2. How did you come across this module?
3. I suspect with the recent performance improvements by Shopify et. al, you should already be seeing enough performance gains within i18n to warrant not using this module.

And given this quote:

# Note that InterpolationCompiler does not yield meaningful results and consequently
# should not be used with Ruby 1.9 (YARV) but improves performance everywhere else
# (jRuby, Rubinius).

And the lack of Rubinius support in i18n... I'm tempted to say that this module itself should be removed altogether in some future i18n release. To me, it's "magical" and unknown and its alleged performance gains are only on a very limited subset of Ruby versions, and therefore probably not worth maintaining within the core of this project itself.

[fnordfish]

I totally forgot about this and I feel bad that you felt obligated to that massive writeup. I found that comment in the code as well, ended up just not using this module and didn't experience any tangible performance hits.

Totally agree to remove or extract that module.