Skip to content

REPL: make truncation behaviour configurable #16011

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Sep 29, 2022
Merged

REPL: make truncation behaviour configurable #16011

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
60f9f45
REPL: make truncation behaviour configurable
mpollmeier Sep 8, 2022
35a61a5
use `-V` for the option prefix, which suggests "verbose" or "view"...
mpollmeier Sep 11, 2022
e48c71e
ctx.settings.VreplMaxPrintElements.value at all use-sites
mpollmeier Sep 11, 2022
3e08a78
cleanup
mpollmeier Sep 11, 2022
0eb5f26
add test for `-Xrepl-disable-display`
mpollmeier Sep 13, 2022
a5fce18
add test for `-Vrepl-max-print-elements`
mpollmeier Sep 13, 2022
f57fb8e
naming the `using` parameter isn't required here (PR comment)
mpollmeier Sep 21, 2022
9f5cc90
rename variable and update comments
mpollmeier Sep 21, 2022
fc58628
partial revert due to inconsistency with settings (pr comment)
mpollmeier Sep 28, 2022
3e21b0b
adapt test expectation... doesn't quite work yet, the changed setting…
mpollmeier Sep 28, 2022
1904a78
fix test: pass `maxElements` into `myReplStringOf`
mpollmeier Sep 28, 2022
2da662a
refactor
mpollmeier Sep 29, 2022
2db4447
indicate that truncation happened
mpollmeier Sep 29, 2022
253143b
test only that new setting, not the string truncation
mpollmeier Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/config/ScalaSettings.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ private sealed trait VerboseSettings:
val Vprofile: Setting[Boolean] = BooleanSetting("-Vprofile", "Show metrics about sources and internal representations to estimate compile-time complexity.")
val VprofileSortedBy = ChoiceSetting("-Vprofile-sorted-by", "key", "Show metrics about sources and internal representations sorted by given column name", List("name", "path", "lines", "tokens", "tasty", "complexity"), "")
val VprofileDetails = IntSetting("-Vprofile-details", "Show metrics about sources and internal representations of the most complex methods", 0)
val VreplMaxPrintElements: Setting[Int] = IntSetting("-Vrepl-max-print-elements", "Number of elements to be printed before output is truncated.", 1000)

/** -W "Warnings" settings
*/
Expand Down
6 changes: 3 additions & 3 deletions compiler/src/dotty/tools/repl/Rendering.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None):

private var myReplStringOf: Object => String = _


/** Class loader used to load compiled code */
private[repl] def classLoader()(using Context) =
if (myClassLoader != null && myClassLoader.root == ctx.settings.outputDir.value) myClassLoader
Expand All @@ -52,6 +51,7 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None):
}

myClassLoader = new AbstractFileClassLoader(ctx.settings.outputDir.value, parent)
val maxPrintElements = ctx.settings.VreplMaxPrintElements.valueIn(ctx.settingsState)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we plan to have 2 separate flags for the limits of elements and characters then this setting would actually correspond to the limit of characters, not the limit of elements, so we should give the setting the proper name from the start instead of renaming it later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above my intention was to keep this PR small, but yes you raise a valid point, so I'll add the split into two properties (with different name and semantics) into this PR. Some time later today - I'll let you know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean making the PR any bigger actually - just renaming replMaxPrintElements and similar to replMaxPrintCharacters. Then adding the new -VreplMaxPrintElements option could be done in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sure - done via 9f5cc90
Note that your comment rather applied to Rendering.truncate below, where I just renamed maxPrintElements to maxPrintCharacters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant doing the rename of every occurance of Element to Character and element to character in the scope of this PR - not just renaming this single variable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I misunderstood the concept of having 2 separate settings for controlling the truncation behaviour suggested by @som-snytt then.
Given a user types List(1, 2, 3, 4) in the REPL, what should be printed when we set

  1. -Vrepl-max-print-characters 3
  2. -Vrepl-max-print-elements 3

?
As far as I understood in case 1) we should get

Lis ... large output truncated, print value to show all

In case 2) that seems not to be fully specified yet but I suppose that would be something like

List(1, 2, 3, ...)

or

List(1, 2, ...)

depending on what we understand as an element - probably a node in some high level syntax tree (?).
An element of a string is a character so stringOf(value, maxElements = maxPrintCharacters) would still make sense.

Or did I get the entire idea wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did I get the entire idea wrong?

No certainly not, your explanation above looks completely correct, up until (but excluding) this sentence:

An element of a string is a character so stringOf(value, maxElements = maxPrintCharacters) would still make sense.

An element of a string certainly is a character. But the value in this case is not a string. It's a List[Int], and stringOf inspects that list and converts the them into a String, up until the maxElements index.

scala> scala.runtime.ScalaRunTime.stringOf(List(10,20,30,40), maxElements = 10)
val res0: String = List(10, 20, 30, 40)

scala> scala.runtime.ScalaRunTime.stringOf(List(10,20,30,40), maxElements = 4)
val res1: String = List(10, 20, 30, 40)

scala> scala.runtime.ScalaRunTime.stringOf(List(10,20,30,40), maxElements = 3)
val res2: String = List(10, 20, 30)

scala> scala.runtime.ScalaRunTime.stringOf(List(10,20,30,40), maxElements = 2)
val res3: String = List(10, 20)

Does that make sense?
Otherwise, it's really no big deal, I can rename that variable, and we clean it up in the followup PR that splits up the one setting into two (-Vrepl-max-print-characters and -Vrepl-max-print-elements).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think the point is that somehow I missed this comment, sorry for that

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.

I would say that in this situations the reasonable options would be to either

  1. add both -Vrepl-max-print-characters and -Vrepl-max-print-elements settings in the same PR

or

  1. add just one setting in this PR and replace the hardcoded value only in the places where it semantically corresponds to this setting and after that add the other setting in a followup PR

I still think we should not introduce a single setting changing these two values (maxElements and maxCharacters) just to later change the semantics of this setting to set just one value instead of two

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest in keeping this PR small, let's go with Option 2.
I reverted to the hardcoded value where it has different semantics (i.e. where it truncates by string length).

If you specify the setting when starting the repl it does work just fine (scala -Vrepl-max-print-elements:20), but dynamically updating the setting value in the repl (e.g. via :settings -Vrepl-max-print-elements:20) does not have an affect. Hence, the ScriptedTest fails.

I tried a few things - e.g. using reflection: given that the invocation of stringOf happens via reflection, I tried to do the same with the setting, but for some reason it doesn't find even the method:

val settingClazz = Class.forName("dotty.tools.dotc.config.Settings$Setting", true, myClassLoader)
val valueInMethod = settingClazz.getMethod("valueIn", classOf[SettingsState])
valueInMethod.invoke(ctx.settings.VreplMaxPrintElements, ctx.settingsState)

// java.lang.NoSuchMethodException: 
// dotty.tools.dotc.config.Settings$Setting.valueIn(dotty.tools.dotc.config.Settings$SettingsState)

Any idea what's going on, or alternative ways to fix it? I'm running out of ideas tbh.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the problem is that when classLoader is evaluated, the else branch gets executed only once at the beginning so myReplStringOf is not set again with a different value of maxElements. The solution might be to change myReplStringOf to a function taking 2 arguments: the value to print and the limit

myReplStringOf = {
// We need to use the ScalaRunTime class coming from the scala-library
// on the user classpath, and not the one available in the current
Expand All @@ -64,12 +64,12 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None):
val meth = scalaRuntime.getMethod(renderer, classOf[Object], classOf[Int], classOf[Boolean])
val truly = java.lang.Boolean.TRUE

(value: Object) => meth.invoke(null, value, Integer.valueOf(MaxStringElements), truly).asInstanceOf[String]
(value: Object) => meth.invoke(null, value, Integer.valueOf(maxPrintElements), truly).asInstanceOf[String]
} catch {
case _: NoSuchMethodException =>
val meth = scalaRuntime.getMethod(renderer, classOf[Object], classOf[Int])

(value: Object) => meth.invoke(null, value, Integer.valueOf(MaxStringElements)).asInstanceOf[String]
(value: Object) => meth.invoke(null, value, Integer.valueOf(maxPrintElements)).asInstanceOf[String]
}
}
myClassLoader
Expand Down
12 changes: 12 additions & 0 deletions compiler/test-resources/repl/settings-repl-disable-display
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
scala> 1
val res0: Int = 1

scala>:settings -Xrepl-disable-display

scala> 2

scala>:reset
Resetting REPL state.

scala> 3
val res0: Int = 3
7 changes: 7 additions & 0 deletions compiler/test-resources/repl/settings-repl-max-print-elements
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
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

scala>:settings -Vrepl-max-print-elements:20

scala> 1.to(300).toList
val res1: List[Int] = List(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should somehow indicate that the printed output was truncated, similarly to how it's done when the limit of characters gets exceeded

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be nice, but ScalaRunTime.stringOf only returns the resulting String...

I can see a few ways to achieve that, but none are particularly nice:

  1. we could invoke it twice - once with the limit, once without. If the string.length differs, we append the info that it was truncated. Downside: slower, and traversing the result may have side effects, dependent on what stringOf actually does.

  2. fork/copy the stringOf implementation and indicate if it was truncated in the result. Downside: duplicating code that looks rather complex.

  3. like 2), but bring this into scala2-library
    Downside: need to wait until the next scala2 release before we can use it.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with 1). As REPL is used mainly for experimenting, I wouldn't expect extremely big outputs very often, so the efficiency of printing should not be a very big problem. On the other hand, getting wrong string representation might bring people to wasting a lot of time trying to figure out why the output was not what they might have expected. And if stringOf could have any side effects, that would mean we should not use it for printing things in REPL at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done - I also added in some minor refactorings, some comments and updated the test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the Scala 2 side PR that adds ellipsis. Maybe I can get that piece accepted quickly (2.13.10 anyone?).

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

The whole point of that PR was to support Scala 3 nicely. (Not supplying extra newlines, etc.)