Make String#insert spec-compliant

include/natalie/string_object.hpp

@@ -173,7 +173,6 @@ class StringObject : public Object {
     EncodingObject *assert_compatible_string_and_update_encoding(Env *, StringObject *);
 
     void prepend_char(Env *, char);
-    void insert(Env *, size_t, char);
 
     void append_char(char);
     void append(signed char);
@@ -400,7 +399,7 @@ class StringObject : public Object {
 
     static size_t byte_index_to_char_index(ArrayObject *chars, size_t byte_index);
 
-    ssize_t char_index_to_byte_index(ssize_t) const;
+    ssize_t char_index_to_byte_index(ssize_t, bool = false) const;
     ssize_t byte_index_to_char_index(size_t) const;
 
     static CaseMapType check_case_options(Env *env, Value arg1, Value arg2, bool downcase = false);

spec/core/string/insert_spec.rb

@@ -1,5 +1,5 @@
 # -*- encoding: utf-8 -*-
-
+# frozen_string_literal: false
 require_relative '../../spec_helper'
 require_relative 'fixtures/classes'
 
@@ -57,19 +57,17 @@
     "ありがとう".insert(1, 'ü').should == "あüりがとう"
   end
 
-  xit "returns a String in the compatible encoding" do
+  it "returns a String in the compatible encoding" do
     str = "".force_encoding(Encoding::US_ASCII)
     str.insert(0, "ありがとう")
     str.encoding.should == Encoding::UTF_8
   end
 
   it "raises an Encoding::CompatibilityError if the encodings are incompatible" do
     pat = "ア".encode Encoding::EUC_JP
-    NATFIXME 'Raise Encoding::CompatibilityError', exception: SpecFailedException do
-      -> do
-        "あれ".insert 0, pat
-      end.should raise_error(Encoding::CompatibilityError)
-    end
+    -> do
+      "あれ".insert 0, pat
+    end.should raise_error(Encoding::CompatibilityError)
   end
 
   it "should not call subclassed string methods" do

src/string_object.cpp

@@ -1954,15 +1954,19 @@ size_t StringObject::byte_index_to_char_index(ArrayObject *chars, size_t byte_in
     return char_index;
 }
 
-ssize_t StringObject::char_index_to_byte_index(ssize_t char_index) const {
+// For String#insert, -1 means *after* the last character.
+// For any method looking at the characters, -1 means to include the last character.
+ssize_t StringObject::char_index_to_byte_index(ssize_t char_index, bool for_insert) const {
     if (char_index < 0) {
         size_t byte_index = bytesize();
-        ssize_t current_char_index = 0;
+        ssize_t current_char_index = for_insert ? -1 : 0;
         TM::StringView view;
-        do {
+        while (char_index < current_char_index) {
+            if (byte_index == 0)
+                return -1;
             view = prev_char(&byte_index);
             current_char_index--;
-        } while (byte_index != 0 && char_index < current_char_index);
+        }
         return byte_index;
     }
 
@@ -2648,27 +2652,36 @@ bool StringObject::include(const char *arg) const {
 
 Value StringObject::insert(Env *env, Value index_obj, Value other_str) {
     assert_not_frozen(env);
-    nat_int_t index_i = index_obj->to_int(env)->to_nat_int_t();
+
+    auto char_index = IntegerObject::convert_to_native_type<ssize_t>(env, index_obj->to_int(env));
     StringObject *string = other_str->to_str(env);
-    size_t count = char_count(env);
-    size_t index = index_i < 0 ? (count + index_i + 1) : index_i;
-    if (index_i == -1) {
+
+    if (char_index == -1) {
+        assert_compatible_string_and_update_encoding(env, string);
         append(string);
         return this;
     }
-    if (index > count) {
-        env->raise("IndexError", "index {} out of string", index_obj);
-    }
-    if (string->is_empty()) {
+
+    auto byte_index = char_index_to_byte_index(char_index, true);
+    if (byte_index < 0)
+        env->raise("IndexError", "index {} out of string", char_index);
+
+    if (string->is_empty())
+        return this;
+
+    if (m_encoding != string->m_encoding)
+        assert_compatible_string_and_update_encoding(env, string);
+
+    if ((size_t)byte_index == m_string.length()) {
+        append(string);
         return this;
     }
-    Value slice = slice_in_place(env, Value::integer(index), Value::integer(count - index));
-    append(string);
-    if (slice->is_string()) {
-        append(slice->as_string());
-    }
-    // NATFIXME: Return string with compatible encoding
-    // NATFIXME: Raise Encoding::CompatibilityError if the encodings are incompatible
+
+    String slice = m_string.substring(byte_index);
+    m_string.truncate(byte_index);
+    m_string.append(string->m_string);
+    m_string.append(slice);
+
     return this;
 }
 
@@ -3309,10 +3322,6 @@ void StringObject::prepend_char(Env *env, char c) {
     m_string.prepend_char(c);
 }
 
-void StringObject::insert(Env *, size_t position, char c) {
-    m_string.insert(position, c);
-}
-
 void StringObject::append_char(char c) {
     m_string.append_char(c);
 }