Fix #4230: Handle import in the REPL correctly

We used to collect user defined imports from the parsed tree and
insert them as untyped trees at the top of the REPL wrapper object.
This caused members shadowing issues.

We now introduce a phase in the REPL compiler that collects imports
after type checking and store them as typed tree. We can then create a
context with its imports set in the correct order and use it to compile
future expressions.

This also fixes #4978 by having user defined import in the run context.
Auto-completions somehow ignored them when they were part of the
untyped tree.

Author: allanrenucci

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -111,7 +111,7 @@ class Compiler {
     List(new Flatten,                // Lift all inner classes to package scope
          new RenameLifted,           // Renames lifted classes to local numbering scheme
          new TransformWildcards,     // Replace wildcards with default values
-         new MoveStatics,            // Move static methods to companion classes
+         new MoveStatics,            // Move static methods from companion to the class itself
          new ExpandPrivate,          // Widen private definitions accessed from nested classes
          new RestoreScopes,          // Repair scopes rendered invalid by moving definitions in prior phases of the group
          new SelectStatic,           // get rid of selects that would be compiled into GetStatic

compiler/src/dotty/tools/dotc/Run.scala

@@ -147,7 +147,7 @@ class Run(comp: Compiler, ictx: Context) extends ImplicitRunInfo with Constraint
     compileUnits()(ctx)
   }
 
-  protected def compileUnits()(implicit ctx: Context) = Stats.maybeMonitored {
+  private def compileUnits()(implicit ctx: Context) = Stats.maybeMonitored {
     if (!ctx.mode.is(Mode.Interactive)) // IDEs might have multi-threaded access, accesses are synchronized
       ctx.base.checkSingleThreaded()
 

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

@@ -0,0 +1,30 @@
+package dotty.tools.repl
+
+import dotty.tools.dotc.ast.Trees._
+import dotty.tools.dotc.ast.tpd
+import dotty.tools.dotc.core.Contexts.Context
+import dotty.tools.dotc.core.Phases.Phase
+
+/** A phase that collects user defined top level imports.
+ *
+ *  These imports must be collected as typed trees and therefore
+ *  after Typer.
+ */
+class CollectTopLevelImports extends Phase {
+  import tpd._
+
+  def phaseName = "collectTopLevelImports"
+
+  private[this] var myImports: List[Import] = _
+  def imports = myImports
+
+  def run(implicit ctx: Context): Unit = {
+    def topLevelImports(tree: Tree) = {
+      val PackageDef(_, _ :: TypeDef(_, rhs: Template) :: Nil) = tree
+      rhs.body.collect { case tree: Import => tree }
+    }
+
+    val tree = ctx.compilationUnit.tpdTree
+    myImports = topLevelImports(tree)
+  }
+}

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

@@ -14,6 +14,7 @@ import dotty.tools.dotc.core.Phases.Phase
 import dotty.tools.dotc.core.StdNames._
 import dotty.tools.dotc.core.Symbols._
 import dotty.tools.dotc.reporting.diagnostic.messages
+import dotty.tools.dotc.transform.PostTyper
 import dotty.tools.dotc.typer.{FrontEnd, ImportInfo}
 import dotty.tools.dotc.util.Positions._
 import dotty.tools.dotc.util.SourceFile
@@ -23,42 +24,52 @@ import dotty.tools.repl.results._
 
 import scala.collection.mutable
 
-/** This subclass of `Compiler` replaces the appropriate phases in order to
- *  facilitate the REPL
+/** This subclass of `Compiler` is adapted for use in the REPL.
  *
- *  Specifically it replaces the front end with `REPLFrontEnd`, and adds a
- *  custom subclass of `GenBCode`. The custom `GenBCode`, `REPLGenBCode`, works
- *  in conjunction with a specialized class loader in order to load virtual
- *  classfiles.
+ *  - compiles parsed expression in the current REPL state:
+ *    - adds the appropriate imports in scope
+ *    - wraps expressions into a dummy object
+ *  - provides utility to query the type of an expression
+ *  - provides utility to query the documentation of an expression
  */
 class ReplCompiler extends Compiler {
-  override protected def frontendPhases: List[List[Phase]] =
-    Phases.replace(classOf[FrontEnd], _ => new REPLFrontEnd :: Nil, super.frontendPhases)
 
-  def newRun(initCtx: Context, objectIndex: Int) = new Run(this, initCtx) {
-    override protected[this] def rootContext(implicit ctx: Context) =
-      addMagicImports(super.rootContext)
-
-    private def addMagicImports(initCtx: Context): Context = {
-      def addImport(path: TermName)(implicit ctx: Context) = {
-        val importInfo = ImportInfo.rootImport { () =>
-          ctx.requiredModuleRef(path)
-        }
-        ctx.fresh.setNewScope.setImportInfo(importInfo)
+  override protected def frontendPhases: List[List[Phase]] = List(
+    List(new REPLFrontEnd),
+    List(new CollectTopLevelImports),
+    List(new PostTyper)
+  )
+
+  def newRun(initCtx: Context, state: State): Run = new Run(this, initCtx) {
+
+    /** Import previous runs and user defined imports */
+    override protected[this] def rootContext(implicit ctx: Context): Context = {
+      def importContext(imp: tpd.Import)(implicit ctx: Context) =
+        ctx.importContext(imp, imp.symbol)
+
+      def importPreviousRun(id: Int)(implicit ctx: Context) = {
+        // we first import the wrapper object id
+        val path = nme.EMPTY_PACKAGE ++ "." ++ objectNames(id)
+        val importInfo = ImportInfo.rootImport(() =>
+          ctx.requiredModuleRef(path))
+        val ctx0 = ctx.fresh.setNewScope.setImportInfo(importInfo)
+
+        // then its user defined imports
+        val imports = state.imports.getOrElse(id, Nil)
+        if (imports.isEmpty) ctx0
+        else imports.foldLeft(ctx0.fresh.setNewScope)((ctx, imp) =>
+          importContext(imp)(ctx))
       }
 
-      (1 to objectIndex)
-        .foldLeft(initCtx) { (ictx, i) =>
-          addImport(nme.EMPTY_PACKAGE ++ "." ++ objectNames(i))(ictx)
-        }
+      (1 to state.objectIndex).foldLeft(super.rootContext)((ctx, id) =>
+        importPreviousRun(id)(ctx))
     }
   }
 
   private[this] val objectNames = mutable.Map.empty[Int, TermName]
   private def objectName(state: State) =
-    objectNames.getOrElseUpdate(state.objectIndex, {
-      (str.REPL_SESSION_LINE + state.objectIndex).toTermName
-    })
+    objectNames.getOrElseUpdate(state.objectIndex,
+      (str.REPL_SESSION_LINE + state.objectIndex).toTermName)
 
   private case class Definitions(stats: List[untpd.Tree], state: State)
 
@@ -86,7 +97,7 @@ class ReplCompiler extends Compiler {
     }
 
     Definitions(
-      state.imports ++ defs,
+      defs,
       state.copy(
         objectIndex = state.objectIndex + (if (defs.isEmpty) 0 else 1),
         valIndex = valIdx
@@ -158,19 +169,18 @@ class ReplCompiler extends Compiler {
   def docOf(expr: String)(implicit state: State): Result[String] = {
     implicit val ctx: Context = state.context
 
-    /**
-     * Extract the "selected" symbol from `tree`.
+    /** Extract the "selected" symbol from `tree`.
      *
-     * Because the REPL typechecks an expression, special syntax is needed to get the documentation
-     * of certain symbols:
+     *  Because the REPL typechecks an expression, special syntax is needed to get the documentation
+     *  of certain symbols:
      *
-     * - To select the documentation of classes, the user needs to pass a call to the class' constructor
-     *   (e.g. `new Foo` to select `class Foo`)
-     * - When methods are overloaded, the user needs to enter a lambda to specify which functions he wants
-     *   (e.g. `foo(_: Int)` to select `def foo(x: Int)` instead of `def foo(x: String)`
+     *  - To select the documentation of classes, the user needs to pass a call to the class' constructor
+     *    (e.g. `new Foo` to select `class Foo`)
+     *  - When methods are overloaded, the user needs to enter a lambda to specify which functions he wants
+     *    (e.g. `foo(_: Int)` to select `def foo(x: Int)` instead of `def foo(x: String)`
      *
-     * This function returns the right symbol for the received expression, and all the symbols that are
-     * overridden.
+     *  This function returns the right symbol for the received expression, and all the symbols that are
+     *  overridden.
      */
     def extractSymbols(tree: tpd.Tree): Iterator[Symbol] = {
       val sym = tree match {
@@ -210,7 +220,7 @@ class ReplCompiler extends Compiler {
         import untpd._
 
         val valdef = ValDef("expr".toTermName, TypeTree(), Block(trees, unitLiteral))
-        val tmpl = Template(emptyConstructor, Nil, EmptyValDef, state.imports :+ valdef)
+        val tmpl = Template(emptyConstructor, Nil, EmptyValDef, List(valdef))
         val wrapper = TypeDef("$wrapper".toTypeName, tmpl)
           .withMods(Modifiers(Final))
           .withPos(Position(0, expr.length))
@@ -261,9 +271,8 @@ class ReplCompiler extends Compiler {
 
       if (errorsAllowed || !ctx.reporter.hasErrors)
         unwrapped(unit.tpdTree, src)
-      else {
+      else
         ctx.reporter.removeBufferedMessages.errors
-      }
     }
   }
 }

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

@@ -44,12 +44,12 @@ import scala.collection.JavaConverters._
  *
  *  @param objectIndex the index of the next wrapper
  *  @param valIndex    the index of next value binding for free expressions
- *  @param imports     the list of user defined imports
+ *  @param imports     a map from object index to the list of user defined imports
  *  @param context     the latest compiler context
  */
 case class State(objectIndex: Int,
                  valIndex: Int,
-                 imports: List[untpd.Import],
+                 imports: Map[Int, List[tpd.Import]],
                  context: Context)
 
 /** Main REPL instance, orchestrating input, compilation and presentation */
@@ -64,14 +64,14 @@ class ReplDriver(settings: Array[String],
 
   /** Create a fresh and initialized context with IDE mode enabled */
   private[this] def initialCtx = {
-    val rootCtx = initCtx.fresh.addMode(Mode.ReadPositions).addMode(Mode.Interactive).addMode(Mode.ReadComments)
+    val rootCtx = initCtx.fresh.addMode(Mode.ReadPositions | Mode.Interactive | Mode.ReadComments)
     val ictx = setup(settings, rootCtx)._2
     ictx.base.initialize()(ictx)
     ictx
   }
 
   /** the initial, empty state of the REPL session */
-  final def initialState = State(0, 0, Nil, rootCtx)
+  final def initialState = State(0, 0, Map.empty, rootCtx)
 
   /** Reset state of repl to the initial state
    *
@@ -144,7 +144,7 @@ class ReplDriver(settings: Array[String],
     Console.withOut(out) { Console.withErr(out) { op } }
 
   private def newRun(state: State) = {
-    val run = compiler.newRun(rootCtx.fresh.setReporter(newStoreReporter), state.objectIndex)
+    val run = compiler.newRun(rootCtx.fresh.setReporter(newStoreReporter), state)
     state.copy(context = run.runContext)
   }
 
@@ -177,9 +177,6 @@ class ReplDriver(settings: Array[String],
       .getOrElse(Nil)
   }
 
-  private def extractImports(trees: List[untpd.Tree]): List[untpd.Import] =
-    trees.collect { case imp: untpd.Import => imp }
-
   private def interpret(res: ParseResult)(implicit state: State): State = {
     val newState = res match {
       case parsed: Parsed if parsed.trees.nonEmpty =>
@@ -209,6 +206,9 @@ class ReplDriver(settings: Array[String],
       case _ => nme.NO_NAME
     }
 
+    def extractTopLevelImports(ctx: Context): List[tpd.Import] =
+      ctx.phases.collectFirst { case phase: CollectTopLevelImports => phase.imports }.get
+
     implicit val state = newRun(istate)
     compiler
       .compile(parsed)
@@ -217,8 +217,11 @@ class ReplDriver(settings: Array[String],
         {
           case (unit: CompilationUnit, newState: State) =>
             val newestWrapper = extractNewestWrapper(unit.untpdTree)
-            val newImports = newState.imports ++ extractImports(parsed.trees)
-            val newStateWithImports = newState.copy(imports = newImports)
+            val newImports = extractTopLevelImports(newState.context)
+            var allImports = newState.imports
+            if (newImports.nonEmpty)
+              allImports += (newState.objectIndex -> newImports)
+            val newStateWithImports = newState.copy(imports = allImports)
 
             val warnings = newState.context.reporter.removeBufferedMessages(newState.context)
             displayErrors(warnings)(newState) // display warnings
@@ -315,7 +318,10 @@ class ReplDriver(settings: Array[String],
       initialState
 
     case Imports =>
-      state.imports.foreach(i => out.println(SyntaxHighlighting(i.show(state.context))))
+      for {
+        objectIndex <- 1 to state.objectIndex
+        imp <- state.imports.getOrElse(objectIndex, Nil)
+      } out.println(imp.show(state.context))
       state
 
     case Load(path) =>

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

@@ -12,7 +12,6 @@ import dotc.core.Contexts.Context
  *  compiler pipeline.
  */
 private[repl] class REPLFrontEnd extends FrontEnd {
-  override def phaseName = "frontend"
 
   override def isRunnable(implicit ctx: Context) = true
 

compiler/test-resources/repl/import-shadowing

@@ -0,0 +1,27 @@
+scala> object A { def f = 1 }
+// defined object A
+
+scala> object B { def f = 2 }
+// defined object B
+
+scala> import A._
+
+scala> val x0 = f
+val x0: Int = 1
+
+scala> import B._
+
+scala> val x1 = f
+val x1: Int = 2
+
+scala> def f = 3
+def f: Int
+
+scala> val x2 = f
+val x2: Int = 3
+
+scala> def f = 4; import A._
+def f: Int
+
+scala> val x3 = f
+val x3: Int = 1

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

@@ -60,4 +60,24 @@ class TabcompleteTests extends ReplTest {
       val expected = List("comp1", "comp2", "comp3")
       assertEquals(expected, tabComplete("(new Foo).comp").sorted)
     }
+
+  @Test def completeFromPreviousState2 =
+    fromInitialState { implicit state  =>
+      val src = "def hello = 1"
+      run(src)
+    }
+    .andThen { implicit state =>
+      val expected = List("hello")
+      assertEquals(expected, tabComplete("hel"))
+    }
+
+  @Test def tabCompleteFromPreviousImport =
+    fromInitialState { implicit state =>
+      val src = "import java.io.FileDescriptor"
+      run(src)
+    }
+    .andThen { implicit state =>
+      val expected = List("FileDescriptor")
+      assertEquals(expected, tabComplete("val foo: FileDesc"))
+    }
 }