sort output before printing in REPL & move rendering

Author: rethab

compiler/src/dotty/tools/dotc/reporting/Message.scala

@@ -24,9 +24,9 @@ object Message {
   * consumed by a subclass of `Reporter`. However, the error position is only
   * part of `Diagnostic`, not `Message`.
   *
-  * NOTE: you should not be persisting  Most messages take an implicit
-  * `Context` and these contexts weigh in at about 4mb per instance, as such
-  * persisting these will result in a memory leak.
+  * NOTE: you should persist a message directly, because most messages
+  * take an implicit `Context` and these contexts weigh in at about 4mb per
+  * instance. Therefore persisting these will result in a memory leak.
   *
   * Instead use the `persist` method to create an instance that does not keep a
   * reference to these contexts.

compiler/src/dotty/tools/repl/Rendering.scala

@@ -5,11 +5,17 @@ import java.io.{ StringWriter, PrintWriter }
 import java.lang.{ ClassLoader, ExceptionInInitializerError }
 import java.lang.reflect.InvocationTargetException
 
+import dotc.ast.tpd
 import dotc.core.Contexts.Context
 import dotc.core.Denotations.Denotation
 import dotc.core.Flags
-import dotc.core.Symbols.Symbol
+import dotc.core.Flags._
+import dotc.core.Symbols.{Symbol, defn}
 import dotc.core.StdNames.str
+import dotc.core.NameOps.NameDecorator
+import dotc.printing.ReplPrinter
+import dotc.reporting.{MessageRendering, Message, Diagnostic}
+import dotc.util.SourcePosition
 
 /** This rendering object uses `ClassLoader`s to accomplish crossing the 4th
  *  wall (i.e. fetching back values from the compiled class files put into a
@@ -21,8 +27,15 @@ import dotc.core.StdNames.str
  */
 private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None) {
 
+  import Rendering._
+
   private val MaxStringElements: Int = 1000  // no need to mkString billions of elements
 
+  /** A `MessageRenderer` for the REPL without file positions */
+  private val messageRenderer = new MessageRendering {
+    override def posStr(pos: SourcePosition, diagnosticLevel: String, message: Message)(implicit ctx: Context): String = ""
+  }
+
   private var myClassLoader: ClassLoader = _
 
   private var myReplStringOf: Object => String = _
@@ -63,7 +76,6 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None) {
    *  Calling this method evaluates the expression using reflection
    */
   private def valueOf(sym: Symbol)(implicit ctx: Context): Option[String] = {
-    val defn = ctx.definitions
     val objectName = sym.owner.fullName.encode.toString.stripSuffix("$")
     val resObj: Class[?] = Class.forName(objectName, true, classLoader())
     val value =
@@ -82,19 +94,33 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None) {
       }
   }
 
+  /** Formats errors using the `messageRenderer` */
+  def formatError(dia: Diagnostic)(implicit state: State): Diagnostic =
+    new Diagnostic(
+      messageRenderer.messageAndPos(dia.msg, dia.pos, messageRenderer.diagnosticLevel(dia))(state.context),
+      dia.pos,
+      dia.level
+    )
+
+  def renderTypeDef(d: Denotation)(implicit ctx: Context): Diagnostic =
+    infoDiagnostic("// defined " ++ d.symbol.showUser, d)
+
+  def renderTypeAlias(d: Denotation)(implicit ctx: Context): Diagnostic =
+    infoDiagnostic("// defined alias " ++ d.symbol.showUser, d)
+
   /** Render method definition result */
-  def renderMethod(d: Denotation)(implicit ctx: Context): String =
-    d.symbol.showUser
+  def renderMethod(d: Denotation)(implicit ctx: Context): Diagnostic =
+    infoDiagnostic(d.symbol.showUser, d)
 
   /** Render value definition result */
-  def renderVal(d: Denotation)(implicit ctx: Context): Option[String] = {
+  def renderVal(d: Denotation)(implicit ctx: Context): Option[Diagnostic] = {
     val dcl = d.symbol.showUser
 
     try {
-      if (d.symbol.is(Flags.Lazy)) Some(dcl)
-      else valueOf(d.symbol).map(value => s"$dcl = $value")
+      if (d.symbol.is(Flags.Lazy)) Some(infoDiagnostic(dcl, d))
+      else valueOf(d.symbol).map(value => infoDiagnostic(s"$dcl = $value", d))
     }
-    catch { case ex: InvocationTargetException => Some(renderError(ex)) }
+    catch { case ex: InvocationTargetException => Some(infoDiagnostic(renderError(ex), d)) }
   }
 
   /** Render the stack trace of the underlying exception */
@@ -108,4 +134,19 @@ private[repl] class Rendering(parentClassLoader: Option[ClassLoader] = None) {
     cause.printStackTrace(pw)
     sw.toString
   }
+
+  private def infoDiagnostic(msg: String, d: Denotation)(implicit ctx: Context): Diagnostic =
+    new Diagnostic.Info(msg, d.symbol.sourcePos)
+
+}
+
+object Rendering {
+
+  implicit class ShowUser(val s: Symbol) extends AnyVal {
+    def showUser(implicit ctx: Context): String = {
+      val printer = new ReplPrinter(ctx)
+      val text = printer.dclText(s)
+      text.mkString(ctx.settings.pageWidth.value, ctx.settings.printLines.value)
+    }
+  }
 }

compiler/src/dotty/tools/repl/ReplDriver.scala

@@ -139,6 +139,7 @@ class ReplDriver(settings: Array[String],
   // TODO: i5069
   final def bind(name: String, value: Any)(implicit state: State): State = state
 
+  // redirecting the output allows us to test `println` in scripted tests
   private def withRedirectedOutput(op: => State): State = {
     val savedOut = System.out
     val savedErr = System.err
@@ -238,19 +239,29 @@ class ReplDriver(settings: Array[String],
               allImports += (newState.objectIndex -> newImports)
             val newStateWithImports = newState.copy(imports = allImports)
 
-            val warnings = newState.context.reporter.removeBufferedMessages(newState.context)
-            displayErrors(warnings)(newState) // display warnings
-            implicit val ctx = newState.context
-            if (!ctx.settings.XreplDisableDisplay.value)
-              displayDefinitions(unit.tpdTree, newestWrapper)(newStateWithImports)
-            else
-              newStateWithImports
+            val warnings = newState.context.reporter
+              .removeBufferedMessages(newState.context)
+              .map(rendering.formatError)
+
+            implicit val ctx: Context = newState.context
+            val (updatedState, definitions) =
+              if (!ctx.settings.XreplDisableDisplay.value)
+                renderDefinitions(unit.tpdTree, newestWrapper)(newStateWithImports)
+              else
+                (newStateWithImports, Seq.empty)
+
+
+            (definitions ++ warnings)
+              .sortBy(_.pos.point) // ensure multi-line output is printed in the order it was typed in
+              .map(_.msg)
+              .foreach(out.println)
+
+            updatedState
         }
       )
   }
 
-  /** Display definitions from `tree` */
-  private def displayDefinitions(tree: tpd.Tree, newestWrapper: Name)(implicit state: State): State = {
+  private def renderDefinitions(tree: tpd.Tree, newestWrapper: Name)(implicit state: State): (State, Seq[Diagnostic]) = {
     implicit val ctx = state.context
 
     def resAndUnit(denot: Denotation) = {
@@ -264,7 +275,7 @@ class ReplDriver(settings: Array[String],
       name.startsWith(str.REPL_RES_PREFIX) && hasValidNumber && sym.info == defn.UnitType
     }
 
-    def displayMembers(symbol: Symbol) = if (tree.symbol.info.exists) {
+    def displayMembers(symbol: Symbol): (State, Seq[Diagnostic]) = if (tree.symbol.info.exists) {
       val info = symbol.info
       val defs =
         info.bounds.hi.finalResultType
@@ -274,51 +285,47 @@ class ReplDriver(settings: Array[String],
             denot.symbol.owner == defn.ObjectClass ||
             denot.symbol.isConstructor
           }
-          .sortBy(_.name)
 
       val vals =
         info.fields
           .filterNot(_.symbol.isOneOf(ParamAccessor | Private | Synthetic | Artifact | Module))
           .filter(_.symbol.name.is(SimpleNameKind))
-          .sortBy(_.name)
 
       val typeAliases =
-        info.bounds.hi.typeMembers.filter(_.symbol.info.isTypeAlias).sortBy(_.name)
+        info.bounds.hi.typeMembers.filter(_.symbol.info.isTypeAlias)
 
-      (
-        typeAliases.map("// defined alias " + _.symbol.showUser) ++
+      val formattedMembers =
+        typeAliases.map(rendering.renderTypeAlias) ++
         defs.map(rendering.renderMethod) ++
-        vals.map(rendering.renderVal).flatten
-      ).foreach(str => out.println(SyntaxHighlighting.highlight(str)))
+        vals.flatMap(rendering.renderVal)
 
-      state.copy(valIndex = state.valIndex - vals.count(resAndUnit))
+      (state.copy(valIndex = state.valIndex - vals.count(resAndUnit)), formattedMembers)
     }
-    else state
+    else (state, Seq.empty)
 
     def isSyntheticCompanion(sym: Symbol) =
       sym.is(Module) && sym.is(Synthetic)
 
-    def displayTypeDefs(sym: Symbol) = sym.info.memberClasses
+    def typeDefs(sym: Symbol): Seq[Diagnostic] = sym.info.memberClasses
       .collect {
         case x if !isSyntheticCompanion(x.symbol) && !x.symbol.name.isReplWrapperName =>
-          x.symbol
-      }
-      .foreach { sym =>
-        out.println(SyntaxHighlighting.highlight("// defined " + sym.showUser))
+          rendering.renderTypeDef(x)
       }
 
-
     ctx.atPhase(ctx.typerPhase.next) {
       // Display members of wrapped module:
       tree.symbol.info.memberClasses
         .find(_.symbol.name == newestWrapper.moduleClassName)
         .map { wrapperModule =>
-          displayTypeDefs(wrapperModule.symbol)
-          displayMembers(wrapperModule.symbol)
+          val formattedTypeDefs = typeDefs(wrapperModule.symbol)
+          val (newState, formattedMembers) = displayMembers(wrapperModule.symbol)
+          val highlighted = (formattedTypeDefs ++ formattedMembers)
+            .map(d => new Diagnostic(d.msg.mapMsg(SyntaxHighlighting.highlight), d.pos, d.level))
+          (newState, highlighted)
         }
         .getOrElse {
           // user defined a trait/class/object, so no module needed
-          state
+          (state, Seq.empty)
         }
     }
   }
@@ -378,18 +385,9 @@ class ReplDriver(settings: Array[String],
       state
   }
 
-  /** A `MessageRenderer` without file positions */
-  private val messageRenderer = new MessageRendering {
-    override def posStr(pos: SourcePosition, diagnosticLevel: String, message: Message)(implicit ctx: Context): String = ""
-  }
-
-  /** Render messages using the `MessageRendering` trait */
-  private def renderMessage(dia: Diagnostic): Context => String =
-    messageRenderer.messageAndPos(dia.msg, dia.pos, messageRenderer.diagnosticLevel(dia))(_)
-
-  /** Output errors to `out` */
+  /** shows all errors nicely formatted */
   private def displayErrors(errs: Seq[Diagnostic])(implicit state: State): State = {
-    errs.map(renderMessage(_)(state.context)).foreach(out.println)
+    errs.map(rendering.formatError).foreach(out.println)
     state
   }
 }

compiler/src/dotty/tools/repl/package.scala

@@ -1,21 +1,10 @@
 package dotty.tools
 
-import dotc.core.Contexts.Context
-import dotc.core.Symbols.Symbol
-import dotc.printing.ReplPrinter
 import dotc.reporting.{HideNonSensicalMessages, StoreReporter, UniqueMessagePositions}
 
 package object repl {
   /** Create empty outer store reporter */
   private[repl] def newStoreReporter: StoreReporter =
     new StoreReporter(null)
     with UniqueMessagePositions with HideNonSensicalMessages
-
-  private[repl] implicit class ShowUser(val s: Symbol) extends AnyVal {
-    def showUser(implicit ctx: Context): String = {
-      val printer = new ReplPrinter(ctx)
-      val text = printer.dclText(s)
-      text.mkString(ctx.settings.pageWidth.value, ctx.settings.printLines.value)
-    }
-  }
 }