Bug 223533 – [JSC] Redeclaring parameter of a generator / async function makes it `undefined`

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Martin]

The following code prints undefined in the console:

async function test(a) {
  console.log(a);
  var a;
}

test("a");

It should print "a" to the console as it does in firefox and chrome.

In fact, if the function is not async it will print "a" as expected.

While I don't recommend writing code like this, this code was produced by our minifier and it did lead to the chat feature on our website being unavailable for all safari users. The code worked as expected on v8, therefore nothing was picked up by our tests.

[Radar WebKit Bug Importer]

<rdar://problem/75899149>

[Ingvar Stepanyan]

We ran into this breaking on code generated by Emscripten too. https://github.com/emscripten-core/emscripten/issues/18357

Emscripten will have to add a workaround, but, considering that this is a core JS bug, it would be great to fix this on the JavaScriptCore side too.

[Ross Kirsling]

This is actually even weirder than the above example suggests, as it's not actually the value of the var that's being printed -- the following *still* prints `undefined`:

```
async function test(foo) { print(foo); var foo = 42; }
test('blah');
```

[Ross Kirsling]

(In reply to Ross Kirsling from comment #3)
> This is actually even weirder than the above example suggests, as it's not
> actually the value of the var that's being printed -- the following *still*
> prints `undefined`:
> 
> ```
> async function test(foo) { print(foo); var foo = 42; }
> test('blah');
> ```

Oh wait no, that's just me forgetting that it's only the declaration itself that gets hoisted. My bad.

[Sam Clegg]

This bug is effecting emscripten users: https://github.com/emscripten-core/emscripten/issues/18357.

It seems that we output this pattern in some modes and the code works on chrome and firefox, but not safari.

[Ross Kirsling]

(Moving my December investigation comments over from Slack...)


This is a surprisingly tricky matter -- it's basically rooted in bug 164087, the fact that our implementation separates async and generator functions into "wrapper" and "body" functions.

This separation makes it incorrect for us to hoist var decls to the top of the body, since:
> async function f(foo) { print(foo); var foo; } f('blah');
would have hoisting behavior analogous to...
> function f(foo) { (function () { print(foo); var foo; })(); } f('blah');
when it would need to be like...
> function f(foo) { (function () { print(foo); })(); var foo; } f('blah');

It's not entirely clear what the appropriate solution is here.

1. If we really view the wrapper/body separation as a mistake, then the current problem would just be a direct result of that mistake.

2. If not, then we need magic to happen, relative to normal nested function behavior...but the magic can't depend on actually *checking* whether the var shadows a param, because we literally don't have that information when reparsing the body (i.e. hasDeclaredParameter isn't valid to call at that timing because the wrapper scope, where the params actually live, is already gone).

2a. Like, we can pretend that the real var decl was up in the wrapper scope and "unconditionally use" it in the body instead, but this falls apart for cases where there wasn't actually a param to capture (i.e. we end up setting to a global).

2b. So maybe it's that the param should come down to the body scope...but I'm not quite sure how we'd mean for this to happen. It's clear that a lot of work was done in order to propagate the `arguments` object down into the body, but then we're also taking great care to not bring any unnecessary individual params down, so it seems like we just end up at the same place: the body needs to be able to say that a given param is "necessary".

[Ross Kirsling]

Test cases below.

# Control cases:

function test0(foo) { print(foo); var foo = 42; } test0('blah');
const test0a = (foo) => { print(foo); var foo = 42; }; test0a('blah');

# Bugged cases:

async function test1(foo) { print(foo); var foo = 42; } test1('blah');
const test1a = async (foo) => { print(foo); var foo = 42; }; test1a('blah');

function* test2(foo) { print(foo); var foo = 42; } test2('blah').next();
async function* test3(foo) { print(foo); var foo = 42; } test3('blah').next();

[Sam Clegg]

FWIW emscripten now avoids this pattern: https://github.com/emscripten-core/emscripten/pull/18482

[Theodore Brown]

I also ran into this issue since UglifyJS frequently declares a variable with the same name as a parameter when minifying code.

[Alexey Shvayka]

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

[Alexey Shvayka]

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

[Jarred Sumner]

Tracking issue in Bun's github repository: https://github.com/oven-sh/bun/issues/6663

[Alexey Shvayka]

Created attachment 468990 [details]
Patch

[Alexey Shvayka]

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

[EWS]

Committed 272666@main (8f6b2012d16c): <https://commits.webkit.org/272666@main>

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

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