Panic assigning named function to variable

[Razican]

I'm trying to execute the following piece of code from the test262 sta.js file:

var $ERROR;
$ERROR = function $ERROR(message) {
  throw new Test262Error(message);
};

I'm not sure why is this defined this way. My JavaScript knowledge is not good enough to know 100% what should happen, but other compilers seem to understand this well. The thing is that it first declares a binding, $ERROR, and then, it declares a named function $ERROR and assigns it to the $ERROR binding.

Creating the $ERROR function will trigger a panic!() in the create_mutable_binding() function of the GlobalEnvironmentRecord struct, here, since the $ERROR binding already exists.

I'm not sure if this should just re-assign the binding. If that's the case, then we just need to conditionally create the binding in the executor in the function declaration. But this could cause a panic if the binding has already been initialized (and probably will on the assignment of the $ERROR variable) here.

[jcdickinson]

lvalue = function name() shouldn't assign name to anything, afaik. The name provided when function is (probably) for debugging/stack traces.

A fn as a statement should assign to the current global, a fn as an expression (rvalue) should not.

There's also some strict mode stuff under assumption here. This code shouldn't panic under non-strict even if this current behavior were hypothetically correct.

[Razican]

lvalue = function name() shouldn't assign name to anything, afaik. The name provided when function is (probably) for debugging/stack traces.

A fn as a statement should assign to the current global, a fn as an expression (rvalue) should not.

So, if we should provide debugging/stack traces, that name should be stored somewhere, right? Or should just be on the parser information?

This code shouldn't panic under non-strict even if this current behavior were hypothetically correct.

In fact, I think the compiler should never panic. It should return an error if there is any.

[jasonwilliams]

hmm this is a good find.
I think @jcdickinson is right about handling this differently when a fn is used as a statement compared to an expression.

The spec covers this somewhat here:
https://tc39.es/ecma262/#sec-function-definitions-runtime-semantics-namedevaluation

The BindingIdentifier in a FunctionExpression can be referenced from inside the FunctionExpression's FunctionBody to allow the function to call itself recursively. However, unlike in a FunctionDeclaration, the BindingIdentifier in a FunctionExpression cannot be referenced from and does not affect the scope enclosing the FunctionExpression.

Looks like in the above we shouldn't be binding the function name to the scope, but the function name does get saved as part of the metadata of the function itself. I was working on jasonwilliams#255 to make this easier

So, if we should provide debugging/stack traces, that name should be stored somewhere, right?

According to the spec, its stored as a property name on the function object
https://tc39.es/ecma262/#sec-setfunctionname

We can put this theory to the test

let a = function hello() { console.log('hello world'); }
a.name // "hello"

[Razican]

Nice, should we wait for #255 to land?

[jasonwilliams]

I need your feedback in jasonwilliams#141

[jasonwilliams]

jasonwilliams#274 jasonwilliams#225 Would also help with this we could finally parse functions as expressions and statements separately

[Razican]

It seems that after #281 landing and #225 being closed, this is still happening.

[jasonwilliams]

Fair enough. At least it should be easier to fix now! I’m sure this was the issue that kicked everything off in the first place 😂

[jasonwilliams]

Moving to v0.8.0

[jasonwilliams]

AST for the above

StatementList(
    [
        VarDecl(
            [
                (
                    "$ERROR",
                    None,
                ),
            ],
        ),
        Assign(
            Local(
                "$ERROR",
            ),
            FunctionDecl(
                Some(
                    "$ERROR",
                ),
                [
                    FormalParameter {        
                        name: "message",     
                        init: None,
                        is_rest_param: false,
                    },
                ],
                StatementList(
                    [
                        Throw(
                            New(
                                Call(
                                    Local(
                                        "Test262Error",
                                    ),
                                    [
                                        Local(
                                            "message",
                                        ),
                                    ],
                                ),
                            ),
                        ),
                    ],
                ),
            ),
        ),
    ],
)

We still need to know if we're in an expression or a statement.
I don't think we can do this right now unless the parser passes some sort of hint

[jasonwilliams]

oh wait nice the parser differentiates them now.
@Razican awesome!

Expression
https://github.com/jasonwilliams/boa/blob/master/boa/src/syntax/parser/expression/primary/function_expression.rs#L58

Statement
https://github.com/jasonwilliams/boa/blob/master/boa/src/syntax/parser/statement/declaration/hoistable.rs#L116

@Razican could you explain what's going on here?

Ok(Node::function_decl::<_, &String, _, _>(name, params, body))

I'm guessing we need a new Node variant for Function Expression?

[Razican]

@Razican could you explain what's going on here?

Ok(Node::function_decl::<_, &String, _, _>(name, params, body))

I'm guessing we need a new Node variant for Function Expression?

The parser differentiates them, but produces the same node. We might need two nodes, yes. This is just creating a FunctionDecl node with a helper, generic function (that avoids having to box things yourself, for example).

If we need to differentiate this on execution, then we need two different nodes.

[Razican]

Another option would be to add a boolean to the function, stating if it's an expression or not. What do you think? How does V8 for example do it?

[HalidOdat]

Another option would be to add a boolean to the function, stating if it's an expression or not. What do you think? How does V8 for example do it?

I think it might be better to have different nodes instead of trying to put everything into one node.