Fix #4230: Handle import in the REPL correctly #4915

allanrenucci · 2018-08-08T20:31:36Z

We used to collect imports from the parsed tree and insert them as
parsed 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.

smarter · 2018-08-09T03:26:59Z

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

@@ -0,0 +1,30 @@
+package dotty.tools.repl


maybe put it in repl.transform ?

Even if that's the only one?

smarter · 2018-08-09T03:29:03Z

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

 */
 class ReplCompiler extends Compiler {
-  override protected def frontendPhases: List[List[Phase]] =
-    Phases.replace(classOf[FrontEnd], _ => new REPLFrontEnd :: Nil, super.frontendPhases)


I think keeping Phases.replace would be cleaner, it just needs to be passed a list of list of phases as a replacement.

That would be trickier. I need to replace FrontEnd by ReplFrontend, remove sbt.ExtractDependencies and sbt.ExtractAPI and finally add CollectTopLevelImports which needs to run after ReplFrontEnd but in its own group

Ah indeed, not worth it then

allanrenucci · 2018-08-21T15:00:32Z

I have a pickling issue:

class dotty.tools.dotc.reporting.diagnostic.messages$Error at ?: pickling difference for class ReplCompiler in dotty/compiler/src/dotty/tools/repl/ReplCompiler.scala

> diff before-pickling.txt after-pickling.txt
2857c2857,2861
<                           :Either[dotty.tools.repl.results.Errors, dotty.tools.dotc.ast.tpd.ValDef]>@<9296..9440>
---
>                           :
>                             Either[dotty.tools.repl.results.Errors, 
>                               dotty.tools.dotc.ast.Trees.ValDef[dotty.tools.dotc.core.Types.Type]
>                             ]
>                           >@<9296..9440>
2859c2863,2867
<                       :Either[dotty.tools.repl.results.Errors, dotty.tools.dotc.ast.tpd.ValDef]>@<9181..9440>
---
>                       :
>                         Either[dotty.tools.repl.results.Errors, 
>                           dotty.tools.dotc.ast.Trees.ValDef[dotty.tools.dotc.core.Types.Type]
>                         ]
>                       >@<9181..9440>

I am not sure how to debug that. cc/ @smarter @nicolasstucki

nicolasstucki · 2018-08-21T15:27:28Z

I'll have a look tomorrow

nicolasstucki · 2018-08-22T08:42:41Z

I think I found the place where it happens in 3402a29 (5cbb512 needs to be reverted, sorry). To debug it you should try writing 3402a29#diff-c7dacf9ef97cabd025fede32386ffdecR269 in some other smaller class, then you should be able to reproduce it with dotc with the TestConfiguration.picklingOptions flags.

nicolasstucki · 2018-08-22T09:14:11Z

@allanrenucci in a01be64 I added a test that reproduces the issue. You can minimize it from that test file.

allanrenucci · 2018-08-22T09:50:04Z

@nicolasstucki Thanks!

allanrenucci · 2018-08-22T15:02:04Z

With this new scheme I still cannot handle:

scala> object A { def f = 1 }                                                                                                                                                                                   
// defined object A

scala> import A._; def f = 2
def f: Int

scala> f                                                                                                                                                                                                        
val res0: Int = 1 // should probably be 2

The scalac REPL handles it properly, but they use a complex object nesting scheme that we don't want to replicate

allanrenucci · 2018-08-22T15:03:25Z

compiler/src/dotty/tools/dotc/interactive/Interactive.scala

@@ -329,7 +329,7 @@ object Interactive {

  def contextOfPath(path: List[Tree])(implicit ctx: Context): Context = path match {
    case Nil | _ :: Nil =>
-      ctx.run.runContext.fresh.setCompilationUnit(ctx.compilationUnit)


@odersky Was this intentional? If so, what's the rational for using the run context?

Pretty sure that's intentional. contextOfPath constructs a context with the correct scoping information. If you start from a random Context then you'll have too much stuff in the scope.

I could patch the rootContext of the new run as we used to do:

class ReplCompiler extends Compiler { def newRun(initCtx: Context, state: State) = new Run(this, initCtx) { override protected[this] def rootContext(implicit ctx: Context) = addMagicImports(super.rootContext) private def addMagicImports(initCtx: Context): Context = ... } }

This way I wouldn't need to modify this line

smarter · 2018-08-24T10:45:03Z

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

+class CollectTopLevelImports extends Phase {
+  import tpd._
+
+  def phaseName = "collecttoplevelimports"


we usually use camelCase.

smarter · 2018-08-24T10:45:35Z

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

- *  custom subclass of `GenBCode`. The custom `GenBCode`, `REPLGenBCode`, works
- *  in conjunction with a specialized class loader in order to load virtual
- *  classfiles.
+ *  Specifically it replaces the front end with `REPLFrontEnd`.
 */


I would just delete this whole comment, it doesn't really say anything interesting. (it could be replaced by an actual description of what REPLCompiler does though :))

smarter

Actually #4915 (comment) needs to be addressed.

smarter · 2018-08-24T10:53:33Z

With this new scheme I still cannot handle:

Can you open an issue for that?

allanrenucci · 2018-08-28T10:28:36Z

@smarter comments addressed

allanrenucci · 2018-08-28T10:29:25Z

Can you open an issue for that?

Will do once this is merged

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 scala#4978 by having user defined import in the run context. Auto-completions somehow ignored them when they were part of the untyped tree.

Comments addressed

allanrenucci added the stat:wip label Aug 8, 2018

smarter reviewed Aug 9, 2018

View reviewed changes

allanrenucci force-pushed the fix-4230 branch from 34366e2 to 377f622 Compare August 21, 2018 14:31

allanrenucci mentioned this pull request Aug 21, 2018

Fix #4978: Complete imported symbol in REPL #4979

Closed

nicolasstucki assigned allanrenucci Aug 22, 2018

allanrenucci mentioned this pull request Aug 22, 2018

Pickler/Unpickler dealias too eagerly #4988

Closed

allanrenucci force-pushed the fix-4230 branch 2 times, most recently from 40da6e6 to 4ab0a5e Compare August 22, 2018 14:22

allanrenucci removed the stat:wip label Aug 22, 2018

allanrenucci requested a review from smarter August 22, 2018 15:02

allanrenucci commented Aug 22, 2018

View reviewed changes

allanrenucci removed their assignment Aug 22, 2018

OlivierBlanvillain assigned smarter Aug 24, 2018

smarter reviewed Aug 24, 2018

View reviewed changes

smarter approved these changes Aug 24, 2018

View reviewed changes

smarter previously requested changes Aug 24, 2018

View reviewed changes

smarter assigned allanrenucci and unassigned smarter Aug 24, 2018

allanrenucci force-pushed the fix-4230 branch 2 times, most recently from e40d390 to 2548813 Compare August 28, 2018 09:15

allanrenucci removed their assignment Aug 28, 2018

allanrenucci assigned smarter Aug 28, 2018

allanrenucci added 3 commits September 3, 2018 14:31

Workaround scala#4988

bb69949

Add missing stripMargin

52a604c

allanrenucci force-pushed the fix-4230 branch from 2548813 to 52a604c Compare September 3, 2018 12:32

allanrenucci merged commit 26c4ecf into scala:master Sep 3, 2018

allanrenucci deleted the fix-4230 branch September 3, 2018 13:01

rochala mentioned this pull request Apr 15, 2022

repl - new heuristic for import shadowing #14951

Closed

Fix #4230: Handle import in the REPL correctly #4915

Fix #4230: Handle import in the REPL correctly #4915

Uh oh!

Conversation

allanrenucci commented Aug 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allanrenucci commented Aug 21, 2018

Uh oh!

nicolasstucki commented Aug 21, 2018

Uh oh!

nicolasstucki commented Aug 22, 2018

Uh oh!

nicolasstucki commented Aug 22, 2018

Uh oh!

allanrenucci commented Aug 22, 2018

Uh oh!

allanrenucci commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

smarter commented Aug 24, 2018

Uh oh!

allanrenucci commented Aug 28, 2018

Uh oh!

allanrenucci commented Aug 28, 2018

Uh oh!

Uh oh!

allanrenucci commented Aug 22, 2018 •

edited

Loading