Bug 167837 - [JSC] Align duplicate declaration checks in EvalDeclarationInstantiation with the spec
Summary: [JSC] Align duplicate declaration checks in EvalDeclarationInstantiation with...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
: 168184 258507 (view as bug list)
Depends on: 163208 259070 259073
Blocks: 260484
  Show dependency treegraph
 
Reported: 2017-02-04 09:29 PST by GSkachkov
Modified: 2023-08-21 13:52 PDT (History)
9 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
mjs: 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 hidden (obsolete)
Comment 12 Build Bot 2017-07-12 03:16:04 PDT Comment hidden (obsolete)
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?
Comment 19 Maciej Stachowiak 2020-05-30 19:46:08 PDT
Comment on attachment 320341 [details]
Patch

Good to make this correct, but unfortunately this patch no longer applies.
Comment 20 Radar WebKit Bug Importer 2023-06-26 07:55:59 PDT
<rdar://problem/111328974>
Comment 21 Alexey Shvayka 2023-06-26 07:59:43 PDT
Pull request: https://github.com/WebKit/WebKit/pull/15294
Comment 22 EWS 2023-06-29 03:01:01 PDT
Committed 265614@main (d0d05739899c): <https://commits.webkit.org/265614@main>

Reviewed commits have been landed. Closing PR #15294 and removing active labels.
Comment 23 Alexey Shvayka 2023-06-29 14:21:37 PDT
*** Bug 168184 has been marked as a duplicate of this bug. ***
Comment 24 WebKit Commit Bot 2023-07-10 13:41:30 PDT
Re-opened since this is blocked by bug 259070
Comment 25 Alexey Shvayka 2023-07-18 07:56:34 PDT Comment hidden (obsolete)
Comment 26 Alexey Shvayka 2023-07-18 08:05:13 PDT Comment hidden (obsolete)
Comment 27 Alexey Shvayka 2023-07-21 12:45:44 PDT
Pull request: https://github.com/WebKit/WebKit/pull/15996
Comment 28 EWS 2023-07-22 07:01:11 PDT
Committed 266229@main (ab0917349c9f): <https://commits.webkit.org/266229@main>

Reviewed commits have been landed. Closing PR #15996 and removing active labels.
Comment 29 Alexey Shvayka 2023-07-24 07:13:12 PDT
*** Bug 258507 has been marked as a duplicate of this bug. ***