Make best-effort fail gracefully

jchyb · jchyb · commit 8cca998f80f0 · 2023-08-07T18:38:01.000+02:00
Makes it so the best-effort compilation does not crash during the
pickling and unpickling phase (mostly to avoid unnecessary crash reports
from reading metals logs, where those may appear), instead providing a
more descriptive error message related to the feature.
Also adjusts the testing framework for best effort to look out for
those errors.
diff --git a/compiler/src/dotty/tools/dotc/Driver.scala b/compiler/src/dotty/tools/dotc/Driver.scala
@@ -39,6 +39,8 @@ class Driver {
       catch
         case ex: FatalError =>
           report.error(ex.getMessage.nn) // signals that we should fail compilation.
+        case ex: Throwable if ctx.usesBestEffortTasty =>
+          report.bestEffortError(ex, "Some best-effort tasty files were not able to be used.")
         case ex: TypeError if !runOrNull.enrichedErrorMessage =>
           println(runOrNull.enrichErrorMessage(s"${ex.toMessage} while compiling ${files.map(_.path).mkString(", ")}"))
           throw ex
diff --git a/compiler/src/dotty/tools/dotc/report.scala b/compiler/src/dotty/tools/dotc/report.scala
@@ -80,6 +80,22 @@ object report:
     if ctx.settings.YdebugError.value then Thread.dumpStack()
     if ctx.settings.YdebugTypeError.value then ex.printStackTrace()
 
+  def bestEffortError(ex: Throwable, msg: String)(using Context): Unit =
+    val stackTrace =
+      Option(ex.getStackTrace()).map { st =>
+        if st.nn.isEmpty then ""
+        else s"Stack trace: \n ${st.nn.mkString("\n ")}".stripMargin
+      }.getOrElse("")
+    // Build tools and our test framework may check precisely for
+    // "Unsuccessful best-effort compilation." error text.
+    val fullMsg =
+      em"""Unsuccessful best-effort compilation.
+          |${msg}
+          |Cause:
+          | ${ex.toString.replace("\n", "\n ")}
+          |${stackTrace}"""
+    ctx.reporter.report(new Error(fullMsg, NoSourcePosition))
+
   def errorOrMigrationWarning(msg: Message, pos: SrcPos, from: SourceVersion)(using Context): Unit =
     if sourceVersion.isAtLeast(from) then
       if sourceVersion.isMigrating && sourceVersion.ordinal <= from.ordinal then migrationWarning(msg, pos)
diff --git a/compiler/src/dotty/tools/dotc/transform/Pickler.scala b/compiler/src/dotty/tools/dotc/transform/Pickler.scala
@@ -18,6 +18,7 @@ import util.concurrent.{Executor, Future}
 import compiletime.uninitialized
 import java.nio.file.Files
 import scala.language.unsafeNulls
+import scala.util.control.NonFatal
 
 object Pickler {
   val name: String = "pickler"
@@ -40,7 +41,7 @@ class Pickler extends Phase {
 
   // No need to repickle trees coming from TASTY
   override def isRunnable(using Context): Boolean =
-    super.isRunnable && !ctx.settings.fromTasty.value
+    super.isRunnable && !ctx.settings.fromTasty.value && (ctx.isBestEffort || !ctx.usesBestEffortTasty)
 
   private def output(name: String, msg: String) = {
     val s = new PrintStream(name)
@@ -75,75 +76,78 @@ class Pickler extends Phase {
   private val executor = Executor[Array[Byte]]()
 
   private def useExecutor(using Context) =
-    Pickler.ParallelPickling && !ctx.settings.YtestPickler.value
+    Pickler.ParallelPickling && !ctx.settings.YtestPickler.value && !ctx.isBestEffort
 
   override def run(using Context): Unit = {
     val unit = ctx.compilationUnit
     val isBestEffort = ctx.reporter.errorsReported || ctx.usesBestEffortTasty
     pickling.println(i"unpickling in run ${ctx.runId}")
-
-    for
-      cls <- dropCompanionModuleClasses(topLevelClasses(unit.tpdTree))
-      tree <- sliceTopLevel(unit.tpdTree, cls)
-    do
-      if ctx.settings.YtestPickler.value then beforePickling(cls) = tree.show
-
-      val pickler = new TastyPickler(cls)
-      val treePkl = new TreePickler(pickler)
-      treePkl.pickle(tree :: Nil)
-      Profile.current.recordTasty(treePkl.buf.length)
-
-      val positionWarnings = new mutable.ListBuffer[Message]()
-      def reportPositionWarnings() = positionWarnings.foreach(report.warning(_))
-
-      def computePickled(): Array[Byte] = inContext(ctx.fresh) {
-        serialized.run { scratch =>
-          treePkl.compactify(scratch)
-          if tree.span.exists then
-            val reference = ctx.settings.sourceroot.value
-            PositionPickler.picklePositions(
-                pickler, treePkl.buf.addrOfTree, treePkl.treeAnnots, reference,
-                unit.source, tree :: Nil, positionWarnings,
-                scratch.positionBuffer, scratch.pickledIndices)
-
-          if !ctx.settings.YdropComments.value then
-            CommentPickler.pickleComments(
-                pickler, treePkl.buf.addrOfTree, treePkl.docString, tree,
-                scratch.commentBuffer)
-
-          val pickled = pickler.assembleParts(isBestEffort)
-
-          def rawBytes = // not needed right now, but useful to print raw format.
-            pickled.iterator.grouped(10).toList.zipWithIndex.map {
-              case (row, i) => s"${i}0: ${row.mkString(" ")}"
-            }
-
-          // println(i"rawBytes = \n$rawBytes%\n%") // DEBUG
-          if pickling ne noPrinter then
-            println(i"**** pickled info of $cls")
-            println(TastyPrinter.showContents(pickled, ctx.settings.color.value == "never"))
-          pickled
+    try
+      for
+        cls <- dropCompanionModuleClasses(topLevelClasses(unit.tpdTree))
+        tree <- sliceTopLevel(unit.tpdTree, cls)
+      do
+        if ctx.settings.YtestPickler.value then beforePickling(cls) = tree.show
+
+        val pickler = new TastyPickler(cls)
+        val treePkl = new TreePickler(pickler)
+        treePkl.pickle(tree :: Nil)
+        Profile.current.recordTasty(treePkl.buf.length)
+
+        val positionWarnings = new mutable.ListBuffer[Message]()
+        def reportPositionWarnings() = positionWarnings.foreach(report.warning(_))
+
+        def computePickled(): Array[Byte] = inContext(ctx.fresh) {
+          serialized.run { scratch =>
+            treePkl.compactify(scratch)
+            if tree.span.exists then
+              val reference = ctx.settings.sourceroot.value
+              PositionPickler.picklePositions(
+                  pickler, treePkl.buf.addrOfTree, treePkl.treeAnnots, reference,
+                  unit.source, tree :: Nil, positionWarnings,
+                  scratch.positionBuffer, scratch.pickledIndices)
+
+            if !ctx.settings.YdropComments.value then
+              CommentPickler.pickleComments(
+                  pickler, treePkl.buf.addrOfTree, treePkl.docString, tree,
+                  scratch.commentBuffer)
+
+            val pickled = pickler.assembleParts(isBestEffort)
+
+            def rawBytes = // not needed right now, but useful to print raw format.
+              pickled.iterator.grouped(10).toList.zipWithIndex.map {
+                case (row, i) => s"${i}0: ${row.mkString(" ")}"
+              }
+
+            // println(i"rawBytes = \n$rawBytes%\n%") // DEBUG
+            if pickling ne noPrinter then
+              println(i"**** pickled info of $cls")
+              println(TastyPrinter.showContents(pickled, ctx.settings.color.value == "never"))
+            pickled
+          }
         }
-      }
 
-      /** A function that returns the pickled bytes. Depending on `Pickler.ParallelPickling`
-       *  either computes the pickled data in a future or eagerly before constructing the
-       *  function value.
-       */
-      val demandPickled: () => Array[Byte] =
-        if useExecutor then
-          val futurePickled = executor.schedule(computePickled)
-          () =>
-            try futurePickled.force.get
-            finally reportPositionWarnings()
-        else
-          val pickled = computePickled()
-          reportPositionWarnings()
-          if ctx.settings.YtestPickler.value then pickledBytes(cls) = pickled
-          () => pickled
-
-      unit.pickled += (cls -> demandPickled)
-    end for
+        /** A function that returns the pickled bytes. Depending on `Pickler.ParallelPickling`
+         *  either computes the pickled data in a future or eagerly before constructing the
+         *  function value.
+         */
+        val demandPickled: () => Array[Byte] =
+          if useExecutor then
+            val futurePickled = executor.schedule(computePickled)
+            () =>
+              try futurePickled.force.get
+              finally reportPositionWarnings()
+          else
+            val pickled = computePickled()
+            reportPositionWarnings()
+            if ctx.settings.YtestPickler.value then pickledBytes(cls) = pickled
+            () => pickled
+
+        unit.pickled += (cls -> demandPickled)
+      end for
+    catch
+      case NonFatal(ex) =>
+        report.bestEffortError(ex, "Some best-effort tasty files will not be generated.")
   }
 
   override def runOn(units: List[CompilationUnit])(using Context): List[CompilationUnit] = {
@@ -168,7 +172,7 @@ class Pickler extends Phase {
           .resolve("META-INF")
           .resolve("best-effort")
       Files.createDirectories(outpath)
-      BestEffortTastyWriter.write(outpath.nn, units)
+      BestEffortTastyWriter.write(outpath.nn, result)
     result
   }
 
diff --git a/compiler/test/dotty/tools/vulpix/ParallelTesting.scala b/compiler/test/dotty/tools/vulpix/ParallelTesting.scala
@@ -657,7 +657,6 @@ trait ParallelTesting extends RunnerOrchestration { self =>
       val flags = flags0 and "-Ywith-best-effort-tasty"
       val reporter = mkReporter
       val driver = new Driver
-      println((flags.all ++ files0.map(_.toString) ++ Array(bestEffortDir.toString)).mkString(" "))
 
       val args = Array("-classpath", flags.defaultClassPath + ":" + bestEffortDir.toString) ++ flags.options
 
@@ -913,6 +912,30 @@ trait ParallelTesting extends RunnerOrchestration { self =>
     override def maybeFailureMessage(testSource: TestSource, reporters: Seq[TestReporter]): Option[String] = None
   }
 
+  private final class NoBestEffortErrorsTest(testSources: List[TestSource], times: Int, threadLimit: Option[Int], suppressAllOutput: Boolean)(implicit summaryReport: SummaryReporting)
+  extends Test(testSources, times, threadLimit, suppressAllOutput) {
+
+    override def suppressErrors = true
+
+    override def maybeFailureMessage(testSource: TestSource, reporters: Seq[TestReporter]): Option[String] =
+      def compilerCrashed = reporters.exists(_.compilerCrashed)
+      val unsucceffulBestEffortErrorMsg = "Unsuccessful best-effort compilation."
+      val failedBestEffortCompilation: Seq[TestReporter] =
+        reporters.collect{
+          case testReporter if testReporter.errors.exists(_.msg.message.startsWith(unsucceffulBestEffortErrorMsg)) =>
+            testReporter
+        }
+
+      Option {
+        if compilerCrashed then s"Compiler crashed when compiling: ${testSource.title}"
+        else if !failedBestEffortCompilation.isEmpty then
+          failedBestEffortCompilation.flatMap(_.consoleOutput.split("\n")).mkString("\n")
+        else null
+      }
+    end maybeFailureMessage
+
+  }
+
 
   /** The `CompilationTest` is the main interface to `ParallelTesting`, it
    *  can be instantiated via one of the following methods:
@@ -1061,12 +1084,28 @@ trait ParallelTesting extends RunnerOrchestration { self =>
       this
     }
 
+    /** Creates a "neg" test run, which makes sure that each test manages successful
+     *  best-effort compilation, without any errors related to pickling/unpickling
+     *  of betasty files.
+     */
+    def checkNoBestEffortError()(implicit summaryReport: SummaryReporting): this.type = {
+      val test = new NoBestEffortErrorsTest(targets, times, threadLimit, shouldFail || shouldSuppressOutput).executeTestSuite()
+
+      cleanup()
+
+      if (test.didFail) {
+        fail("Best-effort test should not have shown a \"Unsuccessful best-effort compilation\" error, but did")
+      }
+
+      this
+    }
+
     /** Creates a "neg" test run, which makes sure that each test generates the
      *  correct number of errors at the correct positions. It also makes sure
      *  that none of these tests crashes the compiler.
      */
     def checkExpectedErrors()(implicit summaryReport: SummaryReporting): this.type =
-      val test = new NegTest(targets, times, threadLimit, shouldFail || shouldSuppressOutput).executeTestSuite()
+      val test = new NegTest(targets, times, threadLimit, shouldSuppressOutput).executeTestSuite()
 
       cleanup()
 
@@ -1490,8 +1529,6 @@ trait ParallelTesting extends RunnerOrchestration { self =>
     val (dirsStep1, filteredPicklingFiles) = compilationTargets(sourceDir, picklingFilter)
     val (dirsStep2, filteredUnpicklingFiles) = compilationTargets(sourceDir, unpicklingFilter)
 
-    // val filteredPicklingFiles = f1//.slice(0, 50)
-
     class BestEffortCompilation(
       name: String,
       file: JFile,
@@ -1593,13 +1630,13 @@ trait ParallelTesting extends RunnerOrchestration { self =>
 
       val step1Compilation = JointCompilationSource(
         testGroup.name, step1SourceFiles, flags.and("-Ybest-effort"), step1OutDir, fromTasty = NotFromTasty
-        )
+      )
 
       val bestEffortDir = new JFile(step1OutDir, s"META-INF${JFile.separator}best-effort")
 
       val step2Compilation = JointCompilationSource(
-        testGroup.name, step2SourceFiles, flags.and("-Ywith-best-effort-tasty"), step2OutDir, fromTasty = WithBestEffortTasty(bestEffortDir)
-        )
+        testGroup.name, step2SourceFiles, flags.and("-Ywith-best-effort-tasty").and("-YisBestEffort"), step2OutDir, fromTasty = WithBestEffortTasty(bestEffortDir)
+      )
       (step1Compilation, step2Compilation, bestEffortDir)
     }.unzip3
 
@@ -1641,8 +1678,8 @@ trait ParallelTesting extends RunnerOrchestration { self =>
   class BestEffortCompilationTest(step1: CompilationTest, step2: CompilationTest, bestEffortDirs: List[JFile], shouldDelete: Boolean)(implicit testGroup: TestGroup) {
 
     def checkNoCrash()(implicit summaryReport: SummaryReporting): this.type = {
-      step1.checkNoCrash() // Compile all files to generate the class files with best effort tasty
-      step2.checkNoCrash() // Compile with best effort tasty
+      step1.checkNoBestEffortError() // Compile all files to generate the class files with best effort tasty
+      step2.checkNoBestEffortError() // Compile with best effort tasty
 
       if (shouldDelete) {
         CompilationTest.aggregateTests(step1, step2).delete()
@@ -1660,7 +1697,7 @@ trait ParallelTesting extends RunnerOrchestration { self =>
     }
 
     def noCrashWithCompilingDependencies()(implicit summaryReport: SummaryReporting): this.type = {
-      step1.checkNoCrash() // Compile all files to generate the class files with best effort tasty
+      step1.checkNoBestEffortError() // Compile all files to generate the class files with best effort tasty
       step2.checkCompile() // Compile with best effort tasty
 
       this
