From 3cbef4d46fdd5d29fc7c3d88f136b5d51ede57e0 Mon Sep 17 00:00:00 2001 From: Wojciech Mazur Date: Tue, 2 Jul 2024 23:12:22 +0200 Subject: [PATCH 1/3] Repl truncation copes with null [Cherry-picked b95cd75286145a8f72409bcf395c9c95a28709cd][modified] --- compiler/src/dotty/tools/repl/Rendering.scala | 22 +++++++++++-------- .../dotty/tools/repl/ReplCompilerTests.scala | 8 +++++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/compiler/src/dotty/tools/repl/Rendering.scala b/compiler/src/dotty/tools/repl/Rendering.scala index b581e82739e1..0d0b970b839d 100644 --- a/compiler/src/dotty/tools/repl/Rendering.scala +++ b/compiler/src/dotty/tools/repl/Rendering.scala @@ -70,13 +70,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 } } @@ -94,8 +97,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 ecdfeb512e1b..42652193953d 100644 --- a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala +++ b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala @@ -353,6 +353,14 @@ class ReplCompilerTests extends ReplTest: @Test def `i13097 expect template after colon` = contextually: assert(ParseResult.isIncomplete("class C:")) + @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 89684587afe485ea6396f4dfa8013c0db8222769 Mon Sep 17 00:00:00 2001 From: Wojciech Mazur Date: Tue, 2 Jul 2024 23:13:13 +0200 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. [Cherry-picked 302aacaecba4b7bb5c92b26f86c0071f56e791ed][modified] --- compiler/src/dotty/tools/repl/Rendering.scala | 93 ++++++++++--------- .../dotty/tools/repl/ReplCompilerTests.scala | 48 +++++----- 2 files changed, 75 insertions(+), 66 deletions(-) diff --git a/compiler/src/dotty/tools/repl/Rendering.scala b/compiler/src/dotty/tools/repl/Rendering.scala index 0d0b970b839d..a6cf26e79d3e 100644 --- a/compiler/src/dotty/tools/repl/Rendering.scala +++ b/compiler/src/dotty/tools/repl/Rendering.scala @@ -49,39 +49,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 } @@ -92,14 +93,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. * @@ -111,17 +118,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 42652193953d..d4ff3c42a3d7 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)") @@ -357,8 +351,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) + 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""")) end ReplCompilerTests object ReplCompilerTests: From eabf647229f1e29883e156b7059fd98bc18551fb Mon Sep 17 00:00:00 2001 From: Wojciech Mazur Date: Tue, 2 Jul 2024 23:13:53 +0200 Subject: [PATCH 3/3] print null result of replStringOf as null [Cherry-picked 390d9567eed83d3bf5814a963e34e02461e238fc][modified] --- compiler/src/dotty/tools/repl/Rendering.scala | 4 +--- .../test/dotty/tools/repl/ReplCompilerTests.scala | 15 +++++++-------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/compiler/src/dotty/tools/repl/Rendering.scala b/compiler/src/dotty/tools/repl/Rendering.scala index a6cf26e79d3e..e1e8567c6734 100644 --- a/compiler/src/dotty/tools/repl/Rendering.scala +++ b/compiler/src/dotty/tools/repl/Rendering.scala @@ -99,9 +99,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 d4ff3c42a3d7..c193cd476d44 100644 --- a/compiler/test/dotty/tools/repl/ReplCompilerTests.scala +++ b/compiler/test/dotty/tools/repl/ReplCompilerTests.scala @@ -352,18 +352,17 @@ 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: 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""")) -end ReplCompilerTests + .andThen: + run("tpolecat") + val last = lines().last + assertTrue(last, last.startsWith("val res0: tpolecat.type = null")) + assertTrue(last, last.endsWith("""// result of "res0.toString" is null""")) object ReplCompilerTests: