Fix #1369 Print a newline after interpreted commands

[benkobalog]

Keeping only one newline for Unit values if there are more of them in a command.

[dottybot]

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

[allanrenucci]

How is scala> print("Hello"); print("Hello") rendered with this fix?

[benkobalog]

scala>  print("Hello"); print("Hello") 
HelloHello
scala>  

[allanrenucci]

What about val foo = 2; (); val bar = 3?

[benkobalog]

scala> val foo = 2; (); val bar = 3 
val foo: Int = 2

val bar: Int = 3
scala>  

The sequence keeps the order. Maybe Units could be dropped if there's something else in the renderVal sequence.
That would produce this:

scala> print("Hello"); val foo = 2; (); val bar = 3 
Helloval foo: Int = 2
val bar: Int = 3
scala> 

[allanrenucci]

Yes, something like:

val renderedVals =
  vals.flatMap(rendering.renderVal)

val toRender =
  typeAliases.map("// defined alias " + _.symbol.showUser) ++
  defs.map(rendering.renderMethod) ++
  renderedVals

toRender.foreach(str => out.println(SyntaxHighlighting(str)))

if (renderedVals.isEmpty && vals.nonEmpty)
  out.println() // prints new line for side-effectful unit value (see #1369)

I would also be fine with a solution, that prints an additional new line for each command interpreted.

[benkobalog]

Yeah, this Unit based solution has many edge cases.

So something like this at ReplDriver.scala:195?

      case parsed: Parsed if parsed.trees.nonEmpty =>
        val newState = compile(parsed)
          .withHistory(parsed.sourceCode :: state.history)
          .newRun(compiler, rootCtx)
        out.println()
        newState

[allanrenucci]

Can you add back the test case? Otherwise LGTM

[benkobalog]

Sure. However I just realised that in this from it's not useful because end of line whitespaces are cut out by this function. So I will add it as a "whitespace sensitve" test, probably in a different folder.

[allanrenucci]

You can rebase to fix the CI failures

[benkobalog]

Thank you!

[allanrenucci]

Can you give a motivation for changing the testing framework? We don't really want to test that the repl prints a new line or not but rather make sure that the repl is not going to eat the output of a command.

Do you need to modify the testing framework to test something like:

scala> print("foo")
foo

If not, I'd rather keep the testing framework at it is.

[benkobalog]

If I do something like this:

scala> print("foo")
foo
scala> 1
val res0: Int = 1

It still doesn't work in the tests with this fix.
Maybe because the print in the ScripedTests file prints to the console which I'm running sbt in and probably not to the test output stream.
So I get this:

[info] Test dotty.tools.repl.ScriptedTests.replTests started
foo[info] Test dotty.tools.repl.ScriptedTests.typePrinterTests started

So I'm not sure how to test this.

[allanrenucci]

Nice catch! The output of print("foo") is not captured by the repl test. I'll look into this issue

[allanrenucci]

I have opened #3940, which should solve this problem

[allanrenucci]

#3940 has been merged. Can you rebase and append a test to https://github.com/dotty-staging/dotty/blob/f068bea112b7f3a9274a61ee5ea3f845e2c11c39/compiler/test-resources/repl/outputStream:

scala> print("foo") // see #1369
foo

[benkobalog]

Now the prints go to the proper place (after adding a print function to the ReplTest class).
However the test is still not good :( It passes regardless I add the newline after commands or not.

The reason seems to be that we don't call prompt printing in the tests, only the interpret function.
In the function runUntilQuit readLine() eventually calls the prompt string printing which overwrites the previously printed text, causing the bug we want to fix here. But in the tests we only call interpret (which comes later in runUntilQuit()).

[benkobalog]

Currently I don't have a better idea than doing something like

echo 'print("asd") ' | ./bin/dotr

And checking the output. But it's probably too slow.
Technically we could do the same thing inside jvm, but StoringPrintStream should work the same way as a terminal does for the relevant features (at least deleting characters should work, not sure if there are other features).
Is this something worth doing or do you have a better idea?

[smarter]

@allanrenucci Could you have another look at this PR and see what can be done to get it merged? Thanks.

[allanrenucci]

@benkobalog I dropped the commit for the REPL tests. I'll fix them in a separate PR

[allanrenucci]

Sorry for the late review. Thanks @benkobalog 🎉