Skip to content

Commit

Permalink
Improve message for "null toString" per review
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@som-snytt @bishabosha
som-snytt authored and bishabosha committed Mar 1, 2024
1 parent b95cd75 commit 302aaca
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 68 deletions.
93 changes: 49 additions & 44 deletions compiler/src/dotty/tools/repl/Rendering.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,39 +50,40 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None):
// We need to use the ScalaRunTime class coming from the scala-library
// on the user classpath, and not the one available in the current
// classloader, so we use reflection instead of simply calling
// `ScalaRunTime.replStringOf`. Probe for new API without extraneous newlines.
// For old API, try to clean up extraneous newlines by stripping suffix and maybe prefix newline.
// `ScalaRunTime.stringOf`. Also probe for new stringOf that does string quoting, etc.
val scalaRuntime = Class.forName("scala.runtime.ScalaRunTime", true, myClassLoader)
val renderer = "stringOf"
def stringOfMaybeTruncated(value: Object, maxElements: Int): String = {
try {
val meth = scalaRuntime.getMethod(renderer, classOf[Object], classOf[Int], classOf[Boolean])
val truly = java.lang.Boolean.TRUE
meth.invoke(null, value, maxElements, truly).asInstanceOf[String]
} catch {
case _: NoSuchMethodException =>
val meth = scalaRuntime.getMethod(renderer, classOf[Object], classOf[Int])
meth.invoke(null, value, maxElements).asInstanceOf[String]
}
}

(value: Object, maxElements: Int, maxCharacters: Int) => {
// `ScalaRuntime.stringOf` may truncate the output, in which case we want to indicate that fact to the user
// In order to figure out if it did get truncated, we invoke it twice - once with the `maxElements` that we
// want to print, and once without a limit. If the first is shorter, truncation did occur.
val notTruncated = stringOfMaybeTruncated(value, Int.MaxValue)
if notTruncated == null then null else
val maybeTruncated =
val maybeTruncatedByElementCount = stringOfMaybeTruncated(value, maxElements)
truncate(maybeTruncatedByElementCount, maxCharacters)

// our string representation may have been truncated by element and/or character count
// if so, append an info string - but only once
if notTruncated.length == maybeTruncated.length then maybeTruncated
else s"$maybeTruncated ... large output truncated, print value to show all"
end if
}

val stringOfInvoker: (Object, Int) => String =
def richStringOf: (Object, Int) => String =
val method = scalaRuntime.getMethod(renderer, classOf[Object], classOf[Int], classOf[Boolean])
val richly = java.lang.Boolean.TRUE // add a repl option for enriched output
(value, maxElements) => method.invoke(null, value, maxElements, richly).asInstanceOf[String]
def poorStringOf: (Object, Int) => String =
try
val method = scalaRuntime.getMethod(renderer, classOf[Object], classOf[Int])
(value, maxElements) => method.invoke(null, value, maxElements).asInstanceOf[String]
catch case _: NoSuchMethodException => (value, maxElements) => String.valueOf(value).take(maxElements)
try richStringOf
catch case _: NoSuchMethodException => poorStringOf
def stringOfMaybeTruncated(value: Object, maxElements: Int): String = stringOfInvoker(value, maxElements)

// require value != null
// `ScalaRuntime.stringOf` returns null iff value.toString == null, let caller handle that.
// `ScalaRuntime.stringOf` may truncate the output, in which case we want to indicate that fact to the user
// In order to figure out if it did get truncated, we invoke it twice - once with the `maxElements` that we
// want to print, and once without a limit. If the first is shorter, truncation did occur.
// Note that `stringOf` has new API in flight to handle truncation, see stringOfMaybeTruncated.
(value: Object, maxElements: Int, maxCharacters: Int) =>
stringOfMaybeTruncated(value, Int.MaxValue) match
case null => null
case notTruncated =>
val maybeTruncated =
val maybeTruncatedByElementCount = stringOfMaybeTruncated(value, maxElements)
truncate(maybeTruncatedByElementCount, maxCharacters)
// our string representation may have been truncated by element and/or character count
// if so, append an info string - but only once
if notTruncated.length == maybeTruncated.length then maybeTruncated
else s"$maybeTruncated ... large output truncated, print value to show all"
}
myClassLoader
}
Expand All @@ -93,14 +94,20 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None):
else str.substring(0, str.offsetByCodePoints(0, maxPrintCharacters - 1))

/** Return a String representation of a value we got from `classLoader()`. */
private[repl] def replStringOf(value: Object)(using Context): String =
private[repl] def replStringOf(sym: Symbol, 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 maxPrintCharacters = ctx.settings.VreplMaxPrintCharacters.valueIn(ctx.settingsState)
Option(value)
.flatMap(v => Option(myReplStringOf(v, maxPrintElements, maxPrintCharacters)))
.getOrElse("null // non-null reference has null-valued toString")
// stringOf returns null if value.toString returns null. Show some text as a fallback.
def toIdentityString(value: Object): String =
s"${value.getClass.getName}@${System.identityHashCode(value).toHexString}"
def fallback = s"""${toIdentityString(value)} // return value of "${sym.name}.toString" is null"""
if value == null then "null" else
myReplStringOf(value, maxPrintElements, maxPrintCharacters) match
case null => fallback
case res => res
end if

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

if (!sym.is(Flags.Method) && sym.info == defn.UnitType)
None
private def stripReplPrefix(s: String): String =
if (s.startsWith(REPL_WRAPPER_NAME_PREFIX))
s.drop(REPL_WRAPPER_NAME_PREFIX.length).dropWhile(c => c.isDigit || c == '$')
else
valueString.map { s =>
if (s.startsWith(REPL_WRAPPER_NAME_PREFIX))
s.drop(REPL_WRAPPER_NAME_PREFIX.length).dropWhile(c => c.isDigit || c == '$')
else
s
}
s

/** Rewrap value class to their Wrapper class
*
Expand Down
50 changes: 26 additions & 24 deletions compiler/test/dotty/tools/repl/ReplCompilerTests.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ import scala.language.unsafeNulls

import java.util.regex.Pattern

import org.junit.Assert.{assertTrue => assert, _}
import org.junit.{Ignore, Test}
import org.junit.Assert.{assertEquals, assertFalse, assertTrue}
import org.junit.Assert.{assertTrue => assert}
import org.junit.Test
import dotty.tools.dotc.core.Contexts.Context

class ReplCompilerTests extends ReplTest:
Expand Down Expand Up @@ -107,28 +108,21 @@ class ReplCompilerTests extends ReplTest:
assertEquals(expected, lines())
}

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

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

initially {
run("""throw new IllegalArgumentException("Hello")""")
assert(storedOutput().startsWith("java.lang.IllegalArgumentException: Hello"))
}
@Test def `i3305 IAE`: Unit = initially:
run("""throw new IllegalArgumentException("Hello")""")
assertTrue(storedOutput().startsWith("java.lang.IllegalArgumentException: Hello"))

initially {
run("val (x, y) = null")
assert(storedOutput().startsWith("scala.MatchError: null"))
}
}
@Test def `i3305 ME`: Unit = initially:
run("val (x, y) = null")
assert(storedOutput().startsWith("scala.MatchError: null"))

@Test def i2789: Unit = initially {
run("(x: Int) => println(x)")
Expand Down Expand Up @@ -441,10 +435,18 @@ class ReplCompilerTests extends ReplTest:
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)

end ReplCompilerTests
val last = lines().last
assertTrue(last, last.startsWith("val tpolecat: Object = anon"))
assertTrue(last, last.endsWith("""// return value of "tpolecat.toString" is null"""))

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

object ReplCompilerTests:

Expand Down

0 comments on commit 302aaca

Please sign in to comment.