From cc16afb7f3b23d3bae4c5fb3e9e410d4d872cb7b Mon Sep 17 00:00:00 2001
From: HASUMI Hitoshi <hasumikin@gmail.com>
Date: Tue, 16 Jul 2024 18:55:26 +0900
Subject: [PATCH] mrb_str_aset_m() should return replace instead of str
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`string[]=(idx, replace)` should return `replace`.

## Actual (wrong)

```
string.[]=(idx, replace) → string
string.[]=(idx, len, replace) → string
```

## Expected

```
string.[]=(idx, replace) → replace
string.[]=(idx, len, replace) → replace
```

## Sidenote

As of the current mruby-compiler, `(string[idx] = 'X')` creates not only "CALL_NODE" but also "ASGN_NODE" and "OP_MOVE", overriding the wrong return value.
On the other hand, `string.[]=(idx, 'X')` creates only "CALL_NODE", exposing the wrong return value.

If my new mruby-compiler2, leveraging Prism, took the place of official compiler, `(string[idx] = 'X')` and `string.[]=(idx, 'X')` would be going to generate the same VM code without "OP_MOVE".
So I paranoidly added tests.

FYI: You can find how the new mruby-compiler2's AST and VM code look like in mruby/c's issue (mruby/c had the same bug): https://github.com/mrubyc/mrubyc/pull/210
---
 src/string.c     |  3 ++-
 test/t/string.rb | 18 +++++++++++++++---
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/string.c b/src/string.c
index c9565f8e9..2a44cb8be 100644
--- a/src/string.c
+++ b/src/string.c
@@ -1452,6 +1452,7 @@ mrb_str_aset(mrb_state *mrb, mrb_value str, mrb_value indx, mrb_value alen, mrb_
  *
  * Modify +self+ by replacing the content of +self+.
  * The portion of the string affected is determined using the same criteria as +String#[]+.
+ * The return value of this expression is +replace+.
  */
 static mrb_value
 mrb_str_aset_m(mrb_state *mrb, mrb_value str)
@@ -1467,7 +1468,7 @@ mrb_str_aset_m(mrb_state *mrb, mrb_value str)
       break;
   }
   mrb_str_aset(mrb, str, indx, alen, replace);
-  return str;
+  return replace;
 }
 
 /* 15.2.10.5.8  */
diff --git a/test/t/string.rb b/test/t/string.rb
index 0bb9acfb3..279530c8c 100644
--- a/test/t/string.rb
+++ b/test/t/string.rb
@@ -129,7 +129,7 @@ end
 assert('String#[]=') do
   # length of args is 1
   a = 'abc'
-  a[0] = 'X'
+  assert_equal 'X', (a[0] = 'X')
   assert_equal 'Xbc', a
 
   b = 'abc'
@@ -152,6 +152,10 @@ assert('String#[]=') do
     assert_equal 'aXc', e
   end
 
+  f = 'abc'
+  assert_equal 'X', f.[]=(0, 'X')
+  assert_equal 'Xbc', f
+
   assert_raise(TypeError) { 'a'[0] = 1 }
   assert_raise(TypeError) { 'a'[:a] = '1' }
 
@@ -176,15 +180,19 @@ assert('String#[]=') do
   assert_equal 'Xabc', d1
 
   e1 = 'abc'
-  e1[1, 3] = 'X'
+  assert_equal 'X', (e1[1, 3] = 'X')
   assert_equal 'aX', e1
 
+  f1 = 'abc'
+  assert_equal 'X', f1.[]=(0, 1, 'X')
+  assert_equal 'Xbc', f1
+
   # args is RegExp
   # It will be tested in mrbgems.
 
   # args is String
   a3 = 'abc'
-  a3['bc'] = 'X'
+  assert_equal 'X', (a3['bc'] = 'X')
   assert_equal a3, 'aX'
 
   b3 = 'abc'
@@ -192,6 +200,10 @@ assert('String#[]=') do
     b3['XX'] = 'Y'
   end
 
+  c3 = 'abc'
+  assert_equal 'X', c3.[]=('bc', 'X')
+  assert_equal 'aX', c3
+
   assert_raise(TypeError) { 'a'[:a, 0] = '1' }
   assert_raise(TypeError) { 'a'[0, :a] = '1' }
   assert_raise(TypeError) { 'a'[0, 1] = 1 }