REPL not sufficiently isolated from class artifacts in current directory

[som-snytt]

Compiler version

3.0.0

Minimized code

➜  snips scala oq.scala
2
0
➜  snips
➜  snips scalac -version
Scala compiler version 3.0.0 -- Copyright 2002-2021, LAMP/EPFL
➜  snips rm *.class *.tasty
➜  snips scala oq.scala
2
2

Output

This should never happen:

➜  snips vi hi.scala
➜  snips cat hi.scala

@main def test() = println("hello, world")
➜  snips scala hi.scala
hello, world
➜  snips scalac hi.scala
➜  snips vi hi.scala
➜  snips scala hi.scala
hello, world
➜  snips cat hi.scala

@main def test() = println("goodbye, cruel world")
➜  snips

Expectation

Running a script or interactive REPL actually runs the source text.

This is a follow-up to #7635 which was improperly closed.

Noticed again at https://gitter.im/scala/scala?at=60a9b889850bfa2d3bdaae09

[bjornregnell]

See also:
https://gitter.im/scala/scala?at=60ba844621d6d00b1fb4fb3f
The error on util.Random not being seen was because I just happened to have a sub-folder named util...

[SethTisue]

This bit somebody again today: https://contributors.scala-lang.org/t/type-of-an-anonymous-class-derivation/5431/5 cc @tpolecat

It routinely bites veterans as well as beginners.

I'm re-adding the "area:repl" label, since I'm certain the REPL is affected.

[som-snytt]

I just read the contributors thread, very funny! or sad! or one of those things! OOTT is that even a thing.

As in the OP "this should never happen". Also we should never waste the time of major contributors. We tend to focus on the "newbie experience".

[SethTisue]

did somebody work on this at the spree today?

[griggt]

I had originally signed up to work on it but was reassigned to a different issue.

[som-snytt]

I finished the bedroom flooring today and next I have to look again at the f-interpolator. But I'd still be willing to put my money where my mouth is. I'll let you know when I come into some cash.

[bjornregnell]

@som-snytt Beware of insufficient bedroom floor isolation and spurious artifacts in the same place. Cash is king.

[SethTisue]

Note that #12607 includes @Ignored tests for this

[som-snytt]

TIL the word "subtle" is a portmanteau of "subdir util".

[bjornregnell]

In Swedish "subtle" is "subtil", i.e. including explicitly an abbreviation of "today I learned" (which I learned today).

I'm so grateful that you work on this, as this is kind of a "ticking" bomb in the sense that it can explode in a spending of a lot of time on"what!?"-stuff.

[SethTisue]

The King James Bible (1611) also spells it "subtil". Most famously in Genesis: "the serpent was more subtil than any beast of the field..."

Dale and I spent about two hours looking at this today and we learned some things but don't really have a solution in sight yet. But both of us want to keep working on it, both for the importance of the bug itself, and because it's good learning and good saw-sharpening.

Some of the code we were studying:

• ReplCompiler#newRun is where each new thing entered in the REPL gets access to the bindings established earlier in the session; a fold repeatedly calls importPreviousRun, which does ctx.fresh and .withRootImports(RootRef(() => requiredModuleRef(path)) :: Nil) where path is successively repl.rs$line$1, repl.rs$line$2, and so on. (The repl. part was added by Tom in Fix #7635: Package REPL wrappers #12607.)

• References to names are resolved in Typer by findRef which calls findRefRecur which calls checkNewOrShadowed, which seems relevant and perhaps if we studied it (and/or added logging and/or stepped through it in a debugger) we'd have an aha moment.

At the broadest level, we still don't know:

• Does something go wrong at the time the directory in the file system is seen and a package symbol created for it?
• Or does something first go wrong at the point when the def or val is entered?
• Or is all of that working as intended, and the bug occurs only when you attempt to reference the def or val?

We sort of circled around between these three possibilities, dipping shallowly into each one, without committing yet to truly proving or disproving a particular one.

Dale has a WIP branch with some added logging (but those logs have yet to produce a smoking gun).

[bjornregnell]

Thrilling excursions! Keeping fingers crossed for a 🚬 🔫

[dwijnand]

Looking at some debug output from findRefRecur revealed that the package foo that blows up is found in the context in which val foo was previously defined. So my current theory is that val foo type checks and returns but also forces the package foo into existence, in a way that it later wins over val foo.

[SethTisue]

Typer line 339:

          // [...] Finally, for the root package
          // we switch back to the original test. This means that top-level packages in
          // the root package take priority over root imports. [...]

ReplCompiler#newRun uses root imports as the mechanism for giving successive REPL entries access to identifiers from previous entries, so surely this is the key piece of logic?

("key piece", I say, but findRefRecur is 250 lines of pretty dense logic...)

[SethTisue]

Here's a slight variant of this bug which doesn't involve a filesystem directory:

Scala 2:

scala 2.13.7> java
              ^
              error: package java is not a value

scala 2.13.7> val java = 3
val java: Int = 3

scala 2.13.7> java
val res1: Int = 3

whereas Scala 3 falls down:

Welcome to Scala 3.1.0
                                                                                                                        
scala> java
-- Error:
1 |java
  |^^^^
  |package java is not a value
                                                                                                                        
scala> val java = 3
javval java: Int = 3
                                                                                                                        
scala> java
-- Error:
1 |java
  |^^^^
  |package java is not a value

This demonstrates that the problem is a bit more fundamental than the current issue title indicates. The fundamental problem is that the REPL uses root imports and root imports are lower priority than a top level package, regardless of how the top level package got on the classpath.

The two basic possible solution paths are:

1. change Typer so that "top-level packages in the root package take priority over root imports" isn't always true (we need it not to be true when the root imports in question are REPL bindings)
2. leave Typer alone and change REPLCompiler to somehow avoid the whole issue

We probably want to consider 2 first, since Typer is complex and fragile. Could REPLCompiler use regular imports instead of root imports to bring previous REPL bindings into scope? Why was the choice made to have it use root imports?

[som-snytt]

Re "subtil", beloved grad school prof recently wrote: Ever since the serpent said to Eve, "Look at that pensile fruit!" has anyone, ever, used this word?

I replied: I will no longer even tell my daughter how to hold a pencil any more.

(School notified us this week that they are doing California sex ed in biology class. First of all, they should introduce it in math class, because it has to do with exponential growth.)

I haven't looked at the Scala 3 sitch yet, but we're just talking about pluggable root contexts ("root imports" is a misnomer). So the Nr 1 sounds wrong to me, but I haven't looked yet to understand it. Experience dictates that REPL must be in "mechanical sympathy" with the compiler, and "workarounds" should be avoided. My Scala 2 PR for REPL to "use actual root contexts" is closed, but that is an example.

[griggt]

ReplCompiler#newRun uses root imports as the mechanism for giving successive REPL entries access to identifiers from previous entries, so surely this is the key piece of logic?

While working on #12607 I recall I convinced myself that the use of the "root imports" mechanism was the crux of the matter and that the Scala 2 REPL perhaps used more localized imports? (but dunno, I'm not too familiar with the Scala 2 codebase and ran out of time to investigate)

[griggt]

The two basic possible solution paths are:

1. change `Typer` so that "top-level packages in the root package take priority over root imports" isn't always true (we need it not to be true when the root imports in question are REPL bindings)

2. leave `Typer` alone and change `REPLCompiler` to somehow avoid the whole issue

I previously considered something akin to (1) but it seemed like a bad idea. I started down the path of (2) but stopped when it no longer looked quick and easy, and I wasn't even sure I was on the right track (but your new example and comments make me think that I was, and express so clearly the muddled mess that was in my head, thank you!)

Why was the choice made to have it use root imports?

I'd love to know this as well.

[griggt]

My Scala 2 PR for REPL to "use actual root contexts" is closed, but that is an example.

Would you happen to have a link for that? I'd love to have a look.

[som-snytt]

Yes, it was pre-pandemic but github is easy for me to search.

scala/scala#8593

Probably there is nothing wrong with whatever it was trying to solve. I like the general idea of "let me plug in whatever contexts I prefer", but that is not our current thinking, unlike plan 9 for instance.

[SethTisue]

Dale is exploring a hypothesis: perhaps the problem is that the root imports for the previous REPL entries are being added to a context whose owner is the root package, whereas they should be added to a context whose owner is the REPL package (if we keep #12607) or the empty package (if we revert #12607).

(I hope I've stated this correctly; Dale's understanding is currently ahead of mine.)

[SethTisue]

PR: #14021 (yes, that hypothesis was the way forward)