repl: handling multiline history needs some case guards

[vsemozhetbyt]

• Version: master
• Platform: Windows 7 x64
• Subsystem: repl

1. Recently added REPL multiline history handling streamlines multiline expressions into one-line expressions, but line comments may change syntax and hang execution:

> [
...   1 // comment
... ]
[ 1 ]
> [  1 // comment]
...

This is relevant if someone pastes copied code blocks with line comments in the REPL. It seems comments need to be stripped.

2. Another case that needs special handling, multiline template literals:

> `a
... b
... c`
'a\nb\nc'
> bc`
...

cc @antsmartian

[antsmartian]

Thanks. I will have a look at it today.

[antsmartian]

@vsemozhetbyt: The second issue, I guess I couldn't able to reproduce it (atleast on master).

> `a
... b
... c`
'a\nb\nc'
> `abc`

First one yes, its an issue. Good catch.

[vsemozhetbyt]

@antsmartian H'm, I cannot reproduce now as well. Maybe I've got some flaky condition then. But we still have an issue: line breaks are part of the code in multiline template literals, they may need to be saved so that restored line would be the `a\nb\nc`, not the `abc`.

[antsmartian]

Hmmm, but current history doesn't show as \n indeed. Its the result of an expression, but history just saves what user types right? Not the result. Thoughts?

[vsemozhetbyt]

I mean, for me it seems these cases should be handled equally:

> `a\nb\nc`
'a\nb\nc'
> `a\nb\nc` // UP ARROW
'a\nb\nc'
> `a
... b
... c`
'a\nb\nc'
> `abc` // UP ARROW
'abc'

Otherwise, we had wrong value second time.

[shobhitchittora]

@vsemozhetbyt The way I see it to handle the comment, you need to check for every line in repl and check if it has a comment. Also you'll want to skip this check for string literals. Weird cases to check thought ! 😛

[antsmartian]

@shobhitchittora : Yes you are right. But fundamentally, on multiline we call acron parser to see if we can expect more tokens or give up with errors. So in the same way, we need to handle this as well. Because code with comment, might not run in history as @vsemozhetbyt shown above. To me its a bug.

[BridgeAR]

The current way how multilines are saved is just wrong. A line is detected when the newline character (\n) is detected. Now that line is added to potential former ones and therefore stripping the new lines. This is changing the actual input and causes multiple issues.

I tried to fix that by adding the new lines but I ran into new issues when doing that due to another bug in the implementation. Both are outlines above and in #24781.

[targos]

Copying from #24781 (comment)

Is there no way for us to keep the line breaks? That's what zsh does.

[BridgeAR]

@targos I tried that but this caused new issues with a different bug that caused the history to become malformed. It will also move the line upwards and if you then move up in the history to e.g. a single line entry, the lower end of the terminal will be empty. I guess this would be the best we can do at the moment?

[antsmartian]

@BridgeAR

The current way how multilines are saved is just wrong. A line is detected when the newline character >(\n) is detected. Now that line is added to potential former ones and therefore stripping the new lines. >This is changing the actual input and causes multiple issues.

Yes this happens on line breaks statements for example string literals, code with comments. Other things like function, object, class declarations seems to be working fine. (fundamentally the implementation is based on kBufferedCommandSymbol on repl, I guess that's the sole reason here as it doesn't work for template literals and like)

[BridgeAR]

@antsmartian they work by chance. The input is still manipulated for all input by removing the newline.

Another example with a function:

function broken() { con
sole.log(5) }

This fails in the repl and it should because it's a syntax error. Going back with the history will now "fix" the syntax error.

However, there is more weirdness going on in the new history: it does not memorize the correct order when an error happened while trying to evaluate a statement.

[antsmartian]

@BridgeAR Ack. I got what you are saying here. Seems to be bit tricky. Thanks for your time on this. My bad, seems to be this feature broken history a bit :( Will see, how we can rectify this.

[vsemozhetbyt]

Another unintended fix are ASI cases:

> (function () {
...   return
...   console.log('should not be logged')
... })()
undefined
>
> (function () {  return  console.log('not logged')})()
should not be logged
undefined
>

[antsmartian]

I'm planning to do this:

1. Don't buffer multiline (which is the main root cause here, skipping \n).
2. But rather, when we see a multiline end, we can form from the history from history array and construct the line with \n as needed.
3. This would infact, fix all the issues mentioned here.
4. We will get multiline only when kBufferedCommandSymbol seen by the repl. Thus leaving string literals etc (as shown in the issue) as it is to the history (single item navigation).
5. This methods works only in repl session, re-login to repl, will treat all history as single entity. I guess, we can improve on this item going forward.

cc @BridgeAR @vsemozhetbyt @targos thoughts?

[BridgeAR]

This sounds very hackish and people will be confused that it works one way in the current session and different from entries from before. I for example often leave the repl and open it again and go back in the history.

I believe we should give this a bit more thought. The main problem I see is the format how we save the history currently. It is simple plain text, line by line and if we want to keep on using that it requires to either add some magic characters (e.g., a rarely used control character sequence) that we replace with newlines when reading the entry. However, this is not ideal. It would be much simpler if the format would not be plain text but this decision was made early on in io.js 3.0 by @Fishrock123 (#2224). I personally believe a JSON file with an array as content would be better. But that would be a significant change to the format and I am not sure it's worth the hassle. Other opinions / thoughts?

// cc @silverwind as you have also been involved with the history before.

Update: I removed a suggestion which turned out as impossible.

[devsnek]

i would keep history with the newlines, and then just print/clean the multiple newlines that occur when traversing it

[antsmartian]

@BridgeAR Moving to JSON format ofcourse is a good idea to solve this problem. I would be happy to implement it, but need other's thought. Keeping \n on our current implementation is a tougher one.

[antsmartian]

But looking from #2224, seems to be that we have moved from JSON to text file. Not sure, whats the real reason behind it.

[BridgeAR]

@devsnek can you elaborate on the that? If I understand correct you want to add empty lines in the history that mark a multiline statement, am I correct? This should be possible. @antsmartian you could try this.

[antsmartian]

@BridgeAR Yes, I guess that's what @devsnek suggesting here. To be on the same page, this is how history looks like, if we implement the logic:

var a = 5

{
a:3
}

var b = 5

On processing, we need to change history array, so that it looks like:

['var a=5',  '{\na:3\n}' , 'var b = 5']

I'm actually trying to implement it.

[BridgeAR]

@antsmartian the second entry should look like: '{\na:3\n}'.

[antsmartian]

@BridgeAR : Yup, its a typo. Fixed.

[BridgeAR]

@antsmartian just as another hint: to keep backwards compatibility with the current format you actually have to do exactly the opposite of how you want the history to look like:

var a = 5
{

a:3

}
var b = 5

Otherwise all existing history entries become a huge multiline statement and we don't want that ;-).

[BridgeAR]

I am closing this, since the feature itself was reverted and has not been added back so far.