Skip to content

Fixed local variables not separated between copied binding objects#5517

Merged
matz merged 1 commit into
mruby:masterfrom
dearblue:binding.4
Mar 14, 2023
Merged

Fixed local variables not separated between copied binding objects#5517
matz merged 1 commit into
mruby:masterfrom
dearblue:binding.4

Conversation

@dearblue

Copy link
Copy Markdown
Contributor

This is because I forgot to implement the Binding#initialize_copy method.

@dearblue dearblue requested a review from matz as a code owner July 25, 2021 07:37
@matz

matz commented Jul 26, 2021

Copy link
Copy Markdown
Member

Hmm, the current behavior seems to comply CRuby behavior. Do we need to fix it?

@dearblue

Copy link
Copy Markdown
Contributor Author

I'm sorry, it may have been a confusing expression.
Can you see the example below?

  • Ruby 3.0.1

    % ruby30 -e 'b1 = binding; b2 = b1.dup; b1.eval("x = 1"); p b2.eval("x")'
    (eval):1:in `<main>': undefined local variable or method `x' for main:Object (NameError)
            from -e:1:in `eval'
            from -e:1:in `<main>'
  • current mruby head (1f1dca1)

    % bin/mruby -e 'b1 = binding; b2 = b1.dup; b1.eval("x = 1"); p b2.eval("x")'
    1
  • this PR

    % bin/mruby -e 'b1 = binding; b2 = b1.dup; b1.eval("x = 1"); p b2.eval("x")'
    trace (most recent call last):
            [1] -e:1
    (eval):1:in eval: undefined method 'x' (NoMethodError)

@matz

matz commented Jul 27, 2021

Copy link
Copy Markdown
Member

OK, I understand the need for the change. But still, we need to check the following behavior:

x = 5
bind1 = binding
bind1.local_variable_set(:y, 10)
bind2 = bind1.dup
p bind2.local_variable_get(:x)      #=> 5
p bind2.local_variable_get(:y)      #=> 10
bind1.local_variable_set(:y, 20) 
p bind2.local_variable_get(:y)      #=> 20 (from CRuby, mruby, 10 from the PR)
p bind1.local_variable_get(:y)      #=> 20 
x = 50
p bind1.local_variable_get(:x)      #=> 50
p bind2.local_variable_get(:x)      #=> 50

@dearblue

Copy link
Copy Markdown
Contributor Author

I'm sorry again, I didn't confirm enough.
For now, the solution is to create a proc object for the origin binding object with the Binding#initialize_copy method.

binding dup

But there is a concern, and if it repeats binding.dup.dup.dup.dup....., the proc gets deeper and deeper. Therefore, I think it is necessary to perform two more processes.

  • (1) If there is no local variable, do not link to origin proc
  • (2) If proc->upper seems to be too deep, raise an exception as "mruby limit"

Please give me some time...

@matz

matz commented Jul 28, 2021

Copy link
Copy Markdown
Member

OK.

Comment on lines +10 to +24
#define BINDING_UPPER_DEFAULT 20
#define BINDING_UPPER_MINIMUM 10
#define BINDING_UPPER_MAXIMUM 100

#ifndef MRB_BINDING_UPPER_MAX
# define BINDING_UPPER_MAX BINDING_UPPER_DEFAULT
#else
# define BINDING_UPPER_MAX ((MRB_BINDING_UPPER_MAX) > BINDING_UPPER_MAXIMUM ? BINDING_UPPER_MAXIMUM : \
(MRB_BINDING_UPPER_MAX) < BINDING_UPPER_MINIMUM ? BINDING_UPPER_MINIMUM : \
(MRB_BINDING_UPPER_MAX))
#endif

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The user can define the depth of the proc with the MRB_BINDING_UPPER_MAX constant macro.

% cat bindingtest.rb
bx = [binding]
50.times { |i|
  begin
    bx[-1].eval("var#{i} = #{i}")
    bx << bx[-1].dup
  rescue => e
    p [i, e]
    break
  end
}

% bin/mruby bindingtest.rb
[19, too many upper procs for local variables (mruby limitation; maximum is 20) (RuntimeError)]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This part was wrong.
If MRB_BINDING_UPPER_MAX is defined, the ?: operator is printed as is in the error message.
I will fix this.

This is because I forgot to implement the `Binding#initialize_copy` method.
@matz matz merged commit 6915e75 into mruby:master Mar 14, 2023
@dearblue dearblue deleted the binding.4 branch March 14, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants