repl: handling multiline history needs some case guards

@antsmartian

Version: master
Platform: Windows 7 x64
Subsystem: repl

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.

Another case that needs special handling, multiline template literals:

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

cc @antsmartian

Thanks. I will have a look at it today.

@vsemozhetbyt

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

@antsmartian

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

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?

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.

@vsemozhetbyt

@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 ! 😛

@shobhitchittora

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

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.

Copying from #24781 (comment)

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

@targos

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

@BridgeAR

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

@antsmartian

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

@BridgeAR

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

@antsmartian

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

@BridgeAR

@BridgeAR : Yup, its a typo. Fixed.

@antsmartian

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