scala3 icon indicating copy to clipboard operation
scala3 copied to clipboard

REPL: make truncation behaviour configurable

Open mpollmeier opened this issue 3 years ago • 7 comments

via -Vrepl-max-print-elements.

The default behaviour is unchanged:

scala> 1.to(300).toList
val res0: List[Int] = List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 
200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 
211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 22 ... large output truncated, print value to show all

(I added the line breaks for readability)

With this change, the user can configure a higher threshold before truncation kicks in:

scala -Vrepl-max-print-elements:2000

scala> 1.to(300).toList
// prints the entire list

mpollmeier avatar Sep 09 '22 15:09 mpollmeier

This was addressed somewhere, but I'd have to investigate where. https://github.com/lampepfl/dotty/pull/11378

The issue is whether truncation is based on elements or the length of the representation. Definitely want to specify N elements!

I'd suggest -V for the option prefix, which suggests "verbose" or "view" for when you want to tweak output (as opposed to compiler behavior).

Probably the runtime does the correct thing for small N, List(1, 2, ...). I recall that is a feature?

som-snytt avatar Sep 09 '22 18:09 som-snytt

This was addressed somewhere, but I'd have to investigate where. https://github.com/lampepfl/dotty/pull/11378

The linked PR changed the truncation behaviour from characters to elements. Which is fine, this PR doesn't touch that semantic. It merely makes the previously hardcoded number of 1000 elements configurable.

Thanks for your feedback, I'll address it shortly.

mpollmeier avatar Sep 10 '22 15:09 mpollmeier

I addressed both your suggestions and updated the description respectively.

mpollmeier avatar Sep 11 '22 07:09 mpollmeier

Side note: we should probably also change the prefix from -X to -V in -XreplDisableDisplay, for consistency. That'd be a separate PR though.

mpollmeier avatar Sep 12 '22 11:09 mpollmeier

It would be nice to have a couple of tests to verify. Or are there already some tests?

In particular, the configurable value is used first for "max elements" and then for "max output string length" (in truncate).

I'm not sure what the comment on truncate means: I would expect to call stringOf with max elements, and then if the resulting string is too long, truncate it. I don't know why stringOf doesn't yet take the "max string length" parameter.

I see that the Scala 2 PR is still hung up for some reason. It adds a boolean to the API for "display verbose Product"; it could also add a parameter for "max string to return". Then ellipses in the result would always look correct.

https://github.com/scala/scala/pull/8885

That is, a long list might render as List(1, 2, 3, ...) or a long string might render as Long text of indeterminate len....

For this PR, I suggest just verifying that there are tests that verify the expected result.

Otherwise, LGTM!

som-snytt avatar Sep 12 '22 17:09 som-snytt

Correct, the value is used in two places with different semantics, namely as the cut-off value for 'number of elements to be rendered' as well as 'number of characters to be displayed'. That doesn't really make sense, but it was like that before, and I wanted to keep this PR as simple as possible and only make the previously hard-coded value configurable.

IMO we should factor that out into a separate PR and also cleanup the TODO comments on Rendering.truncate, but if you prefer we can also add it to this one... If so: I'd say both truncations are useful to have, but they should be two distinct settings - how about -Vrepl-max-print-elements and -Vrepl-max-print-characters?

I added a test for -Vrepl-max-print-elements as suggested (plus one for -Xrepl-disable-display while I was at it).

mpollmeier avatar Sep 13 '22 08:09 mpollmeier

What are the next steps to get this merged? Anything else I can help with?

mpollmeier avatar Sep 16 '22 07:09 mpollmeier

Change suggestion for replStringOf but I couldn't comment that line:

  private[repl] def replStringOf(value: Object)(using Context): String =
    assert(myReplStringOf != null,
      "replStringOf should only be called on values creating using `classLoader()`, but `classLoader()` has not been called so far")
    val maxPrintElements = ctx.settings.VreplMaxPrintElements.valueIn(ctx.settingsState)
    val res = myReplStringOf(value, Integer.valueOf(maxPrintElements))
    if res == null then "null // non-null reference has null-valued toString" else truncate(res)

prolativ avatar Sep 28 '22 10:09 prolativ

Thank you, yes passing the setting into myReplStringOf does the job, the test is green now.

mpollmeier avatar Sep 28 '22 15:09 mpollmeier

I returned to the Scala 2 PR, which actually takes a "max string length" value instead of the original element count, since 100 of something can format to arbitrary (if finite) length.

To recap, the point was for the utility stringOf to add an ellipsis, which it will do for both List(1, 2, 3, ...) and ABCDE... to indicate iterable continues or string value was lopped off.

I'll follow up with aligning the -V option in some agreed way. (Personally, I think length is more intuitive; it's just to keep from accidentally writing long output to the terminal.)

The other degree of freedom is that verboseProduct is currently true; that should be a flag. That output is shown here:

scala> case class C(i: Int, s: String)
// defined case class C

scala> C(42, "hello, world")
val res0: C = C(i = 42, s = "hello, world")

Extra ellipses are not terrible. It correctly detects truncation.

➜  dotty git:(test/repl-printing) ./bin/scala -Vrepl-max-print-elements 20
Welcome to Scala 3.2.2-RC1-bin-SNAPSHOT-git-9d8f751 (1.8.0_302, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> "X"*18
val res0: String = "XXXXXXXXXXXXXXXXXX"

scala> "X"*19
val res1: String = "XXXXXXXXXXXXXXXX... ... large output truncated, print value to show all

scala> (1 to 10).toList
val res2: List[Int] = List(1, 2, 3, ...) ... large output truncated, print value to show all

scala> (1 to 10).map(_ => "X"*10).toList
val res3: List[String] = List("XXXXXXXXXX...) ... large output truncated, print value to show all

scala> (1 to 10).map(_ => "X"*5).toList
val res4: List[String] = List("XXXXX", ...) ... large output truncated, print value to show all

scala> util.Properties.versionString
val res5: String = "version 2.13.10-... ... large output truncated, print value to show all

som-snytt avatar Oct 06 '22 02:10 som-snytt

Follow-up PR that introduces a separate setting for character truncation: https://github.com/lampepfl/dotty/pull/16167

mpollmeier avatar Oct 11 '22 11:10 mpollmeier