var of different scopes overriding each other's values

[kevinputera]

Describe the bug
It seems that there is a problem with the scoping of var in boa. In particular, assigning to var with the same name in different scopes will result in them overriding each other's values.

This issue was discovered because the conformance test built-ins/Object/fromEntries/evaluation-order.js is failing.

To Reproduce
Check out the following minimal example:

function modifyX() {
    var x = 1;
}

var x = 2;
console.log(x); // output: 2

modifyX();
console.log(x); // output: 1

As mentioned above, another example of this issue is in the built-ins/Object/fromEntries/evaluation-order.js conformance test.

Expected behavior
For the given minimal example, the expected output for both console.logs should be 2.

Build environment (please complete the following information):

• OS: macOS Big Sur
• Version: 11.5.2
• Target triple: x86_64-apple-darwin
• Rustc version: rustc 1.54.0 (a178d0322 2021-07-26)

Additional notes

This bug occurs in both strict and non-strict mode.

[lowr]

I've dug into the code and the spec and it turned out to be more complicated than I initially thought it would be.

The cause of the issue is that boa currently doesn't create mutable/immutable bindings until it executes the variable declaration statements (for the sake of comprehensiveness, the substitution to the variable in the wrong scope is executed here). The spec requires the implementations to create the bindings before the execution of the function body, as specified in the abstract operation FunctionDeclarationInstantiation in the spec. Variable declaration statements should only substitute values instead.

The fix seems not much, but naive fixes would break REPL, as there's nowhere for it to create bindings. Node.js addresses this problem by parsing every input in its REPL as a Script, whose bindings are created before the ScriptBody execution in the abstract operation GlobalDeclarationInstantiation, like functions as described above.

I'm interested in fixing this, but it could take several PRs. To avoid temporary REPL breaks, I suppose changing the way REPL works first and then fully implementing FunctionDeclarationInstantiation would be ok. Any thoughts or suggestions would be appreciated! (Sorry if I missed any discussion on this subject in other places)

[jedel1043]

@lowr There's a PR draft addressing part of this: #1391

[lowr]

Ah completely missed it, thanks for the info 👍

[lastmjs]

I think I just ran into this issue as well. For example, this TypeScript code:

export function balance(request: BalanceRequest): Query<BalanceResponse> {
    const aid = getUserAID(request.user);

    const balance = state.balances[aid];

    if (balance === undefined) {
        return {
            ok: 0
        };
    }

    return {
        ok: balance
    };
}


export function claim(): Update<boolean> {
    const callerAddress = addressFromPrincipal(ic.caller);

    const balance = state.balances[callerAddress] ?? 0;

    state.balances[callerAddress] = balance + 100000000;
    
    const supply = state.supply;

    state.supply = supply + 100000000;

    return true;
}

Compiles into this code which I am running in boa:

function balance(request) {
    var aid = getUserAID(request.user);
    var balance = state.balances[aid];
    if (balance === undefined) {
        return {
            ok: 0
        };
    }
    return {
        ok: balance
    };
}
exports.balance = balance;
function claim() {
    var _a;
    var callerAddress = addressFromPrincipal(ic.caller);
    var balance = (_a = state.balances[callerAddress]) !== null && _a !== void 0 ? _a : 0;
    state.balances[callerAddress] = balance + 100000000;
    var supply = state.supply;
    state.supply = supply + 100000000;
    return true;
}
exports.claim = claim;

When I try to call the balance function, I get this error: Value is not callable. I've tracked it down to the fact that I am using a variable named balance and I have a function named balance. I also have a function named supply and the same issue seems to be happening with that function and the local variable supply in the claim function.

I assume my issue will be addressed here, just adding these examples for more information.

[raskad]

This should be fixed via #1829. I tested the example in the this issue and it works as expected.

[Razican]

One question here @raskad do we still need the "hoisting" hack we use to re-order statements?

[raskad]

One question here @raskad do we still need the "hoisting" hack we use to re-order statements?

Do you mean this "hoisting"?

boa/boa_engine/src/syntax/parser/statement/mod.rs

Line 411 in 2c6aceb

items.sort_by(Node::hoistable_order);

This is currently only relevant for function declarations. I'd have to take a closer look. There may be some other things that depend on this.

[lastmjs]

So far this is working for me FYI

[Razican]

One question here @raskad do we still need the "hoisting" hack we use to re-order statements?

Do you mean this "hoisting"?

boa/boa_engine/src/syntax/parser/statement/mod.rs

Line 411 in 2c6aceb

items.sort_by(Node::hoistable_order);

This is currently only relevant for function declarations. I'd have to take a closer look. There may be some other things that depend on this.

Sounds good! Yea I was mentioning that :) it's a hack I had to introduce in the early days to make it work properly, but I would love to remove it as soon as it's possible.

It reduces the parsing speed, and it's not really spec compliant. Also, when printing the AST or the original code, the order is not maintained.