Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make iseq eval consistent with Kernel eval #2298

Closed
wants to merge 4 commits into from

Conversation

@dalehamel
Copy link

@dalehamel dalehamel commented Jul 23, 2019

This should fix https://bugs.ruby-lang.org/issues/12093, and is an additive change only.

This allows this code to work:

obj = Struct.new(:a, :b).new(1, 2)
bind = obj.instance_eval {binding}
RubyVM::InstructionSequence.compile("a + b").eval(bind) #=> 3

As I've verified with make test-all TESTS=test/ruby/test_iseq.rb

I think this should be added to help make the API with Kernel.eval be more consistent:

  • Using Kernel.eval, you can specify source code and a binding to evaluate it within
  • Using RubyVM::InstructionSequence, you can compile source code to an instruction sequence
  • You can execute source code within a binding, but you cannot execute compiled instruction sequences against a binding, so the features of Kernel.eval differ from iseq.eval

To fix this, the signature for iseq.eval is changed to allow for 0-1 arguments to be passed. I see this as keeping the same signature as Kernel.eval, but just dropping the first parameter as the target to evaluate is implicit as self, which I believe helps to keep the API consistent.

This new functionality should not add any performance penalty or code changes because 0 arguments are passed, the current behavior remains, and the current signature is preserved. If one argument is passed, then it is used as the binding, and attempts to mimic the behavior of vm_eval in its handling of the binding.

While the originally introduced eval_with can achieve the same thing, I think that doing this with eval is more consistent with the rest of the ruby API, and with Kernel.eval. The difference being that the thing to evaluate (the instruction sequence) is explicit via self rather than the first required parameter. This is the value I think over the original patch.

Based on original patch: trunk...nobu:feature/12093-iseq-eval_with, but I've updated it follow the conventions of https://github.com/ruby/ruby/blob/master/vm_eval.c#L1350-L1368

I've added a basic test that accepts the binding to the eval call, and verified the existing test continue to pass as i've only extended, not changed, the API here.

I have also updated the doc header for the method, and attempted to keep it consistent with the existing docs for Kernel.eval.

@nobu can you please take a look as this is based on your original patch and discussion in redmine.

Thanks @methodmissing for preliminary review

nobu and others added 3 commits Jul 23, 2019
* iseq.c (iseq_eval_with): RubyVM::InstructionSequence#eval_with.
  [Feature #12093]

* vm.c (rb_iseq_eval_in_scope): evaluate an iseq in the given
  scope.
This implements a solution to [Feature #12093], and builts upon Nobu's
patch from 2016 that started this support.

This makes the RubyVM::InstructionSequence eval API and Kernel.eval
be more consistent:

* Using Kernel.eval, you can specify source code and a binding
to evaluate it within
* Using RubyVM::InstructionSequence, you can compile source code to an
instruction sequence
* You can execute source code within a binding, but you cannot execute
compiled instruction sequences against a binding, so the features of
Kernel.eval differ from iseq.eval

To fix this, the signature for iseq.eval is changed to allow for 0-1
arguments to be passed. I see this as keeping the same signature as
Kernel.eval, but just dropping the first parameter as the target to
evaluate is implicit as self. This allows preserving the original
functionality and performance characteristics of evaluating an instruction
sequence, while also matching the functionality of Kernel.eval for evaluating
compiled ruby code in the context of a specified binding.

RubyVM::InstructionSequence#eval

* iseq.c (iseqw_eval): RubyVM::InstructionSequence#eval.
  [Feature #12093], updated from iseq_eval_with to be
  consistent with Kernel.eval

* vm.c (rb_iseq_eval_in_scope): evaluate an iseq in the given
  scope, updated for Ruby 2.7 rebase

* test/ruby/test_iseq.rb: Added tests for evaluating an iseq
      within the context of a binding
Copy link
Member

@nobu nobu left a comment

This issue is a subject of the next developers' meeting.

Loading

iseq.c Outdated
return rb_iseq_eval(iseqw_check(self));
VALUE scope;

if (argc == 0) {
Copy link
Member

@nobu nobu Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kernel#eval accepts as nil as the binding, so this method too, I think.

Loading

Copy link
Author

@dalehamel dalehamel Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, I checked the Kernel#eval implementation, and i think that just evaluating as if no binding was passed is the best behavior, so I put it in the same if branch

Loading

iseq.c Outdated
return rb_iseq_eval(iseqw_check(self));
}
else {
rb_scan_args(argc, argv, "01", &scope);
Copy link
Member

@nobu nobu Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With rb_check_arity(argc, 0, 1) then rb_secure(1), duplicate code can be reduced.

Loading

Copy link
Author

@dalehamel dalehamel Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, I'll update it to scan check for the arity and refactor it accordingly

Loading

vm.c Outdated

/* save new env */
if (iseq->body->local_table_size > 0) {
vm_bind_update_env(scope, bind, vm_make_env_object(ec, ec->cfp));
Copy link
Member

@nobu nobu Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indent is slipped.

Loading

Copy link
Author

@dalehamel dalehamel Aug 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies for the style error, I've fixed this.

Loading

@nobu
Copy link
Member

@nobu nobu commented Aug 15, 2019

Please add the reference to the ticket in the commit log.

Loading

@dalehamel
Copy link
Author

@dalehamel dalehamel commented Aug 15, 2019

Please add the reference to the ticket in the commit log.

I've added the bug URL to the commit message.

I believe I've addressed all comments, I've asked some of my colleagues to take a look as well.

Loading

@dalehamel dalehamel force-pushed the iseq-eval-binding branch 3 times, most recently from 94b741f to 66ac348 Aug 16, 2019
This implements a solution to [Feature #12093], and builts upon Nobu's
patch from 2016 that started this support.

This makes the RubyVM::InstructionSequence eval API and Kernel.eval
be more consistent:

* Using Kernel.eval, you can specify source code and a binding
to evaluate it within
* Using RubyVM::InstructionSequence, you can compile source code to an
instruction sequence
* You can execute source code within a binding, but you cannot execute
compiled instruction sequences against a binding, so the features of
Kernel.eval differ from iseq.eval

To fix this, the signature for iseq.eval is changed to allow for 0-1
arguments to be passed. I see this as keeping the same signature as
Kernel.eval, but just dropping the first parameter as the target to
evaluate is implicit as self. This allows preserving the original
functionality and performance characteristics of evaluating an instruction
sequence, while also matching the functionality of Kernel.eval for evaluating
compiled ruby code in the context of a specified binding.

RubyVM::InstructionSequence#eval

* iseq.c (iseqw_eval): RubyVM::InstructionSequence#eval.
  [Feature #12093], updated from iseq_eval_with to be
  consistent with Kernel.eval

* vm.c (rb_iseq_eval_in_scope): evaluate an iseq in the given
  scope, updated for Ruby 2.7 rebase

* test/ruby/test_iseq.rb: Added tests for evaluating an iseq
      within the context of a binding

See https://bugs.ruby-lang.org/issues/12093 for more details.
@dalehamel
Copy link
Author

@dalehamel dalehamel commented Aug 16, 2019

I realized that I missed a flaw described in the original issue https://bugs.ruby-lang.org/issues/12093#note-11 so this feature has problems when a local variable and method call have the same name, with the method call overshadowing a local variable with the same identifier from the binding, giving unexpected results.

Loading

@dalehamel
Copy link
Author

@dalehamel dalehamel commented Aug 16, 2019

The ticket is rejected

Loading

@dalehamel dalehamel closed this Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants