A foreign definition induces ambiguity

[som-snytt]

A foreign definition can't shadow a definition
in the current unit. Also, an import that
could shadow the foreign definition can't
shadow the definition in the current unit.

Fixes scala/bug#10662

[som-snytt]

ABORTED. Took 934 min. Say /rebuild on PR... Was that a record? Was I supposed to receive a special t-shirt signed by Jenkins?

I imagine he would scrawl with a sharpie: "LGTM! -- J"

[SethTisue]

PR title by Edward Gorey

[som-snytt]

I see @hrhino did a similar refactor, for a different purpose, so it's not an easy rebase.

[som-snytt]

@hrhino I had wanted encapsulation for that save/restore state, so I had a DefinitionCursor analogous to the one for imports; but I didn't apply that change to avoid any subtle performance difference.

The gist here is that just as it has to go looking for an ambiguating import, it has to go looking for an ambiguating definition if the definition we have is "defined in a different compilation unit", precedence level 4.

[hrhino]

It looks right to me, but who can say? I may have changed this code, but I never claimed to understand it.

Do other people really write packagings this way? I'm glad this wound up being an error, because if you showed me px_2.scala and asked me what happens, I'd have to admit my insufficient Scala understanding. Does the puzzlers guy accept temporary puzzlers?

[adriaanm]

<font face="sharpie sans"> LGTM --a </font> <!-- CSS who? -->

[retronym]

Does this need a forward port to dotty?

[som-snytt]

I created a ticket for Dotty, and discovered that it picks p.X in both cases; Scala 2.13.0 picked the foreign p.q.X in one case, as I remembered, but p.r.X, the import of a locally defined symbol, in the other, which I'd forgotten. It's possible, of course, that p.X is just an opinionated choice.

[retronym]

I've noticed this bug fix comes up quite frequently when migrating 2.12 code bases to 2.13.

For the record here's a canonical example:

// /tmp/test.scala
package p1 {
  class C {
    def c_p1 = ""
  }
}

package p2 {
  class C {
    def c_p2 = ""
  }
}

package p2 {
  import p1._

  class test {
    new C().c_p2
  }
}

$ (export V=2.12.15 && (rm -rf /tmp/out/*; scalac --scala-version $V -d /tmp /tmp/test.scala && scalac --scala-version $V -cp /tmp -d /tmp  /tmp/client.scala))

$ (export V=2.13.8 && (rm -rf /tmp/out/*; scalac --scala-version $V -d /tmp /tmp/test.scala && scalac --scala-version $V -cp /tmp -d /tmp  /tmp/client.scala))

/tmp/client.scala:5: error: value c_p2 is not a member of p1.C
did you mean c_p1?
    new C().c_p2
            ^
1 error

A cross compatible version of client.scala is:

package p2 {
  import p1.{C => _, _}

  class test {
    new C().c_p2
  }
}

IntelliJ's presentation compiler works implements both the old and new behaviour, choosing which one based on the current Scala version: JetBrains/intellij-scala@a427c20

[som-snytt]

The Dotty ticket is scala/scala3#8059

3.x is same as 2.13 for the canonical example.

I commented on the Dotty ticket that for these tests, Dotty picks p.X for the first and p.r.X for the second, which is a change in behavior since the ticket was lodged.