Merge pull request #9005 from rethab/8677/output-lines-order

sort output of definitions in repl, fixes #8677

Author: smarter

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 not 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/dotc/util/Spans.scala

@@ -5,9 +5,9 @@ import language.implicitConversions
 /** The offsets part of a full position, consisting of 2 or 3 entries:
  *    - start:  the start offset of the span, in characters from start of file
  *    - end  :  the end offset of the span
- *    - point:  if given, the offset where a sing;le `^` would be logically placed
+ *    - point:  if given, the offset where a single `^` would be logically placed
  *
- &  Spans are encoded according to the following format in little endian:
+ *  Spans are encoded according to the following format in little endian:
  *  Start: unsigned 26 Bits (works for source files up to 64M)
  *  End: unsigned 26 Bits
  *  Point: unsigned 12 Bits relative to start
@@ -30,8 +30,8 @@ object Spans {
 
   /** A span indicates a range between a start offset and an end offset.
    *  Spans can be synthetic or source-derived. A source-derived span
-   *  has in addition a point lies somewhere between start and end. The point
-   *  is roughly where the ^ would go if an error was diagnosed at that position.
+   *  has in addition a point. The point lies somewhere between start and end. The point
+   *  is roughly where the `^` would go if an error was diagnosed at that position.
    *  All quantities are encoded opaquely in a Long.
    */
   class Span(val coords: Long) extends AnyVal {

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,34 @@ 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)
+
+            // output is printed in the order it was put in. warnings should be
+            // shown before infos (eg. typedefs) for the same line. column
+            // ordering is mostly to make tests deterministic
+            implicit val diagnosticOrdering: Ordering[Diagnostic] =
+              Ordering[(Int, Int, Int)].on(d => (d.pos.line, -d.level, d.pos.column))
+
+            (definitions ++ warnings)
+              .sorted
+              .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 +280,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 extractAndFormatMembers(symbol: Symbol): (State, Seq[Diagnostic]) = if (tree.symbol.info.exists) {
       val info = symbol.info
       val defs =
         info.bounds.hi.finalResultType
@@ -274,51 +290,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
+          rendering.renderTypeDef(x)
       }
-      .foreach { sym =>
-        out.println(SyntaxHighlighting.highlight("// defined " + sym.showUser))
-      }
-
 
     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) = extractAndFormatMembers(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 +390,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).map(_.msg).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)
-    }
-  }
 }

compiler/test-resources/repl/top-level-block

@@ -17,6 +17,6 @@ scala> f + g
 val res3: Int = 10
 
 scala> { val x = 3; 4; val y = 5 }
-val res4: Int = 4
 val x: Int = 3
+val res4: Int = 4
 val y: Int = 5

compiler/test/dotty/tools/repl/LoadTests.scala

@@ -42,8 +42,8 @@ class LoadTests extends ReplTest {
     file    = """|@main def helloWorld = println("Hello, World!")
                  |@main def helloTo(name: String) = println(s"Hello, $name!")
                  |""".stripMargin,
-    defs    = """|def helloTo(name: String): Unit
-                 |def helloWorld: Unit
+    defs    = """|def helloWorld: Unit
+                 |def helloTo(name: String): Unit
                  |
                  |
                  |""".stripMargin,

compiler/test/dotty/tools/repl/ReplCompilerTests.scala

@@ -51,10 +51,10 @@ class ReplCompilerTests extends ReplTest {
 
     val expected = List(
       "def foo: Int",
-      "val res0: Int = 2",
-      "val res1: Int = 20",
       "val x: Int = 10",
-      "var y: Int = 5"
+      "val res0: Int = 2",
+      "var y: Int = 5",
+      "val res1: Int = 20"
     )
 
     assertEquals(expected, lines())
@@ -84,6 +84,25 @@ class ReplCompilerTests extends ReplTest {
       assertEquals("x: Int = 10", storedOutput().trim)
     }
 
+  @Test def i8677 = fromInitialState { implicit state =>
+    run {
+      """|sealed trait T1
+         |case class X() extends T1
+         |case class Y() extends T1
+         | case object O extends T1
+         """.stripMargin
+    }
+
+    val expected = List(
+     "// defined trait T1",
+     "// defined case class X",
+     "// defined case class Y",
+     "// defined case object O"
+    )
+
+    assertEquals(expected, lines())
+  }
+
   // FIXME: Tests are not run in isolation, the classloader is corrupted after the first exception
   @Ignore @Test def i3305: Unit = {
     fromInitialState { implicit s =>

language-server/test/dotty/tools/languageserver/WorksheetTest.scala

@@ -96,7 +96,7 @@ class WorksheetTest {
   @Test def patternMatching1: Unit = {
     ws"""${m1}val (foo, bar) = (1, 2)${m2}""".withSource
       .run(m1,
-        ((m1 to m2), s"val bar: Int = 2${nl}val foo: Int = 1"))
+        ((m1 to m2), s"val foo: Int = 1${nl}val bar: Int = 2"))
   }
 
   @Test def evaluationException: Unit = {