Bug 161099 – We initialize functions too early in an eval

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

For example, this throws an exception:
```eval("'use strict'; class C { constructor() { print('foo'); } }; function foo() { return new C; }; foo()")```
saying that we can't find variable "C".

[GSkachkov]

I suspect that there is some issue in resolving scope for class variable in function within eval:
>>> eval("'use strict'; class C { constructor() { print('foo'); } }; function foo() { return new C; }; print(C); foo()")
class C { constructor() { print('foo'); } }
Exception: ReferenceError: Can't find variable: C

It finds C variable in print but doesn't find it inside of the function

[GSkachkov]

I think issue that during generation of ByteCode for EvalNode we add declared function into codeBlock by following code(https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp#L681):
    for (size_t i = 0; i < functionStack.size(); ++i)
     m_codeBlock->addFunctionDecl(makeFunction(functionStack[i]));
It seems that following code fix this issue:
for (FunctionMetadataNode* function : functionStack)
     m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable));

 But I need to run all tests, before creation of the patch,  to see if this change does not break any other tests

[GSkachkov]

Created attachment 287950 [details]
Patch

WIP

[Darin Adler]

Comment on attachment 287950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287950&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:680
>      const DeclarationStacks::FunctionStack& functionStack = evalNode->functionStack();

I noticed that this patch modernizes the style a bit. Given that, I think this line of code would be better as:

    auto& functionStack = evalNode->functionStack();

But, even better, we don’t need the local variable now that we are using a modern for loop below.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:681
> +    for (FunctionMetadataNode* function : functionStack)

I think this would be better with the type "auto*". The old code didn’t need to explicitly cite the type name, and writing the type out does not make the new code better.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:682
> +        m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable));

I typically prefer to use the initializer list syntax instead of make_pair, so I would write:

    for (auto& function : evalNode->functionStack())
        m_functionsToInitialize.append({ function, GlobalFunctionVariable });

If it compiles like this, I suggest you consider this style. Our auto* if you prefer to emphasize the fact that it’s a pointer.

[Saam Barati]

Comment on attachment 287950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287950&action=review

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:682
>> +        m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable));
> 
> I typically prefer to use the initializer list syntax instead of make_pair, so I would write:
> 
>     for (auto& function : evalNode->functionStack())
>         m_functionsToInitialize.append({ function, GlobalFunctionVariable });
> 
> If it compiles like this, I suggest you consider this style. Our auto* if you prefer to emphasize the fact that it’s a pointer.

Nit: GlobalFunctionVariable will not longer mean what it used to with this patch. We should probably find a new name.

> LayoutTests/js/script-tests/eval-access-to-class-in-function-declaration.js:7
> +shouldNotThrow("eval(\"\'use strict\';class C { constructor() { this.id=\'value\'; } };  function arr() { return new C }; result = arr();\")");

Please add some sloppy mode tests and make sure that the scope before the eval contains the variables that you expect it to.

[Saam Barati]

Comment on attachment 287950 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287950&action=review

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:682
>>> +        m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable));
>> 
>> I typically prefer to use the initializer list syntax instead of make_pair, so I would write:
>> 
>>     for (auto& function : evalNode->functionStack())
>>         m_functionsToInitialize.append({ function, GlobalFunctionVariable });
>> 
>> If it compiles like this, I suggest you consider this style. Our auto* if you prefer to emphasize the fact that it’s a pointer.
> 
> Nit: GlobalFunctionVariable will not longer mean what it used to with this patch. We should probably find a new name.

Actually, I'm not sure if this code is correct. Consider this:

function foo() {
    {
         let Foo = 20;
         eval("function Foo() { }"); // I suspect this will bind to the "let Foo" variable, however, I think we should be throwing a syntax error here upon parsing the eval (this is what other browsers do, however, I'd need to read the spec to convince myself that this is correct).
    }
}

The rules for function declarations inside an eval are quite weird. I believe they're defined in annex B of the spec.

Also, consider this example:

function foo() {
     let Foo = 20;
     {
          eval("function Foo() { }") // Also a syntax error in other browsers after parsing the eval.
     }
     return Foo;
}

Since we're changing the behavior, I think it's probably worth implementing the entire semantics defined in annex B. There are weird block scoping semantics for functions inside the eval, and also, there are semantics for how that function
binds to an identifier outside the eval. For example:
eval("
   let Foo = 20;
   {
        function Foo() { }
        print(Foo); // Should print the function
   }
   print(Foo); // Should print 20
")

[GSkachkov]

(In reply to comment #4)
> Comment on attachment 287950 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287950&action=review
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:680
> >      const DeclarationStacks::FunctionStack& functionStack = evalNode->functionStack();
> 
> I noticed that this patch modernizes the style a bit. Given that, I think
> this line of code would be better as:
> 
>     auto& functionStack = evalNode->functionStack();
> 
> But, even better, we don’t need the local variable now that we are using a
> modern for loop below.
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:681
> > +    for (FunctionMetadataNode* function : functionStack)
> 
> I think this would be better with the type "auto*". The old code didn’t need
> to explicitly cite the type name, and writing the type out does not make the
> new code better.
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:682
> > +        m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable));
> 
> I typically prefer to use the initializer list syntax instead of make_pair,
> so I would write:
> 
>     for (auto& function : evalNode->functionStack())
>         m_functionsToInitialize.append({ function, GlobalFunctionVariable });
> 
> If it compiles like this, I suggest you consider this style. Our auto* if
> you prefer to emphasize the fact that it’s a pointer.

Thanks for review! It seems that this fix require more changes that I expected, but I'll include your suggestion in new patch.

[GSkachkov]

Comment on attachment 287950 [details]
Patch

Patch does not cover more complex case. I'll try to provide new patch soon.

[GSkachkov]

Created attachment 307942 [details]
Patch

Updated patch

[Saam Barati]

Comment on attachment 307942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307942&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:111
> +                    // We know this will resolve to the top level scope or global object because our parser/global initialization code 

Nit: I'd rename the "globalScope"/"globalObjectScope" variable below.

Also, I think this needs some explaining of correctness for eval for sloppy mode:
This is a top level function, and it's an error to ever create a top level function name that would resolve to a lexical variable. E.g:
```
function f() {
      {
            let x;
            {
                   /// error thrown here
                    eval("function x(){}");
            }
      }
}
```
Therefore, we're guaranteed to have this resolve to a top level variable.

Also, it's worth noting that if you fix this case:
```
function foo() {
        try {
            throw 10;
        } catch(e) {
             eval("function e(){}");
        }
}
```

will potentially break how your code is written now.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1173
> +                variableObject->methodTable()->put(variableObject, callFrame, function->name(), jsUndefined(), slot);

I think it's worth a comment here that we need to create these properties because of how bytecode generator emits code.

[GSkachkov]

Created attachment 308110 [details]
Patch

Fix comments

[GSkachkov]

Comment on attachment 307942 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307942&action=review

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:111
>> +                    // We know this will resolve to the top level scope or global object because our parser/global initialization code 
> 
> Nit: I'd rename the "globalScope"/"globalObjectScope" variable below.
> 
> Also, I think this needs some explaining of correctness for eval for sloppy mode:
> This is a top level function, and it's an error to ever create a top level function name that would resolve to a lexical variable. E.g:
> ```
> function f() {
>       {
>             let x;
>             {
>                    /// error thrown here
>                     eval("function x(){}");
>             }
>       }
> }
> ```
> Therefore, we're guaranteed to have this resolve to a top level variable.
> 
> Also, it's worth noting that if you fix this case:
> ```
> function foo() {
>         try {
>             throw 10;
>         } catch(e) {
>              eval("function e(){}");
>         }
> }
> ```
> 
> will potentially break how your code is written now.

I've added your comment.Also I'm working now on the fix for SyntaxError in catch block.

[Saam Barati]

Comment on attachment 308110 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308110&action=review

r=me

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:129
> +                    RefPtr<RegisterID> tolLevelObjectScope = emitResolveScope(nullptr, Variable(metadata->ident()));
> +                    tolLevelScope = newBlockScopeVariable();
> +                    emitMove(tolLevelScope.get(), tolLevelObjectScope.get());

I wonder if the solution in the future is to use your new scope resolution bytecode. Since a syntax error is created if it returns undefined, I think we're guaranteed to find the correct scope.
That said, we'd do it only for sloppy mode, and for strict mode, we would still do this code that's written here. Maybe you'll need to move to this when doing the catch variable.

Also, I think at some point, we should move strict mode off of using the StrictActivation object. It should just use a locally created lexical environment.

> JSTests/stress/eval-func-decl-with-let-const-class.js:78
> +        let f;

can you assert the value of "f" here after the evals.

> JSTests/stress/eval-func-decl-with-let-const-class.js:87
> +        let f;

ditto

> JSTests/stress/eval-func-decl-with-let-const-class.js:95
> +        let f;

ditto

> JSTests/stress/eval-func-decl-with-let-const-class.js:102
> +        let f;

ditto

[GSkachkov]

Patch landed https://trac.webkit.org/r215986

[GSkachkov]

Comment on attachment 308110 [details]
Patch

Clear flags