Skip to content

Scala 3 REPL regression compared to Scala 2: echoed strings are trimmed when they shouldn't, loosing e.g. newline-characters #11558

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

Closed
bjornregnell opened this issue Feb 28, 2021 · 10 comments · Fixed by #11562
Milestone

Comments

@bjornregnell
Copy link
Contributor

bjornregnell commented Feb 28, 2021

Compiler version

3.0.0-RC2-bin-20210227-07d9dd2-NIGHTLY

Minimized code

The Scala 3 REPL trim all string values before echoing, which means that leading whitespace newline is lost and multi-line strings will get first line miss-aligned etc.

scala> case class Box(x: String) { override def toString = s"\n$x" }                                                           
// defined case class Box

Output

scala> Box("new line is lost")
val res0: Box = new line is lost

scala> Box("1 2 3\n1 2 3\n1 2 3")
val res1: Box = 1 2 3
1 2 3
1 2 3

Expectation

Welcome to Scala 2.13.4 (OpenJDK 64-Bit Server VM, Java 11.0.10).
Type in expressions for evaluation. Or try :help.

scala> case class Box(x: String) { override def toString = s"\n$x" }
class Box

scala> Box("new line is NOT lost")
val res0: Box =

new line is NOT lost

scala> Box("1 2 3\n1 2 3\n1 2 3")
val res1: Box =

1 2 3
1 2 3
1 2 3
@bjornregnell
Copy link
Contributor Author

Related to #11555

@bjornregnell
Copy link
Contributor Author

@som-snytt Would this also be fixed if your Scala 2 PR on REPL result printing would be ported to Scala 3 ?
scala/scala#8885

@bjornregnell
Copy link
Contributor Author

@smarter Would it be too naive just to skip the .trim here (if this is the place where white space gets killed?)?:
https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/repl/Rendering.scala#L98

@som-snytt
Copy link
Contributor

You're assuming the object is rendered with toString.

scala> Box("xxx")
val res2: Box = xxx

scala> Box("xxx").toString
val res3: String = "
xxx"

@bjornregnell
Copy link
Contributor Author

Yes, I assumed that toString is involved in the rendering as it somehow knows what I've put in the overridden thing...
Or am I missing something?
Scala 3:

scala> case class Box(x: String) { override def toString = s"\n$x from the overridden toString " }
// defined case class Box

scala> Box("hello")
val res4: Box = hello from the overridden toString

Scala 2:

scala> case class Box(x: String) { override def toString = s"\n$x from the overridden toString " }
class Box

scala> Box("hello")
val res2: Box =

hello from the overridden toString

@som-snytt
Copy link
Contributor

som-snytt commented Feb 28, 2021

I misread replStringOf, which wraps multiline result strings in newlines (FSR -- maybe to make columns in some output line up?).

It only wraps inbound String in quotes if it has leading or trailing space. (That's the line I replaced a pandemic year ago in that PR to encode escapes, as well as always add the quotes.)

Perhaps Scala 3 is trimming to cope with the extra newlines. If that's true, it makes sense to me to fix replStringOf not to do that, nor add newline in the default case. Let the caller add more extraneous whitespace as required. Then Scala 3 can be tweaked not to trim.

I see it's that other PR that special-cases non-tuple Product under verbose-product mode; that must have been what I was thinking of; before that, there is no special handling for case classes.

@som-snytt
Copy link
Contributor

@bjornregnell After I blinked a few times, I saw that current replStringOf always appends newline, and also prepends on multiline. It's not foolproof, but the fix is to trim at most that much, if there is a newline suffix.

The PR'd replStringOf will no longer do that, and Scala 3 REPL will reflectively invoke it if it is available.

@som-snytt
Copy link
Contributor

Took me a while to see that my trim fix was tested against the recent truncate fix, which I was missing; where the test of printing the maximal string no longer failed. The expected/actual diff format was very hard to read; I cut it into two files to find the diff.

Similarly, when I tested against my Scala 2 PR, fixing broken tests was tedious. I didn't experiment if -update-checks works with repl scripted tests, but I will do that when that PR is merged and the tests break again.

@bjornregnell
Copy link
Contributor Author

Thanks, for all your efforts @som-snytt ! And nice that you spotted the interaction with my truncate fix despite the hard-to-read diffs- I didn't realize that this interaction would surface via tests. (Best would perhaps be if Scala 2 and Scala 3 could share some of the REPL rendering code, at least the actual string presentation, but I guess they are too far apart under the hood for that.)

@bjornregnell
Copy link
Contributor Author

I'll try to give some review feedback in the PR.

@Kordyjan Kordyjan added this to the 3.0.1 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants