Skip to content

Root package priority is wrong #12566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
som-snytt opened this issue Mar 26, 2022 · 20 comments
Closed

Root package priority is wrong #12566

som-snytt opened this issue Mar 26, 2022 · 20 comments
Labels

Comments

@som-snytt
Copy link

Reproduction steps

Scala version: 2.13.8

Welcome to Scala 2.13.8 (OpenJDK 64-Bit Server VM, Java 17.0.2).
Type in expressions for evaluation. Or try :help.

scala> object sun { val x = 42 }
object sun

scala> import sun.x
import sun.x

scala> import jdk.CollectionConverters._
                  ^
       error: object CollectionConverters is not a member of package jdk

At scala/scala3#14781

Problem

Scala 3 agrees about jdk, but the REPLs disagree about sun.

@som-snytt
Copy link
Author

Today's Scala 3 REPL works, but scalac only doesn't like unqualified jdk. So Scala 2 agrees with that.

It's weird that Scala 3 REPL disagrees with scalac on jdk.

@som-snytt
Copy link
Author

som-snytt commented Mar 26, 2022

Maybe sdkman is messing with me. It touches the environment. Time to finally switch to cs or CSI or whatever.

JAVA_HOME=/home/amarki/.sdkman/candidates/java/current
SBT_HOME=/home/amarki/.sdkman/candidates/sbt/current
SCALA_HOME=/home/amarki/.sdkman/candidates/scala/current

Just keeping fingers busy while listening to Don Carlos.

@som-snytt
Copy link
Author

TIL the reason for this behavior is that jdk is presented as a definition in the outermost context. That gives it the wrong name-binding precedence.

The outermost context should perhaps be an import context (importing from the root class), like the other so-called root imports.

I tried that out, an easy change but with other adjustments required.

Possible future innovations include other special packages in root, such as for REPL.

@som-snytt som-snytt changed the title Root package priority inconsistent with Scala 3 Root package priority is wrong Mar 30, 2022
@SethTisue
Copy link
Member

(to fill in a bit of unstated context here that wasn't obvious to me initially,) note that there is _root_.jdk package:

scala> import _root_.jdk.
dynalink     internal     jpackage     net          security     vm
editpad      javadoc      jshell       nio          swing        xml
incubator    jfr          management   random       tools

hence the conflict.

@som-snytt
Copy link
Author

Scala 3 has the same problem, although the ticket is currently in suspension.

I have a PR forthcoming. The fix is that root contexts should all be import contexts; currently that _root_.jdk has the priority of a definition, which is plain wrong. The change was not trivial for some reason that escapes me at the moment.

@SethTisue
Copy link
Member

https://scala-lang.org/files/archive/spec/2.13/02-identifiers-names-and-scopes.html says:

Bindings of different kinds have precedence defined on them:

  1. Definitions and declarations that are local, inherited, or made available by a package clause and also defined in the same compilation unit as the reference to them, have the highest precedence.
  2. Explicit imports have the next highest precedence.
  3. Wildcard imports have the next highest precedence.
  4. Definitions made available by a package clause, but not also defined in the same compilation unit as the reference to them, as well as imports which are supplied by the compiler but not explicitly written in source code, have the lowest precedence.

The import of scala._, which would pick up scala.jdk, is one of the "imports which are supplied by the compiler but not explicitly written in source code" (bullet 4)

And the presence of jdk under _root_, which bullet does that fall under? It seems to me it doesn't fall under any of them; is there some other place in the spec that says why things under _root_ are in scope? I looked at Chapter 9 as well but the wordings in https://scala-lang.org/files/archive/spec/2.13/09-top-level-definitions.html#packagings don't seem — to me, anyway — to clearly specify how _root_ should behave.

currently that root.jdk has the priority of a definition, which is plain wrong

Can you spell out why that is plainly wrong? And what priority exactly are you suggesting it should have? I don't disagree necessarily, it just isn't so "plain" to me without further explanation/substantiation.

In any case, as Dale points out, it's just plain lousy that the REPL and the batch compiler don't agree here.

It would be awfully late in the Scala 2 cycle to change the behavior of the batch compiler, so I guess a PR that aligned with the REPL with the batch compiler would be welcome? (depending on exactly how the change would be justified, perhaps... I'd like to get clear on that before anybody starts hacking on this)

@som-snytt
Copy link
Author

Sorry to have used that "obviously" language. I think the name binding is priority 4. But note that a package is not a definition; a package exists only by virtue of a definition which has the package name as a prefix of its full dotted name. (I don't know what package p { } does without trying it.)

So, to split hairs, I prefer "imports supplied by the compiler" and to implement it that way.

The alternative is to view package p has having an empty prefix which makes _root_.* visible. (That is actually close to the implementation.) But I think that is conceptually mistaken. The "empty package" as understood on the JVM is just another package whose name is empty; is it not a top-most package which might be called _root_. The visibility of the empty package has been problematic in Scala 3 for this reason.

Since _root_.jdk is not defined in the current compilation unit (in the example), whichever way you model it, its priority is 4 for purposes of name binding.

The bug is just that name lookup returns jdk as a definition in the current compilation unit.

@SethTisue
Copy link
Member

Sorry to have used that "obviously" language

(brings to mind the old mathematician joke: https://hsm.stackexchange.com/questions/7247/in-a-popular-anecdote-who-took-20-minutes-to-decide-that-a-thing-was-obvious)

@SethTisue
Copy link
Member

whichever way you model it, its priority is 4 for purposes of name binding.

✔️

The bug is just that name lookup returns jdk as a definition in the current compilation unit

👍

@som-snytt
Copy link
Author

-Yimports:scala.jdk.CollectionConverters

To recap, import jdk.CollectionConverters._ is rejected because it takes it as _root_.jdk on recent JVM.

The exception is Scala 3 REPL; but that REPL has a history of getting its class loaders wrong so that it sees the wrong thing.

➜  snips scala
Welcome to Scala 2.13.9 (OpenJDK 64-Bit Server VM, Java 18.0.2.1).
Type in expressions for evaluation. Or try :help.

scala> import jdk.CollectionConverters._
                  ^
       error: object CollectionConverters is not a member of package jdk

scala>
:quit
➜  snips scalac -d /tmp t12566.scala
t12566.scala:2: error: object CollectionConverters is not a member of package jdk
import jdk.CollectionConverters._
           ^
t12566.scala:5: error: value asScala is not a member of java.util.List[String]
  def f(xs: java.util.List[String]) = xs.asScala
                                         ^
2 errors
➜  snips

➜  snips scala
Welcome to Scala 3.2.0 (18.0.2.1, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> import jdk.CollectionConverters._

scala>
➜  snips scalac -d /tmp t12566.scala
-- [E008] Not Found Error: t12566.scala:2:11 ---------------------------------------------------------------------------
2 |import jdk.CollectionConverters._
  |       ^^^^^^^^^^^^^^^^^^^^^^^^
  |       value CollectionConverters is not a member of jdk
1 error found
➜  snips

I quickly double-checked, same result on JDK 8.

This use case is annoying. But if I have this on my class path:

➜  snips cat kollect.scala

package collection {
  class Map
}

what do I expect new collection.Map to do?

I would expect that I must import scala.{collection as _, *} to see the rooted collection package. But that only works for predef objects. In fact, I would have to import _root_.{collection as kollection} if it were hidden.

The argument for the status quo is that the "root contexts" are the imports of last resort. Rooted packages are available from the class path as simple identifiers, and there is nothing to "import".

The alternative intuition is that the class path is not constrained -- a random logging library can turn up and claim the namespace _root_.logging. (Or a random new version of the JDK can claim _root_.jdk.) But the root contexts, even if augmented by -Yimports is both finite and under finer control. (Note --release 8 does not hide jdk.)

@som-snytt
Copy link
Author

On reflection, although my intuition is that root contexts should shadow root packages, the real use case is to have a --release facility for both root contexts and the class path. This has been mentioned in the past as a way to restrict exposure to java.lang. It could also define a profile for scala lib and by extension anything on the class path.

One could imagine a learner's profile that reduces the API on String, so that teachers could assign "code indexOf" without adding, "without using indexOf".

@SethTisue
Copy link
Member

SethTisue commented Nov 17, 2022

I've removed the "help wanted" label here, as I think the discussion here has become confused. See also discussion on scala/scala3#15904, also rather confused.

@som-snytt Let's try to achieve some clarity here.

There is a broader design question here, of whether we prefer scala.jdk or _root_.jdk to take precedence.

I think that broader design question is out of scope for Scala 2. It's been the way it is for a long time — _root_.jdk takes precedence — and we should leave it alone.

@som-snytt but do you also think there's something in this ticket that's a just-plain-bug and we should just-plain-fix it? (It seems above that I concluded that there is, but my past self didn't write down my reasoning and thus my current self doesn't know what to think.)

If you do think there's something here that is a just-plain-bug, can you state as clearly as possible what you think it is?

@som-snytt
Copy link
Author

The argument that this behavior is simply a bug is odersky's comment on the dotty ticket, which claims that the rooted name is at precedence 4.

Name look-up should be satisified in the first context with the name. Per your comment, you can't just break scoping rules. A name in a nested context shadows a name of the same precedence in an outer context. Life on earth depends on basic science.

This bug has been latent because there are few root packages, even in Scala, which likes to sprinkle names like akka and doobie at top level. Most ecosystem names resolve via com, org, io.

When there is a problem, people do notice and report it, witness this ticket and the dotty ticket about its REPL.

It's not obvious to me that fixing the bug would incur a cost that makes it not worth it. io is the only (?) example I can think of where people intend _root_.io and not the scala.io or shadowed java.io. If Java had this bug, use of the io namespace would have noticed it long ago.

Also worth adding that when we fixed name binding previously, it was quickly obvious that people had latent naming bugs: witness also the scala-dev tickets asking whether scala imports and nested packages were a failed experiment and should be deprecated and removed like related modularity features such as package objects and implicits in package prefixes.

Also worth adding that this ticket puts a premium on not polluting root context namespaces, including scala.*.

@som-snytt
Copy link
Author

Oh, I see I already created a feature request for custom release profiles. Cool!

scala/scala3#17690

@SethTisue
Copy link
Member

The argument that this behavior is simply a bug is odersky's comment on the dotty ticket, which claims that the rooted name is at precedence 4.

It seems to me that in the _root_.io vs scala.io conflict, both options are level 4 (separated by "...as well as..."), so the spec doesn't settle the question of which should take precedence.

@som-snytt
Copy link
Author

som-snytt commented Nov 17, 2022

No, it's in the spec now that root contexts are ordered and scala comes after scala.Predef.

I was about to amend that formulation. It's ordered but it doesn't talk about what contributes root packages.

The old description used curly braces as a metaphor, where the notion of "root enclosing context" was implied, but we decided that metaphor is not accurate.

@SethTisue
Copy link
Member

@som-snytt where?

@som-snytt
Copy link
Author

som-snytt commented Nov 17, 2022

[previous comment edited]. I fixed the wording but did not overspecify the behavior under discussion. Probably the new wording even undermines my argument.

[edit] In fact, the new wording makes it more "urgent" to address the "empty package" model.

Working on imports in Scala 3, I noticed a comment somewhere which acknowledges that the "empty root package" model isn't quite right, but all the ecosystem tooling assumes it now, so they were reluctant to futz with it.

So, to undermine myself, that model says jdk is resolved by ordinary lookup and not by root context at all.

In other words, scala context does not "sit between" my local context and _root_.

[edit] In more other words, jdk is not made available as a magic binding from a root context but because it is a member of the (fictional) empty root package which all top packages belong to (but which is not the "empty package" from Java, which is visible only to members of the empty package).

Although it would be a crude distortion to view the root package as the empty package which is somehow not visible to subpackages, Scala 3 does have this situation:

val x = ???

package p:
  def f = x

Here, x in the empty package is visible to p.f in the same compilation unit (file). I think I got that right.

@som-snytt
Copy link
Author

As a footnote, "classpath" and its inherent ordering is an implementation or platform detail. From this perspective, it doesn't matter where root packages "come from". (I see from my comments last spring that the root context implementation supplies jdk as a definition during name lookup, which is why it isn't shadowed by scala.jdk. I was arguing from a different model from "empty root package", namely, that root packages are supplied by a root context ordered last of all. Although I don't like the inconvenience for import jdk.*, fixing it makes import io.* inconvenient, IIUC.) Closing on that basis.

@SethTisue SethTisue removed this from the Backlog milestone Nov 17, 2022
@som-snytt som-snytt closed this as not planned Won't fix, can't repro, duplicate, stale Jun 11, 2024
@som-snytt
Copy link
Author

To expand on my last comment, and to actually motivate the status quo, definitions on the class path are not different from definitions on the source path (or anything compiled together with the current compilation unit). If instead of a pre-compiled class path, our dependencies were compiled from source, the visibility of those definitions would be the same. Also, -Yno-imports does not mean "don't import from the class path". What makes class paths difficult to reason about is that they are on-demand, dynamically constructed, and not managed in source.

If we can't have release profiles (to control visibility of platform classes), then perhaps we can support modules.

Welcome to Scala 2.13.14 (OpenJDK 64-Bit Server VM, Java 21.0.2).
Type in expressions for evaluation. Or try :help.

scala> import sun.util.BuddhistCalendar
import sun.util.BuddhistCalendar

scala> new BuddhistCalendar
java.lang.IllegalAccessError: class $iw (in unnamed module @0x3a3b10f4) cannot access class sun.util.BuddhistCalendar (in module java.base) because module java.base does not export sun.util to unnamed module @0x3a3b10f4
  ... 30 elided

scala>
:quit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants