YJIT: Fix defined?(yield) and block_given? at top level#14208
YJIT: Fix defined?(yield) and block_given? at top level#14208XrXr merged 2 commits intoruby:masterfrom
defined?(yield) and block_given? at top level#14208Conversation
Previously, YJIT returned truthy for the block given query at the top level. That's incorrect because the top level script never receives a block, and `yield` is a syntax error there. Inside methods, the number of hops to get from `iseq` to `iseq->body->local_iseq` is the same as the number of `VM_ENV_PREV_EP(ep)` hops to get to an environment with `VM_ENV_FLAG_LOCAL`. YJIT and the interpreter both rely on this as can be seen in get_lvar_level(). However, this identity does not hold for the top level frame because of vm_set_eval_stack(), which sets up `TOPLEVEL_BINDING`. Since only methods can take a block that `yield` goes to, have ISEQs that are the child of a non-method ISEQ return falsy for the block given query. This fixes the issue for the top level script and is an optimization for non-method contexts such as inside `ISEQ_TYPE_CLASS`.
| // the current iseq or a parent. Only the "method" iseq type can be passed a | ||
| // block handler. (e.g. `yield` in the top level script is a syntax error.) |
There was a problem hiding this comment.
I was kind of confused about ISEQ_TYPE_BLOCK since you can pass a block to a block, but I guess we should still return false because the block_given? method refers to the local environment (LEP) that a block would inherit from a frame of non-ISEQ_TYPE_BLOCK ISEQ.
I'm also unsure what ISEQ_TYPE_MAIN (when there's already ISEQ_TYPE_TOP) and ISEQ_TYPE_PLAIN are, but the assumption probably holds for the rest of the types.
There was a problem hiding this comment.
It might be useful to have an assertion for debug builds in either the original C function or generated code to improve our confidence, but compatibility-wise it seems optional since it's passing tests anyway.
There was a problem hiding this comment.
I guess we should still return false because the block_given? method refers to the local environment (LEP) that a block would inherit from a frame of non-ISEQ_TYPE_BLOCK ISEQ.
Well, but then the LEP frame could indeed be ISEQ_TYPE_METHOD.
Does it support this kind of situation? (Is the { block_given? } ISEQ not a ISEQ_TYPE_BLOCK?)
irb(main)[01:0]> def foo = self.then { block_given? }
=> :foo
irb(main)[02:0]> foo
=> false
irb(main)[03:0]> foo {}
=> trueThere was a problem hiding this comment.
Does it support this kind of situation? (Is the { block_given? } ISEQ not a ISEQ_TYPE_BLOCK?)
Yes it works; lots of test code call block_given? from within a block. The block ISEQ in this example is indeed ISEQ_TYPE_BLOCK, but the ISEQ type check happens on block_iseq->body->local_iseq, not on block_iseq, and block_iseq->body->local_iseq->type == ISEQ_TYPE_METHOD.
There was a problem hiding this comment.
Sorry, I missed the rb_get_iseq_body_local_iseq portion. Thanks for the explanation!
* Originally, k0kubun added a change to respect namespace in gen_block_given ruby@d129669 * Just after the change, XrXr proposes a change on master and he says it can fix the problem around block_given?, and it has been merged into the master ruby#14208 * tagomoris respect the commit on master then will see if it works
* Originally, k0kubun added a change to respect namespace in gen_block_given ruby@d129669 * Just after the change, XrXr proposes a change on master and he says it can fix the problem around block_given?, and it has been merged into the master ruby#14208 * tagomoris respect the commit on master then will see if it works
* Originally, k0kubun added a change to respect namespace in gen_block_given ruby@d129669 * Just after the change, XrXr proposes a change on master and he says it can fix the problem around block_given?, and it has been merged into the master ruby#14208 * tagomoris respect the commit on master then will see if it works
* Originally, k0kubun added a change to respect namespace in gen_block_given d129669 * Just after the change, XrXr proposes a change on master and he says it can fix the problem around block_given?, and it has been merged into the master #14208 * tagomoris respect the commit on master then will see if it works
File backport:
Previously, YJIT returned truthy for the block given query at the top
level. That's incorrect because the top level script never receives a
block, and
yieldis a syntax error there.Inside methods, the number of hops to get from
iseqtoiseq->body->local_iseqis the same as the number ofVM_ENV_PREV_EP(ep)hops to get to an environment withVM_ENV_FLAG_LOCAL. YJIT and the interpreter both rely on this as canbe seen in get_lvar_level(). However, this identity does not hold for
the top level frame because of vm_set_eval_stack(), which sets up
TOPLEVEL_BINDING.Since only methods can take a block that
yieldgoes to, have ISEQsthat are the child of a non-method ISEQ return falsy for the block given
query. This fixes the issue for the top level script and is an
optimization for non-method contexts such as inside
ISEQ_TYPE_CLASS.I think this also fixes the issue in #13454. There's no need for a runtime check.