Side effects of RHS in BinaryNode

[Hannes Wallnoefer]

I thought about this again this morning and I think your version is 
actually not correct.

It's not enough to make sure the AST leaves are side effects free 
because operator nodes may trigger conversion to JS primitives which may 
have side effects of its own.

E.g. in "-x", the minus unary operator will trigger conversion of x to a 
number, with possible side effects.

On the other hand, for IdentNodes we don't really have to care if they 
have primitive type, since we only care about the loading part 
(conversion of right hand side comes last anyway). And loading should be 
side-effects free whenever the symbol is stored in a local slot.

So it seems that the following would be safe:

     private static boolean loadHasNoSideEffects(final Expression rhs) {
         if (rhs instanceof LiteralNode.PrimitiveLiteralNode) {
             return true;
         } else if (rhs instanceof IdentNode && 
!rhs.getSymbol().isScope()) {
             return true;
         }
         return false;
     }

Am I right with this?

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é/