8051439: Wrong type calculated for ADD operator with undefined operand

Reviewed-by: jlaskey, sundar

nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java

@@ -314,7 +314,7 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
         if (!symbol.isScope()) {
             final Type type = identNode.getType();
             if(type == Type.UNDEFINED) {
-                return method.loadUndefined(Type.OBJECT);
+                return method.loadUndefined(resultBounds.widest);
             }
 
             assert symbol.hasSlot() || symbol.isParam();

nashorn/src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java

@@ -1196,9 +1196,7 @@ final class LocalVariableTypesCalculator extends NodeVisitor<LexicalContext>{
                 } else if(binaryNode.isOptimisticUndecidedType()) {
                     // At this point, we can assign a static type to the optimistic binary ADD operator as now we know
                     // the types of its operands.
-                    final Type type = Type.widest(binaryNode.lhs().getType(), binaryNode.rhs().getType());
+                    return binaryNode.decideType();
-                    // Use Type.CHARSEQUENCE instead of Type.STRING to avoid conversion of ConsStrings to Strings.
-                    return binaryNode.setType(type.equals(Type.STRING) ? Type.CHARSEQUENCE : type);
                 }
                 return binaryNode;
             }

nashorn/src/jdk/nashorn/internal/ir/BinaryNode.java

@@ -183,17 +183,31 @@ public final class BinaryNode extends Expression implements Assignment<Expressio
         switch (tokenType()) {
         case ADD:
         case ASSIGN_ADD: {
+            // Compare this logic to decideType(Type, Type); it's similar, but it handles the optimistic type
+            // calculation case while this handles the conservative case.
             final Type lhsType = lhs.getType(localVariableTypes);
             final Type rhsType = rhs.getType(localVariableTypes);
             if(lhsType == Type.BOOLEAN && rhsType == Type.BOOLEAN) {
+                // Will always fit in an int, as the value range is [0, 1, 2]. If we didn't treat them specially here,
+                // they'd end up being treated as generic INT operands and their sum would be conservatively considered
+                // to be a LONG in the generic case below; we can do better here.
                 return Type.INT;
+            } else if(isString(lhsType) || isString(rhsType)) {
+                // We can statically figure out that this is a string if either operand is a string. In this case, use
+                // CHARSEQUENCE to prevent it from being proactively flattened.
+                return Type.CHARSEQUENCE;
             }
-            final Type widestOperandType = Type.widest(lhs.getType(localVariableTypes), rhs.getType(localVariableTypes));
+            final Type widestOperandType = Type.widest(undefinedToNumber(booleanToInt(lhsType)), undefinedToNumber(booleanToInt(rhsType)));
             if(widestOperandType == Type.INT) {
                 return Type.LONG;
             } else if (widestOperandType.isNumeric()) {
                 return Type.NUMBER;
             }
+            // We pretty much can't know what it will be statically. Must presume OBJECT conservatively, as we can end
+            // up getting either a string or an object when adding something + object, e.g.:
+            // 1 + {} == "1[object Object]", but
+            // 1 + {valueOf: function() { return 2 }} == 3. Also:
+            // 1 + {valueOf: function() { return "2" }} == "12".
             return Type.OBJECT;
         }
         case SHR:
@@ -256,10 +270,18 @@ public final class BinaryNode extends Expression implements Assignment<Expressio
         }
     }
 
+    private static boolean isString(final Type type) {
+        return type == Type.STRING || type == Type.CHARSEQUENCE;
+    }
+
     private static Type booleanToInt(final Type type) {
         return type == Type.BOOLEAN ? Type.INT : type;
     }
 
+    private static Type undefinedToNumber(final Type type) {
+        return type == Type.UNDEFINED ? Type.NUMBER : type;
+    }
+
     /**
      * Check if this node is an assignment
      *
@@ -527,7 +549,7 @@ public final class BinaryNode extends Expression implements Assignment<Expressio
 
     private Type getTypeUncached(final Function<Symbol, Type> localVariableTypes) {
         if(type == OPTIMISTIC_UNDECIDED_TYPE) {
-            return Type.widest(lhs.getType(localVariableTypes), rhs.getType(localVariableTypes));
+            return decideType(lhs.getType(localVariableTypes), rhs.getType(localVariableTypes));
         }
         final Type widest = getWidestOperationType(localVariableTypes);
         if(type == null) {
@@ -536,6 +558,32 @@ public final class BinaryNode extends Expression implements Assignment<Expressio
         return Type.narrowest(widest, Type.widest(type, Type.widest(lhs.getType(localVariableTypes), rhs.getType(localVariableTypes))));
     }
 
+    private static Type decideType(final Type lhsType, final Type rhsType) {
+        // Compare this to getWidestOperationType() for ADD and ASSIGN_ADD cases. There's some similar logic, but these
+        // are optimistic decisions, meaning that we don't have to treat boolean addition separately (as it'll become
+        // int addition in the general case anyway), and that we also don't conservatively widen sums of ints to
+        // longs, or sums of longs to doubles.
+        if(isString(lhsType) || isString(rhsType)) {
+            return Type.CHARSEQUENCE;
+        }
+        // NOTE: We don't have optimistic object-to-(int, long) conversions. Therefore, if any operand is an Object, we
+        // bail out of optimism here and presume a conservative Object return value, as the object's ToPrimitive() can
+        // end up returning either a number or a string, and their common supertype is Object, for better or worse.
+        final Type widest = Type.widest(undefinedToNumber(booleanToInt(lhsType)), undefinedToNumber(booleanToInt(rhsType)));
+        return widest.isObject() ? Type.OBJECT : widest;
+    }
+
+    /**
+     * If the node is a node representing an add operation and has {@link #isOptimisticUndecidedType() optimistic
+     * undecided type}, decides its type. Should be invoked after its operands types have been finalized.
+     * @return returns a new node similar to this node, but with its type set to the type decided from the type of its
+     * operands.
+     */
+    public BinaryNode decideType() {
+        assert type == OPTIMISTIC_UNDECIDED_TYPE;
+        return setType(decideType(lhs.getType(), rhs.getType()));
+    }
+
     @Override
     public BinaryNode setType(final Type type) {
         if (this.type == type) {

nashorn/test/script/basic/JDK-8051439.js

@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2010, 2014, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ * 
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ * 
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ * 
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ * 
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * JDK-8051439: Wrong type calculated for ADD operator with undefined operand
+ *
+ * @test
+ * @run
+ */
+
+// Test + operator
+function f1() { 
+    var x; 
+    for (var i = 0;i < 3; i++) { 
+        x = x + i; 
+    }
+    x = x + "test"; 
+    return x; 
+} 
+
+// Test += operator
+function f2() { 
+    var x; 
+    for (var i = 0;i < 3; i++) { 
+        x += i; 
+    }
+    x += "test"; 
+    return x; 
+} 
+
+print(f1());
+print(f2());

nashorn/test/script/basic/JDK-8051439.js.EXPECTED

@@ -0,0 +1,2 @@
+NaNtest
+NaNtest