Merge pull request #2320 from herwinw/thread_element_ref

spec/core/thread/element_reference_spec.rb

@@ -37,6 +37,17 @@
     t2["value"].should == 2
   end
 
+  it "converts a key that is neither String nor Symbol with #to_str" do
+    key = mock('value')
+    key.should_receive(:to_str).and_return('value')
+
+    th = Thread.new do
+      Thread.current[:value] = 1
+    end.join
+
+    th[key].should == 1
+  end
+
   it "raises exceptions on the wrong type of keys" do
     -> { Thread.current[nil] }.should raise_error(TypeError)
     -> { Thread.current[5] }.should raise_error(TypeError)

spec/core/thread/element_set_spec.rb

@@ -12,10 +12,33 @@
       th.freeze
       -> {
         th[:foo] = "bar"
-      }.should raise_error(FrozenError, /frozen/)
+      }.should raise_error(FrozenError, "can't modify frozen thread locals")
     end.join
   end
 
+  it "accepts Strings and Symbols" do
+    t1 = Thread.new do
+      Thread.current[:value] = 1
+    end.join
+    t2 = Thread.new do
+      Thread.current["value"] = 2
+    end.join
+
+    t1[:value].should == 1
+    t2[:value].should == 2
+  end
+
+  it "converts a key that is neither String nor Symbol with #to_str" do
+    key = mock('value')
+    key.should_receive(:to_str).and_return('value')
+
+    th = Thread.new do
+      Thread.current[key] = 1
+    end.join
+
+    th[:value].should == 1
+  end
+
   it "raises exceptions on the wrong type of keys" do
     -> { Thread.current[nil] = true }.should raise_error(TypeError)
     -> { Thread.current[5] = true }.should raise_error(TypeError)

spec/core/thread/key_spec.rb

@@ -16,6 +16,13 @@
     @th.key?(:stanley.to_s).should == false
   end
 
+  it "converts a key that is neither String nor Symbol with #to_str" do
+    key = mock('key')
+    key.should_receive(:to_str).and_return('oliver')
+
+    @th.key?(key).should == true
+  end
+
   it "raises exceptions on the wrong type of keys" do
     -> { Thread.current.key? nil }.should raise_error(TypeError)
     -> { Thread.current.key? 5 }.should raise_error(TypeError)

spec/core/thread/thread_variable_get_spec.rb

@@ -41,13 +41,20 @@
     @t.thread_variable_get(:a).should be_nil
   end
 
-  it "does not raise a TypeError if the key is neither Symbol nor String, nor responds to #to_str" do
-    @t.thread_variable_get(123).should be_nil
+  it "raises a TypeError if the key is neither Symbol nor String when thread variables are already set" do
+    @t.thread_variable_set(:a, 49)
+    -> { @t.thread_variable_get(123) }.should raise_error(TypeError, /123 is not a symbol/)
   end
 
-  it "does not try to convert the key with #to_sym" do
-    key = mock('key')
-    key.should_not_receive(:to_sym)
-    @t.thread_variable_get(key).should be_nil
+  ruby_version_is '3.4' do
+    it "raises a TypeError if the key is neither Symbol nor String when no thread variables are set" do
+      -> { @t.thread_variable_get(123) }.should raise_error(TypeError, /123 is not a symbol/)
+    end
+
+    it "raises a TypeError if the key is neither Symbol nor String without calling #to_sym" do
+      key = mock('key')
+      key.should_not_receive(:to_sym)
+      -> { @t.thread_variable_get(key) }.should raise_error(TypeError, /#{Regexp.escape(key.inspect)} is not a symbol/)
+    end
   end
 end

spec/core/thread/thread_variable_spec.rb

@@ -41,13 +41,20 @@
     @t.thread_variable?(:a).should be_false
   end
 
-  it "does not raise a TypeError if the key is neither Symbol nor String, nor responds to #to_str" do
-    @t.thread_variable?(123).should be_false
+  it "raises a TypeError if the key is neither Symbol nor String when thread variables are already set" do
+    @t.thread_variable_set(:a, 49)
+    -> { @t.thread_variable?(123) }.should raise_error(TypeError, /123 is not a symbol/)
   end
 
-  it "does not try to convert the key with #to_sym" do
-    key = mock('key')
-    key.should_not_receive(:to_sym)
-    @t.thread_variable?(key).should be_false
+  ruby_version_is '3.4' do
+    it "raises a TypeError if the key is neither Symbol nor String when no thread variables are set" do
+      -> { @t.thread_variable?(123) }.should raise_error(TypeError, /123 is not a symbol/)
+    end
+
+    it "raises a TypeError if the key is neither Symbol nor String without calling #to_sym" do
+      key = mock('key')
+      key.should_not_receive(:to_sym)
+      -> { @t.thread_variable?(key) }.should raise_error(TypeError, /#{Regexp.escape(key.inspect)} is not a symbol/)
+    end
   end
 end

src/thread_object.cpp

@@ -143,10 +143,10 @@ namespace Natalie {
 thread_local ThreadObject *tl_current_thread = nullptr;
 
 static Value validate_key(Env *env, Value key) {
-    if (key->is_string())
-        key = key->as_string()->to_sym(env);
+    if (key->is_string() || key->respond_to(env, "to_str"_s))
+        key = key->to_str(env)->to_sym(env);
     if (!key->is_symbol())
-        env->raise("TypeError", "wrong argument type {} (expected Symbol)", key->klass()->inspect_str());
+        env->raise("TypeError", "{} is not a symbol", key->inspect_str(env));
     return key;
 }
 
@@ -541,32 +541,21 @@ Value ThreadObject::refeq(Env *env, Value key, Value value) {
 }
 
 bool ThreadObject::has_thread_variable(Env *env, Value key) const {
-    if (!key->is_symbol() && !key->is_string() && key->respond_to(env, "to_str"_s))
-        key = key->to_str(env);
-    if (key->is_string())
-        key = key->as_string()->to_sym(env);
+    key = validate_key(env, key);
     return m_thread_variables && m_thread_variables->has_key(env, key);
 }
 
 Value ThreadObject::thread_variable_get(Env *env, Value key) {
     if (!m_thread_variables)
         return NilObject::the();
-    if (!key->is_symbol() && !key->is_string() && key->respond_to(env, "to_str"_s))
-        key = key->to_str(env);
-    if (key->is_string())
-        key = key->as_string()->to_sym(env);
+    key = validate_key(env, key);
     return m_thread_variables->ref(env, key);
 }
 
 Value ThreadObject::thread_variable_set(Env *env, Value key, Value value) {
     if (is_frozen())
         env->raise("FrozenError", "can't modify frozen thread locals");
-    if (!key->is_symbol() && !key->is_string() && key->respond_to(env, "to_str"_s))
-        key = key->to_str(env);
-    if (key->is_string())
-        key = key->as_string()->to_sym(env);
-    if (!key->is_symbol())
-        env->raise("TypeError", "{} is not a symbol", key->inspect_str(env));
+    key = validate_key(env, key);
     if (!m_thread_variables)
         m_thread_variables = new HashObject;
     if (value->is_nil()) {