Skip to content

Commit 6f63368

Browse files
som-snyttsom-snytt
committed
Improve message for "null toString" per review
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.
1 parent 9c3c850 commit 6f63368

File tree

2 files changed

+75
-68
lines changed

2 files changed

+75
-68
lines changed

‎compiler/src/dotty/tools/repl/Rendering.scala

Lines changed: 49 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -50,39 +50,40 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None):
5050
// We need to use the ScalaRunTime class coming from the scala-library
5151
// on the user classpath, and not the one available in the current
5252
// classloader, so we use reflection instead of simply calling
53-
// `ScalaRunTime.replStringOf`. Probe for new API without extraneous newlines.
54-
// For old API, try to clean up extraneous newlines by stripping suffix and maybe prefix newline.
53+
// `ScalaRunTime.stringOf`. Also probe for new stringOf that does string quoting, etc.
5554
val scalaRuntime = Class.forName("scala.runtime.ScalaRunTime", true, myClassLoader)
5655
val renderer = "stringOf"
57-
def stringOfMaybeTruncated(value: Object, maxElements: Int): String = {
58-
try {
59-
val meth = scalaRuntime.getMethod(renderer, classOf[Object], classOf[Int], classOf[Boolean])
60-
val truly = java.lang.Boolean.TRUE
61-
meth.invoke(null, value, maxElements, truly).asInstanceOf[String]
62-
} catch {
63-
case _: NoSuchMethodException =>
64-
val meth = scalaRuntime.getMethod(renderer, classOf[Object], classOf[Int])
65-
meth.invoke(null, value, maxElements).asInstanceOf[String]
66-
}
67-
}
68-
69-
(value: Object, maxElements: Int, maxCharacters: Int) => {
70-
// `ScalaRuntime.stringOf` may truncate the output, in which case we want to indicate that fact to the user
71-
// In order to figure out if it did get truncated, we invoke it twice - once with the `maxElements` that we
72-
// want to print, and once without a limit. If the first is shorter, truncation did occur.
73-
val notTruncated = stringOfMaybeTruncated(value, Int.MaxValue)
74-
if notTruncated == null then null else
75-
val maybeTruncated =
76-
val maybeTruncatedByElementCount = stringOfMaybeTruncated(value, maxElements)
77-
truncate(maybeTruncatedByElementCount, maxCharacters)
78-
79-
// our string representation may have been truncated by element and/or character count
80-
// if so, append an info string - but only once
81-
if notTruncated.length == maybeTruncated.length then maybeTruncated
82-
else s"$maybeTruncated ... large output truncated, print value to show all"
83-
end if
84-
}
85-
56+
val stringOfInvoker: (Object, Int) => String =
57+
def richStringOf: (Object, Int) => String =
58+
val method = scalaRuntime.getMethod(renderer, classOf[Object], classOf[Int], classOf[Boolean])
59+
val richly = java.lang.Boolean.TRUE // add a repl option for enriched output
60+
(value, maxElements) => method.invoke(null, value, maxElements, richly).asInstanceOf[String]
61+
def poorStringOf: (Object, Int) => String =
62+
try
63+
val method = scalaRuntime.getMethod(renderer, classOf[Object], classOf[Int])
64+
(value, maxElements) => method.invoke(null, value, maxElements).asInstanceOf[String]
65+
catch case _: NoSuchMethodException => (value, maxElements) => String.valueOf(value).take(maxElements)
66+
try richStringOf
67+
catch case _: NoSuchMethodException => poorStringOf
68+
def stringOfMaybeTruncated(value: Object, maxElements: Int): String = stringOfInvoker(value, maxElements)
69+
70+
// require value != null
71+
// `ScalaRuntime.stringOf` returns null iff value.toString == null, let caller handle that.
72+
// `ScalaRuntime.stringOf` may truncate the output, in which case we want to indicate that fact to the user
73+
// In order to figure out if it did get truncated, we invoke it twice - once with the `maxElements` that we
74+
// want to print, and once without a limit. If the first is shorter, truncation did occur.
75+
// Note that `stringOf` has new API in flight to handle truncation, see stringOfMaybeTruncated.
76+
(value: Object, maxElements: Int, maxCharacters: Int) =>
77+
stringOfMaybeTruncated(value, Int.MaxValue) match
78+
case null => null
79+
case notTruncated =>
80+
val maybeTruncated =
81+
val maybeTruncatedByElementCount = stringOfMaybeTruncated(value, maxElements)
82+
truncate(maybeTruncatedByElementCount, maxCharacters)
83+
// our string representation may have been truncated by element and/or character count
84+
// if so, append an info string - but only once
85+
if notTruncated.length == maybeTruncated.length then maybeTruncated
86+
else s"$maybeTruncated ... large output truncated, print value to show all"
8687
}
8788
myClassLoader
8889
}
@@ -93,14 +94,20 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None):
9394
else str.substring(0, str.offsetByCodePoints(0, maxPrintCharacters - 1))
9495

9596
/** Return a String representation of a value we got from `classLoader()`. */
96-
private[repl] def replStringOf(value: Object)(using Context): String =
97+
private[repl] def replStringOf(sym: Symbol, value: Object)(using Context): String =
9798
assert(myReplStringOf != null,
9899
"replStringOf should only be called on values creating using `classLoader()`, but `classLoader()` has not been called so far")
99100
val maxPrintElements = ctx.settings.VreplMaxPrintElements.valueIn(ctx.settingsState)
100101
val maxPrintCharacters = ctx.settings.VreplMaxPrintCharacters.valueIn(ctx.settingsState)
101-
Option(value)
102-
.flatMap(v => Option(myReplStringOf(v, maxPrintElements, maxPrintCharacters)))
103-
.getOrElse("null // non-null reference has null-valued toString")
102+
// stringOf returns null if value.toString returns null. Show some text as a fallback.
103+
def toIdentityString(value: Object): String =
104+
s"${value.getClass.getName}@${System.identityHashCode(value).toHexString}"
105+
def fallback = s"""${toIdentityString(value)} // return value of "${sym.name}.toString" is null"""
106+
if value == null then "null" else
107+
myReplStringOf(value, maxPrintElements, maxPrintCharacters) match
108+
case null => fallback
109+
case res => res
110+
end if
104111

105112
/** Load the value of the symbol using reflection.
106113
*
@@ -112,17 +119,15 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None):
112119
val symValue = resObj
113120
.getDeclaredMethods.find(_.getName == sym.name.encode.toString)
114121
.flatMap(result => rewrapValueClass(sym.info.classSymbol, result.invoke(null)))
115-
val valueString = symValue.map(replStringOf)
122+
symValue
123+
.filter(_ => sym.is(Flags.Method) || sym.info != defn.UnitType)
124+
.map(value => stripReplPrefix(replStringOf(sym, value)))
116125

117-
if (!sym.is(Flags.Method) && sym.info == defn.UnitType)
118-
None
126+
private def stripReplPrefix(s: String): String =
127+
if (s.startsWith(REPL_WRAPPER_NAME_PREFIX))
128+
s.drop(REPL_WRAPPER_NAME_PREFIX.length).dropWhile(c => c.isDigit || c == '$')
119129
else
120-
valueString.map { s =>
121-
if (s.startsWith(REPL_WRAPPER_NAME_PREFIX))
122-
s.drop(REPL_WRAPPER_NAME_PREFIX.length).dropWhile(c => c.isDigit || c == '$')
123-
else
124-
s
125-
}
130+
s
126131

127132
/** Rewrap value class to their Wrapper class
128133
*

‎compiler/test/dotty/tools/repl/ReplCompilerTests.scala

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@ import scala.language.unsafeNulls
44

55
import java.util.regex.Pattern
66

7-
import org.junit.Assert.{assertTrue => assert, _}
8-
import org.junit.{Ignore, Test}
7+
import org.junit.Assert.{assertEquals, assertFalse, assertTrue}
8+
import org.junit.Assert.{assertTrue => assert}
9+
import org.junit.Test
910
import dotty.tools.dotc.core.Contexts.Context
1011

1112
class ReplCompilerTests extends ReplTest:
@@ -107,28 +108,21 @@ class ReplCompilerTests extends ReplTest:
107108
assertEquals(expected, lines())
108109
}
109110

110-
// FIXME: Tests are not run in isolation, the classloader is corrupted after the first exception
111-
@Ignore @Test def i3305: Unit = {
112-
initially {
113-
run("null.toString")
114-
assert(storedOutput().startsWith("java.lang.NullPointerException"))
115-
}
111+
@Test def `i3305 SOE meh`: Unit = initially:
112+
run("def foo: Int = 1 + foo; foo")
113+
assert(storedOutput().startsWith("java.lang.StackOverflowError"))
116114

117-
initially {
118-
run("def foo: Int = 1 + foo; foo")
119-
assert(storedOutput().startsWith("def foo: Int\njava.lang.StackOverflowError"))
120-
}
115+
@Test def `i3305 NPE`: Unit = initially:
116+
run("null.toString")
117+
assert(storedOutput().startsWith("java.lang.NullPointerException"))
121118

122-
initially {
123-
run("""throw new IllegalArgumentException("Hello")""")
124-
assert(storedOutput().startsWith("java.lang.IllegalArgumentException: Hello"))
125-
}
119+
@Test def `i3305 IAE`: Unit = initially:
120+
run("""throw new IllegalArgumentException("Hello")""")
121+
assertTrue(storedOutput().startsWith("java.lang.IllegalArgumentException: Hello"))
126122

127-
initially {
128-
run("val (x, y) = null")
129-
assert(storedOutput().startsWith("scala.MatchError: null"))
130-
}
131-
}
123+
@Test def `i3305 ME`: Unit = initially:
124+
run("val (x, y) = null")
125+
assert(storedOutput().startsWith("scala.MatchError: null"))
132126

133127
@Test def i2789: Unit = initially {
134128
run("(x: Int) => println(x)")
@@ -351,10 +345,18 @@ class ReplCompilerTests extends ReplTest:
351345
initially:
352346
run("val tpolecat = new Object { override def toString(): String = null }")
353347
.andThen:
354-
assertEquals("val tpolecat: Object = null // non-null reference has null-valued toString", lines().head)
355-
356-
end ReplCompilerTests
348+
val last = lines().last
349+
assertTrue(last, last.startsWith("val tpolecat: Object = anon"))
350+
assertTrue(last, last.endsWith("""// return value of "tpolecat.toString" is null"""))
357351

352+
@Test def `i17333 print toplevel object with null toString`: Unit =
353+
initially:
354+
run("object tpolecat { override def toString(): String = null }")
355+
.andThen:
356+
run("tpolecat")
357+
val last = lines().last
358+
assertTrue(last, last.startsWith("val res0: tpolecat.type = tpolecat"))
359+
assertTrue(last, last.endsWith("""// return value of "res0.toString" is null"""))
358360

359361
object ReplCompilerTests:
360362

0 commit comments

Comments
 (0)