1694518 - Console does not allow await in a top level block

Status: ASSIGNED

(DevTools :: Console, defect, P3)

Description

[Robin Steuber (they/them) [:bytesized]]

STR:

1. Open the Browser Console (Control+Shift+J)
2. Enter this code: await OS.File.read("C:\\mozilla-build\\VERSION");
3. Observe that this runs, as expected.
4. Enter this code: try { await OS.File.read("C:\\mozilla-build\\VERSION"); } catch (ex) {}

Expected Result:
The second line of code entered should have the same effect as the first line, except that any error should be caught.

Actual Result:
The second line of code entered gives this error:

Uncaught SyntaxError: await is only valid in async functions and async generators

I'm using Nightly on Windows 10.

Comment 1

[Julian Descottes [:jdescottes]]

Nicolas, can you add some pointers to fix this bug?

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

This isn't Browser Console only. You can reproduce it with

{ await 1 }

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Did some investigation on this.
This fails here: devtools/client/debugger/src/workers/parser/mapAwaitExpression.js#187-188 , where the expression is considered to not have a top-level await expression

We are doing this check to avoid rewriting any function declared in the expression for example, but here it's failing.
The check we do is pretty basic in devtools/client/debugger/src/workers/parser/mapAwaitExpression.js#14 : from the computed AST, we go through the different nodes, and check if we have a top-level await one.

And here is the logic for the "isTopLevel" part devtools/client/debugger/src/workers/parser/utils/helpers.js#180-184

So here the code is inacurate. We might want to check if we don't have body with a type of FunctionDeclaration
This doesn't seem to be enough, and wrapExpressionFromAst should probably be modified as well.

Comment 4

[Sai Hemanth]

Hi Nicolas, this bug looks interesting, and I'd love to work on it. I don't have any experience in debugger, but I've worked on a few good first bugs in other panels. I might need some guidance to get started. Let me know if this task requires someone with better understanding.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Hello Sai, this might require some knowledge about AST, but you can probably learn along the way :)
This used the parser worker, so if you work in the files I pointed to in Comment 3, you'd need to update the worker bundle
This is done by doing cd devtools/client/debugger && yarn && node bin/bundle.js.

I'm assigning the byg to you for now, let me know if you need any help :)

Comment 6

[Sai Hemanth]

Thanks for assigning, Nicolas. Adding upon comment #3, following are my observations,

isTopLevel returns true if exactly on of the AwaitExpression's ancestor is of key "body". I assume this was implemented because Program node is assigned the key "body". However, even BlockStatement nodes are assigned the same key. So, in the case of {await 1}, the await expression has two ancestors with key "body", Program and BlockStatement.

isTopLevel can be updated from

return ancestors.filter(ancestor => ancestor.key == "body").length == 1;

to

Comment 7

[Sai Hemanth]

Thanks for assigning, Nicolas. Adding upon comment #3, following are my observations,

isTopLevel returns true if exactly on of the AwaitExpression's ancestor is of key "body". I assume this was implemented because Program node is assigned the key "body". However, even BlockStatement nodes are assigned the same key. So, in the case of {await 1}, the await expression has two ancestors with key "body", Program and BlockStatement.

I think isTopLevel can be updated from

return ancestors.filter(ancestor => ancestor.key == "body").length == 1;

to

return !ancestors.some(ancestor => t.isFunction(ancestor.node));

It seems to be working for most variations of top level await I can think of.
I'll start looking into wrapExpressionFromAst once you confirm if the proposed change looks good, and if I'm on the right track.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Sai Hemanth from comment #7)

Thanks for assigning, Nicolas. Adding upon comment #3, following are my observations,

isTopLevel returns true if exactly on of the AwaitExpression's ancestor is of key "body". I assume this was implemented because Program node is assigned the key "body". However, even BlockStatement nodes are assigned the same key. So, in the case of {await 1}, the await expression has two ancestors with key "body", Program and BlockStatement.

I think isTopLevel can be updated from

return ancestors.filter(ancestor => ancestor.key == "body").length == 1;

to

return !ancestors.some(ancestor => t.isFunction(ancestor.node));

It seems to be working for most variations of top level await I can think of.
I'll start looking into wrapExpressionFromAst once you confirm if the proposed change looks good, and if I'm on the right track.

Sorry for the delay Sai.
Yes please go ahead. If it seems good from your manual testing, it's worth putting a patch to review (see contribution_quickref.html#to-submit-a-patch )

Comment 9

[Sai Hemanth]

Attachment: Bug 1694518 - Update top level await expression logic. r=nchevobbe

Comment 10

[microrffr]

I noticed I also get this error with the following:

let a = await 0;
for (const v of []) { }
let b = 1;

Does that fit in to the understanding here?

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

(In reply to microrffr from comment #10)

I noticed I also get this error with the following:

let a = await 0;
for (const v of []) { }
let b = 1;

Does that fit in to the understanding here?

This looks like a different issue, I filed Bug 1773627 to investigate

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 15

[Artem Manushenkov]

Hi! I'd like to take this @nchevobbe

I've been working on this today and there's more to it that meets the eye initially. Seems like in some cases we'll need to recursively/iteratively add returnStatement to all block statements, for example in nested if statement like if (cond1) { if (cond2) { 3 } else { await 4 } } else { await 2 } (it can go very deep, and we don't know which BlockStatement will yield us the result, hence we need to add it to every BlockStatement).

if we have this kind of deep if nesting without await it works fine currently, but i couldn't figure how it does it haha. if you could point out how/where maybe a better way to handle the nested return await will become apparent

Also something weird happened with one test i did

async function asyncFunc() {return 'hi'}

1. try {1; asyncFunc()} catch(e) { } returns promise
2. try {await 1; asyncFunc()} catch(e) { } returns 'hi'
3. try {1; await asyncFunc()} catch(e) { } returns 'hi'
4. try {await 1; await asyncFunc()} catch(e) { } 'returns hi'

1,3,4 work fine, 2 acts weirdly
after all the mappings at the end of mapExpression function it turns into
(async () => { try { await 1; return asyncFunc(); } catch (e) { }})()
But when it runs it doesn't return a promise, as it should, like the first test does, or if you just take it and run it in node/console. i think I'm missing some further manipulation done to it

And a couple of partially relevant things to the bugticket:

• how do i run the tests in the worker folder? client/debugger/src/workers/parser/tests i can't get them to run with ./mach test

• when i do node bin/watch.js i get Error: Cannot find module 'shelljs'.

• I don't see the worker build process documented anywhere, what you described cd devtools/client/debugger && yarn && node bin/bundle.js, i'd like to write it somewhere after we figure out the bug

Comment 16

[Artem Manushenkov]

Attachment: Bug 1694518 - fix top-level-await for await's nested in block statements. r=nchevobbe

Comment 17

[Artem Manushenkov]

I submitted what i've done so far, so that try {await 1; asyncFunc()} catch(e) { } could be reproduced. i just copied the tests from the last revision so far so don't mind them

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Hey Artem, thanks for looking into this
I can't look at the parser questions at the moment, but I can answer questions from the last section

(In reply to Artem Manushenkov from comment #15)

• how do i run the tests in the worker folder? client/debugger/src/workers/parser/tests i can't get them to run with ./mach test

from devtools/client/debugger, run yarn once to install npm packages, and then you can run tests with yarn test parser/tests/

I don't see the worker build process documented anywhere, what you described cd devtools/client/debugger && yarn && node bin/bundle.js, i'd like to write it somewhere after we figure out the bug

It's mentioned in https://searchfox.org/mozilla-central/rev/109bb25545f0d2df31954dc0a9afbf30d900b6bb/devtools/docs/contributor/tests/node-tests.md#67-76 :)