repl: support mult-line string-keyed objects #21805

rubys · 2018-07-13T22:50:13Z

isRecoverableError is completely reimplemented using acorn and an
acorn plugin that examines the state of the parser at the time of the
error to determine if the code could be completed on a subsequent line.

fixes #21657

Checklist

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

vsemozhetbyt · 2018-07-14T07:59:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15871/

jdalton · 2018-07-14T17:17:07Z

lib/repl.js

@@ -47,10 +47,11 @@ const {
  makeRequireFunction,
  addBuiltinLibsToObject
 } = require('internal/modules/cjs/helpers');
+const acorn = require('internal/deps/acorn/dist/acorn');
 const {


Should these additions be placed near the other acorn work in lib/internal/repl/?

I'm not sure what you mean by these additions here. The require isn't new, just the capturing of the name as acorn is new. More generally, isRecoverableError isn't new, just completely rewritten.

Perhaps you are suggesting that isRecoverableError be split out into a separate file and placed into lib/internal/repl/? If so, I can do that.

I'm suggesting keeping the acorn extensions and other acorn bits closer together.

jdalton · 2018-07-14T17:18:12Z

lib/repl.js

+        nextToken.call(this, pos, message);
+
+        if (parser.type === acorn.tokTypes.eof) recoverable = true;
+      };


acorn.tokTypes is commonly stored in a var called tt in acorn plugins.
So then it would be tt.eof.

jdalton · 2018-07-14T17:19:43Z

lib/repl.js

+        nextToken.call(this, pos, message);
+
+        if (parser.type === acorn.tokTypes.eof) recoverable = true;
+      };


I believe parse.type can be this.type

jdalton · 2018-07-14T17:20:39Z

lib/repl.js

+      return function(pos, message) {
+        switch (message) {
+          case 'Unterminated template':
+          case 'Unterminated comment':


👆 Might put a note that these message strings are subject to change and to periodically check in on them.

I'll ponder how to word that. Meanwhile, I'll note that should these message strings change, a test will fail.

We have to manually update acorn to run into this issue. Having a failed test in such a circumstance would be fine and we'd have to invest that one way or the other. So I guess it is fine to keep it is as.

jdalton · 2018-07-14T17:22:38Z

lib/repl.js

+  // here as the point is to test for potentially valid but incomplete
+  // expressions.
+  if (/^\s*\{/.test(code) && isRecoverableError(e, `(${code}`)) return true;
+


👆 Acorn has a thorough regexp for whitespace. Might use that for the leading optional whitespace check.

Here I think it is very important that the regular expression exactly match the one in defaultEval as it would be problematic for this code to treat a given input as recoverable only to later find out that defaultEval would consider it to be a syntax error when done.

jdalton · 2018-07-14T17:24:46Z

lib/repl.js

+
+          case 'Unterminated string constant':
+            const token = parser.input.slice(parser.lastTokStart, parser.pos);
+            recoverable = token.endsWith('\\\n');


Same nit as above, inconsistent use of parser vs. this.

jdalton · 2018-07-14T17:25:56Z

lib/repl.js

+    parser.extend('nextToken', (nextToken) => {
+      return function(pos, message) {
+        nextToken.call(this, pos, message);
+


Instead of nextToken.call and other .call's we should have const ReflectApply = Reflect.apply and then use it.

For my education, can I ask why? First, I don't see const ReflectApply = Reflect.apply as a common pattern inside of the node codebase (I only found two occurrences, and even those two differed). Second, I believe that .call is more concise and clearer (at a minimum, I wouldn't need to put [] around the arguments). What am I missing?

Since core is not run in its own isolated context but in the one shared with the user, its builtins are the same ones that user-land can touch and interact with. Grabbing references to builtin prototype methods and static builtin methods before user-land code executes is an attempt to strengthen the core against accidental user-land augmentation. While handled inconsistently in Node core, new code added seems to have these guards. It's reasonable, where possible, to keep that up.

The use of ReflectApply covers the Function#call case.

@jdalton there was a long discussion around this but no conclusion. As long as this is not something everyone agrees to I think it is best to keep these things to the PR openers.

BridgeAR

LGTM. Great work @rubys and thanks a lot for taking up my suggestion.

BridgeAR · 2018-07-15T00:26:36Z

lib/repl.js

+    parser.extend('nextToken', (nextToken) => {
+      return function(pos, message) {
+        nextToken.call(this, pos, message);
+


@jdalton there was a long discussion around this but no conclusion. As long as this is not something everyone agrees to I think it is best to keep these things to the PR openers.

BridgeAR · 2018-07-15T00:28:16Z

lib/repl.js

+      return function(pos, message) {
+        switch (message) {
+          case 'Unterminated template':
+          case 'Unterminated comment':


We have to manually update acorn to run into this issue. Having a failed test in such a circumstance would be fine and we'd have to invest that one way or the other. So I guess it is fine to keep it is as.

BridgeAR · 2018-07-15T00:31:13Z

CI https://ci.nodejs.org/job/node-test-pull-request/15883/

TimothyGu

This is awesome. Thanks!

TimothyGu · 2018-07-15T15:05:24Z

lib/internal/repl/recoverable.js

+  //       change these messages in the future, this will lead to a test
+  //       failure, indicating that this code needs to be updated.
+  //
+  acorn.plugins.repl = (parser) => {


Nit: replRecoverable maybe?

addaleax · 2018-07-18T14:01:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15935/

jdalton · 2018-07-18T14:26:15Z

lib/internal/repl/recoverable.js

+
+          case 'Unterminated string constant':
+            const token = this.input.slice(this.lastTokStart, this.pos);
+            recoverable = token.endsWith('\\\n');


👆 Line continuations allow more than just \ followed by \n. See this bit of this.
This might be hit by folks copy-pasting code chunks in.

Good catch. I think it could be more than just Windows users (and potentially more than acorn currently supports). HTML specifies \r\n terminators for form input. And the ECMA standard defines more than \r and \n.

jdalton

Change requested here for Windows user considerations.

jdalton · 2018-07-19T06:09:32Z

lib/internal/repl/recoverable.js

+            const token = this.input.slice(this.lastTokStart, this.pos);
+            // see https://www.ecma-international.org/ecma-262/#sec-line-terminators
+            recoverable = token.match(/\\(\r\n?|\n|\u2028|\u2029)$/);
+        }


token.match() will return an array or null.
You probably want /\\(?:\r\n?|\n|\u2028|\u2029)$/.test(token).

sagirk

LGTM 🎉, with a minor nit.

sagirk · 2018-07-26T16:43:55Z

lib/internal/repl/recoverable.js

+    // but Acorn detected no issue.  Presume that additional text won't
+    // address this issue.
+    return false;
+  } catch (e) {


Nit: since e is not being used inside the catch block, can we get rid of it?

we have to use them until eslint updates to support optional catch bindings and we update to that eslint.

seems to work just fine:

$ git diff diff --git a/lib/internal/repl/recoverable.js b/lib/internal/repl/recoverable.js index 29411d6534..465d77451a 100644 --- a/lib/internal/repl/recoverable.js +++ b/lib/internal/repl/recoverable.js @@ -69,7 +69,7 @@ function isRecoverableError(e, code) { // but Acorn detected no issue. Presume that additional text won't // address this issue. return false; - } catch (e) { + } catch { return recoverable; } } $ make jslint Running JS linter... Please use lint-js instead of jslint

Should I push it?

Yes it's fine. We have babel-eslint and already use optional catch bindings in the codebase

trivikr · 2018-08-01T14:05:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16117/

Trott · 2018-08-01T22:02:25Z

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16138/

trivikr · 2018-08-02T04:48:54Z

@rubys This PR needs a rebase from upstream/master.
The instructions are described in Step 5

isRecoverableError is completely reimplemented using acorn and an acorn plugin that examines the state of the parser at the time of the error to determine if the code could be completed on a subsequent line.

Added comments for regular expressions that need to match and comparisons against exception message strings produced by Acorn.

rubys · 2018-08-02T14:02:03Z

There wasn't a conflict with master, but I rebased, tested, and pushed it anyway.

Trott · 2018-08-02T21:30:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16161/

trivikr · 2018-08-03T15:38:45Z

Resumed CI: https://ci.nodejs.org/job/node-test-pull-request/16174/

Trott · 2018-08-04T03:20:29Z

Landed in a2ec808

isRecoverableError is completely reimplemented using acorn and an acorn plugin that examines the state of the parser at the time of the error to determine if the code could be completed on a subsequent line. PR-URL: nodejs#21805 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

isRecoverableError is completely reimplemented using acorn and an acorn plugin that examines the state of the parser at the time of the error to determine if the code could be completed on a subsequent line. PR-URL: #21805 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 13, 2018

rubys mentioned this pull request Jul 13, 2018

repl: support mult-line string-keyed objects #21775

Closed

3 tasks

jdalton reviewed Jul 14, 2018

View reviewed changes

BridgeAR approved these changes Jul 15, 2018

View reviewed changes

TimothyGu approved these changes Jul 15, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 18, 2018

jdalton reviewed Jul 18, 2018

View reviewed changes

jdalton suggested changes Jul 18, 2018

View reviewed changes

jdalton reviewed Jul 19, 2018

View reviewed changes

jdalton approved these changes Jul 19, 2018

View reviewed changes

sagirk approved these changes Jul 26, 2018

View reviewed changes

jasnell approved these changes Jul 30, 2018

View reviewed changes

rubys mentioned this pull request Jul 31, 2018

dynamic import can't be used in the REPL #19570

Closed

rubys added 5 commits August 2, 2018 09:40

repl: support mult-line string-keyed objects

9affc6f

isRecoverableError is completely reimplemented using acorn and an acorn plugin that examines the state of the parser at the time of the error to determine if the code could be completed on a subsequent line.

parser. => this. and acorn.tokTypes => tt

7bb9bf1

split out isRecoverableError, changed call to Reflect.apply.

d426b00

Added comments for regular expressions that need to match and comparisons against exception message strings produced by Acorn.

s/plugins.repl/plugins.replRecoverable/g

974b337

all Line Terminators matter

02c5827

rubys added 2 commits August 2, 2018 09:40

token.match() => .test(token)

e58b861

remove unused exception variable

d426d86

rubys force-pushed the repl-json branch from 9dbd64c to d426d86 Compare August 2, 2018 14:01

Trott closed this Aug 4, 2018

rvagg mentioned this pull request Aug 13, 2018

Release proposal: v10.9.0 #22295

Merged

This was referenced Aug 17, 2018

Node REPL says "SyntaxError: unexpected symbol :" when parsing multi-line string-keyed objects (e.g. JSON) #21657

Closed

repl contains v8-specific syntax error message sniffs #21478

Closed

repl: support mult-line string-keyed objects #21805

repl: support mult-line string-keyed objects #21805

Conversation

rubys commented Jul 13, 2018

Checklist

vsemozhetbyt commented Jul 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jul 15, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trivikr commented Aug 1, 2018

Trott commented Aug 1, 2018

trivikr commented Aug 2, 2018

rubys commented Aug 2, 2018

Trott commented Aug 2, 2018

trivikr commented Aug 3, 2018

Trott commented Aug 4, 2018

jdalton left a comment •

edited

Loading