From b95cd75286145a8f72409bcf395c9c95a28709cd Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Mon, 24 Apr 2023 08:50:23 -0700 Subject: [PATCH 1/3] Repl truncation copes with null --- compiler/src/dotty/tools/repl/Rendering.scala | 22 +++++++++++-------- .../dotty/tools/repl/ReplCompilerTests.scala | 9 ++++++++ 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/compiler/src/dotty/tools/repl/Rendering.scala b/compiler/src/dotty/tools/repl/Rendering.scala index 517815615f2a..fe3bd3bc4668 100644 --- a/compiler/src/dotty/tools/repl/Rendering.scala +++ b/compiler/src/dotty/tools/repl/Rendering.scala @@ -71,13 +71,16 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None): // 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) - val maybeTruncatedByElementCount = stringOfMaybeTruncated(value, maxElements) - val maybeTruncated = 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) maybeTruncated - else s"$maybeTruncated ... large output truncated, print value to show all" + 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 } } @@ -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) + .flatMap(v => Option(myReplStringOf(v, maxPrintElements, maxPrintCharacters))) + .getOrElse("null // non-null reference has null-valued toString") /** Load the value of the symbol using reflection. * diff --git a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala index cfae36f394af..0afedd4d7626 100644 --- a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala +++ b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala @@ -437,6 +437,15 @@ class ReplCompilerTests extends ReplTest: s2 } + @Test def `i17333 print null result of toString`: Unit = + 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 + + object ReplCompilerTests: private val pattern = Pattern.compile("\\r[\\n]?|\\n"); From 302aacaecba4b7bb5c92b26f86c0071f56e791ed Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Thu, 27 Apr 2023 10:18:10 -0700 Subject: [PATCH 2/3] 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. --- compiler/src/dotty/tools/repl/Rendering.scala | 93 ++++++++++--------- .../dotty/tools/repl/ReplCompilerTests.scala | 50 +++++----- 2 files changed, 75 insertions(+), 68 deletions(-) diff --git a/compiler/src/dotty/tools/repl/Rendering.scala b/compiler/src/dotty/tools/repl/Rendering.scala index fe3bd3bc4668..adefe62d2b5f 100644 --- a/compiler/src/dotty/tools/repl/Rendering.scala +++ b/compiler/src/dotty/tools/repl/Rendering.scala @@ -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 } @@ -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. * @@ -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 * diff --git a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala index 0afedd4d7626..a179b5e47f47 100644 --- a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala +++ b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala @@ -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: @@ -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)") @@ -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: From 390d9567eed83d3bf5814a963e34e02461e238fc Mon Sep 17 00:00:00 2001 From: Jamie Thompson Date: Fri, 1 Mar 2024 14:33:28 +0100 Subject: [PATCH 3/3] print null result of replStringOf as null --- compiler/src/dotty/tools/repl/Rendering.scala | 4 +--- compiler/test/dotty/tools/repl/ReplCompilerTests.scala | 8 ++++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/compiler/src/dotty/tools/repl/Rendering.scala b/compiler/src/dotty/tools/repl/Rendering.scala index adefe62d2b5f..d5688d1038b4 100644 --- a/compiler/src/dotty/tools/repl/Rendering.scala +++ b/compiler/src/dotty/tools/repl/Rendering.scala @@ -100,9 +100,7 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None): val maxPrintElements = ctx.settings.VreplMaxPrintElements.valueIn(ctx.settingsState) val maxPrintCharacters = ctx.settings.VreplMaxPrintCharacters.valueIn(ctx.settingsState) // 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""" + def fallback = s"""null // result of "${sym.name}.toString" is null""" if value == null then "null" else myReplStringOf(value, maxPrintElements, maxPrintCharacters) match case null => fallback diff --git a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala index a179b5e47f47..a5bdddc51118 100644 --- a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala +++ b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala @@ -436,8 +436,8 @@ class ReplCompilerTests extends ReplTest: run("val tpolecat = new Object { override def toString(): String = null }") .andThen: val last = lines().last - assertTrue(last, last.startsWith("val tpolecat: Object = anon")) - assertTrue(last, last.endsWith("""// return value of "tpolecat.toString" is null""")) + assertTrue(last, last.startsWith("val tpolecat: Object = null")) + assertTrue(last, last.endsWith("""// result of "tpolecat.toString" is null""")) @Test def `i17333 print toplevel object with null toString`: Unit = initially: @@ -445,8 +445,8 @@ class ReplCompilerTests extends ReplTest: .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""")) + assertTrue(last, last.startsWith("val res0: tpolecat.type = null")) + assertTrue(last, last.endsWith("""// result of "res0.toString" is null""")) object ReplCompilerTests: