Skip to content

Commit

Permalink
[JSC] Align duplicate declaration checks in EvalDeclarationInstantiat…
Browse files Browse the repository at this point in the history
…ion with the spec

https://bugs.webkit.org/show_bug.cgi?id=167837
<rdar://problem/111328974>

Reviewed by Yusuke Suzuki.

This change is a re-land of 265614@main with getSloppyModeHoistedFunctions() fixed not to set
IsSloppyModeHoistingCandidate bit for `var` declarations. This ensures that for a `var` binding,
which shadows a lexical declaration from an outer scope, a SyntaxError is raised even if
there is Annex B hoisted function by the same identifer.

---

For the sloppy-mode eval(), this change:

1. Removes slowish TypeError-throwing logic from executeEval() that also wasn't spec-compliant
   (SyntaxError should be raised instead), harmonizing error messages.

2. Expands resolveScopeForHoistingFuncDeclInEval() to be called for all declared variables, which
   currently includes function declarations as well, ensuring SyntaxError is thrown for duplicates
   with upper yet non-top lexical scopes [1], all while skipping CatchScopeWithSimpleParameter [2].

3. Introduces emitPutToScopeDynamic(), which circumvents default ResolveType resolution that isn't
   correct wrt skipping CatchScopeWithSimpleParameter as resolveScopeForHoistingFuncDeclInEval() does.

   We can't possibly tweak BytecodeGenerator::resolveType() to account for eval().

   This fixes both top-level and block-level function declarations to be hoisted correctly from eval()
   within simple parameter catch block by the same name.

4. Removes isExtensible() check from resolveScopeForHoistingFuncDeclInEval() because for declared
   variables, CanDeclareGlobalVar [3] is already implemented, while for Annex B hoisted functions,
   the implementation doesn't appear correct to unconditionally rely on isExtensible() even if the
   property is already present.

   Furthermore, performing CanDeclareGlobalVar in resolveScopeForHoistingFuncDeclInEval() is kinda
   superfluous given we put jsUndefined() variables in executeEval(), and results in incorrect
   error being thrown (SyntaxError instead of TypeError) if global object is non-extensible.

[1]: https://tc39.es/ecma262/#sec-evaldeclarationinstantiation (step 3.d.i.2.a.i)
[2]: https://tc39.es/ecma262/#sec-variablestatements-in-catch-blocks
[3]: https://tc39.es/ecma262/#sec-candeclareglobalvar

All JSTests changes were proven to align JSC with V8 and SpiderMonkey.

* JSTests/ChakraCore/test/Closures/bug_OS_2299723.baseline-jsc:
* JSTests/stress/const-not-strict-mode.js:
* JSTests/stress/eval-func-decl-by-the-same-name-as-callee.js: Added.
* JSTests/stress/eval-func-decl-in-eval-within-catch-scope.js:
* JSTests/stress/eval-func-decl-in-global-of-eval.js:
* JSTests/stress/eval-func-decl-within-eval-duplicate-declaration.js: Added.
* JSTests/stress/eval-func-decl-within-eval-with-reassign-to-var.js:
* JSTests/stress/eval-let-const-redeclararion.js: Added.
* JSTests/stress/global-lexical-var-injection.js:
* JSTests/stress/lexical-let-not-strict-mode.js:
* JSTests/test262/expectations.yaml: Mark 160 tests as passing.
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::generate):
(JSC::BytecodeGenerator::hoistSloppyModeFunctionIfNecessary):
(JSC::BytecodeGenerator::emitPutToScopeDynamic):
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:
* Source/JavaScriptCore/interpreter/Interpreter.cpp:
(JSC::Interpreter::executeEval):
* Source/JavaScriptCore/parser/Parser.h:
(JSC::Scope::getSloppyModeHoistedFunctions):
* Source/JavaScriptCore/runtime/JSScope.cpp:
(JSC::JSScope::resolveScopeForHoistingFuncDeclInEval):

Canonical link: https://commits.webkit.org/266229@main
  • Loading branch information
Alexey Shvayka committed Jul 22, 2023
1 parent b567579 commit ab09173
Show file tree
Hide file tree
Showing 16 changed files with 490 additions and 416 deletions.
3 changes: 2 additions & 1 deletion JSTests/ChakraCore/test/Closures/bug_OS_2299723.baseline-jsc
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
eval('var x = 5') threw 'Can't create duplicate variable in eval: 'x''
x: 5
eval('var y = 5') threw 'Attempted to assign to readonly property.'
eval('var y = 5') threw 'Can't create duplicate variable in eval: 'y''
eval('y = 5') threw 'Attempted to assign to readonly property.'
y: 1
12 changes: 6 additions & 6 deletions JSTests/stress/const-not-strict-mode.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,13 @@ function foo() {
try {
eval("var x = 20;");
} catch(e) {
if (e.name.indexOf("TypeError") !== -1 && e.message.indexOf("readonly") !== -1)
threw = true;
threw = true;
assert(e.toString() === "SyntaxError: Can't create duplicate variable in eval: 'x'");
}
assert(threw);
assert(x === 40);
}
assert(x === undefined);
assert(typeof x === "undefined");
}

function bar() {
Expand All @@ -87,13 +87,13 @@ function bar() {
try {
eval("var x = 20;");
} catch(e) {
if (e.name.indexOf("TypeError") !== -1 && e.message.indexOf("readonly") !== -1)
threw = true;
threw = true;
assert(e.toString() === "SyntaxError: Can't create duplicate variable in eval: 'x'");
}
assert(threw);
assert(x === 40);
}
assert(x === undefined);
assert(typeof x === "undefined");
}

function baz() {
Expand Down
24 changes: 24 additions & 0 deletions JSTests/stress/eval-func-decl-by-the-same-name-as-callee.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error(`Bad value: ${actual}!`);
}

(function foo(a) {
shouldBe(foo, undefined);

if (true) {
function foo(b) {}
}

shouldBe(foo.toString(), "function foo(b) {}");
})();

const bar__ = function bar(a) {
shouldBe(bar, bar__);

eval(`if (true) { function bar(b) {} }`);

shouldBe(bar.toString(), "function bar(b) {}");
};

bar__();
42 changes: 42 additions & 0 deletions JSTests/stress/eval-func-decl-in-eval-within-catch-scope.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var err = new Error();
err.e = "foo";

function assert(x) {
if (!x)
Expand All @@ -25,7 +26,31 @@ function shouldThrow(func, errorMessage) {
throw err;
} catch (e) {
eval(`function e() { return 1; }`); // no error
assert(e === err);
}
assert(e() === 1);
})();

(function() {
var e = 1;
try {
throw err;
} catch (e) {
eval(`if (true) { function e() { return 1; } }`); // no error
assert(e === err);
}
assert(e() === 1);
})();

(function() {
var e = 1;
try {
throw err;
} catch ({e}) {
eval(`if (true) { function e() { return 1; } }`); // no error
assert(e === "foo");
}
assert(e === 1);
})();

shouldThrow(function() {
Expand All @@ -36,3 +61,20 @@ shouldThrow(function() {
eval(`function e() { return 1; }`); // syntax error
}
}, "SyntaxError: Can't create duplicate variable in eval: 'e'");

shouldThrow(function() {
var e = 2;
try {
throw err;
} catch ({e}) {
eval(`var e = 1;`); // syntax error
}
}, "SyntaxError: Can't create duplicate variable in eval: 'e'");

shouldThrow(function() {
try {
throw err;
} catch ({...e}) {
eval(`var e;`); // syntax error
}
}, "SyntaxError: Can't create duplicate variable in eval: 'e'");
43 changes: 43 additions & 0 deletions JSTests/stress/eval-func-decl-in-global-of-eval.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,46 @@ function foobar() {
foobar();
assertThrow(() => g, "ReferenceError: Can't find variable: g");

(function() {
try {
let b;
let c;
eval('var a; var b; var c;');
} catch (e) {
var error = e;
}

assert(error.toString(), "SyntaxError: Can't create duplicate variable in eval: 'c'");
assertThrow(() => a, "ReferenceError: Can't find variable: a");
assertThrow(() => b, "ReferenceError: Can't find variable: b");
assertThrow(() => c, "ReferenceError: Can't find variable: c");
})();

(function() {
try {
let x1;
eval('function x1() {} function x2() {} function x3() {}');
} catch (e) {
var error = e;
}

assert(error.toString(), "SyntaxError: Can't create duplicate variable in eval: 'x1'");
assertThrow(() => x1, "ReferenceError: Can't find variable: x1");
assertThrow(() => x2, "ReferenceError: Can't find variable: x2");
assertThrow(() => x3, "ReferenceError: Can't find variable: x3");
})();

(function() {
var x3;
try {
let x2;
eval('function x1() {} function x2() {} function x3() {}');
} catch (e) {
var error = e;
}

assert(error.toString(), "SyntaxError: Can't create duplicate variable in eval: 'x2'");
assertThrow(() => x1, "ReferenceError: Can't find variable: x1");
assertThrow(() => x2, "ReferenceError: Can't find variable: x2");
assert(x3, undefined);
})();
37 changes: 37 additions & 0 deletions JSTests/stress/eval-func-decl-within-eval-duplicate-declaration.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error(`Bad value: ${actual}!`);
}

function shouldThrow(func, expectedError) {
let errorThrown = false;
try {
func();
} catch (error) {
errorThrown = true;
if (error.toString() !== expectedError)
throw new Error(`Bad error: ${error}!`);
}
if (!errorThrown)
throw new Error("Didn't throw!");
}

(() => {
eval(`
try {
function foo(a) {}
eval('try { function foo(b) {} } catch {} function foo(c) {}');
} catch {}
`);

shouldBe(foo.toString(), "function foo(a) {}");
})();

shouldThrow(() => {
eval(`
if (true) {
function foo(a) {}
eval('if (true) { function foo(b) {} } function foo(c) {}');
}
`);
}, "SyntaxError: Can't create duplicate variable in eval: 'foo'");
23 changes: 13 additions & 10 deletions JSTests/stress/eval-func-decl-within-eval-with-reassign-to-var.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,8 @@ for (var i = 0; i < 10000; i++){
assertThrow(() => _bar, "ReferenceError: Can't find variable: _bar");
}

// Fixme: https://bugs.webkit.org/show_bug.cgi?id=167837
// Current test does not work because it should raise exception
// that f could not be redeclared
/*
function goo() {
{
var error = false;
Expand All @@ -79,13 +77,14 @@ for (var i = 0; i < 10000; i++) {
goo();
assert(typeof f, "undefined", "#7");
}
*/

function hoo() {
{
let h = 20;
eval('var h = 15; eval(" if (false){ function h() { }; } ");');
assert(h, 15);
try { eval('var h = 15;'); } catch (e) { var evalError = e; }
assert(evalError.toString(), "SyntaxError: Can't create duplicate variable in eval: 'h'");
eval('eval("if (false) { function h() {} } ");');
assert(h, 20);
}
assert(typeof h, "undefined");
}
Expand Down Expand Up @@ -124,12 +123,16 @@ for (var i = 0; i < 10000; i++){
}

function loo() {
let h = 20;
eval("var h; if (false) { function h() { } }");
return h;
}
let error;
try {
let h = 20;
eval("var h; if (false) { function h() { } }");
} catch (e) {
error = e;
}

assert(loo(), 20);
assert(`${error}`, "SyntaxError: Can't create duplicate variable in eval: 'h'", "#13");
}

for (var i = 0; i < 10000; i++) {
loo();
Expand Down
Loading

0 comments on commit ab09173

Please sign in to comment.