Repl truncation copes with null #17336
Conversation
| initially: | ||
| run("val tpolecat = new Object { override def toString(): String = null }") | ||
| .andThen: | ||
| assertEquals("val tpolecat: Object = null // non-null reference has null-valued toString", lines().head) |
There was a problem hiding this comment.
I think this is easier to understand, what do you think?
| assertEquals("val tpolecat: Object = null // non-null reference has null-valued toString", lines().head) | |
| assertEquals("val tpolecat: Object = null // .toString() was null", lines().head) |
There was a problem hiding this comment.
I agree. I wondered even, return value of .toString() was null after the NPE message
scala> X.toString.toString
java.lang.NullPointerException: Cannot invoke "Object.toString()" because the return value of "X$.toString()" is null
and now looking at it, I wonder if it is more useful and less redundant to use the default toString
scala> X
val res5: X.type = X$@23ba2c1e // return value of "X$.toString()" is null
or similar.
| @@ -95,8 +98,9 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None): | |||
| "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 maxPrintCharacters = ctx.settings.VreplMaxPrintCharacters.valueIn(ctx.settingsState) | |||
| val res = myReplStringOf(value, maxPrintElements, maxPrintCharacters) | |||
| if res == null then "null // non-null reference has null-valued toString" else res | |||
| Option(value) | |||
There was a problem hiding this comment.
if value is already expected to be non-null
| Option(value) | |
| Some(value) |
There was a problem hiding this comment.
Thanks for the contribution. I like the idea of the comment explaining that the reference was not null even though .toString is null. Nevertheless, it seems that it is also shown for null references. That causes fail in one of test cases run by the ScriptedTests.
|
Thanks for reviews. I will follow up expeditiously. |
som-snytt
force-pushed
the
issue/17333-repl-null
branch
from
440f68d
to
3ef1be0
Compare
|
TIL |
som-snytt
force-pushed
the
issue/17333-repl-null
branch
from
3ef1be0
to
6f63368
Compare
|
The update corrects null handling and "improves" the message. Someone somewhere asked why is Scala 3 REPL relying on Scala 2 runtime to "render a thing". After this tweak, and realizing that the Scala 2 hasn't been "upgraded", I have the same question. Usually I prefer "code sharing", but probably Scala 3 should just ingest the There is an issue about rendering infinite things; I haven't tested whether the code which tests the lengths of renderings actually works in that case. |
Also refactor the stringOf reflection to lookup method once. Add worst-case fallback to use String.valueOf. Re-enable some tests which appear not to crash.
|
I think it might be best to just do scala> class Foo { override def toString = null }
// defined class Foo
scala> Foo()
val res0: Foo = Foo@4f7150bd // return value of "res0.toString" is null
scala> List(Foo())
val res1: List[Foo] = List(null)
scala> |
bishabosha
force-pushed
the
issue/17333-repl-null
branch
from
6f63368
to
390d956
Compare
Backports #17336 to the LTS branch. PR submitted by the release tooling. [skip ci]
Fixes #17333