Improve message for "null toString" per review

WojciechMazur · WojciechMazur · commit 89684587afe4 · 2024-07-02T23:13:13.000+02:00
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 302aaca][modified]
diff --git 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
@@ -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:
