Fix #3356: Copy value of ensure-protected body in a tmp and return it

* My memory of IR details are a bit fuzzy now since I have been away for so long, but this should work. * Updated internal copy of JRuby rubyspec with new expectations.
jruby · Oct 9, 2015 · 89efda6 · 89efda6
1 parent e24b6e2
commit 89efda6
Showing 2 changed files with 31 additions and 4 deletions.
diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -2246,15 +2246,20 @@ public Operand buildEnsureInternal(Node ensureBodyNode, Node ensurerNode) {
         activeRescuers.push(ebi.dummyRescueBlockLabel);
 
         // Generate IR for code being protected
+        Variable ensureExprValue = createTemporaryVariable();
         Operand rv = ensureBodyNode instanceof RescueNode ? buildRescueInternal((RescueNode) ensureBodyNode, ebi) : build(ensureBodyNode);
 
         // End of protected region
         addInstr(new ExceptionRegionEndMarkerInstr());
         activeRescuers.pop();
 
-        // Clone the ensure body and jump to the end.  Don't bother if the protected body ended in a return
-        // OR if we are really processing a rescue node
-        if (ensurerNode != null && rv != U_NIL && !(ensureBodyNode instanceof RescueNode)) {
+        // Is this a begin..(rescue..)?ensure..end node that actually computes a value?
+        // (vs. returning from protected body)
+        boolean isEnsureExpr = ensurerNode != null && rv != U_NIL && !(ensureBodyNode instanceof RescueNode);
+
+        // Clone the ensure body and jump to the end
+        if (isEnsureExpr) {
+            addInstr(new CopyInstr(ensureExprValue, rv));
             ebi.cloneIntoHostScope(this);
             addInstr(new JumpInstr(ebi.end));
         }
@@ -2290,7 +2295,7 @@ public Operand buildEnsureInternal(Node ensureBodyNode, Node ensurerNode) {
         // End label for the exception region
         addInstr(new LabelInstr(ebi.end));
 
-        return rv;
+        return isEnsureExpr ? ensureExprValue : rv;
     }
 
     public Operand buildEvStr(EvStrNode node) {

diff --git a/spec/ruby/language/ensure_spec.rb b/spec/ruby/language/ensure_spec.rb
@@ -74,6 +74,28 @@
   end
 end
 
+describe "The value of an ensure expression," do
+  it "in no-exception scenarios, is the value of the last stmt of the protected body" do
+    begin
+      v = 1
+      eval('x=1') # to prevent opts from triggering
+      v
+    ensure
+      v = 2
+    end.should == 1
+  end
+
+  it "when an exception is rescued, is the value of the rescuing block" do
+    begin
+      raise 'foo'
+    rescue
+      v = 3
+    ensure
+      v = 2
+    end.should == 3
+  end
+end
+
 describe "An ensure block inside a method" do
   before :each do
     @obj = EnsureSpec::Container.new