Bug 167837 – [JSC] Align duplicate declaration checks in EvalDeclarationInstantiation with the spec

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[GSkachkov]

function goo() {
    {   
        var error = false;
        try {
            let f = 20;
            eval('var f = 15; eval(" { function f() { }; } ")');
        } catch (e) {
            error = e instanceof SyntaxError;
        }
        print(error); // true 
    }
    print(typeof f);  // "undefined"
}

[GSkachkov]

According to  https://tc39.github.io/ecma262/#sec-evaldeclarationinstantiation p 5.d.ii.2.a.i and 
https://tc39.github.io/ecma262/#sec-functiondeclarationinstantiation p.9.2.12.29

[GSkachkov]

Created attachment 314583 [details]
Patch

Patch

[Saam Barati]

Comment on attachment 314583 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Current patch allow to prevent the redeclaration of let/const variable

This doesn’t look like the right description for what this does. Don’t we already prevent redecoration? I thought this is preventing the hoisting of var over let/const?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1182
> +            // TODO: rename function

What’s this for?

[GSkachkov]

Created attachment 314651 [details]
Patch

Fix comments

[GSkachkov]

Comment on attachment 314583 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        Current patch allow to prevent the redeclaration of let/const variable
> 
> This doesn’t look like the right description for what this does. Don’t we already prevent redecoration? I thought this is preventing the hoisting of var over let/const?

I did for cases when we declare function within eval. Current patch prevent override by var variable.  i.e. `(function () { let a; eval('var a') })()`
I've updated fix description

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:1182
>> +            // TODO: rename function
> 
> What’s this for?

Removed

[Saam Barati]

Comment on attachment 314651 [details]
Patch

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

mostly LGTM, just a few questions

> Source/JavaScriptCore/ChangeLog:8
> +        Current patch allow to prevent the redeclaration of let/const variable 

"allow to prevent" => "prevents"

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1183
> +                JSValue resolvedScope = JSScope::resolveScopeForHoistingFuncDeclInEval(callFrame, scope, ident);

This name seems slightly off now since you're using this for variables as well as functions. Maybe we can use a name that better describes what it's used for now

> Source/JavaScriptCore/runtime/JSScope.cpp:262
> +        return scope->isVarScope() || scope->isCatchScope();

Is it specified behavior to allow var hoisting over catch variable? This seems counter intuitive. And do we want this both for functions and vars?

[GSkachkov]

Created attachment 314925 [details]
Patch

Fixed comments

[GSkachkov]

Created attachment 315176 [details]
Patch

Fix comments

[GSkachkov]

Comment on attachment 314651 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        Current patch allow to prevent the redeclaration of let/const variable 
> 
> "allow to prevent" => "prevents"

Fixed

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:1183
>> +                JSValue resolvedScope = JSScope::resolveScopeForHoistingFuncDeclInEval(callFrame, scope, ident);
> 
> This name seems slightly off now since you're using this for variables as well as functions. Maybe we can use a name that better describes what it's used for now

Renamed to resolveScopeForHoistingVarOrFuncDeclInEval

>> Source/JavaScriptCore/runtime/JSScope.cpp:262
>> +        return scope->isVarScope() || scope->isCatchScope();
> 
> Is it specified behavior to allow var hoisting over catch variable? This seems counter intuitive. And do we want this both for functions and vars?

Oh, you are right It is wrong. We need go up throw catch block the same as 'withScope' for function, but do bind in case of variable

[GSkachkov]

Created attachment 315215 [details]
Patch

New patch

[Build Bot]

Comment on attachment 315215 [details]
Patch

Attachment 315215 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4105979

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 315225 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Saam Barati]

Comment on attachment 315215 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Current patch allows prevent the redeclaration of let/const variable 

allows prevent => prevents

> Source/JavaScriptCore/ChangeLog:15
> +        outside of eval or within eval but if top level function declared i.e:
> +        ```
> +        {
> +             let foo;
> +             eval('function foo(){}');
> +        }

I'm confused about your wording here. What happens in this example?

> Source/JavaScriptCore/ChangeLog:28
> +        to higher var scope. This case is present by following bugs: 

What do you mean by "case is present" here?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2502
> +ResolveType BytecodeGenerator::resolveType(bool doHoistInEval)

I'm not a fan of adding this parameter here. It seems like your branch on the value is wrong, it should just be the first branch in this function, since we always want it to be dynamic if doHoistInEval is true.

I sort of think it'd be cleaner if you didn't override the existing BytecodeGenerator::putToScope(.) function, and just defined your own special put_to_scope that just did exactly what you need for this special hoisting use case.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1208
> +            JSValue resolvedScope = JSScope::resolveScopeForHoistingVarOrFuncDeclInEval(callFrame, scope, function->name());

style nit: Indentation looks wrong here, but maybe it's just bugzilla.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1211
> +                if (!variableObject->hasProperty(callFrame, function->name().impl())) {

Don't you need an exception check here? Or maybe just assert that there isn't one if it's not possible since hasProperty might throw in the general case.

Also, on a more general note, why is this needed? What was wrong with the previous code?

[GSkachkov]

Created attachment 316839 [details]
Patch

Fix comments

[Saam Barati]

Comment on attachment 316839 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Current patch prevent the redeclaration of let/const variable 

prevent => prevents

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:137
> +                    RefPtr<RegisterID> topLevelObjectScope = doHoistInEval
> +                        ? emitResolveScopeForHoistingFuncDeclInEval(nullptr, metadata->ident())

Will this always resolve to the same scope for every single eval variable?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2642
> +            ResolveType type  = resolveType();

style: extra space before "="

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2670
> +    instructions().append(!!offset ? offset.offset() : 0);

offset is a constant above, what is this supposed to be doing?

[GSkachkov]

Comment on attachment 316839 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:137
>> +                        ? emitResolveScopeForHoistingFuncDeclInEval(nullptr, metadata->ident())
> 
> Will this always resolve to the same scope for every single eval variable?

Yeah, I'm expecting that they all should be bound to same var scope:

```
function f() {   
    var foo; 
    try {
        let boo;
        eval('function foo(){} function boo() {}'); 
    } catch (e) {
        print(e);
    }
   print(typeof foo === 'function'); 
   print(typeof boo==='function'); 
}
```
Binding to different scope from var scope, should be prevented by SyntaxErorr Interpreter.cpp.

But I'll double check in spec how following code should behave, I mean check if there should be:
true 
false
or 
false
false
With my patch JSC returns false-false, but Chrome (true-false) and Firefox(false-false) behave in different way.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2642
>> +            ResolveType type  = resolveType();
> 
> style: extra space before "="

Fixed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2670
>> +    instructions().append(!!offset ? offset.offset() : 0);
> 
> offset is a constant above, what is this supposed to be doing?

Ohh, You are right, this condition is unnecessary.

[GSkachkov]

Created attachment 320341 [details]
Patch

Patch

[Saam Barati]

Comment on attachment 320341 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:32
> +            try {
> +                let boo2;
> +                eval('function foo(){} function boo() {}');

Why would this throw here?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2678
> +    // put_to_scope scope, id, value, GetPutInfo, Structure, Operand

This comment doesn't match what you're emitting below.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1186
> +            if (!eval->isStrictMode()) {
> +                JSValue resolvedScope = JSScope::resolveScopeForHoistingVarOrFuncDeclInEval(callFrame, scope, ident);
> +                if (resolvedScope.isUndefined())
> +                    return checkedReturn(throwSyntaxError(callFrame, throwScope, makeString("Cannot declare a var variable that shadows a let/const/class variable: '", String(ident.impl()), "'")));
> +            }

Doesn't the spec say we need to check that none of these would throw before storing any of them into the actual scope object?

[Maciej Stachowiak]

Comment on attachment 320341 [details]
Patch

Good to make this correct, but unfortunately this patch no longer applies.

[Radar WebKit Bug Importer]

<rdar://problem/111328974>

[Alexey Shvayka]

Pull request: https://github.com/WebKit/WebKit/pull/15294

[EWS]

Committed 265614@main (d0d05739899c): <https://commits.webkit.org/265614@main>

Reviewed commits have been landed. Closing PR #15294 and removing active labels.

[Alexey Shvayka]

*** Bug 168184 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Re-opened since this is blocked by bug 259070

[Alexey Shvayka]

I'm still investigating the snippet we were crashing on, and it's super weird why

eval(`
  try {
    function foo() {}
    eval('try { function foo() {}; print("ok") } catch {} function foo() {}');
  } catch {}
`);

and

eval(`
  if (true) {
    function foo() {}
    eval('if (true) { function foo() {}; print("ok") } function foo() {}');
  } 
`);

have different semantics in V8 / SM.

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #25)
> I'm still investigating the snippet we were crashing on, and it's super
> weird why

Oh the first snippet "swallows" the SyntaxError from eval(), hence the perceived difference.

[Alexey Shvayka]

Pull request: https://github.com/WebKit/WebKit/pull/15996

[EWS]

Committed 266229@main (ab0917349c9f): <https://commits.webkit.org/266229@main>

Reviewed commits have been landed. Closing PR #15996 and removing active labels.

[Alexey Shvayka]

*** Bug 258507 has been marked as a duplicate of this bug. ***