repl: support top-level await

[TimothyGu]

This PR depends upon and is rebased on top of #16392.

This PR endeavors to fix #13209, following the footsteps of Google Chrome.

Algorithmically, it uses much of the same strategies as the Chromium CL -- namely, using a JS parser (Acorn) and then transforming the to-be-executed source code based on that AST. In fact, the AST node visitors are pretty much a direct port of DevTools code from DevTools' homebrew AST walker to one supplied by Acorn.

Unlike the usual parse-transform-serialize pipeline (i.e. "the Babel way"), however, I decided to adopt "the Bublé way", which is to mutate the source code directly based on the locations in the AST. While by design this approach is more fragile than a full AST serialization, it is 1) much faster, 2) requires less bundled code, 3) the approach used by Chrome DevTools, and 4) just enough for this specific use case, because we know transformed code does not need to be transformed again thereafter.

A huge focus of #13209 (and a must-have) is support for await expressions used in variable declarations and assignments. Like DevTools, this PR supports variable declarations by transforming them into assignments. Take

var a = await func();

it is transformed to

(async () => { void (a = await func();) })()

While this works well for var, it doesn't for top-level let and const (let and const still work as expected in blocks), which will lose their lexical specialness (or constantness) with this approach. However, I simply can't think of any good way to support top-level let and const fully. (Note, DevTools has the same problem; try const a = await Promise.resolve(1); back-to-back multiple times.) If you've got an idea, please comment!

The implementation in this PR is fully complete with the above caveat (though a comment or two can perhaps be added in the right places). However, tests still need to be added, especially around the SIGINT handling part, which turned out to be much more complex than the actual source transformation. For source code transformation, Chromium has two very comprehensive sets of tests that we can borrow:

• https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/devtools/console/console-top-level-await.js
• https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/tests/inspector-unit/preprocess-top-level-awaits-expected.txt

Anyway, please test and report back how it works!

Many thanks to @ak239 for his DevTools implementation; wouldn't have been able to finish this without his work!

/cc @benjamingr @princejwesley

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

repl, readline, deps

[ghost]

Can you write a small example where let and const are failing?

[benjamingr]

Great work! This is the right approach IMO.

Is everything with acorn OK license wise?

[TimothyGu]

@Inf3rno

Can you write a small example where let and const are failing?

Sure.

const a = await Promise.resolve(1);

Enter

a = 3;

Expected: TypeError: Assignment to constant variable.

Actual: 3

[TimothyGu]

@benjamingr Acorn uses the MIT License so our licenses are compatible. I've also added Acorn to tools/license-builder.sh and the root-level LICENSE file.

[addaleax]

I mean, Node’s REPL already has its problems with top-level scope const and let … if we’re starting to transform anyway, is replacing those a viable option?

(Btw, I would like it if the REPL just allowed overwriting const variables …)

[TimothyGu]

@addaleax

Node’s REPL already has its problems with top-level scope const and let …

That is certainly true. (e.g. #13919)

if we’re starting to transform anyway, is replacing those a viable option?

(Btw, I would like it if the REPL just allowed overwriting const variables …)

Technically yes, but I'm not convinced yet that's a good idea as it goes against the keywords' intended semantics, and what Chrome is doing. It sounds like a slippery slope.

[benjamingr]

+1 for allowing writing over const variables

[Qard]

REPL is an environment meant for experimentation. I feel like true const-ness actually doesn't really make much sense there anyway.

[ghost]

@TimothyGu

Is it transformed to the following?

// const a = await Promise.resolve(1);
// a = 3

(async () => { void (a = await Promise.resolve(1)) })()
a = 3

If every await gets its own asnyc function wrapper, then I don't think we can do much about it. Still I think it is worth breaking this const feature. Maybe we should allow only var by await and send an error message if somebody tries to use const or let with await.

I am not that experienced with this REPL project, but if this

var a = await Promise.resolve(1);
console.log(a); // 1
a = 3;
console.log(a); // 3

is transformed into this:

(async () => { void (a = await Promise.resolve(1)) })()
// console.log(a); // ReferenceError - a is not declared

a = 3
console.log(a); // 3

// setTimeout(() => {
//	console.log(a); // 1
// }, 10);

then I think we have a problem, because I'd expect this behavior:

(async () => {
	var a = await Promise.resolve(1);
	console.log(a); // 1
	
	a = 3;
	console.log(a); // 3
})()

To make it work this way we need to change the REPL state to pending until the promise is fulfilled and continue with the next line only after that. I am not sure whether that would be an acceptable behavior. Another possible solution would be to display a message if the promise is fulfilled and you can use the results. If you choose that one, then it might be beneficial to prevent the user from setting a new value to the variable before it gets its value from the promise. Maybe I am misunderstanding something, because I don't have a testing environment to check your commit and tbh. I don't fully understand it, but this might be a harder problem than just wrapping the actual line.

[princejwesley]

@TimothyGu Behaviour is difference in canary.

[TimothyGu]

@princejwesley

Behaviour is difference in canary.

I'm sorry, I don't quite understand what you mean. The code you put in Canary's DevTools and the output you got is the exact same as what you would get from this PR.

> const b = await 4
undefined
> b
4
> const c = await Promise.resolve(1)
undefined
> c
1
> c = 4
4
> c
4

[princejwesley]

@TimothyGu
can we translate var|const|let a = await func(); into

(async (self) => {
  var $ret = await func();

  // const case
  Object.defineProperty( self, 'a', {
      get: function () {
          return $ret;
      },
      set: function() {
          throw new TypeError('Assignment to constant variable')
      }
  });

  // let re-declaring case
  // if self has 'a', throw SyntaxError
  Object.keys(self).findIndex((key) => key === 'a') !== -1 &&  throw new SyntaxError("Identifier 'a' has already been declared");

  // let and var case

  self['a'] = $ret;

  void 0;

})(global);

?

[TimothyGu]

@Inf3rno

I am not that experienced with this REPL project, but if this

var a = await Promise.resolve(1);
console.log(a); // 1
a = 3;
console.log(a); // 3

is transformed into this:

(async () => { void (a = await Promise.resolve(1)) })()
// console.log(a); // ReferenceError - a is not declared

a = 3
console.log(a); // 3

// setTimeout(() => {
//	console.log(a); // 1
// }, 10);

then I think we have a problem

No it doesn't actually transform to that. After you enter the first line, the REPL will PAUSE until the promise returned from the async function is resolved. So when you can enter the next line, a has to be defined already.

> var a = await Promise.resolve(1);
undefined
> console.log(a)
1
undefined
> a = 3
3
> console.log(a)
3
undefined

---

Again, I encourage y'all to actually build this version and test it out. I'm fine with answering questions, but the fastest way to answer those questions are to find out about them yourselves 😘

[TimothyGu]

@princejwesley No, because that would break cases like

Wrong stuff, ignore

var a = 0
let a = await 0

[TimothyGu]

@princejwesley let and const are not supposed to influence the global object at all.

[princejwesley]

@TimothyGu I am just trying to mimic the let/const/var behaviour with the context.

[TimothyGu]

@princejwesley I got you. While there are indeed some creative ways to do that, seeing how people seem to not like the restrictions of let and const at all in REPL (1/2/3) I don't think we should try to make it worse ;)

[ghost]

@TimothyGu Okay, thanks!

[mcollina]

I think we should revert.

[bmeck]

@benjamingr I'm not opposing the feature. I just favor reverting until the feature is more consistent so we don't land it in a release with odd behavior we are stuck with.

[cjihrig]

I'm in favor of this feature, but am also +1 on reverting until this is better sorted out.

If we do decide to go the revert route, here is a PR: #17807

[TimothyGu]

we don't land it in a release with odd behavior we are stuck with

There are no REPL implications we would be “stuck” with, since any fix to REPL would be a semver-patch change.

The special async mode is certainly not ideal, but the way we are operating regarding source code transformation is identical to how DevTools is; and it is my strong belief that if something is good enough for the most widely used browser in the world, it should be good enough for Node.js (not saying it couldn’t be better).

I’m fine with putting this behind a flag, but given the positive developer feedback we have received so far, and the strictly additive nature of this feature (it doesn’t break any existing use cases) I am against reverting this wholesale.

[bmeck]

The special async mode is certainly not ideal, but the way we are operating regarding source code transformation is identical to how DevTools is; and it is my strong belief that if something is good enough for the most widely used browser in the world, it should be good enough for Node.js (not saying it couldn’t be better).

V8 also have an open issue on moving to a more native/feature safe without code transform to do this. Given that they are seeking to change things as well, I'm not swayed that because it is broken other places it should be ok to be broken all the places.

I’m fine with putting this behind a flag, but given the positive developer feedback we have received so far, and the strictly additive nature of this feature (it doesn’t break any existing use cases) I am against reverting this wholesale.

That seems fine, I wouldn't want it unflagged until stuff is ironed out. Some of the behavior does need to be specified like how this mode made const not const, allowed variable re-binding, and is using the global object instead of scope.

[jedwards1211]

Now that I see how this is done, it looks like this could easily be done in userland with Node 8, right?

[bmeck]

@jedwards1211 it is non-trivial and I am having to patch internals on node's repl to fix various things. I think it is unlikely to be able to be done completely in userland without something very large in scope that reimplements a large part of the repl.

[jdalton]

@jedwards1211

Now that I see how this is done, it looks like this could easily be done in userland with Node 8, right?

Go for it! I'd love to see what userland comes up with 🙌

[jedwards1211]

@bmeck The following hack works okay in my personal scripts. Obviously it's not production quality:

    if (/\bawait\b/.test(cmd) && !/\basync\b/.test(cmd)) {
      const match = /^\s*var\b(.*)/.exec(cmd)
      if (match) {
        cmd = `(async () => { void (${match[1]}) })()`
      } else {
        cmd = `(async () => { return ${cmd} })()`
      }
    }

What part of the repl are you having to reimplement?

[bmeck]

@jedwards1211 if you enable such a transform to always be present repl.js most of the test suite for REPL fails. In the case of the specific above it has problems for multiple statement commands. In the case of the top level await transform that landed various things are problematic: see comment above , rewriting the pause mechanic is pretty significant and making sure that none of the internals rely on synchronous behavior is a main reason why the test suite fails. The transform that landed side stepped the failures by splitting the REPL into two evaluation modes and did not run the existing tests against the new mode, running the transform in the test suite you can see lots of problems.

[jedwards1211]

@bmeck I'm confused why a pause mechanic was necessary, since the eval option accepts a callback, which was sufficient for what I'm doing in userland.

[bmeck]

@jedwards1211 there are 3 pause mechanics in repl

1. the readline interface.pause()

Which is managing interrupt events from TTY for the most part.

2. the readable stream.pause()

Which is managing backpressure for the most part.

3. the internal REPL pause system

Which is intended to queue lines while paused, but doesn't do so if you use APIs like replserver.eval or fire events like 'line' on the other 2 pausable mechanisms directly. It does not fully queue things during an asynchronous eval.

Edit To see more complete examples of this behavior not being fully paused in the current mechanics pipe into the repl using cat multi-line-file | node -i and mix await with other things. (irc log link)

[wmertens]

@jedwards1211 sorry to bother you, I'd like to use your hack, but I don't know where this code should go?

[jedwards1211]

@wmertens it goes within the eval function you pass when creating the repl