From f304deff219f3a214f301ea978143a682f840496 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Thu, 23 Feb 2023 15:47:00 -0600 Subject: [PATCH 1/7] Try and appease the eval+binding gods When we eval with binding we will construct a new binding that contains the original binding and that original binding will construct a new evalDynamicScope. If I do something like: ```ruby p TOPLEVEL_BINDING.local_variable_defined? :c p TOPLEVEL_BINDING.local_variable_defined? :_xxx_var_ b =TOPLEVEL_BINDING.dup p b.local_variable_defined? :c p b.local_variable_defined? :_xxx_var_ ``` I should still see _xxx_var_ after the dup. This will make that do that but two different RubyBinding instances will end up with the exact same backing Binding and I think this is wrong. Landing anyways to see if anything breaks. --- core/src/main/java/org/jruby/RubyBinding.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/jruby/RubyBinding.java b/core/src/main/java/org/jruby/RubyBinding.java index 76987c0c62e..778f304d025 100644 --- a/core/src/main/java/org/jruby/RubyBinding.java +++ b/core/src/main/java/org/jruby/RubyBinding.java @@ -104,7 +104,12 @@ public IRubyObject initialize(ThreadContext context) { return this; } - + + @JRubyMethod + public IRubyObject dup(ThreadContext context) { + return newBinding(context.runtime, binding); + } + @JRubyMethod(name = "initialize_copy", required = 1, visibility = Visibility.PRIVATE) @Override public IRubyObject initialize_copy(IRubyObject other) { From 4c1473a794f2cd596bf9e29de9ac3ba58a501cdc Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Wed, 1 May 2024 14:14:19 -0400 Subject: [PATCH 2/7] shallow clone of binding happen in Binding#dup --- core/src/main/java/org/jruby/RubyBinding.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/jruby/RubyBinding.java b/core/src/main/java/org/jruby/RubyBinding.java index 607a821014b..7ef4618d7d8 100644 --- a/core/src/main/java/org/jruby/RubyBinding.java +++ b/core/src/main/java/org/jruby/RubyBinding.java @@ -108,7 +108,7 @@ public IRubyObject initialize(ThreadContext context) { @JRubyMethod public IRubyObject dup(ThreadContext context) { - return newBinding(context.runtime, binding); + return newBinding(context.runtime, binding.clone()); } @JRubyMethod(name = "initialize_copy", visibility = Visibility.PRIVATE) From d994845e08ff1c9a2aef1d12f70599b980b67e98 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Wed, 1 May 2024 14:49:24 -0400 Subject: [PATCH 3/7] dup will dup binding state already saved so it will keep variable values but they will be their own thing from that point onward --- core/src/main/java/org/jruby/RubyBinding.java | 2 +- core/src/main/java/org/jruby/parser/StaticScope.java | 2 +- core/src/main/java/org/jruby/runtime/Binding.java | 7 +++++++ core/src/main/java/org/jruby/runtime/DynamicScope.java | 4 ++++ 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/jruby/RubyBinding.java b/core/src/main/java/org/jruby/RubyBinding.java index 7ef4618d7d8..ce36b1944ca 100644 --- a/core/src/main/java/org/jruby/RubyBinding.java +++ b/core/src/main/java/org/jruby/RubyBinding.java @@ -108,7 +108,7 @@ public IRubyObject initialize(ThreadContext context) { @JRubyMethod public IRubyObject dup(ThreadContext context) { - return newBinding(context.runtime, binding.clone()); + return newBinding(context.runtime, binding.dup(context)); } @JRubyMethod(name = "initialize_copy", visibility = Visibility.PRIVATE) diff --git a/core/src/main/java/org/jruby/parser/StaticScope.java b/core/src/main/java/org/jruby/parser/StaticScope.java index 1b37fac8a3a..fb50733084d 100644 --- a/core/src/main/java/org/jruby/parser/StaticScope.java +++ b/core/src/main/java/org/jruby/parser/StaticScope.java @@ -76,7 +76,7 @@ * will point to the previous scope of the enclosing module/class (cref). * */ -public class StaticScope implements Serializable { +public class StaticScope implements Serializable, Cloneable { public static final int MAX_SPECIALIZED_SIZE = 50; private static final long serialVersionUID = 3423852552352498148L; diff --git a/core/src/main/java/org/jruby/runtime/Binding.java b/core/src/main/java/org/jruby/runtime/Binding.java index 2be702dd3fa..1cfeaa92f9c 100644 --- a/core/src/main/java/org/jruby/runtime/Binding.java +++ b/core/src/main/java/org/jruby/runtime/Binding.java @@ -316,4 +316,11 @@ public BacktraceElement getBacktrace() { // Can't use Frame.DUMMY because of circular static init seeing it before it's assigned new Frame(), Visibility.PUBLIC); + + public Binding dup(ThreadContext context) { + Binding newBinding = new Binding(this); + DynamicScope scope = getEvalScope(context.runtime); + newBinding.evalScope = scope.dup(); + return newBinding; + } } diff --git a/core/src/main/java/org/jruby/runtime/DynamicScope.java b/core/src/main/java/org/jruby/runtime/DynamicScope.java index 01ed6d18294..611db598afd 100644 --- a/core/src/main/java/org/jruby/runtime/DynamicScope.java +++ b/core/src/main/java/org/jruby/runtime/DynamicScope.java @@ -598,4 +598,8 @@ public DynamicScope cloneScope() { throw new RuntimeException("BUG: failed to clone scope type " + getClass().getName()); } } + + public DynamicScope dup() { + return newDynamicScope(staticScope.duplicate(), parent); + } } From 2b9b9d9a230742944f8ffc2b582091817ea4b3be Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Wed, 1 May 2024 15:30:20 -0400 Subject: [PATCH 4/7] bindings can change size..just making sure this fixes issues. I likely will rename this from dup --- core/src/main/java/org/jruby/runtime/DynamicScope.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/jruby/runtime/DynamicScope.java b/core/src/main/java/org/jruby/runtime/DynamicScope.java index 611db598afd..5b60f095213 100644 --- a/core/src/main/java/org/jruby/runtime/DynamicScope.java +++ b/core/src/main/java/org/jruby/runtime/DynamicScope.java @@ -32,6 +32,7 @@ import org.jruby.ir.JIT; import org.jruby.parser.StaticScope; import org.jruby.runtime.builtin.IRubyObject; +import org.jruby.runtime.scope.ManyVarsDynamicScope; public abstract class DynamicScope implements Cloneable { // Static scoping information for this scope @@ -600,6 +601,6 @@ public DynamicScope cloneScope() { } public DynamicScope dup() { - return newDynamicScope(staticScope.duplicate(), parent); + return new ManyVarsDynamicScope(staticScope.duplicate(), parent); } } From 0cc945b203133dc21842fb038a84dbc182850f7b Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Wed, 1 May 2024 16:10:15 -0400 Subject: [PATCH 5/7] dup'd binding copy all existing values into new eval scope --- core/src/main/java/org/jruby/runtime/DynamicScope.java | 7 ++++++- spec/tags/ruby/core/kernel/eval_tags.txt | 2 +- spec/tags/ruby/library/erb/result_tags.txt | 1 - spec/tags/ruby/library/erb/run_tags.txt | 1 - 4 files changed, 7 insertions(+), 4 deletions(-) delete mode 100644 spec/tags/ruby/library/erb/result_tags.txt delete mode 100644 spec/tags/ruby/library/erb/run_tags.txt diff --git a/core/src/main/java/org/jruby/runtime/DynamicScope.java b/core/src/main/java/org/jruby/runtime/DynamicScope.java index 5b60f095213..a1d75f80426 100644 --- a/core/src/main/java/org/jruby/runtime/DynamicScope.java +++ b/core/src/main/java/org/jruby/runtime/DynamicScope.java @@ -601,6 +601,11 @@ public DynamicScope cloneScope() { } public DynamicScope dup() { - return new ManyVarsDynamicScope(staticScope.duplicate(), parent); + DynamicScope newScope = new ManyVarsDynamicScope(staticScope.duplicate(), parent); + IRubyObject[] values = getValues(); + for (int i = 0; i < values.length; i++) { + newScope.setValueDepthZero(values[i], i); + } + return newScope; } } diff --git a/spec/tags/ruby/core/kernel/eval_tags.txt b/spec/tags/ruby/core/kernel/eval_tags.txt index da14ef98460..ad3754bf056 100644 --- a/spec/tags/ruby/core/kernel/eval_tags.txt +++ b/spec/tags/ruby/core/kernel/eval_tags.txt @@ -1,3 +1,3 @@ fails:Kernel#eval raises a LocalJumpError if there is no lambda-style closure in the chain fails:Kernel#eval with a magic encoding comment ignores the magic encoding comment if it is after a frozen_string_literal magic comment -fails:Kernel#eval makes flip-flop operator work correctly + diff --git a/spec/tags/ruby/library/erb/result_tags.txt b/spec/tags/ruby/library/erb/result_tags.txt deleted file mode 100644 index 0607580c953..00000000000 --- a/spec/tags/ruby/library/erb/result_tags.txt +++ /dev/null @@ -1 +0,0 @@ -fails:ERB#result use TOPLEVEL_BINDING if binding is not passed diff --git a/spec/tags/ruby/library/erb/run_tags.txt b/spec/tags/ruby/library/erb/run_tags.txt deleted file mode 100644 index 47996d0b847..00000000000 --- a/spec/tags/ruby/library/erb/run_tags.txt +++ /dev/null @@ -1 +0,0 @@ -fails:ERB#run use TOPLEVEL_BINDING if binding is not passed From eb73e3b3708fdde9d80159c030f2f4df03dba9bf Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Thu, 2 May 2024 09:49:57 -0400 Subject: [PATCH 6/7] some docs and speed up value copying --- core/src/main/java/org/jruby/runtime/Binding.java | 9 +++++++-- .../main/java/org/jruby/runtime/DynamicScope.java | 13 +++++++------ .../jruby/runtime/scope/ManyVarsDynamicScope.java | 7 +++++++ 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/jruby/runtime/Binding.java b/core/src/main/java/org/jruby/runtime/Binding.java index 1cfeaa92f9c..7a4b24ace7a 100644 --- a/core/src/main/java/org/jruby/runtime/Binding.java +++ b/core/src/main/java/org/jruby/runtime/Binding.java @@ -317,10 +317,15 @@ public BacktraceElement getBacktrace() { new Frame(), Visibility.PUBLIC); + /** + * Duplicate this binding and setup the proper cloned instance of the eval scope so that any previously + * captured variables still exist but are not shared with the original binding. + * @param context the current thread context + * @return the duplicated binding + */ public Binding dup(ThreadContext context) { Binding newBinding = new Binding(this); - DynamicScope scope = getEvalScope(context.runtime); - newBinding.evalScope = scope.dup(); + newBinding.evalScope = getEvalScope(context.runtime).dupEvalScope(); return newBinding; } } diff --git a/core/src/main/java/org/jruby/runtime/DynamicScope.java b/core/src/main/java/org/jruby/runtime/DynamicScope.java index a1d75f80426..30f6f43808d 100644 --- a/core/src/main/java/org/jruby/runtime/DynamicScope.java +++ b/core/src/main/java/org/jruby/runtime/DynamicScope.java @@ -600,12 +600,13 @@ public DynamicScope cloneScope() { } } - public DynamicScope dup() { - DynamicScope newScope = new ManyVarsDynamicScope(staticScope.duplicate(), parent); - IRubyObject[] values = getValues(); - for (int i = 0; i < values.length; i++) { - newScope.setValueDepthZero(values[i], i); - } + /** + * Binding needs to clone its scope with all the current values. + * @return a duplicate of this scope with all the current values + */ + public DynamicScope dupEvalScope() { + ManyVarsDynamicScope newScope = new ManyVarsDynamicScope(staticScope.duplicate(), parent); + newScope.setVariableValues(getValues()); return newScope; } } diff --git a/core/src/main/java/org/jruby/runtime/scope/ManyVarsDynamicScope.java b/core/src/main/java/org/jruby/runtime/scope/ManyVarsDynamicScope.java index ea568c26d1a..2f53433fcbe 100644 --- a/core/src/main/java/org/jruby/runtime/scope/ManyVarsDynamicScope.java +++ b/core/src/main/java/org/jruby/runtime/scope/ManyVarsDynamicScope.java @@ -50,6 +50,13 @@ private void allocate() { if(variableValues == null) variableValues = IRubyObject.array(staticScope.getNumberOfVariables()); } + // WARNING: This is used when dup'ing an eval scope. We know the size and that it will 100% always be + // a ManyVarsDynamicScope. This should not be used by anything else. If there ever ends up being another + // use then it should be documented in this warning. + public void setVariableValues(IRubyObject[] variableValues) { + this.variableValues = variableValues; + } + public IRubyObject[] getValues() { return variableValues; } From 1fdac51616db076cca8a215d17c01465a25bb920 Mon Sep 17 00:00:00 2001 From: "Thomas E. Enebo" Date: Thu, 2 May 2024 10:29:42 -0400 Subject: [PATCH 7/7] Add spec to more explicitly show binding#dup behavior --- spec/ruby/core/binding/dup_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/spec/ruby/core/binding/dup_spec.rb b/spec/ruby/core/binding/dup_spec.rb index 55fac6e3332..f30d63b708f 100644 --- a/spec/ruby/core/binding/dup_spec.rb +++ b/spec/ruby/core/binding/dup_spec.rb @@ -10,4 +10,21 @@ bind.frozen?.should == true bind.dup.frozen?.should == false end + + it "retains original binding variables but the list is distinct" do + bind1 = binding + eval "a = 1", bind1 + + bind2 = bind1.dup + eval("a = 2", bind2) + eval("a", bind1).should == 2 + eval("a", bind2).should == 2 + + eval("b = 2", bind2) + -> { eval("b", bind1) }.should raise_error(NameError) + eval("b", bind2).should == 2 + + bind1.local_variables.sort.should == [:a, :bind1, :bind2] + bind2.local_variables.sort.should == [:a, :b, :bind1, :bind2] + end end