Skip to content

Conversation

@lihaoyi
Copy link
Contributor

@lihaoyi lihaoyi commented Jan 25, 2026

Attempts to fix #25078, vibe coded. I won't pretend I understand the fix, but I manually tested the issue reproduction and it seems to work

● Summary of Changes

The diff shows one functional fix and two minor refactorings:

The Fix (line 298)

// Before:
(!flat || isAbsent(classRep)) // only checks isAbsent when flat=true

// After:
isAbsent(classRep) // always checks isAbsent

Minor Refactorings

  • Extract isUnderScala variable (line 340)
  • Extract existingDecl variable (line 369)

Root Cause

When using :dep org.scala-lang.modules::scala-parallel-collections:1.2.0 in the REPL, Coursier resolves the dependency and returns 3 JARs:

  1. scala-parallel-collections (the requested library)
  2. scala-library (transitive dependency)
  3. Another transitive dependency

The mergeNewEntries function processes each JAR and calls enterClasses to add classes to the symbol table. The problem is in enterClasses:

for (classRep <- classReps)
if (!maybeModuleClass(classRep) && hasFlatName(classRep) == flat &&
(!flat || isAbsent(classRep))) // BUG: when flat=false, this is always true!
initializeFromClassPath(root.symbol, classRep)

The condition (!flat || isAbsent(classRep)) means:

  • When flat=true: check isAbsent (correct)
  • When flat=false: condition is true, skip the isAbsent check (bug!)

When processing scala-library.jar, classes like Option, List, Seq, etc. that already exist in the symbol table are re-entered because the isAbsent check is skipped. This corrupts the symbol table, causing:

  • @Repeated annotation not recognized (varargs broken)
  • TASTy unpickling crashes
  • Other type errors

Why the Fix Works

By changing to isAbsent(classRep) (always check), we prevent re-entering classes that already exist in the symbol table. When scala-library.jar is processed, all its classes are skipped because they already exist, preserving the correct symbol definitions.

@lihaoyi
Copy link
Contributor Author

lihaoyi commented Jan 25, 2026

CC @bishabosha

@bishabosha bishabosha self-assigned this Jan 25, 2026
@bishabosha
Copy link
Member

bishabosha commented Jan 25, 2026

First impression without looking at the wider context - is there a time where actually you do want to re-enter the symbol - such as recompilation and the classpath adds a new definition- unless that is handled at a higher level

Of course if there are already incremental compilation tests that pass that scenario then this is probably not a concern

@lihaoyi
Copy link
Contributor Author

lihaoyi commented Jan 25, 2026

Yeah this area of the code seems pretty gnarly but I'm assuming that if I do something wrong, then something in CI should break???

for (classRep <- classReps)
if (!maybeModuleClass(classRep) && hasFlatName(classRep) == flat &&
(!flat || isAbsent(classRep))) // on 2nd enter of flat names, check that the name has not been entered before
isAbsent(classRep)) // Always check isAbsent to avoid re-entering existing classes (important for REPL :jar/:dep)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This edit makes the two loops identical, for module class and non-module.

@bishabosha
Copy link
Member

bishabosha commented Jan 26, 2026

i havent reviewed the code, but i have tried and indeed REPL works now with scala-parallel-collections, scala-parser-combinators, scala-collection-contrib added through :dep

@bishabosha
Copy link
Member

bishabosha commented Jan 26, 2026

@lihaoyi it seems libraries with some sort of jar dep on a library that introduces the same package, e.g. os-lib-watch still makes a cyclic error though (edit: also com.lihaoyi::scalasql-simple:0.2.7, com.typesafe.akka::akka-actor-typed:2.8.8, org.typelevel::cats-effect:3.6.3):

scala> :dep com.lihaoyi::os-lib-watch:0.11.6
Exception in thread "main" dotty.tools.dotc.core.CyclicReference: Cyclic reference involving module class os

 Run with -explain-cyclic for more details.

(couldnt actually get the -explain-cyclic option to work)

@lihaoyi
Copy link
Contributor Author

lihaoyi commented Jan 26, 2026

Did some more vibing, os-lib-watch seems to work now

agent@claude-sandbox-2026-01-21-224815:/Users/lihaoyi/Github/scala3$  sbt scala3-repl/run
Welcome to Scala 3.8.3-RC1-bin-SNAPSHOT-git-67dddb2 (17.0.17, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> :dep com.lihaoyi::os-lib-watch:0.11.6-M1
Resolved 1 dependencies (7 JARs)

scala> os.watch.watch
val res0: (Seq[os.Path], Set[os.Path] => Unit, (String, Any) => Unit, os.Path =>
   Boolean) => AutoCloseable = Lambda$2361/0x0000008801790400@48f3210f

Good question. Let me trace through the root cause more carefully.

The Root Cause

When loading a new JAR via :dep/:jar, mergeNewEntries is called to add new classes to existing packages. The original code for non-scala packages did:

else if fullClasspath.hasPackage(fullPackageName) then
packageClass.info = new PackageLoader(packageVal, fullClasspath) // <-- Problem!

This replaces the package's info with a new PackageLoader, which makes the already-completed package incomplete again. The next time anything accesses that package (e.g., looking up a symbol, accessing decls), it triggers the PackageLoader.doComplete method:

def doComplete(root: SymDenotation)(using Context): Unit = {
// ... sets Touched flag to detect cycles ...
root.info = ClassInfo(...)
enterClasses(root, packageName, flat = false) // calls isAbsent
// ...
}

The cycle happens because:

  1. Package os is accessed → triggers doComplete → sets Touched flag
  2. Inside doComplete, enterClasses calls isAbsent(classRep)
  3. isAbsent calls root.unforcedDecls.lookup(...)
  4. If this lookup triggers any symbol resolution that needs the os package info again, it sees the Touched flag already set → CyclicReference

How My Fix Addresses This

My fix has two parts:

  1. Don't replace the package info - Instead of making the package incomplete again, we enter new classes directly into the existing (already completed) package's scope:

else if fullClasspath.hasPackage(fullPackageName) then
// DON'T do: packageClass.info = new PackageLoader(...)
// Instead, enter classes directly into existing scope:
val loader = new PackageLoader(packageVal, jarClasspath)
loader.enterClasses(packageClass, fullPackageName, flat = false, forceAbsentCheck = true)
loader.enterClasses(packageClass, fullPackageName, flat = true, forceAbsentCheck = true)

Since the package is already completed, calling isAbsent (via unforcedDecls.lookup) just returns the existing decls without triggering any completion. No cycle possible.

  1. Restore original enterClasses behavior - The forceAbsentCheck parameter ensures that during normal package completion (not JAR merging), we don't unnecessarily call isAbsent for non-flat names, which was the original safe behavior.

Why the scala package special case existed

The original code already had this fix for scala packages (the ScalaPackageClass check) because scala-parallel-collections triggered the same cyclic reference issue. My fix simply extends this approach to all packages, not just scala packages.

@lihaoyi
Copy link
Contributor Author

lihaoyi commented Jan 26, 2026

scalasql-simple also seems to work now

scala> :dep com.lihaoyi::scalasql-simple:0.2.7
Resolved 1 dependencies (11 JARs)

scala> import scalasql.simple.*

scala> type T[V] = SimpleTable[V]
// defined alias type T[V] = scalasql.namedtuples.SimpleTable[V]

@lihaoyi
Copy link
Contributor Author

lihaoyi commented Jan 26, 2026

Seems like not quite there yet, a long session still results in errors

scala>  :dep com.lihaoyi::scalasql-simple:0.2.7
-- [E103] Syntax Error: --------------------------------------------------------
1 | :dep com.lihaoyi::scalasql-simple:0.2.7
  | ^
  | Illegal start of statement
  |
  | longer explanation available when compiling with `-explain`

scala> ^[[A
-- 
  |^
  |illegal character '\u001b'

scala> :dep com.lihaoyi::scalasql-simple:0.2.7
Resolved 1 dependencies (11 JARs)

scala> import scalasql.simple.*

scala> type T[V] = SimpleTable[V]
// defined alias type T[V] = scalasql.namedtuples.SimpleTable[V]

scala> :dep com.lihaoyi::os-lib-watch:0.11.6-M1
Resolved 1 dependencies (7 JARs)

scala> os.watch.watch
exception caught when loading module class Predef$: java.lang.AssertionError: assertion failed: scala.Predef$.Ensuring$ already has a symbol
-- [E044] Cyclic Error: --------------------------------------------------------
1 |os.watch.watch
  |         ^
  |         Overloaded or recursive method watch needs return type
  |
  |          Run with -explain-cyclic for more details.
  |
  | longer explanation available when compiling with `-explain`
1 error found

@lihaoyi
Copy link
Contributor Author

lihaoyi commented Jan 26, 2026

Ok seems to work now, at least at a cursory glance

scala> :dep com.lihaoyi::scalasql-simple:0.2.7
Resolved 1 dependencies (11 JARs)

scala> import scalasql.simple.*

scala> type T[V] = SimpleTable[V]
// defined alias type T[V] = scalasql.namedtuples.SimpleTable[V]

scala> :dep com.lihaoyi::os-lib-watch:0.11.6-M1
Resolved 1 dependencies (7 JARs)

scala> os.watch.watch
val res0: (Seq[os.Path], Set[os.Path] => Unit, (String, Any) => Unit, os.Path =>
   Boolean) => AutoCloseable = Lambda$2251/0x00000006016a8400@4cda59e6

scala> :dep org.scala-lang.modules::scala-parallel-collections:1.2.0
Resolved 1 dependencies (3 JARs)

scala> import scala.collection.parallel.CollectionConverters.seqIsParallelizable

scala> List(1,2,3).par
val res1: collection.parallel.ParSeq[Int] = ParVector(1, 2, 3)

scala> scala.collection.parallel.ParSeq(List(1,2,3)*)
val res2: collection.parallel.ParSeq[Int] = ParArray(1, 2, 3)

@bishabosha
Copy link
Member

rebuilt on your latest changes - seems loading os-lib has some problems when compiler searches packages for implicits:

scala> :dep com.lihaoyi::os-lib:0.11.6
Resolved 1 dependencies (5 JARs)

scala> List(1,2,3).foo // compiler will search for extension method `foo`.
exception caught when loading module class ZipFile$: java.lang.AssertionError: os.shaded_org_apache_tools_zip.ZipFile$Entry
-- [E008] Not Found Error: -----------------------------------------------------
1 |List(1,2,3).foo
  |^^^^^^^^^^^^^^^
  |value foo is not a member of List[Int]
1 error found

unsure if these os.shaded_org_apache_tools_zip classes are now broken

@lihaoyi
Copy link
Contributor Author

lihaoyi commented Jan 27, 2026

The last few commits reuse _platform where possible seems to make the exception caught when loading module class ZipFile$: java.lang.AssertionError: os.shaded_org_apache_tools_zip.ZipFile$Entry problem go away. Let's see if it breaks anything else

Root Cause

The issue was in ContextBase.initialize() in Contexts.scala:

def initialize()(using Context): Unit = {
_platform = newPlatform // Always creates a NEW platform!
definitions.init()
}

The problem: Every time a new compilation Run is created (which happens for each REPL input), Run.rootContext calls ctx.initialize(). This replaced the platform with a brand new one, discarding all JARs that were added via :dep or :jar commands.

The flow was:

  1. User runs :dep com.lihaoyi::os-lib:0.11.6
  2. JARs are added to platform.classPath (platform identity: X)
  3. User types List(1,2,3).foo
  4. A new Run is created → initialize() is called → new platform created (identity: Y)
  5. When the compiler tries to complete ZipFile.class, it uses platform Y which has no knowledge of the JARs added to platform X
  6. Inner class lookup fails → AssertionError

The Fix

Make initialize() only create a new platform if one doesn't already exist:

def initialize()(using Context): Unit = {
if _platform == null then
_platform = newPlatform
definitions.init()
}

This preserves the platform (and its classpath modifications) across multiple Run instances within the same REPL session, allowing :dep and :jar to work correctly.

// This is important because :dep/:jar commands add JARs to the platform's classpath,
// and we must not lose those when a new Run is created for each input.
// In non-interactive mode, always create a fresh platform to preserve original behavior.
if _platform == null || !ctx.mode.is(Mode.Interactive) then
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we need to guard this under !ctx.mode.is(Mode.Interactive), but if we don't then all the CI jobs explode, and since the problem seems to be limited in scope to the REPL guarding it under the REPL seems fine???

Copy link
Member

@bishabosha bishabosha Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess the more conservative solution is that platform has some extra classpath field like "dep jars" and you copy that classpath after calling newPlatform rather than re-use the whole platform.

Edit: idk tho splitting the classpath and merging on access seems dodgy, even AggregateClassPath is mutable, although just a cache)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any opinion here between the options, compiler/src/dotty/tools/dotc/config/JavaPlatform.scala looks pretty thin with only var currentClassPath as its mutable state. So whether we re-use the Platform itself or re-use the currentClassPath within the Platform seems immaterial

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Still can't use scala-parallel-collections in REPL via :dep

3 participants