Skip to content

Import outside a package clause is not shadowed by an import inside the package clause #21405

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 Aug 20, 2024 · 4 comments · Fixed by #21539
Closed
Labels
area:import Issues tied to imports. itype:bug
Milestone

Comments

@som-snytt
Copy link
Contributor

som-snytt commented Aug 20, 2024

Compiler version

3.4.2

Minimized code

package p {
  class IO
}
import java.io.*
package q {
  import p.*
  class D extends IO
}

Output

-- [E049] Reference Error: masked.scala:8:18 ---------------------------------------------------------------------------
8 |  class D extends IO
  |                  ^^
  |                  Reference to IO is ambiguous.
  |                  It is both imported by import java.io._
  |                  and imported subsequently by import p._
  |
  | longer explanation available when compiling with `-explain`
1 error found

Expectation

p.IO shadows java.io.IO as in Scala 2.

This test requires JDK 23, but let me minimize properly, which also fails in the same way:

package r {
  class IO
}
package p {
  class IO
}
//import java.io.*
import r.*
package q {
  import p.*
  class D extends IO

  object Test extends App {
    println(new D().getClass.getSuperclass.getName)
  }
}

Note that -Yimports:java.io,p works correctly (without imports in source), as expected.

The issue is urgent where p is cats.effect.

@som-snytt som-snytt added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Aug 20, 2024
@Gedochao Gedochao added area:import Issues tied to imports. Spree Suitable for a future Spree and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Aug 21, 2024
@mbovel
Copy link
Member

mbovel commented Sep 2, 2024

This issue was picked for the Scala Issue Spree of tomorrow, September 3rd. @dwijnand, @hamzaremmal and @nmcb will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

@SethTisue
Copy link
Member

SethTisue commented Sep 4, 2024

(just a snapshot of our thinking/investigating so far:)

Dale notes that it works as expected with object but fails with package. Entering object creates a new Scope but entering package does not, which we think is why this code (in Typer.scala, added in 2014 in 3affebe):

      /** Would import of kind `prec` be not shadowed by a nested higher-precedence definition? */
      def isPossibleImport(prec: BindingPrec)(using Context) =
        !noImports &&
        (prevPrec.ordinal < prec.ordinal || prevPrec == prec && (prevCtx.scope eq ctx.scope))

gives a different answer in the two cases (because of the scope check), leading to the different overall answer.

Perhaps the logic in this method just needs updating? We're not sure yet.

@SethTisue
Copy link
Member

I wondered if it was crucial that we have an import that's totally top level (not inside any package ... { } or after any un-braced package ...), since then it's like where even are we, the empty package, the root package? but Dale says that the bug still happens even if you precede the whole business with package z, say.

@SethTisue
Copy link
Member

SethTisue commented Sep 4, 2024

some relevant spec links: https://scala-lang.org/files/archive/spec/3.4/09-top-level-definitions.html#compilation-units , https://scala-lang.org/files/archive/spec/3.4/09-top-level-definitions.html#packagings , https://scala-lang.org/files/archive/spec/3.4/04-basic-definitions.html#non-given-imports

the bug can't be reproduced with just package p syntax because those can only come first in a compilation unit. you need the package p { ... } syntax (a "packaging", in spec-ese). that syntax isn't used much, so it seems a bit niche

@mbovel mbovel removed the Spree Suitable for a future Spree label Sep 23, 2024
@WojciechMazur WojciechMazur added this to the 3.6.3 milestone Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:import Issues tied to imports. itype:bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants