Skip to content

Commit

Permalink
Fix op asgn calls with keywords
Browse files Browse the repository at this point in the history 
Examples of such calls:

```ruby
obj[kw: 1] += fo
obj[**kw] &&= bar
```

Before this patch, literal keywords would segfault in the compiler,
and keyword splat usage would result in TypeError.

This handles all cases I can think of:

* literal keywords
* keyword splats
* combined with positional arguments
* combined with regular splats
* both with and without blocks
* both popped and non-popped cases

This also makes sure that to_hash is only called once on the keyword
splat argument, instead of twice, and make sure it is called before
calling to_proc on a passed block.

Fixes [Bug #20051]

Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>
  • Loading branch information
@jeremyevans @nobu
jeremyevans and nobu committed Dec 12, 2023
1 parent f671c5d commit 2f1d6da
Show file tree
Hide file tree
Showing 2 changed files with 251 additions and 21 deletions.
112 changes: 91 additions & 21 deletions compile.c
View file
Expand Up @@ -8848,6 +8848,8 @@ compile_op_asgn1(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node
int asgnflag = 0;
ID id = RNODE_OP_ASGN1(node)->nd_mid;
int boff = 0;
int keyword_len = 0;
struct rb_callinfo_kwarg *keywords = NULL;

/*
* a[x] (op)= y
Expand Down Expand Up @@ -8885,12 +8887,28 @@ compile_op_asgn1(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node
boff = 1;
/* fall through */
default:
argc = setup_args(iseq, ret, RNODE_OP_ASGN1(node)->nd_index, &flag, NULL);
argc = setup_args(iseq, ret, RNODE_OP_ASGN1(node)->nd_index, &flag, &keywords);
if (flag & VM_CALL_KW_SPLAT) {
if (boff) {
ADD_INSN(ret, node, splatkw);
}
else {
/* Make sure to_hash is only called once and not twice */
ADD_INSN(ret, node, dup);
ADD_INSN(ret, node, splatkw);
ADD_INSN(ret, node, pop);
}
}
CHECK(!NIL_P(argc));
}
ADD_INSN1(ret, node, dupn, FIXNUM_INC(argc, 1 + boff));
int dup_argn = FIX2INT(argc) + 1 + boff;
if (keywords) {
keyword_len = keywords->keyword_len;
dup_argn += keyword_len;
}
ADD_INSN1(ret, node, dupn, INT2FIX(dup_argn));
flag |= asgnflag;
ADD_SEND_WITH_FLAG(ret, node, idAREF, argc, INT2FIX(flag));
ADD_SEND_R(ret, node, idAREF, argc, NULL, INT2FIX(flag), keywords);

if (id == idOROP || id == idANDOP) {
/* a[x] ||= y or a[x] &&= y
Expand All @@ -8915,62 +8933,114 @@ compile_op_asgn1(rb_iseq_t *iseq, LINK_ANCHOR *const ret, const NODE *const node

CHECK(COMPILE(ret, "NODE_OP_ASGN1 nd_rvalue: ", RNODE_OP_ASGN1(node)->nd_rvalue));
if (!popped) {
ADD_INSN1(ret, node, setn, FIXNUM_INC(argc, 2+boff));
ADD_INSN1(ret, node, setn, INT2FIX(dup_argn+1));
}
if (flag & VM_CALL_ARGS_SPLAT) {
ADD_INSN1(ret, node, newarray, INT2FIX(1));
if (boff > 0) {
ADD_INSN1(ret, node, dupn, INT2FIX(3));
if (flag & VM_CALL_KW_SPLAT) {
ADD_INSN1(ret, node, topn, INT2FIX(2 + boff));
ADD_INSN(ret, node, swap);
ADD_INSN1(ret, node, newarray, INT2FIX(1));
ADD_INSN(ret, node, concatarray);
ADD_INSN1(ret, node, setn, INT2FIX(2 + boff));
ADD_INSN(ret, node, pop);
}
ADD_INSN(ret, node, concatarray);
else {
ADD_INSN1(ret, node, newarray, INT2FIX(1));
if (boff > 0) {
ADD_INSN1(ret, node, dupn, INT2FIX(3));
ADD_INSN(ret, node, swap);
ADD_INSN(ret, node, pop);
}
ADD_INSN(ret, node, concatarray);
if (boff > 0) {
ADD_INSN1(ret, node, setn, INT2FIX(3));
ADD_INSN(ret, node, pop);
ADD_INSN(ret, node, pop);
}
}
ADD_SEND_R(ret, node, idASET, argc, NULL, INT2FIX(flag), keywords);
}
else if (flag & VM_CALL_KW_SPLAT) {
if (boff > 0) {
ADD_INSN1(ret, node, topn, INT2FIX(2));
ADD_INSN(ret, node, swap);
ADD_INSN1(ret, node, setn, INT2FIX(3));
ADD_INSN(ret, node, pop);
ADD_INSN(ret, node, pop);
}
ADD_SEND_WITH_FLAG(ret, node, idASET, argc, INT2FIX(flag));
ADD_INSN(ret, node, swap);
ADD_SEND_R(ret, node, idASET, FIXNUM_INC(argc, 1), NULL, INT2FIX(flag), keywords);
}
else if (keyword_len) {
ADD_INSN1(ret, node, opt_reverse, INT2FIX(keyword_len+boff+1));
ADD_INSN1(ret, node, opt_reverse, INT2FIX(keyword_len+boff+0));
ADD_SEND_R(ret, node, idASET, FIXNUM_INC(argc, 1), NULL, INT2FIX(flag), keywords);
}
else {
if (boff > 0)
ADD_INSN(ret, node, swap);
ADD_SEND_WITH_FLAG(ret, node, idASET, FIXNUM_INC(argc, 1), INT2FIX(flag));
ADD_SEND_R(ret, node, idASET, FIXNUM_INC(argc, 1), NULL, INT2FIX(flag), keywords);
}
ADD_INSN(ret, node, pop);
ADD_INSNL(ret, node, jump, lfin);
ADD_LABEL(ret, label);
if (!popped) {
ADD_INSN1(ret, node, setn, FIXNUM_INC(argc, 2+boff));
ADD_INSN1(ret, node, setn, INT2FIX(dup_argn+1));
}
ADD_INSN1(ret, node, adjuststack, FIXNUM_INC(argc, 2+boff));
ADD_INSN1(ret, node, adjuststack, INT2FIX(dup_argn+1));
ADD_LABEL(ret, lfin);
}
else {
CHECK(COMPILE(ret, "NODE_OP_ASGN1 nd_rvalue: ", RNODE_OP_ASGN1(node)->nd_rvalue));
ADD_SEND(ret, node, id, INT2FIX(1));
if (!popped) {
ADD_INSN1(ret, node, setn, FIXNUM_INC(argc, 2+boff));
ADD_INSN1(ret, node, setn, INT2FIX(dup_argn+1));
}
if (flag & VM_CALL_ARGS_SPLAT) {
ADD_INSN1(ret, node, newarray, INT2FIX(1));
if (boff > 0) {
ADD_INSN1(ret, node, dupn, INT2FIX(3));
if (flag & VM_CALL_KW_SPLAT) {
ADD_INSN1(ret, node, topn, INT2FIX(2 + boff));
ADD_INSN(ret, node, swap);
ADD_INSN1(ret, node, newarray, INT2FIX(1));
ADD_INSN(ret, node, concatarray);
ADD_INSN1(ret, node, setn, INT2FIX(2 + boff));
ADD_INSN(ret, node, pop);
}
ADD_INSN(ret, node, concatarray);
else {
ADD_INSN1(ret, node, newarray, INT2FIX(1));
if (boff > 0) {
ADD_INSN1(ret, node, dupn, INT2FIX(3));
ADD_INSN(ret, node, swap);
ADD_INSN(ret, node, pop);
}
ADD_INSN(ret, node, concatarray);
if (boff > 0) {
ADD_INSN1(ret, node, setn, INT2FIX(3));
ADD_INSN(ret, node, pop);
ADD_INSN(ret, node, pop);
}
}
ADD_SEND_R(ret, node, idASET, argc, NULL, INT2FIX(flag), keywords);
}
else if (flag & VM_CALL_KW_SPLAT) {
if (boff > 0) {
ADD_INSN1(ret, node, topn, INT2FIX(2));
ADD_INSN(ret, node, swap);
ADD_INSN1(ret, node, setn, INT2FIX(3));
ADD_INSN(ret, node, pop);
ADD_INSN(ret, node, pop);
}
ADD_SEND_WITH_FLAG(ret, node, idASET, argc, INT2FIX(flag));
ADD_INSN(ret, node, swap);
ADD_SEND_R(ret, node, idASET, FIXNUM_INC(argc, 1), NULL, INT2FIX(flag), keywords);
}
else if (keyword_len) {
ADD_INSN(ret, node, dup);
ADD_INSN1(ret, node, opt_reverse, INT2FIX(keyword_len+boff+2));
ADD_INSN1(ret, node, opt_reverse, INT2FIX(keyword_len+boff+1));
ADD_INSN(ret, node, pop);
ADD_SEND_R(ret, node, idASET, FIXNUM_INC(argc, 1), NULL, INT2FIX(flag), keywords);
}
else {
if (boff > 0)
ADD_INSN(ret, node, swap);
ADD_SEND_WITH_FLAG(ret, node, idASET, FIXNUM_INC(argc, 1), INT2FIX(flag));
ADD_SEND_R(ret, node, idASET, FIXNUM_INC(argc, 1), NULL, INT2FIX(flag), keywords);
}
ADD_INSN(ret, node, pop);
}
Expand Down
160 changes: 160 additions & 0 deletions test/ruby/test_call.rb
View file
Expand Up @@ -121,6 +121,166 @@ def test_call_bmethod_proc_restarg
assert_equal([10], a(10))
end

def test_call_op_asgn_keywords
h = Class.new do
attr_reader :get, :set
def v; yield; [*@get, *@set] end
def [](*a, **b, &c) @get = [a, b, c]; @set = []; 3 end
def []=(*a, **b, &c) @set = [a, b, c] end
end.new

a = []
kw = {}
b = lambda{}

# +=, without block, non-popped
assert_equal([[], {}, nil, [4], {}, nil], h.v{h[**kw] += 1})
assert_equal([[0], {}, nil, [0, 4], {}, nil], h.v{h[0, **kw] += 1})
assert_equal([[0], {}, nil, [0, 4], {}, nil], h.v{h[0, *a, **kw] += 1})
assert_equal([[], {kw: 5}, nil, [4], {kw: 5}, nil], h.v{h[kw: 5] += 1})
assert_equal([[], {kw: 5, a: 2}, nil, [4], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] += 1})
assert_equal([[], {kw: 5, a: 2}, nil, [4], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] += 1})
assert_equal([[0], {kw: 5, a: 2}, nil, [0, 4], {kw: 5, a: 2}, nil], h.v{h[0, kw: 5, a: 2] += 1})
assert_equal([[0], {kw: 5, a: 2, nil: 3}, nil, [0, 4], {kw: 5, a: 2, nil: 3}, nil], h.v{h[0, *a, kw: 5, a: 2, nil: 3] += 1})

# +=, with block, non-popped
assert_equal([[], {}, b, [4], {}, b], h.v{h[**kw, &b] += 1})
assert_equal([[0], {}, b, [0, 4], {}, b], h.v{h[0, **kw, &b] += 1})
assert_equal([[0], {}, b, [0, 4], {}, b], h.v{h[0, *a, **kw, &b] += 1})
assert_equal([[], {kw: 5}, b, [4], {kw: 5}, b], h.v{h[kw: 5, &b] += 1})
assert_equal([[], {kw: 5, a: 2}, b, [4], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] += 1})
assert_equal([[], {kw: 5, a: 2}, b, [4], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] += 1})
assert_equal([[0], {kw: 5, a: 2}, b, [0, 4], {kw: 5, a: 2}, b], h.v{h[0, kw: 5, a: 2, &b] += 1})
assert_equal([[0], {kw: 5, a: 2, b: 3}, b, [0, 4], {kw: 5, a: 2, b: 3}, b], h.v{h[0, *a, kw: 5, a: 2, b: 3, &b] += 1})

# +=, without block, popped
assert_equal([[], {}, nil, [4], {}, nil], h.v{h[**kw] += 1; nil})
assert_equal([[0], {}, nil, [0, 4], {}, nil], h.v{h[0, **kw] += 1; nil})
assert_equal([[0], {}, nil, [0, 4], {}, nil], h.v{h[0, *a, **kw] += 1; nil})
assert_equal([[], {kw: 5}, nil, [4], {kw: 5}, nil], h.v{h[kw: 5] += 1; nil})
assert_equal([[], {kw: 5, a: 2}, nil, [4], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] += 1; nil})
assert_equal([[], {kw: 5, a: 2}, nil, [4], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] += 1; nil})
assert_equal([[0], {kw: 5, a: 2}, nil, [0, 4], {kw: 5, a: 2}, nil], h.v{h[0, kw: 5, a: 2] += 1; nil})
assert_equal([[0], {kw: 5, a: 2, nil: 3}, nil, [0, 4], {kw: 5, a: 2, nil: 3}, nil], h.v{h[0, *a, kw: 5, a: 2, nil: 3] += 1; nil})

# +=, with block, popped
assert_equal([[], {}, b, [4], {}, b], h.v{h[**kw, &b] += 1; nil})
assert_equal([[0], {}, b, [0, 4], {}, b], h.v{h[0, **kw, &b] += 1; nil})
assert_equal([[0], {}, b, [0, 4], {}, b], h.v{h[0, *a, **kw, &b] += 1; nil})
assert_equal([[], {kw: 5}, b, [4], {kw: 5}, b], h.v{h[kw: 5, &b] += 1; nil})
assert_equal([[], {kw: 5, a: 2}, b, [4], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] += 1; nil})
assert_equal([[], {kw: 5, a: 2}, b, [4], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] += 1; nil})
assert_equal([[0], {kw: 5, a: 2}, b, [0, 4], {kw: 5, a: 2}, b], h.v{h[0, kw: 5, a: 2, &b] += 1; nil})
assert_equal([[0], {kw: 5, a: 2, b: 3}, b, [0, 4], {kw: 5, a: 2, b: 3}, b], h.v{h[0, *a, kw: 5, a: 2, b: 3, &b] += 1; nil})

# &&=, without block, non-popped
assert_equal([[], {}, nil, [1], {}, nil], h.v{h[**kw] &&= 1})
assert_equal([[0], {}, nil, [0, 1], {}, nil], h.v{h[0, **kw] &&= 1})
assert_equal([[0], {}, nil, [0, 1], {}, nil], h.v{h[0, *a, **kw] &&= 1})
assert_equal([[], {kw: 5}, nil, [1], {kw: 5}, nil], h.v{h[kw: 5] &&= 1})
assert_equal([[], {kw: 5, a: 2}, nil, [1], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] &&= 1})
assert_equal([[], {kw: 5, a: 2}, nil, [1], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] &&= 1})
assert_equal([[0], {kw: 5, a: 2}, nil, [0, 1], {kw: 5, a: 2}, nil], h.v{h[0, kw: 5, a: 2] &&= 1})
assert_equal([[0], {kw: 5, a: 2, nil: 3}, nil, [0, 1], {kw: 5, a: 2, nil: 3}, nil], h.v{h[0, *a, kw: 5, a: 2, nil: 3] &&= 1})

# &&=, with block, non-popped
assert_equal([[], {}, b, [1], {}, b], h.v{h[**kw, &b] &&= 1})
assert_equal([[0], {}, b, [0, 1], {}, b], h.v{h[0, **kw, &b] &&= 1})
assert_equal([[0], {}, b, [0, 1], {}, b], h.v{h[0, *a, **kw, &b] &&= 1})
assert_equal([[], {kw: 5}, b, [1], {kw: 5}, b], h.v{h[kw: 5, &b] &&= 1})
assert_equal([[], {kw: 5, a: 2}, b, [1], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] &&= 1})
assert_equal([[], {kw: 5, a: 2}, b, [1], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] &&= 1})
assert_equal([[0], {kw: 5, a: 2}, b, [0, 1], {kw: 5, a: 2}, b], h.v{h[0, kw: 5, a: 2, &b] &&= 1})
assert_equal([[0], {kw: 5, a: 2, b: 3}, b, [0, 1], {kw: 5, a: 2, b: 3}, b], h.v{h[0, *a, kw: 5, a: 2, b: 3, &b] &&= 1})

# &&=, without block, popped
assert_equal([[], {}, nil, [1], {}, nil], h.v{h[**kw] &&= 1; nil})
assert_equal([[0], {}, nil, [0, 1], {}, nil], h.v{h[0, **kw] &&= 1; nil})
assert_equal([[0], {}, nil, [0, 1], {}, nil], h.v{h[0, *a, **kw] &&= 1; nil})
assert_equal([[], {kw: 5}, nil, [1], {kw: 5}, nil], h.v{h[kw: 5] &&= 1; nil})
assert_equal([[], {kw: 5, a: 2}, nil, [1], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] &&= 1; nil})
assert_equal([[], {kw: 5, a: 2}, nil, [1], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] &&= 1; nil})
assert_equal([[0], {kw: 5, a: 2}, nil, [0, 1], {kw: 5, a: 2}, nil], h.v{h[0, kw: 5, a: 2] &&= 1; nil})
assert_equal([[0], {kw: 5, a: 2, nil: 3}, nil, [0, 1], {kw: 5, a: 2, nil: 3}, nil], h.v{h[0, *a, kw: 5, a: 2, nil: 3] &&= 1; nil})

# &&=, with block, popped
assert_equal([[], {}, b, [1], {}, b], h.v{h[**kw, &b] &&= 1; nil})
assert_equal([[0], {}, b, [0, 1], {}, b], h.v{h[0, **kw, &b] &&= 1; nil})
assert_equal([[0], {}, b, [0, 1], {}, b], h.v{h[0, *a, **kw, &b] &&= 1; nil})
assert_equal([[], {kw: 5}, b, [1], {kw: 5}, b], h.v{h[kw: 5, &b] &&= 1; nil})
assert_equal([[], {kw: 5, a: 2}, b, [1], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] &&= 1; nil})
assert_equal([[], {kw: 5, a: 2}, b, [1], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] &&= 1; nil})
assert_equal([[0], {kw: 5, a: 2}, b, [0, 1], {kw: 5, a: 2}, b], h.v{h[0, kw: 5, a: 2, &b] &&= 1; nil})
assert_equal([[0], {kw: 5, a: 2, b: 3}, b, [0, 1], {kw: 5, a: 2, b: 3}, b], h.v{h[0, *a, kw: 5, a: 2, b: 3, &b] &&= 1; nil})

# ||=, without block, non-popped
assert_equal([[], {}, nil], h.v{h[**kw] ||= 1})
assert_equal([[0], {}, nil], h.v{h[0, **kw] ||= 1})
assert_equal([[0], {}, nil], h.v{h[0, *a, **kw] ||= 1})
assert_equal([[], {kw: 5}, nil], h.v{h[kw: 5] ||= 1})
assert_equal([[], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] ||= 1})
assert_equal([[], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] ||= 1})
assert_equal([[0], {kw: 5, a: 2}, nil], h.v{h[0, kw: 5, a: 2] ||= 1})
assert_equal([[0], {kw: 5, a: 2, nil: 3}, nil], h.v{h[0, *a, kw: 5, a: 2, nil: 3] ||= 1})

# ||=, with block, non-popped
assert_equal([[], {}, b], h.v{h[**kw, &b] ||= 1})
assert_equal([[0], {}, b], h.v{h[0, **kw, &b] ||= 1})
assert_equal([[0], {}, b], h.v{h[0, *a, **kw, &b] ||= 1})
assert_equal([[], {kw: 5}, b], h.v{h[kw: 5, &b] ||= 1})
assert_equal([[], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] ||= 1})
assert_equal([[], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] ||= 1})
assert_equal([[0], {kw: 5, a: 2}, b], h.v{h[0, kw: 5, a: 2, &b] ||= 1})
assert_equal([[0], {kw: 5, a: 2, b: 3}, b], h.v{h[0, *a, kw: 5, a: 2, b: 3, &b] ||= 1})

# ||=, without block, popped
assert_equal([[], {}, nil], h.v{h[**kw] ||= 1; nil})
assert_equal([[0], {}, nil], h.v{h[0, **kw] ||= 1; nil})
assert_equal([[0], {}, nil], h.v{h[0, *a, **kw] ||= 1; nil})
assert_equal([[], {kw: 5}, nil], h.v{h[kw: 5] ||= 1; nil})
assert_equal([[], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] ||= 1; nil})
assert_equal([[], {kw: 5, a: 2}, nil], h.v{h[kw: 5, a: 2] ||= 1; nil})
assert_equal([[0], {kw: 5, a: 2}, nil], h.v{h[0, kw: 5, a: 2] ||= 1; nil})
assert_equal([[0], {kw: 5, a: 2, nil: 3}, nil], h.v{h[0, *a, kw: 5, a: 2, nil: 3] ||= 1; nil})

# ||=, with block, popped
assert_equal([[], {}, b], h.v{h[**kw, &b] ||= 1; nil})
assert_equal([[0], {}, b], h.v{h[0, **kw, &b] ||= 1; nil})
assert_equal([[0], {}, b], h.v{h[0, *a, **kw, &b] ||= 1; nil})
assert_equal([[], {kw: 5}, b], h.v{h[kw: 5, &b] ||= 1; nil})
assert_equal([[], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] ||= 1; nil})
assert_equal([[], {kw: 5, a: 2}, b], h.v{h[kw: 5, a: 2, &b] ||= 1; nil})
assert_equal([[0], {kw: 5, a: 2}, b], h.v{h[0, kw: 5, a: 2, &b] ||= 1; nil})
assert_equal([[0], {kw: 5, a: 2, b: 3}, b], h.v{h[0, *a, kw: 5, a: 2, b: 3, &b] ||= 1; nil})

end

def test_kwsplat_block_order_op_asgn
o = Object.new
ary = []
o.define_singleton_method(:to_a) {ary << :to_a; []}
o.define_singleton_method(:to_hash) {ary << :to_hash; {}}
o.define_singleton_method(:to_proc) {ary << :to_proc; lambda{}}

def o.[](...) 2 end
def o.[]=(...) end

o[kw: 1] += 1
assert_equal([], ary)

o[**o] += 1
assert_equal([:to_hash], ary)

ary.clear
o[**o, &o] += 1
# to_proc called twice because no VM instruction for coercing to proc
assert_equal([:to_hash, :to_proc, :to_proc], ary)

ary.clear
o[*o, **o, &o] += 1
assert_equal([:to_a, :to_hash, :to_proc, :to_proc], ary)
end

def test_call_splat_order
bug12860 = '[ruby-core:77701] [Bug# 12860]'
ary = [1, 2]
Expand Down

0 comments on commit 2f1d6da

Please sign in to comment.