Skip to content

Don't strip spaces for regular REPL input#2246

Closed
edef1c wants to merge 1 commit intonodejs:masterfrom
edef1c:fix-repl-dots
Closed

Don't strip spaces for regular REPL input#2246
edef1c wants to merge 1 commit intonodejs:masterfrom
edef1c:fix-repl-dots

Conversation

@edef1c
Copy link
Contributor

@edef1c edef1c commented Jul 25, 2015

This means .method() isn't misparsed as a REPL keyword, and significantly improves the usability of .load.

@edef1c
don't strip spaces for regular REPL input
08bf133 
This means `  .method()` isn't misparsed as a REPL keyword,
and significantly improves the usability of `.load`.
@edef1c edef1c changed the title don't strip spaces for regular REPL input Don't strip spaces for regular REPL input Jul 25, 2015
@edef1c edef1c closed this Jul 25, 2015
@edef1c edef1c reopened this Jul 25, 2015
@edef1c
Copy link
Contributor Author

edef1c commented Jul 25, 2015

Expected (and this patch's) behaviour:

> .load test.js
> function sum(arr) {
...   return arr
...     .reduce(function(acc, x) { return acc + x }, 0)
... }
undefined
> console.log(sum([20, 1, 21]))
42
undefined
>

Current behaviour:

> .load test.js
> function sum(arr) {
...   return arr
...     .reduce(function(acc, x) { return acc + x }, 0)
Invalid REPL keyword
undefined
> }
SyntaxError: Unexpected token }
    at Object.exports.createScript (vm.js:24:10)
    at REPLServer.defaultEval (repl.js:131:25)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:308:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:209:10)
    at REPLServer.Interface._line (readline.js:548:8)
    at REPLServer.Interface._ttyWrite (readline.js:879:20)
> console.log(sum([20, 1, 21]))
ReferenceError: sum is not defined
    at repl:1:13
    at REPLServer.defaultEval (repl.js:154:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:308:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:209:10)
    at REPLServer.Interface._line (readline.js:548:8)
    at REPLServer.Interface._ttyWrite (readline.js:879:20)
>

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

silverwind commented Jul 25, 2015

cc @thefourtheye

@Fishrock123
Copy link
Contributor

Fishrock123 commented Jul 25, 2015

Oops, probably my fault. Hmm, will need to fix this before the 2.5.0 release.

@Fishrock123 Fishrock123 added this to the 2.5.0 milestone Jul 25, 2015
@edef1c
Copy link
Contributor Author

edef1c commented Jul 25, 2015

@Fishrock123 Nah, this has been the case for years.

@thefourtheye
Copy link
Contributor

thefourtheye commented Jul 25, 2015

Hmmm. I knew about this weak REPL command check when I submitted REPL bug fixes PR, but I could not come up with a practical usecase. So, apart from removing the check mentioned in this PR, it would be better if we improved the REPL command check with a regular expression. Instead of

cmd && cmd.charAt(0) === '.' && isNaN(parseFloat(cmd))

if we had

cmd && isNaN(parseFloat(cmd)) && /.[a-z]+\s*[a-z]*/.test(cmd)

it would be better. Also, I would recommend including a test as well.

Edit: To me, stripping spaces is not the actual problem here (even though leaving the spaces when we are not inside a string literal is not a big deal), but considering the current line of input as a REPL command just because it starts with . doesn't seem right. That is why I proposed the RegEx above.

Edit 2: It would be better if the RegEx is /^.[a-z]+\s*[\w]*$/.test(cmd)

@targos
Copy link
Member

targos commented Jul 25, 2015

@thefourtheye you can easily run into this issue by copy-pasting something in the REPL

@thefourtheye
Copy link
Contributor

thefourtheye commented Jul 25, 2015

@targos Yup, I understand that after seeing the example shown in the PR. That is why using a RegEx to validate the string should work here. What do you think?

@Fishrock123 Fishrock123 removed this from the 2.5.0 milestone Jul 27, 2015
@targos
Copy link
Member

targos commented Jul 27, 2015

@thefourtheye Yes a RegExp could be the solution. We just need to make sure that it does not identify too much valid JS as a REPL command.

@thefourtheye thefourtheye mentioned this pull request Jul 27, 2015
Closed
@Fishrock123 Fishrock123 added the confirmed-bug Issues with confirmed bugs. label Jul 27, 2015
thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Aug 11, 2015
@thefourtheye
repl: better REPL commands detection
893ab37 
Fixes the problem shown in nodejs#2246

The REPL module, treats any line which starts with a dot character as
a REPL command and this limits the ability to use function calls in
the next lines to improve readability.

This patch checks if the current line starts with `.` and then a series
of lowercase letters and then any characters.

For example:

    > process.version
    'v2.4.0'
    > function sum(arr) {
    ... return arr
    ...     .reduce(function(c, x) { return x + c; });
    Invalid REPL keyword
    undefined

but then, this patches allows this:

    > process.version
    'v2.4.1-pre'
    > function sum(arr) {
    ... return arr
    ...     .reduce(function(c, x) { return x + c; });
    ... }
    undefined
@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@nathan7 @thefourtheye @Fishrock123 ... what's the status on this one?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015
@princejwesley
Copy link
Contributor

princejwesley commented Dec 24, 2015

@Fishrock123 @thefourtheye /^\.[a-z][^(]*$/ is sufficient, right?

@thefourtheye
Copy link
Contributor

thefourtheye commented Dec 25, 2015

@princejwesley I proposed almost the same in #2254. But the problem is, load and save accept filenames as inputs following them. If they had ( in them then this solution will break.

@princejwesley
Copy link
Contributor

princejwesley commented Dec 25, 2015

@jasnell @thefourtheye #3835 fixed this issue.

@edef1c edef1c closed this Dec 26, 2015
@edef1c edef1c deleted the fix-repl-dots branch December 26, 2015 10:08
@snyk-bot snyk-bot mentioned this pull request Aug 11, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem. stalled Issues and PRs that are stalled.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@edef1c @silverwind @Fishrock123 @thefourtheye @targos @jasnell @princejwesley @brendanashworth

Comments