708801 - Boolean converted to number when optimising

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Stian Pedersen]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.2 (KHTML, like Gecko) Chrome/15.0.874.121 Safari/535.2

Steps to reproduce:

function A()
{	
    var b = 1 <= 1;
    this.bar = b;
}

foo = new A();
print(foo.bar); print(" ");

if (typeof foo.bar == "boolean") {
    print("Boolean");
} else {
    print("Number");
}


Actual results:

Rhino when called as /usr/bin/rhino -opt 9 -f, converts this into a number. If the expression is copied into this.bar, then all is fine.


Expected results:

Should be boolean.

Comment 1

[Hannes Wallnoefer]

Thanks for reporting. Fixed in git master and rhino_1_8 branches.

https://github.com/mozilla/rhino/commit/c598a334fad088a85f1c78a5648ddfe7a7c69f79

Comment 2

[André Bargull [:anba]]

(In reply to Hannes Wallnoefer from comment #1)
> Thanks for reporting. Fixed in git master and rhino_1_8 branches.
> 
> https://github.com/mozilla/rhino/commit/
> c598a334fad088a85f1c78a5648ddfe7a7c69f79

The patch does not resolve all issues, e.g. try eval'ing the following script: `var b = typeof(1,1); print(typeof b)`. It will still report "number" instead of "string".

Comment 3

[André Bargull [:anba]]

Attachment: New patch

I've attached a (partially tested) patch, to sketch out what needs to be changed as well. You'll note that I've altered the default case for Block#findExpressionType() to return `Optimizer.AnyType`, I think this is more sane resp. future-proof than just descending into the child nodes.

Comment 4

[André Bargull [:anba]]

Attachment: JUnit test case for the proposed patch

Here's a JUnit test case for my proposed patch. For the sake of simplicity, the test case exploits another optimizer bug, but that one will be a bit harder to fix, cf. comment in the attached file.

Comment 5

[Hannes Wallnoefer]

Thanks André! When I saw that that very sparse switch statement I already thought this would not be the end of it. I'll try to wrap my mind around this tomorrow.

Comment 6

[Hannes Wallnoefer]

I think the other bug you discovered shows an error in our general type flow analysis algorithm. Var types are OR'ed from all predecessor blocks, but this is wrong for vars that may still be undefined (NoType). A var will be marked as number type if just one of its predecessors thinks it is. I think what we need to do is keep a list of var types for each block and actually make sure each of a block's predecessors thinks it's a number (or other specific type).

The changes you made regarding this bug make sense to me. I'm wondering how we can be sure to actually cover all cases. Maybe we should pull the recursive child loop in findDefPoints() out of the switch statement and perform it for all nodes, it makes the method a bit simpler and seems to work. I.e. something like this:


    private static boolean findDefPoints(OptFunctionNode fn, Node n,
                                         int[] varTypes)
    {
        boolean result = false;
        Node child = n.getFirstChild();
        switch (n.getType()) {
            case Token.DEC :
            case Token.INC :
                if (child.getType() == Token.GETVAR) {
                    // theVar is a Number now
                    int i = fn.getVarIndex(child);
                    result = assignType(varTypes, i, Optimizer.NumberType);
                }
                break;
            case Token.SETPROP :
            case Token.SETPROP_OP :
                if (child.getType() == Token.GETVAR) {
                    int i = fn.getVarIndex(child);
                    assignType(varTypes, i, Optimizer.AnyType);
                }
                break;
            case Token.SETVAR : {
                Node rValue = child.getNext();
                int theType = findExpressionType(fn, rValue, varTypes);
                int i = fn.getVarIndex(n);
                result = assignType(varTypes, i, theType);
                break;
            }
        }
        while (child != null) {
            result |= findDefPoints(fn, child, varTypes);
            child = child.getNext();
        }
        return result;
    }

Comment 7

[André Bargull [:anba]]

Running the attached JUnit test with your proposed change will return some failures. This happens since you've moved the recursive loop to the end instead of the beginning. Consider this let-expression: `var b = let(x=1) x`. If the child expression `let(x=1) x` is processed at the end of findDefPoints(), it isn't possible to infer the right type for `b` (which is 'number') since `let(x=1)` wasn't yet handled. 

Therefore I'd suggest the following change:
---
private static boolean findDefPoints (...) {
    boolean result = false;
    Node first = n.getFirstChild();
    for (Node child = first; child != null; child = child.getNext()) {
        result |= findDefPoints(fn, child, varTypes);
    }
    switch (n.getType()) {
        // ...
    }
    return result;
}
---

Comment 8

[Hannes Wallnoefer]

I see. 

BTW, unless I'm misunderstanding something I think the SETPROP/SETPROP_OP case in that switch statement is bogus. When we access a property of a number or other primitive value it is converted to an object, but only within the scope of the statement - the value of the variable remains the same. I think this can be removed safely.

Comment 9

[André Bargull [:anba]]

Yep, the 'SETPROP/SETPROP_OP' case can be removed if Rhino works as specified in ECMAScript because the internal [[Put]] method for primitive base values doesn't actually alter the primitive, cf. ES5.1 8.7.2 "PutValue".

Maybe that extra case was added because most times 'SETPROP/SETPROP_OP' works on objects and not on primitives, so the conclusion was drawn that the variable in question is an object as well. And for the rather uncommon case of primitives, not performing the optimization to use "double" instead of "java.lang.Object" actually saves extra calls to "OptRuntime.wrapDouble()".

Comment 10

[André Bargull [:anba]]

(In reply to Hannes Wallnoefer from comment #6)
> I think the other bug you discovered shows an error in our general type flow
> analysis algorithm. Var types are OR'ed from all predecessor blocks, but
> this is wrong for vars that may still be undefined (NoType). [...]
> 

Actually we've been fooled by "typeof". "o.m.j.optimizer.Block" performs two runs: first a 'definition-data-flow' and then the 'type-flow'. In the first run, Block detects whether a variable is used before defined. And later all variables from the live-set of the first (program-)block will be assigned to type "any". Variables are marked as used before defined if they appear in a Token.GETVAR instruction. And here's the issue with "typeof": 
"typeof foo" is not compiled to "TYPEOF [GETVAR [NAME foo]]" but instead to "TYPEOFNAME foo". So basically it's just necessary to handle "Token.TYPEOFNAME" in a couple of switch statements in addition to "Token.GETVAR". 

I'll add new patch later.

Comment 11

[André Bargull [:anba]]

Attachment: Revised patch

Patch details:
Block#lookForVariableAccess()
- add missing case for nested expressions in increment/decrement
- handle Token.TYPEOFNAME to note variable access
Block#findExpressionType()
- add more cases for known operations
- return AnyType for any unknown operation instead of simply iterating over the child nodes
Block#findDefPoints()
- recursively call method for all node types
- always update "result" variable
- I did _not_ remove the SETPROP/SETPROP_OP case, because other code may depend on that behaviour (cf. the "can never happen ???" comment in Optimizer#rewriteForNumberVariables())

OptFunctionNode#getVarIndex()
- handle TYPEOFNAME, this is required for #lookForVariableAccess() to work

Optimizer#rewriteForNumberVariables()
- add missing check for GETPROP

Comment 12

[André Bargull [:anba]]

Attachment: More thorough test case

And a new JUnit test case, note that the test case is expected to be placed in the "org.mozilla.javascript.optimizer" package to access package-private classes

Comment 13

[Hannes Wallnoefer]

Attachment: Stack trace of assertion error with proposed patch

I'm getting lots of assertion errors in OptFunctionNode.getVarIndex() when running the test suite with this patch. Stack trace is attached.

Comment 14

[Hannes Wallnoefer]

(In reply to Hannes Wallnoefer from comment #13)
> 
> I'm getting lots of assertion errors in OptFunctionNode.getVarIndex() when
> running the test suite with this patch. Stack trace is attached.

Found the problem: names used with TYPEOFNAME may be undefined, so you have to handle them differently from GETVAR in Block.lookForVariableAccess().

Looks like the assertion failed in one of the test driver scripts, which is why I got so many of these.

Comment 15

[André Bargull [:anba]]

Attachment: Patch for assertion error

I'm in a bit of a hurry, but I guess this additional patch should cover the assertion error you've seen.

Comment 16

[Hannes Wallnoefer]

I committed your patch and test case with a few changes described below. Thanks so much for the the excellent work, André!

https://github.com/mozilla/rhino/commit/e076756c72488716e9797b1fffec6d51b139f5c7

Changes:
 
 - Handle Token.TYPEOFNAME independently in Block#lookForVariableAccess(). As noted in #comment 14, typeof <name> can be used with totally undefined names, which threw an assertion error with your patch.

 - I did remove the SETPROP/SETPROP_OP case in Block#findDefPoints() because I'm positively sure it is not needed and in fact wrong. I also removed the "can never happen ???" comment in Optimizer#rewriteForNumberVariables()) because this case can and should happen.

 - Added a few tests.