Skip to content
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

REPL: scala.* namespaces shadow other classes #15904

Closed
mpollmeier opened this issue Aug 24, 2022 · 14 comments · Fixed by #16389
Closed

REPL: scala.* namespaces shadow other classes #15904

mpollmeier opened this issue Aug 24, 2022 · 14 comments · Fixed by #16389

Comments

@mpollmeier
Copy link
Contributor

Compiler version

Scala 3.1.3
Scala 3.2.0-RC4

Minimized code

// Bar.scala
package io.foo

object Bar {
  def baz: Int = 42
}
scalac Bar.scala
zip -r foo.jar io
scala -classpath foo.jar
scala> _root_.io.foo.Bar.baz
val res0: Int = 42

scala> io.foo.Bar.baz
-- [E008] Not Found Error: -------
1 |io.foo.Bar.baz
  |^^^^^^
  |value foo is not a member of io
1 error found

Expectation

The repl should resolve io.foo without the _root_ prefix.

Initial analysis

scala.io shadows io.foo. Same goes for all scala.* packages.
The above works fine in ammonite's scala3 repl as well as the default scala2 repl.

@mpollmeier mpollmeier added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Aug 24, 2022
@som-snytt
Copy link
Contributor

This duplicates #14781 where

import jdk.CollectionConverters.*

succeeds in REPL but fails in scalac because recent JDK has root package jdk.

As shown in the Scala 2 ticket (linked from the dotty ticket), Scala 2 REPL also fails (i.e., disagrees with Scala 3 REPL).

The "root import" that makes scala.io and scala.jdk available should have binding precedence 4.

What about top-level packages io (from the user class path) or jdk (from the jdk)? I suggest that they are also precedence 4, because they are "made available by packaging", where the shared prefix of the fully qualified name is the empty prefix.

The current implementation in Scala 2 and 3 really does model a "root package" with an "empty name". However, the lookup of jdk as _root_.jdk assigns it precedence 1 as a "definition in the current compilation unit". I think that is wrong. It should be level 4.

I think the root import import scala.* should shadow the top-level symbol from the class path.

For example, a usage io.Source should still compile if I add the user io package to the class path. (It currently fails to compile.)

A file could import scala.{io as _, given, *} in that case to disable the root import and make the root io visible in that file.

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

@mpollmeier
Copy link
Contributor Author

mpollmeier commented Aug 25, 2022

Thanks, yes that's some valid context, and precedence is obviously a very important issue. Note however that my example above isn't really affected by precedence. It defines an io.foo namespace, which doesn't even exist under the scala root package.

Hence, if precedence requires that we first lookup scala.io.foo and we don't find it, that's fine. Because then the repl should really not give up there, and look at _root_.io.foo next. That's what scalac does.

The error message suggests that it actually did look at io.foo rather than only scala.io.foo, which isn't really helpful unfortunately. That would be ok though, as long as we actually do look at _root_.io.foo.

@som-snytt
Copy link
Contributor

The actual mechanism is that it looks up io first, and once that is bound it doesn't back out.

I had not considered continuing the search for the maximal path. I don't know if that has been discussed in the past and deemed too expensive or confusing.

One consequence is that io could bind differently in io.foo and io.bar. If I ask for completions, it would also have to offer both. Maybe it ought to do that already.

It would solve the annoying problem of common prefixes such as util, where changing packagings to nested style introduce other names that shadow.

I don't understand your comment about the error message, but I see that it would be more helpful if package references were fully qualified. The compiler works hard to disambiguate other symbols with superscripts, etc.

scala> new util.Foo
                ^
       error: type Foo is not a member of package util

scala> new scala.util.Foo
                      ^
       error: type Foo is not a member of package util

@Kordyjan Kordyjan added area:repl area:classpath and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Aug 25, 2022
@odersky
Copy link
Contributor

odersky commented Aug 26, 2022

The current implementation in Scala 2 and 3 really does model a "root package" with an "empty name". However, the lookup of jdk as root.jdk assigns it precedence 1 as a "definition in the current compilation unit". I think that is wrong. It should be level 4.

I think in Scala 3 it is level 4. That's where the two versions differ.

@mpollmeier
Copy link
Contributor Author

I don't understand your comment about the error message

scala> io.foo.Bar.baz
-- [E008] Not Found Error: -------
1 |io.foo.Bar.baz
  |^^^^^^
  |value foo is not a member of io

The user may or may not know that io actually refers to scala.io, so IMO the error message would be more helpful if it used the full package name(s) that the compiler looked at. In this case it could say value foo is not a member of scala.io.

Anyway, that's more of a side note - the main issue remains that the compiler finds that io.foo from the root package. I'm certainly not the right person to say if it should work via continued search or different precedence, but I do think it should be consistent between scalac and repl.

@SethTisue
Copy link
Member

SethTisue commented Nov 17, 2022

I had not considered continuing the search for the maximal path. I don't know if that has been discussed in the past and deemed too expensive or confusing.

I think we should not even consider that approach. That just isn't how scoping in Scala works. If I write foo.bar, the foo that is used should always be the same foo I get if I just write foo. Hypothetically changing that would be a much deeper change than just fiddling with priorities.

@som-snytt
Copy link
Contributor

I'll nag a bit by posting my test, as my previous comment was unclear (to me; quick let me write it down while I remember it):

The behavior is different in Scala 3 REPL. import jdk. autocompletes scala.jdk in Scala 3 REPL but _root_.jdk in Scala 2 REPL.

The compiler takes rooted jdk in both versions, which I see as a bug (in the linked ticket which is closed).

The code sample shows that you must use scala.jdk:

//import scala.jdk.CollectionConverters._
// not in Scala 2/3
import jdk.CollectionConverters._
import sun._
// in Scala 2
//scalac -Yimports:java,scala,scala.Predef,scala.jdk hh.scala
//import CollectionConverters._

object HelloWorld {
  def main(args: Array[String]) = {
    val ss = java.util.List.of("Hello", who)
    println(ss.asScala.mkString("", ", ", "!"))
  }
}

object sun {
  val who = "world"
}

This ticket shows that in Scala 3 REPL, you must use _root_.jdk for the rooted package (which no one intends ever).

Both jdk names ought to be lowest precedence, and scala root context ought to shadow the empty root package root context. Seth asks on the Scala 2 ticket, "On what basis are root names bound?" and I think I don't know. My assumption is that they are package names made available by virtue of the fiction of a shared "empty prefix", like any other nested package. However, I think it would be cleaner just to specify root packages from class path as something magic from the root context, especially because (obviously) order matters for shadowing; but possibly this is just implementation detail.

@som-snytt
Copy link
Contributor

I think we should not even consider that approach.

However, autocompleting util by cycling through all known util packages sounds like a great feature. (It would need to show the disambiguating prefix, and insert it as necessary.) (I don't know if IDEs already do that.)

@SethTisue
Copy link
Member

scala root context ought to shadow the empty root package root context

Well, that's the design question here, and I'm not sure I agree.

I think the behavior of the Scala 2 batch compiler for the last decade-plus is a really strong precedent here and if we wanted to change it — in either Scala 2 or Scala 3 — we need a realllllllly strong argument and we need to be really sure.

@som-snytt
Copy link
Contributor

Edit: sorry, I didn't mean to pollute the issue here (which is that REPL's behavior is different from batch).

Also, I see I replicated my comment from summertime, because I forgot the details again.

Also not relevant here, but worth pointing out, is that taking util.chaining quasi-opaquely in the face of a competing util package doesn't seem to me to violate scoping. One may say that the _root_ prefix is inferred by the compiler, which tries to be maximally helpful. The least it might do is ask, "Did you mean...?" on error.

@SethTisue
Copy link
Member

SethTisue commented Nov 17, 2022

So I suggest that at present, we should simply fix the Scala 3 REPL to match the behavior of the Scala 3 batch compiler.

#14021 has some good relevant history/code/tests.

@SethTisue
Copy link
Member

IMO the error message would be more helpful if it used the full package name(s) that the compiler looked at. In this case it could say value foo is not a member of scala.io.

Agree.

@mpollmeier
Copy link
Contributor Author

I think we're all on the same boat when it comes to 'where we want to sail', see #15904 (comment)

That's a great first step I guess...

@dwijnand dwijnand linked a pull request Nov 21, 2022 that will close this issue
@dwijnand dwijnand self-assigned this Nov 21, 2022
@mpollmeier
Copy link
Contributor Author

🥳 🍰 thanks everyone!

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

Successfully merging a pull request may close this issue.

6 participants