Name clash between wildcard import and class in same package regression 2.12 -> 2.13

[oxbowlakes]

Consider the following code in the source root src/main/scala/foo/X.scala:

package foo {
  class Properties

  import java.util._
  object X extends App {
    def bar(x: Properties): Unit = println(x.getClass.getName)
    bar(new Properties)
  }
}

It's my assumption that this code is equivalent to having the following

In src/main/scala/foo/Properties.scala

package foo
class Properties

In src/main/scala/foo/X.scala

package foo
import java.util._
object X extends App {
  def bar(x: Properties): Unit = println(x.getClass.getName)
  bar(new Properties)
}

In scala 2.12 the two seem to be equivalent; in both cases the program X prints foo.Properties. However in scala 2.13 this is no longer the case. The former still prints foo.Properties but the latter prints java.util.Properties

So I think that there are two separate issues here:

1. That in scala 2.13 the two separate ways of constructing the foo package are not equivalent
2. That there is a regression in the behaviour of the second (and probably more typical) construction. That a reference to Properties in the body of X would previously (in 2.12.x) have been resolved against foo.Properties but is now (in 2.13.x) resolved against java.util.Properties

[odersky]

In fact, this fixes a previous regression (I am not sure from when on). The Scala 2.13 behavior is according to spec. Here it is in Chapter 2:

Bindings of different kinds have a precedence defined on them:

• 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, have highest precedence.
• Explicit imports have next highest precedence.
• Wildcard imports have next highest precedence.
• Definitions made available by a package clause, but not also defined in the same compilation unit as the reference, have lowest precedence.

The reason for the spec wording is that one often has little insight or control over what is in a package. E.g. anything could be dumped in the empty package. So it is dangerous and unintuitive that this should shadow an import.

[dwijnand]

While acknowledging this is as specified, the thing I find troubling is the fact that the behaviour is different only depending on if it's in the same compilation unit or not. To me that feels like it's breaking some kind of "law" or invariant.

Are there other examples where such a difference is present?

[odersky]

sealed also depends on the compilation unit.

[dwijnand]

Sure, you must use the same compilation unit to have access to and benefit from sealed-ness.

But that's different, here the same definitions split into separate files changes the behaviour.

[dwijnand]

Companionship also depends on the compilation unit.

That's a good example. Companionship (e.g. with type aliases) can be a pitfall unless you know all the intricacies.

I don't understand your nested package example, but I'm fine with the argumentation being that import java.util._ should mean that java.util.Properties is printed. My argument is that it should be consistent, i.e. also print that in the single file example (example 1).

[oxbowlakes]

Apologies for not finding this (despite searching to see if it was an intended change)

[dwijnand]

I'm saying c.A sounds fine, whether a.A is defined in this file or any other, at least it's consistent behaviour.

I guess I'll take my argument to the contributors ML.

[oxbowlakes]

What is perhaps interesting is that this one change (fix) has caused us by far the most pain in project migration to 2.13 because the compiler errors look like "method foo does not exist on Bar" and IDEs have not necessarily been "made aware" of the change (i.e. the IDE thinks the code is fine, only scalac thinks it's broken)

What is more, the wildcard import makes it often extremely difficult to understand where the other class could be coming from!

Note also that the argument provided by @som-snytt is not very convincing:

package a //let's say that a.A exists
import foo._
new A //let's say that foo.A is later added. My code is now broken

[SethTisue]

PR was scala/scala#6589

[SethTisue]

the argument provided by @som-snytt is not very convincing

(I find it convincing, on the grounds that Som's example does not involve a wildcard import, but your example does.)

[dwijnand]

Here's a slightly modified version of @oxbowlakes's example where you take two files, concatenate them, and now compiling and running that program has different behaviour:

$ cat file1.scala
package foo { class Properties }
$ cat file2.scala
package foo { import java.util._; object X extends App { println((new Properties).getClass) } }
$ scalac file1.scala file2.scala && scala foo.X
class java.util.Properties
$ cat file1.scala file2.scala > file.scala
$ rm -r foo
$ scalac file.scala && scala foo.X
class foo.Properties

Does that seem right?

[SethTisue]

Does that seem right?

Isn't it inherent to the combination of 1) separate compilation and 2) packages being open?

[dwijnand]

Isn't it inherent to the combination of 1) separate compilation and 2) packages being open?

Why can't we take those factors and then make it have the same behaviour in both cases?

I don't care if you want to argue that "import java.util._ comes after so it makes sense it's java.util.Properties" or "you're in package foo, so despite the import it makes sense it's foo.Properties". I just care that it does the same thing no matter if I do the same thing in two files or the same thing in one file.

[dwijnand]

I accept a compilation unit is a thing. But I think it should operate at the "you can't extend this because it's sealed and in that other file" level, and not in such a subtle way. Companion objects are similarly problematic (and I don't have a solution there).

[SethTisue]

@dwijnand now that I take a closer look at your example, it does seem wrong to me that the shadowing behavior would be different in those two cases. I hadn't completely understood this before.

@som-snytt I do accept that compilation units are a thing, but I don't understand why being in the same compilation unit or not is, or should be, relevant in this case...? shouldn't the import always win?

[dwijnand]

(By the way, I'm concerned that I might be coming across as combative towards you - I know you helped align the implementation with the spec and I'm sorry I didn't make my arguments at that time. But I'm arguing against the specified behaviour, not against you or your efforts.)

[odersky]

I still think the spec and the current implementation is the best compromise. Examples:

// file1.scala
package p
class A

// file2.scala
package test
import p.A
object B extends A

//file3.scala
package test
class A

It would be very surprising if a random file in the same package (which might even be the empty package) overrides an explicitly given import. On the other hand:

// file4.scala
package test
import p._
class A
object B extends A

It should be blindingly obvious that we mean test's A here, not the import. The general scope rules support that idea since they say that, within one scope, a definition shadows an import. Two definitions
in the same compilation unit can be constructed as being in the same "scope", whereas it's stretch to call two definitions in different files as being in the same scope. Technically this is true, but conceptually it's counter-intuitive.

The intuitive meaning of "scope" rules is as a region of code. In that sense, the outermost "scope" of a compilation unit is everything following the package clause. Definitions in that scope shadow imports, as is the case everywhere else. That scope is in turn encloses by the package-level scopes, which follow package nesting structure. So, arguably, the current spec is not only the most intuitive but also the most regular. And yes, in that sense compilation units are very much a thing.

[Jasper-M]

The reason for the spec wording is that one often has little insight or control over what is in a package. E.g. anything could be dumped in the empty package. So it is dangerous and unintuitive that this should shadow an import.

I find this a pretty convincing argument.
But in the following code it would seem safer to me if the compiler would tell me that there's a name clash, which I can fix, instead of just picking one over the other.

import java.util.Properties
class Properties
new Properties

I'm not sure how I feel about the wildcard case. Perhaps give a error for the previous code and a warning for the following:

import java.util._
class Properties
new Properties

[dwijnand]

Thanks, @odersky, for responding.

It would be very surprising if a random file in the same package (which might even be the empty package) overrides an explicitly given import.

Seeing as this is compile-time, I think it's fine for the compiler to complain (either via a warning or an error) about such ambiguities. Also given the JPMS. I'm fine with doing something different about the empty package, as it's a weird one.

It should be blindingly obvious that we mean test's A here, not the import.

I disagree it's so obvious.

---

Continuing on my thought process that the compilation unit shouldn't matter, I explored below the various situations.

First given file1.scala:

package p { class A }

Here are the combinations, grouped up. The idea is:

• file2.scala == line 2
• file3.scala == line 3
• file4.scala == line 2 + line 3
• file2b.scala == line 2b
• file4b.scala == line 2b + line 3

named package

package test { import p.A; object B extends A } // line 2
package test { import p._; object B extends A } // line 2b (wildcard alternative)
package test { class A }                        // line 3

❌ error: ambiguous if p.A or test.A
🔧 fix by import renaming: import test.{ A => TestA } or import p.{ A => PA }

empty package

package test { import p.A; object B extends A } // line 2
package test { import p._; object B extends A } // line 2b (wildcard alternative)
class A                                         // line 3

✅ works, uses p.A
⚠️ a lint warns that <empty>.A is shadowed by p.A; resolved by... deleting/moving A? suppressing the warning at import p.A?

[odersky]

It should be blindingly obvious that we mean test's A here, not the import.

I disagree it's so obvious.

I disagree. It is obvious. It has been like this for all Scala versions and is completely in line with how Scala handles scopes. That part is non-negotiable.

[som-snytt]

We could add -Xlint:captain-obvious for the edge case that was corrected. I had a little trouble following Dale's example.

$ cat file1.scala file3.scala file4.scala 

package p { class A }

package test { class A { override def toString = "testy" } }

package test { import p._ ; class B extends A ; object Main extends App { println(new B()) } }
$ ~/scala-2.12.8/bin/scalac file1.scala file3.scala file4.scala && ~/scala-2.12.8/bin/scala test.Main
testy
$ scalac file1.scala file3.scala file4.scala && scala test.Main
test.B@4d1b0d2a
$ cat file1.scala file3.scala file4.scala > file5.scala
$ scalac file5.scala && scala test.Main
testy

It could be a somewhat expensive lint because it would inspect packages (and also package objects). It would warn for both cases: "Your import of test.A is shadowed by a definition in this compilation unit", and "The definition test.A in another compilation until is shadowed by your import."

[dwijnand]

I disagree. It is obvious. It has been like this for all Scala versions and is completely in line with how Scala handles scopes. That part is non-negotiable.

OK. Let me try again, holding this behaviour as non-negotiable:

package test { import p._; object B extends A }
package test { class A }

single file case: object B must be extending test.A (the import is unused)

package test { import p._; object B extends A }

package test { class A }

split file case: in order to not have different behaviours by splitting a file this must have the same behaviour: object B extends test.A (+ unused import)

Now, using named imports, not wildcards.

package test { import p.A; object B extends A }
package test { class A }

single file case: ambiguous reference error?

package test { import p.A; object B extends A }

package test { class A }

split file case: ambiguous reference error?

(Leaving the empty package case to one side for now.)

WDYT?

[som-snytt]

Today's Strands puzzle.