Fixes #4807. Wrong control flow order with Constant ||=.

More specifically this fixes: 1. emitting rhs of ||= in wrong order 1. not calling lhs and evaluating it twice 1. making &&= semantics correct (we need specs for that somewhere) 1. correcting return value emitted from ||= and &&= expression 1. colon3 nodes in ||= or &&= in AST was giving itself name of '::' So many things in a tiny esoteric corner of Ruby.
jruby · Oct 27, 2017 · 8dae5af · 8dae5af
1 parent fcd7d73
commit 8dae5af
Showing 9 changed files with 80 additions and 34 deletions.
diff --git a/core/src/main/java/org/jruby/ast/OpAsgnConstDeclNode.java b/core/src/main/java/org/jruby/ast/OpAsgnConstDeclNode.java
@@ -21,6 +21,7 @@ public OpAsgnConstDeclNode(ISourcePosition position, Node lhs, String operator,
     }
 
     @Override
+    // This can only be Colon3 or Colon2
     public Node getFirstNode() {
         return lhs;
     }

diff --git a/core/src/main/java/org/jruby/ir/IRBuilder.java b/core/src/main/java/org/jruby/ir/IRBuilder.java
@@ -3268,30 +3268,48 @@ public Operand buildOpAsgn(OpAsgnNode opAsgnNode) {
         return result;
     }
 
-    // FIXME: All three paths feel a bit inefficient.  They all interact with the constant first to know
-    // if it exists or whether it should raise if it doesn't...then it will access it against for the put
-    // logic.  I could avoid that work but then there would be quite a bit more logic in Java.   This is a
-    // pretty esoteric corner of Ruby so I am not inclined to put anything more than a comment that it can be
-    // improved.
+    private Operand buildColon2ForConstAsgnDeclNode(Node lhs, Variable valueResult, boolean constMissing) {
+        Variable leftModule = createTemporaryVariable();
+        String name;
+
+        if (lhs instanceof Colon2Node) {
+            Colon2Node colon2Node = (Colon2Node) lhs;
+            name = colon2Node.getName();
+            Operand leftValue = build(colon2Node.getLeftNode());
+            copy(leftModule, leftValue);
+        } else { // colon3
+            copy(leftModule, new ObjectClass());
+            name = ((Colon3Node) lhs).getName();
+        }
+
+        addInstr(new SearchModuleForConstInstr(valueResult, leftModule, name, false, constMissing));
+
+        return leftModule;
+    }
+
     public Operand buildOpAsgnConstDeclNode(OpAsgnConstDeclNode node) {
         String op = node.getOperator();
 
         if ("||".equals(op)) {
             Variable result = createTemporaryVariable();
-            Label isDefined = getNewLabel();
             Label done = getNewLabel();
-            Variable defined = createTemporaryVariable();
-            addInstr(new CopyInstr(defined, buildGetDefinition(node.getFirstNode())));
-            addInstr(BNEInstr.create(isDefined, defined, manager.getNil()));
-            addInstr(new CopyInstr(result, putConstantAssignment(node, build(node.getSecondNode()))));
-            addInstr(new JumpInstr(done));
-            addInstr(new LabelInstr(isDefined));
-            addInstr(new CopyInstr(result, build(node.getFirstNode())));
+            Operand module = buildColon2ForConstAsgnDeclNode(node.getFirstNode(), result, false);
+            addInstr(BNEInstr.create(done, result, UndefinedValue.UNDEFINED));
+            Operand rhsValue = build(node.getSecondNode());
+            copy(result, rhsValue);
+            addInstr(new PutConstInstr(module, ((Colon3Node) node.getFirstNode()).getName(), rhsValue));
             addInstr(new LabelInstr(done));
             return result;
         } else if ("&&".equals(op)) {
-            build(node.getFirstNode()); // Get once to make sure it is there or will throw if not
-            return addResultInstr(new CopyInstr(createTemporaryVariable(), putConstantAssignment(node, build(node.getSecondNode()))));
+            Variable result = createTemporaryVariable();
+            Label done = getNewLabel();
+            Operand module = buildColon2ForConstAsgnDeclNode(node.getFirstNode(), result, true);
+            addInstr(new BFalseInstr(done, result));
+            Operand rhsValue = build(node.getSecondNode());
+            copy(result, rhsValue);
+            addInstr(new PutConstInstr(module, ((Colon3Node) node.getFirstNode()).getName(), rhsValue));
+            addInstr(new LabelInstr(done));
+            return result;
         }
 
         Variable result = createTemporaryVariable();

diff --git a/core/src/main/java/org/jruby/ir/instructions/SearchModuleForConstInstr.java b/core/src/main/java/org/jruby/ir/instructions/SearchModuleForConstInstr.java
@@ -23,16 +23,23 @@
  */
 public class SearchModuleForConstInstr extends OneOperandResultBaseInstr implements FixedArityInstr {
     String   constName;
-    private final boolean  noPrivateConsts;
+    private final boolean noPrivateConsts;
+    private final boolean callConstMissing;
 
     // Constant caching
     private volatile transient ConstantCache cache;
 
     public SearchModuleForConstInstr(Variable result, Operand currentModule, String constName, boolean noPrivateConsts) {
+        this(result, currentModule, constName, noPrivateConsts, true);
+    }
+
+    public SearchModuleForConstInstr(Variable result, Operand currentModule, String constName,
+                                     boolean noPrivateConsts, boolean callConstMissing) {
         super(Operation.SEARCH_MODULE_FOR_CONST, result, currentModule);
 
         this.constName = constName;
         this.noPrivateConsts = noPrivateConsts;
+        this.callConstMissing = callConstMissing;
     }
 
     public Operand getCurrentModule() {
@@ -47,10 +54,14 @@ public boolean isNoPrivateConsts() {
         return noPrivateConsts;
     }
 
+    public boolean callConstMissing() {
+        return callConstMissing;
+    }
+
     @Override
     public Instr clone(CloneInfo ii) {
         return new SearchModuleForConstInstr(ii.getRenamedVariable(result),
-                getCurrentModule().cloneForInlining(ii), constName, noPrivateConsts);
+                getCurrentModule().cloneForInlining(ii), constName, noPrivateConsts, callConstMissing);
     }
 
     @Override
@@ -73,10 +84,12 @@ public void encode(IRWriterEncoder e) {
         e.encode(getCurrentModule());
         e.encode(getConstName());
         e.encode(isNoPrivateConsts());
+        e.encode(callConstMissing());
     }
 
     public static SearchModuleForConstInstr decode(IRReaderDecoder d) {
-        return new SearchModuleForConstInstr(d.decodeVariable(), d.decodeOperand(), d.decodeString(), d.decodeBoolean());
+        return new SearchModuleForConstInstr(d.decodeVariable(), d.decodeOperand(), d.decodeString(),
+                d.decodeBoolean(), d.decodeBoolean());
     }
 
     @Override
@@ -90,7 +103,11 @@ public Object interpret(ThreadContext context, StaticScope currScope, DynamicSco
         Object result = !ConstantCache.isCachedFrom(module, cache) ? cache(context.runtime, module) : cache.value;
 
         if (result == null) {
-            result = module.callMethod(context, "const_missing", context.runtime.fastNewSymbol(constName));
+            if (callConstMissing) {
+                result = module.callMethod(context, "const_missing", context.runtime.fastNewSymbol(constName));
+            } else {
+                result = UndefinedValue.UNDEFINED;
+            }
         }
 
         return result;

diff --git a/core/src/main/java/org/jruby/ir/targets/ConstantLookupSite.java b/core/src/main/java/org/jruby/ir/targets/ConstantLookupSite.java
@@ -32,22 +32,24 @@ public class ConstantLookupSite extends MutableCallSite {
     private static final Logger LOG = LoggerFactory.getLogger(ConstantLookupSite.class);
     private final String name;
     private final boolean publicOnly;
+    private final boolean callConstMissing;
 
     private volatile RubySymbol symbolicName;
 
     private final SiteTracker tracker = new SiteTracker();
 
-    public static final Handle BOOTSTRAP = new Handle(Opcodes.H_INVOKESTATIC, p(ConstantLookupSite.class), "constLookup", sig(CallSite.class, MethodHandles.Lookup.class, String.class, MethodType.class, String.class, int.class));
+    public static final Handle BOOTSTRAP = new Handle(Opcodes.H_INVOKESTATIC, p(ConstantLookupSite.class), "constLookup", sig(CallSite.class, MethodHandles.Lookup.class, String.class, MethodType.class, String.class, int.class, int.class));
 
-    public ConstantLookupSite(MethodType type, String name, boolean publicOnly) {
+    public ConstantLookupSite(MethodType type, String name, boolean publicOnly, boolean callConstMissing) {
         super(type);
 
         this.name = name;
         this.publicOnly = publicOnly;
+        this.callConstMissing = callConstMissing;
     }
 
-    public static CallSite constLookup(MethodHandles.Lookup lookup, String searchType, MethodType type, String constName, int publicOnly) {
-        ConstantLookupSite site = new ConstantLookupSite(type, constName, publicOnly == 0 ? false : true);
+    public static CallSite constLookup(MethodHandles.Lookup lookup, String searchType, MethodType type, String constName, int publicOnly, int callConstMissing) {
+        ConstantLookupSite site = new ConstantLookupSite(type, constName, publicOnly == 0 ? false : true, callConstMissing == 0 ? false : true);
 
         MethodHandle handle = Binder
                 .from(lookup, type)
@@ -81,7 +83,11 @@ public IRubyObject searchConst(ThreadContext context, StaticScope staticScope) {
 
         // Call const_missing or cache
         if (constant == null) {
-            return module.callMethod(context, "const_missing", getSymbolicName(context));
+            if (callConstMissing) {
+                return module.callMethod(context, "const_missing", getSymbolicName(context));
+            } else {
+                return UndefinedValue.UNDEFINED;
+            }
         }
 
         SwitchPoint switchPoint = (SwitchPoint) runtime.getConstantInvalidator(name).getData();
@@ -116,7 +122,11 @@ public IRubyObject searchModuleForConst(ThreadContext context, IRubyObject cmVal
 
         // Call const_missing or cache
         if (constant == null) {
-            return module.callMethod(context, "const_missing", getSymbolicName(context));
+            if (callConstMissing) {
+                return module.callMethod(context, "const_missing", getSymbolicName(context));
+            } else {
+                return UndefinedValue.UNDEFINED;
+            }
         }
 
         // bind constant until invalidated

diff --git a/core/src/main/java/org/jruby/ir/targets/IRBytecodeAdapter.java b/core/src/main/java/org/jruby/ir/targets/IRBytecodeAdapter.java
@@ -510,7 +510,7 @@ public static BlockPassType fromIR(ClosureAcceptingInstr callInstr) {
      * @param name name of the constant
      * @param noPrivateConsts whether to ignore private constants
      */
-    public abstract void searchModuleForConst(String name, boolean noPrivateConsts);
+    public abstract void searchModuleForConst(String name, boolean noPrivateConsts, boolean callConstMissing);
 
     /**
      * Lookup a constant from a given class or module.

diff --git a/core/src/main/java/org/jruby/ir/targets/IRBytecodeAdapter6.java b/core/src/main/java/org/jruby/ir/targets/IRBytecodeAdapter6.java
@@ -742,19 +742,19 @@ private void performSuper(String file, int line, String name, int arity, boolean
     }
 
     public void searchConst(String name, boolean noPrivateConsts) {
-        adapter.invokedynamic("searchConst", sig(JVM.OBJECT, params(ThreadContext.class, StaticScope.class)), ConstantLookupSite.BOOTSTRAP, name, noPrivateConsts ? 1 : 0);
+        adapter.invokedynamic("searchConst", sig(JVM.OBJECT, params(ThreadContext.class, StaticScope.class)), ConstantLookupSite.BOOTSTRAP, name, noPrivateConsts ? 1 : 0, 0);
     }
 
-    public void searchModuleForConst(String name, boolean noPrivateConsts) {
-        adapter.invokedynamic("searchModuleForConst", sig(JVM.OBJECT, params(ThreadContext.class, IRubyObject.class)), ConstantLookupSite.BOOTSTRAP, name, noPrivateConsts ? 1 : 0);
+    public void searchModuleForConst(String name, boolean noPrivateConsts, boolean callConstMissing) {
+        adapter.invokedynamic("searchModuleForConst", sig(JVM.OBJECT, params(ThreadContext.class, IRubyObject.class)), ConstantLookupSite.BOOTSTRAP, name, noPrivateConsts ? 1 : 0, callConstMissing ? 1 : 0);
     }
 
     public void inheritanceSearchConst(String name, boolean noPrivateConsts) {
-        adapter.invokedynamic("inheritanceSearchConst", sig(JVM.OBJECT, params(ThreadContext.class, IRubyObject.class)), ConstantLookupSite.BOOTSTRAP, name, noPrivateConsts ? 1 : 0);
+        adapter.invokedynamic("inheritanceSearchConst", sig(JVM.OBJECT, params(ThreadContext.class, IRubyObject.class)), ConstantLookupSite.BOOTSTRAP, name, noPrivateConsts ? 1 : 0, 0);
     }
 
     public void lexicalSearchConst(String name) {
-        adapter.invokedynamic("lexicalSearchConst", sig(JVM.OBJECT, params(ThreadContext.class, StaticScope.class)), ConstantLookupSite.BOOTSTRAP, name, 0);
+        adapter.invokedynamic("lexicalSearchConst", sig(JVM.OBJECT, params(ThreadContext.class, StaticScope.class)), ConstantLookupSite.BOOTSTRAP, name, 0, 0);
     }
 
     public void pushNil() {

diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -2090,7 +2090,7 @@ public void SearchModuleForConstInstr(SearchModuleForConstInstr instr) {
         jvmMethod().loadContext();
         visit(instr.getCurrentModule());
 
-        jvmMethod().searchModuleForConst(instr.getConstName(), instr.isNoPrivateConsts());
+        jvmMethod().searchModuleForConst(instr.getConstName(), instr.isNoPrivateConsts(), instr.callConstMissing());
         jvmStoreLocal(instr.getResult());
     }
 

diff --git a/core/src/main/java/org/jruby/parser/RubyParser.java b/core/src/main/java/org/jruby/parser/RubyParser.java
@@ -2940,7 +2940,7 @@ public Object yyparse (RubyLexer yyLex) throws java.io.IOException {
 states[227] = new ParserState() {
   @Override public Object execute(ParserSupport support, RubyLexer lexer, Object yyVal, Object[] yyVals, int yyTop) {
                     ISourcePosition pos = lexer.getPosition();
-                    yyVal = support.newOpConstAsgn(pos, new Colon3Node(pos, ((String)yyVals[-3+yyTop])), ((String)yyVals[-1+yyTop]), ((Node)yyVals[0+yyTop]));
+                    yyVal = support.newOpConstAsgn(pos, new Colon3Node(pos, ((String)yyVals[-2+yyTop])), ((String)yyVals[-1+yyTop]), ((Node)yyVals[0+yyTop]));
     return yyVal;
   }
 };

diff --git a/core/src/main/java/org/jruby/parser/RubyParser.y b/core/src/main/java/org/jruby/parser/RubyParser.y
@@ -1070,7 +1070,7 @@ arg             : lhs '=' arg {
                 }
                 | tCOLON3 tCONSTANT tOP_ASGN arg {
                     ISourcePosition pos = lexer.getPosition();
-                    $$ = support.newOpConstAsgn(pos, new Colon3Node(pos, $1), $3, $4);
+                    $$ = support.newOpConstAsgn(pos, new Colon3Node(pos, $2), $3, $4);
                 }
                 | backref tOP_ASGN arg {
                     support.backrefAssignError($1);