Ensure package objects are opened after namer in interactive #10988

tgodzik · 2025-01-28T21:22:30Z

Package objects need to be opened during the packageobjects
phase. This was not happening in presentation compilation
because the phase check looks at globalPhase, but in presentation
compilation (completion) the code is processed through compileLate
which only advances the SymbolTable phase.

Fixes scala/bug#12663

tgodzik · 2025-01-28T21:25:37Z

I am not 100% about the fix, but I got kind of lost in what is going on there.

We seem to have two deferredOpen sets at the same time and because the package name is the same as a package name of a classfile dependency, there seem to be some weirdness going on.

Maybe it's connected to the lazy analyser in interactive.Global that overrides the other lazy analyser? I am not sure about how that could be involved.

tgodzik · 2025-01-29T08:16:53Z

Looks like this should have been a part of #9661 since it makes sense in terms of:

This PR takes it a step further and defers forcing the info of all package.class until the packageObjects phase.

lrytz · 2025-01-29T14:04:53Z

src/compiler/scala/tools/nsc/typechecker/Analyzer.scala

@@ -80,7 +80,7 @@ trait Analyzer extends AnyRef

      def apply(unit: CompilationUnit): Unit = {
        openPackageObjectsTraverser(unit.body)
-        deferredOpen.foreach(openPackageModule(_))
+        deferredOpen.foreach(openPackageModule(_, force = true))


What's confusing: this change is in the packageobjects phase, which is after namer.

openPackageModule checks isPast(currentRun.namerPhase), so it should not delay opening.

but isPast compares with globalPhase, and it turns out that in this case we get here through compileLate, which only sets phase but not globalPhase.

at scala.tools.nsc.typechecker.Analyzer$packageObjects$$anon$2.apply(Analyzer.scala:84) at scala.tools.nsc.Global$GlobalPhase.applyPhase(Global.scala:483) at scala.tools.nsc.Global$Run.$anonfun$compileLate$3(Global.scala:1703) ... at scala.tools.nsc.Global$Run.compileLate(Global.scala:1702) at scala.tools.nsc.interactive.Global.parseAndEnter(Global.scala:669)

so yeah, i think this looks right. it should not ever make a difference in batch compilation.

note that there is already some logic in interactive.Global.openPackageModule to set force = true in more cases, but that doesn't fire here either (unit.isParsed is false)

ha, a brief line comment would be nice. // globalPhase may not be past namer when compileLate. I looked briefly last night and knew I was missing something.

Added! Let me know if the comment is enough

lrytz

running sbt:scala2> partest test/files/presentation/package-object-issues locally i get a difference:

!! 1 - presentation/package-object-issues        [output differs]
% diff /Users/luc/code/scala/scala13/test/files/presentation/package-object-issues.check /Users/luc/code/scala/scala13/test/files/presentation/package-object-issues-presentation.log
@@ -3,9 +3,8 @@ reload: Main.scala
 askTypeCompletion at Main.scala(7,6)
 ================================================================================
 [response] askTypeCompletion at (7,6)
-retrieved 46 members
-[inaccessible] private[package lang] def getStackTraceDepth(): Int
-[inaccessible] private[package lang] def getStackTraceElement(x$1: Int): StackTraceElement
+retrieved 45 members
+[inaccessible] final private[package lang] def setCause(x$1: Throwable): Unit
 [inaccessible] protected[package lang] def clone(): Object
 [inaccessible] protected[package lang] def finalize(): Unit
 def +(other: String): String

can you explain this? maybe it depends on the java version? i'm on 21.0.1+12-LTS.

tgodzik · 2025-01-29T22:08:09Z

can you explain this? maybe it depends on the java version? i'm on 21.0.1+12-LTS.

Yes, I had the same on JDK 21. I am not 100% sure why it's different, but I don't think it has anything to do with the fixed bug. We would probably get the same if a normal import.

tgodzik · 2025-01-29T22:17:20Z

I added a similar case without the scala.concurrent package and the result is the same.

som-snytt · 2025-01-30T02:00:55Z

I found my old branch, but I don't remember how the tests work. I'll report on it below.

I got this test to work by changing the pres global to always delegate:

  override def openPackageModule(pkgClass: Symbol, force: Boolean): Unit = {
    val isPastNamer = force || currentTyperRun == null || (currentTyperRun.currentUnit match {
      case unit: RichCompilationUnit => unit.isParsed
      case _                         => true
    })
    super.openPackageModule(pkgClass, force = isPastNamer)

and nsc.Global to add a phase check per lrytz:

  override def openPackageModule(pkgClass: Symbol, force: Boolean): Unit = {
    val forceNow = force || isPast(currentRun.namerPhase) ||
      isGlobalInitialized && isAtPhaseAfter(currentRun.namerPhase)
    if (forceNow) super.openPackageModule(pkgClass, force = true)
    else analyzer.packageObjects.deferredOpen.addOne(pkgClass)
  }

My printlns indicate that pres Global defers the package, then package objects (PO) dequeues them and pres Global's isPastNamer check makes it dump it back to deferred. I don't know anything at this point, however. I don't know if there is a motivation to move the force flag into global. What if there is a good reason for global to move a package back onto the queue?

[] parsing: Main.scala
[] OPEN pkgmod package concurrent force? false isPastNamer? false
DEFERRED OPEN package concurrent
PO package concurrent
[] OPEN pkgmod package concurrent force? false isPastNamer? false
DEFERRED OPEN package concurrent
[] responded, delay = 8ms

in the working case, nsc Global correctly decides to force based on phase.

[] parsing: Main.scala
[] OPEN pkgmod package concurrent force? false isPastNamer? false
GLB open package concurrent frc false now false
PO package concurrent
[] OPEN pkgmod package concurrent force? false isPastNamer? false
GLB open package concurrent frc false now true
[] responded, delay = 14ms

My test had the package object source in the test dir, not in src. But it works trivially if it is in the src dir. So I don't know yet how the test is structured, or I've long forgotten.

test/files/presentation/t12663:
src  Test.scala

test/files/presentation/t12663/src:
Snippet.scala  Source_1.scala

➜  scala git:(issue/12663-pc-pkgobj-alias) ✗ cat test/files/presentation/t12663/src/*la
package p {
  object Main extends App {
    def i: I = 42
    def f = i./*!*/
  }
}

package object p {
  type I = Int
}

lrytz · 2025-01-30T07:53:10Z

Yes, I had the same on JDK 21

The problem is that this will fail CI after merge. PR validation is on JDK 8 only, but after merging we test on many JDKs.

lrytz · 2025-01-30T08:45:04Z

@som-snytt I agree the change to interactive.Global (always delegate to nsc.Global.openPackageModule`) looks right. It doesn't make sense to defer once we're after namer phase.

val forceNow = force || isPast(currentRun.namerPhase) ||
isGlobalInitialized && isAtPhaseAfter(currentRun.namerPhase)

yeah, probably that is the more principled fix. a bit unfortunate we have manually check global initialization. but not unexpected as we're in this difficult area of forcing things not too early, but also not too late...

I pushed those changes.

Package objects need to be opened during the packageobjects phase. This was not happening in presentation compilation because the phase check looks at `globalPhase`, but in presentation compilation (completion) the code is processed through `compileLate` which only advances the SymbolTable phase.

lrytz

LGTM, thanks!

Squashed.

scala-jenkins added this to the 2.13.17 milestone Jan 28, 2025

tgodzik force-pushed the fix-package branch from 1110b05 to 72fa22f Compare January 28, 2025 22:25

tgodzik marked this pull request as ready for review January 29, 2025 07:31

lrytz reviewed Jan 29, 2025

View reviewed changes

lrytz requested changes Jan 29, 2025

View reviewed changes

tgodzik force-pushed the fix-package branch 3 times, most recently from 76c3724 to 9ef1368 Compare January 30, 2025 09:40

lrytz force-pushed the fix-package branch from 9ef1368 to c6b1879 Compare January 30, 2025 10:56

lrytz changed the title ~~bugfix: Try to fix package objects issues~~ Ensure package objects are opened after namer in interactive Jan 30, 2025

lrytz approved these changes Jan 30, 2025

View reviewed changes

lrytz merged commit 082a924 into scala:2.13.x Jan 30, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Ensure package objects are opened after namer in interactive #10988

Ensure package objects are opened after namer in interactive #10988

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ensure package objects are opened after namer in interactive #10988

Ensure package objects are opened after namer in interactive #10988

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!