8051439: Wrong type calculated for ADD operator with undefined operand
Reviewed-by: jlaskey, sundar
--- a/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java Wed Aug 06 10:42:46 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java Wed Aug 06 11:02:14 2014 +0200
@@ -314,7 +314,7 @@
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();
--- a/nashorn/src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java Wed Aug 06 10:42:46 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java Wed Aug 06 11:02:14 2014 +0200
@@ -1196,9 +1196,7 @@
} 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());
- // 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.decideType();
}
return binaryNode;
}
--- a/nashorn/src/jdk/nashorn/internal/ir/BinaryNode.java Wed Aug 06 10:42:46 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/BinaryNode.java Wed Aug 06 11:02:14 2014 +0200
@@ -183,17 +183,31 @@
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 @@
}
}
+ 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 @@
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 @@
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) {
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/test/script/basic/JDK-8051439.js Wed Aug 06 11:02:14 2014 +0200
@@ -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());
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/test/script/basic/JDK-8051439.js.EXPECTED Wed Aug 06 11:02:14 2014 +0200
@@ -0,0 +1,2 @@
+NaNtest
+NaNtest