Bug 167837 - [JSC] Throw exception in case of redeclaration let variable in eval
Summary: [JSC] Throw exception in case of redeclaration let variable in eval
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: GSkachkov
URL:
Keywords:
Depends on: 163208
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-04 09:29 PST by GSkachkov
Modified: 2017-10-04 12:11 PDT (History)
5 users (show)

See Also:


Attachments
Patch (23.82 KB, patch)
2017-07-04 16:01 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (25.01 KB, patch)
2017-07-05 13:49 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (29.32 KB, patch)
2017-07-08 13:35 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (48.09 KB, patch)
2017-07-11 15:40 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (41.27 KB, patch)
2017-07-12 01:34 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (7.40 MB, application/zip)
2017-07-12 03:16 PDT, Build Bot
no flags Details
Patch (41.88 KB, patch)
2017-08-01 03:42 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (43.40 KB, patch)
2017-09-09 04:17 PDT, GSkachkov
gskachkov: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2017-02-04 09:29:52 PST
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"
}
Comment 2 GSkachkov 2017-07-04 16:01:58 PDT
Created attachment 314583 [details]
Patch

Patch
Comment 3 Saam Barati 2017-07-04 19:29:56 PDT
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?
Comment 4 GSkachkov 2017-07-05 13:49:16 PDT
Created attachment 314651 [details]
Patch

Fix comments
Comment 5 GSkachkov 2017-07-05 13:51:56 PDT
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
Comment 6 Saam Barati 2017-07-06 14:01:16 PDT
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?
Comment 7 GSkachkov 2017-07-08 13:35:10 PDT
Created attachment 314925 [details]
Patch

Fixed comments
Comment 8 GSkachkov 2017-07-11 15:40:12 PDT
Created attachment 315176 [details]
Patch

Fix comments
Comment 9 GSkachkov 2017-07-11 15:51:44 PDT
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
Comment 10 GSkachkov 2017-07-12 01:34:07 PDT
Created attachment 315215 [details]
Patch

New patch
Comment 11 Build Bot 2017-07-12 03:16:02 PDT
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
Comment 12 Build Bot 2017-07-12 03:16:04 PDT
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
Comment 13 Saam Barati 2017-07-25 23:24:51 PDT
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?
Comment 14 GSkachkov 2017-08-01 03:42:14 PDT
Created attachment 316839 [details]
Patch

Fix comments
Comment 15 Saam Barati 2017-08-29 14:47:05 PDT
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?
Comment 16 GSkachkov 2017-09-05 12:44:31 PDT
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.
Comment 17 GSkachkov 2017-09-09 04:17:14 PDT
Created attachment 320341 [details]
Patch

Patch
Comment 18 Saam Barati 2017-10-04 12:11:07 PDT
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?