Merge pull request #388 from OriPekelman/fix/poly-ivar-write-and-nil-return-dispatch

fix(codegen): two silent miscompiles on poly receivers

Author: matz

spinel_codegen.rb

@@ -28558,6 +28558,15 @@ def poly_dispatch_return_type(mname)
       @needs_rb_value = 1
       return "poly"
     end
+    # Setters: mname ends with "=" and at least one class has an
+    # attr_writer for the bare name. Return type is the ivar type
+    # (Ruby returns the rhs from `x = v`); without this, the result
+    # tmp's C type defaults to `mrb_int` and `tmp = rhs` mismatches
+    # for non-int slots.
+    setter_bname = ""
+    if mname.length > 1 && mname[mname.length - 1] == "="
+      setter_bname = mname[0, mname.length - 1]
+    end
     common = ""
     ci = 0
     while ci < @cls_names.length
@@ -28567,6 +28576,9 @@ def poly_dispatch_return_type(mname)
       elsif cls_has_attr_reader(ci, mname) == 1
         # An attr_reader returns the ivar type. Issue #119.
         rt = cls_ivar_type(ci, "@" + mname)
+      elsif setter_bname != "" && cls_has_attr_writer(ci, setter_bname) == 1
+        # An attr_writer setter returns the ivar's type.
+        rt = cls_ivar_type(ci, "@" + setter_bname)
       end
       if rt != ""
         if common == ""
@@ -28851,6 +28863,39 @@ def compile_poly_method_call(nid, rc, mname)
         else
           emit("    if (" + recv_tmp + ".cls_id == " + i.to_s + ") " + tmp + " = " + rhs + ";")
         end
+      elsif mname.length > 1 && mname[mname.length - 1] == "=" && cls_has_attr_writer(i, mname[0, mname.length - 1]) == 1
+        # An auto-registered attr_writer setter (`obj.x = v`) on a
+        # poly-typed receiver. Without this arm the cls_id dispatch
+        # finds neither a real method nor an attr_reader, falls
+        # through silently, and the assignment never executes —
+        # `obj.x = v` becomes a no-op.
+        bname = mname[0, mname.length - 1]
+        raw_val = arg_compiled.length > 0 ? arg_compiled[0] : "0"
+        raw_t   = arg_types.length > 0 ? arg_types[0] : ""
+        slot_t  = cls_ivar_type(i, "@" + bname)
+        # Match the rhs to the slot's concrete C type for *this arm*.
+        # Three cases:
+        #   slot poly + arg concrete  -> box the arg.
+        #   slot concrete + arg poly  -> unbox the arg.
+        #   else                      -> direct assign.
+        # Different arms in the dispatch may take different branches
+        # (e.g. one slot is `int`, another is `string`).
+        store_val = raw_val
+        if slot_t == "poly" && raw_t != "poly" && raw_t != ""
+          store_val = box_value_to_poly(raw_t, raw_val)
+        elsif slot_t != "poly" && raw_t == "poly"
+          store_val = unbox_poly_to(slot_t, raw_val)
+        end
+        ivar_lhs = "((sp_" + cname + " *)" + recv_tmp + ".v.p)->" + sanitize_ivar("@" + bname)
+        # `x = v` returns v in Ruby. The result tmp's type is set by
+        # poly_dispatch_return_type (poly when slot types diverge,
+        # otherwise the common slot type). Box / coerce the stored
+        # value into the tmp's expected shape.
+        rhs_for_tmp = store_val
+        if is_poly_ret == 1
+          rhs_for_tmp = box_val_to_poly(store_val, slot_t)
+        end
+        emit("    if (" + recv_tmp + ".cls_id == " + i.to_s + ") { " + ivar_lhs + " = " + store_val + "; " + tmp + " = " + rhs_for_tmp + "; }")
       end
       i = i + 1
     end
@@ -29254,7 +29299,13 @@ def box_non_nullable_value_to_poly(at, val)
       return "sp_box_bool(" + val + ")"
     end
     if at == "nil"
-      return "sp_box_nil()"
+      # `val` may be a side-effecting call (e.g. an arm in a
+      # cls_id-dispatched table where the user-defined override
+      # returns nil because its body is a `puts` / similar). Drop
+      # the value but keep the side effect via the comma operator;
+      # otherwise the generated dispatch silently elides the call
+      # for any subclass whose method's static return type is nil.
+      return "((void)(" + val + "), sp_box_nil())"
     end
     if at == "symbol"
       return "sp_box_sym(" + val + ")"

test/poly_attr_writer.rb

@@ -0,0 +1,95 @@
+# Regression: a setter call (`obj.x = val`) on a parameter typed as
+# a class union must execute the assignment. Previously the dispatch
+# loop emitted no arms for attr_writer setters on poly receivers and
+# silently dropped the store.
+#
+# Three subcases:
+#  1. Single class with `x`, second class without -- the union case
+#     where the dispatch arm is "skipped silently."
+#  2. Two classes share an ivar name; same value type. Verifies
+#     both arms emit and the result temp's C type fits the slot.
+#  3. Two classes share an ivar name; differing value types
+#     (string vs int). Forces the new arm to box the rhs poly-style
+#     for one cls_id and unbox for the other.
+#
+# Verifying via the *side effect* (the ivar's value) rather than the
+# setter expression's return value -- propagating setter returns
+# through the dispatch into the enclosing function's return is a
+# separate concern that this PR doesn't try to fix.
+
+# --- 1. Single class (the no-op repro) ---------------------------------
+
+class Foo
+  attr_accessor :x
+  def initialize; @x = 0; end
+end
+
+class Bar
+  # No `x`; coexists only to widen `obj` to a class union.
+end
+
+def set_x(obj, v)
+  obj.x = v
+end
+
+foo = Foo.new
+bar = Bar.new
+set_x(foo, 42)
+set_x(bar, 99) rescue nil
+
+puts foo.x
+
+
+# --- 2. Two classes share `body` (string in both) ---------------------
+
+class Req
+  attr_accessor :body
+  def initialize; @body = ""; end
+end
+
+class Res
+  attr_accessor :body
+  def initialize; @body = ""; end
+end
+
+def set_body(o, b)
+  o.body = b
+end
+
+req = Req.new
+res = Res.new
+set_body(req, "REQ")
+set_body(res, "RES")
+
+puts req.body
+puts res.body
+
+
+# --- 3. Two classes share `slot` with *different* value types ---------
+# Pre-fix poly_dispatch_return_type returned "int" for any setter
+# whose name didn't appear as an explicit method, so the result temp
+# was mrb_int and the string arm's `tmp = lv` mismatched. The new
+# arm now also unboxes the rhs to the slot's concrete type when
+# the function param widened to poly.
+
+class IBox
+  attr_accessor :slot
+  def initialize; @slot = 0; end
+end
+
+class SBox
+  attr_accessor :slot
+  def initialize; @slot = ""; end
+end
+
+def set_slot(o, v)
+  o.slot = v
+end
+
+ibox = IBox.new
+sbox = SBox.new
+set_slot(ibox, 7)
+set_slot(sbox, "hello")
+
+puts ibox.slot
+puts sbox.slot

test/poly_attr_writer.rb.expected

@@ -0,0 +1,5 @@
+42
+REQ
+RES
+7
+hello

test/poly_nil_return_dispatch.rb

@@ -0,0 +1,33 @@
+# Regression: a subclass override whose static return type is `nil`
+# must still be called when dispatched through a poly receiver.
+# Previously `box_value_to_poly("nil", call_expr)` dropped `call_expr`
+# and emitted a bare `sp_box_nil()` for that arm, so the body of the
+# override never ran.
+#
+# Two checks:
+#  1. The override's side effect runs (the `puts` inside Sub#hook).
+#  2. The dispatched call's *return value* is correctly observable as
+#     nil (i.e. the boxing path threads through, not just the side
+#     effect).
+
+class Base
+  def hook(arg); end          # empty body -> static return "nil"
+end
+
+class Sub < Base
+  def hook(arg)
+    puts "subclass-ran: " + arg   # `puts` returns nil
+  end
+end
+
+class Holder
+  attr_accessor :h
+  def initialize; @h = Base.new; end
+  def set(x); @h = x; end
+  def call_hook(arg); @h.hook(arg); end
+end
+
+h = Holder.new
+h.set(Sub.new)
+result = h.call_hook("ok")
+puts result == nil

test/poly_nil_return_dispatch.rb.expected

@@ -0,0 +1,2 @@
+subclass-ran: ok
+true