[GR-29687] Fix control flow bug when assigning constants using ||= (#1489)

PullRequest: truffleruby/2448

Author: Nicolas Laurent

CHANGELOG.md

@@ -20,6 +20,7 @@ Bug fixes:
 * Fix `Truffle::Interop.source_location` to return unavailable source sections for modules instead of null (#2257).
 * Fix usage of `Thread.handle_interrupt` in `MonitorMixin#mon_synchronize`.
 * Fixed `TruffleRuby.synchronized` to handle guest safepoints (#2277).
+* Fix control flow bug when assigning constants using ||= (#1489).
 
 Compatibility:
 
@@ -87,7 +88,7 @@ Changes:
 
 Release notes:
 
-* The new IRB is quite slow when copy/pasting code into it. This is due to an inefficient `io/console` implementation which will be addressed in the next release. A workaround is to use `irb --readline`, which disables some IRB features but is much faster for copy/pasting code. 
+* The new IRB is quite slow when copy/pasting code into it. This is due to an inefficient `io/console` implementation which will be addressed in the next release. A workaround is to use `irb --readline`, which disables some IRB features but is much faster for copy/pasting code.
 
 New features:
 
@@ -184,7 +185,7 @@ Compatibility:
 * Pass the final `super` specs (#2104, @chrisseaton).
 * Fix arity for arguments with optional kwargs (#1669, @ssnickolay)
 * Fix arity for `Proc` (#2098, @ssnickolay)
-* Check bounds for `FFI::Pointer` accesses when the size of the memory behind is known. 
+* Check bounds for `FFI::Pointer` accesses when the size of the memory behind is known.
 * Implement negative line numbers for eval (#1482).
 * Support refinements for `#to_s` called by string interpolation (#2110, @ssnickolay)
 * Module#using raises error in method scope (#2112, @ssnickolay)

spec/mspec/lib/mspec/helpers.rb

@@ -10,4 +10,4 @@
 require 'mspec/helpers/ruby_exe'
 require 'mspec/helpers/scratch'
 require 'mspec/helpers/tmp'
-require 'mspec/helpers/warning'
+require 'mspec/helpers/warning'

spec/mspec/lib/mspec/helpers/warning.rb

@@ -1,5 +1,7 @@
 require 'mspec/guards/version'
 
+# You might be looking for #silence_warnings, use #suppress_warning instead.
+# MSpec calls it #suppress_warning for consistency with EnvUtil.suppress_warning in CRuby test/.
 def suppress_warning
   verbose = $VERBOSE
   $VERBOSE = nil

spec/ruby/language/constants_spec.rb

@@ -381,52 +381,6 @@ class << self
     ConstantSpecs::ClassA.constx.should == :CS_CONSTX
     ConstantSpecs::ClassA.new.constx.should == :CS_CONSTX
   end
-
-  describe "with ||=" do
-    it "assigns a scoped constant if previously undefined" do
-      ConstantSpecs.should_not have_constant(:OpAssignUndefined)
-      module ConstantSpecs
-        OpAssignUndefined ||= 42
-      end
-      ConstantSpecs::OpAssignUndefined.should == 42
-      ConstantSpecs::OpAssignUndefinedOutside ||= 42
-      ConstantSpecs::OpAssignUndefinedOutside.should == 42
-      ConstantSpecs.send(:remove_const, :OpAssignUndefined)
-      ConstantSpecs.send(:remove_const, :OpAssignUndefinedOutside)
-    end
-
-    it "assigns a global constant if previously undefined" do
-      OpAssignGlobalUndefined ||= 42
-      ::OpAssignGlobalUndefinedExplicitScope ||= 42
-      OpAssignGlobalUndefined.should == 42
-      ::OpAssignGlobalUndefinedExplicitScope.should == 42
-      Object.send :remove_const, :OpAssignGlobalUndefined
-      Object.send :remove_const, :OpAssignGlobalUndefinedExplicitScope
-    end
-
-  end
-
-  describe "with &&=" do
-    it "re-assigns a scoped constant if already true" do
-      module ConstantSpecs
-        OpAssignTrue = true
-      end
-      suppress_warning do
-        ConstantSpecs::OpAssignTrue &&= 1
-      end
-      ConstantSpecs::OpAssignTrue.should == 1
-      ConstantSpecs.send :remove_const, :OpAssignTrue
-    end
-
-    it "leaves scoped constant if not true" do
-      module ConstantSpecs
-        OpAssignFalse = false
-      end
-      ConstantSpecs::OpAssignFalse &&= 1
-      ConstantSpecs::OpAssignFalse.should == false
-      ConstantSpecs.send :remove_const, :OpAssignFalse
-    end
-  end
 end
 
 describe "Constant resolution within a singleton class (class << obj)" do
@@ -775,4 +729,4 @@ class PrivateClass
       eval("mod::ἍBB").should == 1
     end
   end
-end
+end

spec/ruby/language/optional_assignments_spec.rb

@@ -1,4 +1,5 @@
 require_relative '../spec_helper'
+require_relative '../fixtures/constants'
 
 describe 'Optional variable assignments' do
   describe 'using ||=' do
@@ -351,3 +352,107 @@ def []=(k, v)
     end
   end
 end
+
+describe 'Optional constant assignment' do
+  describe 'with ||=' do
+    it "assigns a scoped constant if previously undefined" do
+      ConstantSpecs.should_not have_constant(:OpAssignUndefined)
+      module ConstantSpecs
+        OpAssignUndefined ||= 42
+      end
+      ConstantSpecs::OpAssignUndefined.should == 42
+      ConstantSpecs::OpAssignUndefinedOutside ||= 42
+      ConstantSpecs::OpAssignUndefinedOutside.should == 42
+      ConstantSpecs.send(:remove_const, :OpAssignUndefined)
+      ConstantSpecs.send(:remove_const, :OpAssignUndefinedOutside)
+    end
+
+    it "assigns a global constant if previously undefined" do
+      OpAssignGlobalUndefined ||= 42
+      ::OpAssignGlobalUndefinedExplicitScope ||= 42
+      OpAssignGlobalUndefined.should == 42
+      ::OpAssignGlobalUndefinedExplicitScope.should == 42
+      Object.send :remove_const, :OpAssignGlobalUndefined
+      Object.send :remove_const, :OpAssignGlobalUndefinedExplicitScope
+    end
+
+    it 'correctly defines non-existing constants' do
+      ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT1 ||= :assigned
+      ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT1.should == :assigned
+    end
+
+    it 'correctly overwrites nil constants' do
+      suppress_warning do # already initialized constant
+      ConstantSpecs::ClassA::NIL_OR_ASSIGNED_CONSTANT1 = nil
+      ConstantSpecs::ClassA::NIL_OR_ASSIGNED_CONSTANT1 ||= :assigned
+      ConstantSpecs::ClassA::NIL_OR_ASSIGNED_CONSTANT1.should == :assigned
+      end
+    end
+
+    it 'causes side-effects of the module part to be applied only once (for undefined constant)' do
+      x = 0
+      (x += 1; ConstantSpecs::ClassA)::OR_ASSIGNED_CONSTANT2 ||= :assigned
+      x.should == 1
+      ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT2.should == :assigned
+    end
+
+    it 'causes side-effects of the module part to be applied (for nil constant)' do
+      suppress_warning do # already initialized constant
+      ConstantSpecs::ClassA::NIL_OR_ASSIGNED_CONSTANT2 = nil
+      x = 0
+      (x += 1; ConstantSpecs::ClassA)::NIL_OR_ASSIGNED_CONSTANT2 ||= :assigned
+      x.should == 1
+      ConstantSpecs::ClassA::NIL_OR_ASSIGNED_CONSTANT2.should == :assigned
+      end
+    end
+
+    it 'does not evaluate the right-hand side if the module part raises an exception (for undefined constant)' do
+      x = 0
+      y = 0
+
+      -> {
+        (x += 1; raise Exception; ConstantSpecs::ClassA)::OR_ASSIGNED_CONSTANT3 ||= (y += 1; :assigned)
+      }.should raise_error(Exception)
+
+      x.should == 1
+      y.should == 0
+      defined?(ConstantSpecs::ClassA::OR_ASSIGNED_CONSTANT3).should == nil
+    end
+
+    it 'does not evaluate the right-hand side if the module part raises an exception (for nil constant)' do
+      ConstantSpecs::ClassA::NIL_OR_ASSIGNED_CONSTANT3 = nil
+      x = 0
+      y = 0
+
+      -> {
+        (x += 1; raise Exception; ConstantSpecs::ClassA)::NIL_OR_ASSIGNED_CONSTANT3 ||= (y += 1; :assigned)
+      }.should raise_error(Exception)
+
+      x.should == 1
+      y.should == 0
+      ConstantSpecs::ClassA::NIL_OR_ASSIGNED_CONSTANT3.should == nil
+    end
+  end
+
+  describe "with &&=" do
+    it "re-assigns a scoped constant if already true" do
+      module ConstantSpecs
+        OpAssignTrue = true
+      end
+      suppress_warning do
+        ConstantSpecs::OpAssignTrue &&= 1
+      end
+      ConstantSpecs::OpAssignTrue.should == 1
+      ConstantSpecs.send :remove_const, :OpAssignTrue
+    end
+
+    it "leaves scoped constant if not true" do
+      module ConstantSpecs
+        OpAssignFalse = false
+      end
+      ConstantSpecs::OpAssignFalse &&= 1
+      ConstantSpecs::OpAssignFalse.should == false
+      ConstantSpecs.send :remove_const, :OpAssignFalse
+    end
+  end
+end

src/main/java/org/truffleruby/language/constants/AutoloadException.java

@@ -0,0 +1,26 @@
+/*
+ * Copyright (c) 2021 Oracle and/or its affiliates. All rights reserved. This
+ * code is released under a tri EPL/GPL/LGPL license. You can use it,
+ * redistribute it and/or modify it under the terms of the:
+ *
+ * Eclipse Public License version 2.0, or
+ * GNU General Public License version 2, or
+ * GNU Lesser General Public License version 2.1.
+ */
+package org.truffleruby.language.constants;
+
+import com.oracle.truffle.api.exception.AbstractTruffleException;
+import org.truffleruby.language.control.RaiseException;
+
+/** Wraps a {@link RaiseException} occuring during a module autoload, whenever the autoloaded constant is itself the
+ * module for another constant read. */
+public class AutoloadException extends AbstractTruffleException {
+
+    private static final long serialVersionUID = 1L;
+
+    public final RaiseException raiseException;
+
+    public AutoloadException(RaiseException raiseException) {
+        this.raiseException = raiseException;
+    }
+}

src/main/java/org/truffleruby/language/constants/OrAssignConstantNode.java

@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) 2021 Oracle and/or its affiliates. All rights reserved. This
+ * code is released under a tri EPL/GPL/LGPL license. You can use it,
+ * redistribute it and/or modify it under the terms of the:
+ *
+ * Eclipse Public License version 2.0, or
+ * GNU General Public License version 2, or
+ * GNU Lesser General Public License version 2.1.
+ */
+package org.truffleruby.language.constants;
+
+import com.oracle.truffle.api.CompilerDirectives;
+import com.oracle.truffle.api.frame.VirtualFrame;
+import com.oracle.truffle.api.profiles.ConditionProfile;
+import org.truffleruby.core.cast.BooleanCastNode;
+import org.truffleruby.core.cast.BooleanCastNodeGen;
+import org.truffleruby.core.module.RubyModule;
+import org.truffleruby.language.RubyConstant;
+import org.truffleruby.language.RubyContextSourceNode;
+import org.truffleruby.utils.RunTwiceBranchProfile;
+
+/** e.g. for {@code module::FOO ||= 1}
+ *
+ * <p>
+ * We need a separate class for this because we need to check if the constant is defined. Doing so will evaluate the
+ * module part, which will be evaluated again when assigning the constant. If evaluating the module part has any
+ * side-effect, this is incorrect and differs from MRI semantics. */
+public class OrAssignConstantNode extends RubyContextSourceNode {
+
+    @Child protected ReadConstantNode readConstant;
+    @Child protected WriteConstantNode writeConstant;
+    @Child private BooleanCastNode cast;
+
+    private final ConditionProfile triviallyUndefined = ConditionProfile.create();
+    private final ConditionProfile defined = ConditionProfile.create();
+    private final RunTwiceBranchProfile writeTwiceProfile = new RunTwiceBranchProfile();
+
+    public OrAssignConstantNode(ReadConstantNode readConstant, WriteConstantNode writeConstant) {
+        this.readConstant = readConstant;
+        this.writeConstant = writeConstant;
+    }
+
+    @Override
+    public Object execute(VirtualFrame frame) {
+
+        if (triviallyUndefined.profile(readConstant.isModuleTriviallyUndefined(frame, getLanguage(), getContext()))) {
+            // It might not be defined because of autoloaded constants (maybe other reasons?),
+            // simply attempt writing (which will trigger autoloads if required).
+            // Since we didn't evaluate the module part yet, no side-effects can occur.
+            writeTwiceProfile.enter();
+            return writeConstant.execute(frame);
+        }
+
+        // Conceptually, we want to rewrite `<x>::Foo ||= <y>` to `(defined?(<x>) && <x>) || (<x>::Foo = <y>)`
+        // BUT, we want the side-effects of <x> (the module part) to be only triggered once.
+        // We do let any exception raised bubble through. Normally they would be swallowed by `defined?`, but
+        // MRI raises them anyway, *before* evaluation the right-hand side (which is a different behaviour from
+        // regular constant assignment, which evaluates the right-hand side first).
+        final RubyModule module = readConstant.evaluateModule(frame);
+
+        // Next we check if the constant itself is defined, and if it is, we get its value.
+        final RubyConstant constant = readConstant.getConstantIfDefined(module);
+
+        final boolean isDefined = defined.profile(constant != null);
+
+        final Object value = isDefined
+                ? readConstant.getConstant(module, constant)
+                : null;
+
+        // Write if the constant is undefined or if its value is falsy.
+        if (!isDefined || !castToBoolean(value)) {
+            writeTwiceProfile.enter();
+            return writeConstant.execute(frame, module);
+        } else {
+            return value;
+        }
+    }
+
+    private boolean castToBoolean(final Object value) {
+        if (cast == null) {
+            CompilerDirectives.transferToInterpreterAndInvalidate();
+            cast = insert(BooleanCastNodeGen.create(null));
+        }
+        return cast.executeToBoolean(value);
+    }
+}