REPL bug fixes #2163

thefourtheye · 2015-07-11T19:58:41Z

There are four bug fixes.

1. Better line continuation feature

As it is, REPL doesn't honour the line continuation feature very well.
This patch

keeps track of the beginning of the string literals and if they
don't end or current line doesn't end with line continuation, then
error out.
monitors if the line continuation character is used without the
string literal and errors out if that happens.

2. Removing `undefined` when unknown REPL command is used

When an invalid REPL keyword is used, we actually print undefined as
well in the console.

> process.version
'v2.3.4'
> .invalid_repl_command
Invalid REPL keyword
undefined
>

This patch prevents printing undefined in this case.

> process.version
'v2.3.5-pre'
> .invalid_repl_command
Invalid REPL keyword

3. preventing REPL crash with inherited properties

When an inherited property is used as a REPL keyword, the REPL crashes.

➜  Desktop  iojs
> process.version
'v2.3.4'
> .toString
readline.js:913
        stream[ESCAPE_DECODER].next(r[i]);
                                ^
TypeError: Cannot read property 'call' of undefined
    at REPLServer.parseREPLKeyword (repl.js:746:15)
    at REPLServer.<anonymous> (repl.js:284:16)
    at emitOne (events.js:77:13)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:210:10)
    at REPLServer.Interface._line (readline.js:549:8)
    at REPLServer.Interface._ttyWrite (readline.js:826:14)
    at ReadStream.onkeypress (readline.js:105:10)
    at emitTwo (events.js:87:13)
    at ReadStream.emit (events.js:172:7)
➜  Desktop

This patch makes the internal commands object inherit from null so
that there will be no inherited properties.

> process.version
'v2.3.5-pre'
> .toString
Invalid REPL keyword
>

4. Better empty line handling

In REPL, if we try to evaluate an empty line, we get undefined.

> process.version
'v2.3.4'
>
undefined
>
undefined
>

This patch prevents undefined from printing if the string is empty.

> process.version
'v2.3.5-pre'
>
>
>

Fishrock123 · 2015-07-12T00:24:35Z

cc @chrisdickinson

Fishrock123 · 2015-07-16T22:29:30Z

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/161/

thefourtheye · 2015-07-17T02:53:52Z

None of the CI failures are because of this change. Yay :-)

Fishrock123 · 2015-07-20T18:53:17Z

test/parallel/test-repl.js

+      expect: /'thefourtheye'/ },
+    // empty lines in the REPL should be allowed
+    { client: client_unix, send: '\n\n\n',
+      expect: prompt_unix + prompt_unix + prompt_unix },


I suggest also trying \r and other related characters.

Also: os.EOL

Fishrock123 · 2015-07-21T00:39:54Z

The other three LGTM, but maybe if @chrisdickinson could review just thefourtheye@498eccb / repl: improving line continuation handling / the 3rd commit, that'd be great.

Fishrock123 · 2015-07-21T00:40:28Z

Also @thefourtheye anywhere you modify or add a variable that could be a const, could you please make it one? Thanks!

silverwind · 2015-07-21T12:22:04Z

lib/repl.js

+      } else if (isWithinStrLiteral && !wasWithinStrLiteral) {
+        // was not part of a string literal, but it is now, trim only the start
+        cmd = cmd.replace(/^\s+/, '');
+      }


Just yesterday I learned about String.prototype.trimLeft() and String.prototype.trimRight(). These are nonstandard, but there's some discussion about standardizing them on the es-discuss mailing list, so I think we're safe to use them. Also, on the first case, let's use standard String.prototype.trim() :)

thefourtheye · 2015-07-22T18:38:55Z

@Fishrock123 @silverwind I addressed all the review comments (no exceptions, I addressed them all). PTAL now :-)

PS: I rebased with master to make sure that these don't break anything new.

When an invalid REPL keyword is used, we actually print `undefined` as well in the console. > process.version 'v2.3.4' > .invalid_repl_command Invalid REPL keyword undefined > This patch prevents printing `undefined` in this case. > process.version 'v2.3.5-pre' > .invalid_repl_command Invalid REPL keyword >

When an inherited property is used as a REPL keyword, the REPL crashes. ➜ Desktop iojs > process.version 'v2.3.4' > .toString readline.js:913 stream[ESCAPE_DECODER].next(r[i]); ^ TypeError: Cannot read property 'call' of undefined at REPLServer.parseREPLKeyword (repl.js:746:15) at REPLServer.<anonymous> (repl.js:284:16) at emitOne (events.js:77:13) at REPLServer.emit (events.js:169:7) at REPLServer.Interface._onLine (readline.js:210:10) at REPLServer.Interface._line (readline.js:549:8) at REPLServer.Interface._ttyWrite (readline.js:826:14) at ReadStream.onkeypress (readline.js:105:10) at emitTwo (events.js:87:13) at ReadStream.emit (events.js:172:7) ➜ Desktop This patch makes the internal `commands` object inherit from `null` so that there will be no inherited properties. > process.version 'v2.3.5-pre' > .toString Invalid REPL keyword >

Fishrock123 · 2015-07-22T21:49:00Z

test/parallel/test-repl.js

+    { client: client_unix, send: '\'thefourth\\\n.help\neye\'',
+      expect: /'thefourtheye'/ },
+    // empty lines in the REPL should be allowed
+    { client: client_unix, send: '\n\r\n\r\n',


Also: os.EOL

@Fishrock123 Other places in the test also use \n only and they seem to work fine in Windows also (we never had any failures in Win32 land because of this, right?) Moreover the test itself is called as unix_tests. Should we really change this?

Hmm, I'm not really sure, perhaps it should be in it's own test bit. I guess it's not really necessary.

Fishrock123 · 2015-07-22T22:02:19Z

Two nits. LGTM. Please re-run the CI after you add os.EOL. :P

As it is, REPL doesn't honour the line continuation feature very well. This patch 1. keeps track of the beginning of the string literals and if they don't end or current line doesn't end with line continuation, then error out. 2. monitors if the line continuation character is used without the string literal and errors out if that happens.

In REPL, if we try to evaluate an empty line, we get `undefined`. > process.version 'v2.3.4' > undefined > undefined > This patch prevents `undefined` from printing if the string is empty. > process.version 'v2.3.5-pre' > > >

Fishrock123 · 2015-07-23T19:06:37Z

Here's a new CI then. Land if it's green(-ish). https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/187/

thefourtheye · 2015-07-24T07:36:20Z

@Fishrock123 Except the win2008r2 (which fails with Command "git clean -fdx" returned status code 1:), other failures are not related to this change. Should we trigger again?

Edit: The problematic win2008r2 CI job link

Fishrock123 · 2015-07-24T16:51:34Z

@thefourtheye yeah do the thing, that just looks like jenkins hasn't had enough coffee or something.

Edit: looks like the windows2008r2-1 machine is having issues.

When an invalid REPL keyword is used, we actually print `undefined` as well in the console. > process.version 'v2.3.4' > .invalid_repl_command Invalid REPL keyword undefined > This patch prevents printing `undefined` in this case. > process.version 'v2.3.5-pre' > .invalid_repl_command Invalid REPL keyword > PR-URL: #2163 Reviewed-By: Jeremiah Senkpiel <[email protected]>

When an inherited property is used as a REPL keyword, the REPL crashes. ➜ Desktop iojs > process.version 'v2.3.4' > .toString readline.js:913 stream[ESCAPE_DECODER].next(r[i]); ^ TypeError: Cannot read property 'call' of undefined at REPLServer.parseREPLKeyword (repl.js:746:15) at REPLServer.<anonymous> (repl.js:284:16) at emitOne (events.js:77:13) at REPLServer.emit (events.js:169:7) at REPLServer.Interface._onLine (readline.js:210:10) at REPLServer.Interface._line (readline.js:549:8) at REPLServer.Interface._ttyWrite (readline.js:826:14) at ReadStream.onkeypress (readline.js:105:10) at emitTwo (events.js:87:13) at ReadStream.emit (events.js:172:7) ➜ Desktop This patch makes the internal `commands` object inherit from `null` so that there will be no inherited properties. > process.version 'v2.3.5-pre' > .toString Invalid REPL keyword > PR-URL: #2163 Reviewed-By: Jeremiah Senkpiel <[email protected]>

As it is, REPL doesn't honour the line continuation feature very well. This patch 1. keeps track of the beginning of the string literals and if they don't end or current line doesn't end with line continuation, then error out. 2. monitors if the line continuation character is used without the string literal and errors out if that happens. PR-URL: #2163 Reviewed-By: Jeremiah Senkpiel <[email protected]>

In REPL, if we try to evaluate an empty line, we get `undefined`. > process.version 'v2.3.4' > undefined > undefined > This patch prevents `undefined` from printing if the string is empty. > process.version 'v2.3.5-pre' > > > PR-URL: #2163 Reviewed-By: Jeremiah Senkpiel <[email protected]>

thefourtheye · 2015-07-24T19:05:37Z

Thanks @Fishrock123 :-) Landed at afd7e37, 81ea52a, 30edb5a and 77fa385

Tweak the better empty line handling introduced in nodejs#2163 so that empty lines are still passed to the eval function. This is required for the debugger to repeat the last command on an empty line. Fixes: nodejs#6010

brendanashworth added the repl Issues and PRs related to the REPL subsystem. label Jul 11, 2015

thefourtheye force-pushed the repl-bug-fixes branch 2 times, most recently from 24f85ef to 498eccb Compare July 12, 2015 16:02

thefourtheye changed the title ~~Repl bug fixes~~ REPL bug fixes Jul 12, 2015

thefourtheye mentioned this pull request Jul 15, 2015

repl: removing the buggy trimWhitespace function #2185

Closed

Fishrock123 reviewed Jul 20, 2015
View reviewed changes

silverwind reviewed Jul 21, 2015
View reviewed changes

thefourtheye force-pushed the repl-bug-fixes branch from 8cb8083 to ccece25 Compare July 22, 2015 18:38

thefourtheye added 2 commits July 23, 2015 00:11

thefourtheye force-pushed the repl-bug-fixes branch from ccece25 to aac19e2 Compare July 22, 2015 18:41

indutny force-pushed the master branch from bffb204 to eb35968 Compare July 22, 2015 21:21

Fishrock123 reviewed Jul 22, 2015
View reviewed changes

thefourtheye added 2 commits July 23, 2015 23:37

repl: better empty line handling

b54f153

In REPL, if we try to evaluate an empty line, we get `undefined`. > process.version 'v2.3.4' > undefined > undefined > This patch prevents `undefined` from printing if the string is empty. > process.version 'v2.3.5-pre' > > >

thefourtheye force-pushed the repl-bug-fixes branch from aac19e2 to b54f153 Compare July 23, 2015 18:11

thefourtheye closed this Jul 24, 2015

thefourtheye deleted the repl-bug-fixes branch July 24, 2015 19:05

cjihrig mentioned this pull request Jul 28, 2015

Release Proposal: v2.5.0 #2239

Merged

lrowe mentioned this pull request Apr 3, 2016

repl: eval empty lines, fixing debugger repeat #6025

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REPL bug fixes #2163

REPL bug fixes #2163

thefourtheye commented Jul 11, 2015

Fishrock123 commented Jul 12, 2015

Fishrock123 commented Jul 16, 2015

thefourtheye commented Jul 17, 2015

Fishrock123 Jul 20, 2015

Fishrock123 Jul 22, 2015

Fishrock123 commented Jul 21, 2015

Fishrock123 commented Jul 21, 2015

silverwind Jul 21, 2015

thefourtheye commented Jul 22, 2015

Fishrock123 Jul 22, 2015

thefourtheye Jul 23, 2015

Fishrock123 Jul 23, 2015

Fishrock123 commented Jul 22, 2015

Fishrock123 commented Jul 23, 2015

thefourtheye commented Jul 24, 2015

Fishrock123 commented Jul 24, 2015

thefourtheye commented Jul 24, 2015

REPL bug fixes #2163

REPL bug fixes #2163

Conversation

thefourtheye commented Jul 11, 2015

1. Better line continuation feature

2. Removing undefined when unknown REPL command is used

3. preventing REPL crash with inherited properties

4. Better empty line handling

Fishrock123 commented Jul 12, 2015

Fishrock123 commented Jul 16, 2015

thefourtheye commented Jul 17, 2015

Fishrock123 Jul 20, 2015

Choose a reason for hiding this comment

Fishrock123 Jul 22, 2015

Choose a reason for hiding this comment

Fishrock123 commented Jul 21, 2015

Fishrock123 commented Jul 21, 2015

silverwind Jul 21, 2015

Choose a reason for hiding this comment

thefourtheye commented Jul 22, 2015

Fishrock123 Jul 22, 2015

Choose a reason for hiding this comment

thefourtheye Jul 23, 2015

Choose a reason for hiding this comment

Fishrock123 Jul 23, 2015

Choose a reason for hiding this comment

Fishrock123 commented Jul 22, 2015

Fishrock123 commented Jul 23, 2015

thefourtheye commented Jul 24, 2015

Fishrock123 commented Jul 24, 2015

thefourtheye commented Jul 24, 2015

2. Removing `undefined` when unknown REPL command is used