hg: nashorn/jdk8/nashorn: 8026137: Fix Issues with Binary Evaluation Order

[André Bargull]

Yeah, sure strings need to be handled explicitly here.

On 10/14/2013 10:32 PM, Hannes Wallnoefer wrote:
> Looks good. One thing I'm noting is that String is currently handled 
> as ObjectType (i.e. getType().isObject() will return true), which is 
> true in Java terms, but in this context we should treat String as 
> primitive, right?
>
> Hannes
>
> Am 2013-10-14 21:59, schrieb André Bargull:
>> "primitive type" was a bad choice of words, I rather meant 
>> "side-effect-free".
>>
>> When all operands are primitives, most operations won't trigger 
>> side-effects. Only most operations because in `0 - (0 instanceof 1)` 
>> all operands are primitives, but `(0 instanceof 1)` still needs to be 
>> executed before the subtraction. So I was thinking of the following 
>> change for Codegen (incomplete -> `instanceof` etc. not handled!):
>>
>>
>>     private static boolean isSideEffectFree(final Expression rhs) {
>>         if (rhs instanceof LiteralNode.PrimitiveLiteralNode) {
>>             return true;
>>         } else if (rhs instanceof IdentNode) {
>>             return !rhs.getSymbol().isScope() && 
>> !rhs.getType().isObject();
>>         } else if (rhs instanceof UnaryNode) {
>>             UnaryNode unary = (UnaryNode) rhs;
>>             return isSideEffectFree(unary.rhs());
>>         } else if (rhs instanceof BinaryNode) {
>>             BinaryNode binary = (BinaryNode) rhs;
>>             return isSideEffectFree(binary.lhs())
>>                     && isSideEffectFree(binary.rhs());
>>         } else if (rhs instanceof TernaryNode) {
>>             TernaryNode ternary = (TernaryNode) rhs;
>>             return isSideEffectFree(ternary.getTest())
>>                     && isSideEffectFree(ternary.getTrueExpression())
>>                     && isSideEffectFree(ternary.getFalseExpression());
>>         }
>>         return false;
>>     }
>>
>>     private static boolean safeLiteral(final Expression rhs) {
>> //        return isSideEffectFree(rhs);
>>         return rhs instanceof LiteralNode && !(rhs instanceof 
>> ArrayLiteralNode);
>>     }
>>
>>
>> - André
>>
>>
>>> Side effects can still occur with primitive types. As an example, this
>>> script from the test case as a numeric right hand side:
>>>
>>> ({valueOf: function(){throw 0}}) - ({valueOf: function(){throw 1}} - 1)
>>>
>>> But maybe something like this (pseudo-code) could work in
>>> CodeGenerator#safeLiteral:
>>>
>>>     isPrimitiveLiteralNode || (isIdentNode && isPrimitiveType)
>>>
>>> An IdentNode can have side effects if it is a global with a getter, but
>>> if we know it's type that shouldn't be the case.
>>>
>>> Hannes
>>>
>>> Am 2013-10-09 18:44, schrieb André Bargull:
>>> >/  Quick question for this change set:
>>> />/
>>> />/  Why does CodeGenerator#safeLiteral(...) need to test for 
>>> literal nodes
>>> />/  instead of using type information? When the right-hand-side is a
>>> />/  primitive type, side effects cannot occur, so the additional
>>> />/  swap/dup/pop instructions can be omitted safely.
>>> />/
>>> />/  For example `function f(a){var c = 1; return a - c }` now emits:
>>> />/      ALOAD 1
>>> />/      ILOAD 4
>>> />/      SWAP
>>> />/      INVOKESTATIC jdk/nashorn/internal/runtime/JSType.toNumber
>>> />/  (Ljava/lang/Object;)D
>>> />/      DUP2_X1
>>> />/      POP2
>>> />/      I2D
>>> />/      DSUB
>>> />/
>>> />/  Using type info it's possible to change the bytecode to:
>>> />/      ALOAD 1
>>> />/      INVOKESTATIC jdk/nashorn/internal/runtime/JSType.toNumber
>>> />/  (Ljava/lang/Object;)D
>>> />/      ILOAD 4
>>> />/      I2D
>>> />/      DSUB
>>> />/
>>> />/
>>> />/  (Also: That was one of the bug reports which worried me a bit, 
>>> because
>>> />/  I expected the changes to be non-trivial. :-(
>>> />/
>>> />/
>>> />/  - André/
>
>