Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REPL bug fixes #2163

Closed
wants to merge 4 commits into from
Closed

Conversation

thefourtheye
Copy link
Contributor

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

  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.

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'
>
>
>

@brendanashworth brendanashworth added the repl Issues and PRs related to the REPL subsystem. label Jul 11, 2015
@Fishrock123
Copy link
Contributor

cc @chrisdickinson

@thefourtheye thefourtheye force-pushed the repl-bug-fixes branch 2 times, most recently from 24f85ef to 498eccb Compare July 12, 2015 16:02
@thefourtheye thefourtheye changed the title Repl bug fixes REPL bug fixes Jul 12, 2015
@Fishrock123
Copy link
Contributor

@thefourtheye
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest also trying \r and other related characters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: os.EOL

@Fishrock123
Copy link
Contributor

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
Copy link
Contributor

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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
    >
{ 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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: os.EOL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Fishrock123
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

@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.

thefourtheye added a commit that referenced this pull request Jul 24, 2015
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]>
thefourtheye added a commit that referenced this pull request Jul 24, 2015
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]>
thefourtheye added a commit that referenced this pull request Jul 24, 2015
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]>
thefourtheye added a commit that referenced this pull request Jul 24, 2015
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
Copy link
Contributor Author

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

@thefourtheye thefourtheye deleted the repl-bug-fixes branch July 24, 2015 19:05
lrowe added a commit to lrowe/node that referenced this pull request Apr 4, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants