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" }
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
Created attachment 314583 [details] Patch Patch
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?
Created attachment 314651 [details] Patch Fix comments
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 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?
Created attachment 314925 [details] Patch Fixed comments
Created attachment 315176 [details] Patch Fix comments
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
Created attachment 315215 [details] Patch New patch
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
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 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?
Created attachment 316839 [details] Patch Fix comments
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 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.
Created attachment 320341 [details] Patch Patch
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 on attachment 320341 [details] Patch Good to make this correct, but unfortunately this patch no longer applies.
<rdar://problem/111328974>
Pull request: https://github.com/WebKit/WebKit/pull/15294
Committed 265614@main (d0d05739899c): <https://commits.webkit.org/265614@main> Reviewed commits have been landed. Closing PR #15294 and removing active labels.
*** Bug 168184 has been marked as a duplicate of this bug. ***
Re-opened since this is blocked by bug 259070
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.
(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.
Pull request: https://github.com/WebKit/WebKit/pull/15996
Committed 266229@main (ab0917349c9f): <https://commits.webkit.org/266229@main> Reviewed commits have been landed. Closing PR #15996 and removing active labels.
*** Bug 258507 has been marked as a duplicate of this bug. ***