Under -Xsource:3, warn that inherited members no longer take precedence over outer definitions in Scala 3

[lrytz]

Continuation of @som-snytt's work at #10177.

Backport from Scala 3: an inherited name no longer takes precedence over a definition in an outer scope. The following reference m is ambiguous:

class C { def m = ??? }
def f(m: Int) = new C {
  def g = m   // use this.m
}

It's an error under -Xsource:3, a warning otherwise.

Fixes scala/bug#11921.

Don't issue an ambiguity error if

• a symbol is a type alias and the two types are equivalent (=:=)
• both symbols are stable and their singleton types are equivalent (=:=)

Example:

class C
class A { val c: B.c.type = B.c }
object B {
  val c = new C
  val a = new A {
    def m = c    // not ambiguous, `this.c` vs `B.c`
  }
}

Type example:

class C[T] { type TT = T }
object O {
  type TT = String
  class D extends C[TT] {
    def n(x: TT) = x  // not ambiguous, `this.TT` vs `O.TT`
  }
}

[lrytz]

Scala 3 variant: scala/scala3#16401

[som-snytt]

There must be a word for when you are able to contribute versions of a fix on both 2 and 3, coincidentally using the prefix "ambi-".

I propose "ambiversal".

[lrytz]

The most common pattern leading to spurious errors is this:

class C(val name: String)
object Test {
  def m(name: String) = new C(name) {
    println(name)
  }
}

I spent some time coming up with a heuristic to see through the most common such pattern [EDIT: but the heuristic was entirely discarded ultimately, so the example above warns].

The difficulty is, new C(name) { ... } is pulled apart by the type checker into new $anon { def <init>() = { super.<init>(name); ... } ... }, we don't get a typed AST that represents new C(name). But at least the Ident(name) constructor argument that we see in the template.parents has a symbol assigned (I assume by chance, that's a side-effect of typedIdent).

I pushed this for now to give it a try. If we keep it, we should try to do something similar for Scala 3.

The heuristic currently breaks here:

abstract class A(val name: String)
class C(name: String) extends A(name)
object Test {
  def m(name: String) = new C(name) {
    println(name)
  }
}

[lrytz]

I think the last experiment is probably not worth it. The implementation is rather ad-hoc, and it would have to be replicated to more compilers (Scala 3, IntelliJ).

Instead I changed it to be a warning (WIP) and made it even more verbose.

[som-snytt]

Very nice!

[som-snytt]

This was on my to-do list yesterday, right after "override message" and "dentist" (for child). There was a cavity. 😢

The code inherited from the other PR (foundCompetingSymbol) is not as clear as one would like.

The amendment to nextDefinition is very nice and doesn't introduce extra tests until required.

Triggering the warning in typer is especially clever yet simple. Just warning is probably sufficient, since -Werror is well advertised.

[lrytz]

Thanks, I should also learn writing supportive review messages.

[som-snytt]

For fun, I verified the warning for the linked PR.

[warn] .../scala/src/library/scala/collection/immutable/BitSet.scala:251:19: reference to empty is ambiguous;
[warn] it is both defined in the enclosing object BitSet and available in the enclosing class BitSetN as method empty (defined in class BitSet)
[warn] Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope.
[warn] To continue using the symbol from the superclass, write `this.empty`.
[warn] Or use `-Wconf:msg=legacy-binding:s` to silence this warning.
[warn]                   empty
[warn]                   ^
[warn] .../scala/src/library/scala/collection/immutable/BitSet.scala:330:15: reference to empty is ambiguous;
[warn] it is both defined in the enclosing object BitSet and available in the enclosing class BitSetN as method empty (defined in class BitSet)
[warn] Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope.
[warn] To continue using the symbol from the superclass, write `this.empty`.
[warn] Or use `-Wconf:msg=legacy-binding:s` to silence this warning.
[warn]               empty
[warn]               ^

The build has other warnings under JDK 19, so if you're inclined to ignore warnings, you might miss it. I also wondered if "Since 3" reads normally. "Since 3 P.M."

I thought I'd contributed something to mitigate the JDK19 warnings.

[lrytz]

Since 3 is definitely a typo, will fix it.

[SethTisue]

@lrytz this caused https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/4184/artifact/logs/kafka-build.log — the source file is https://github.com/ennru/kafka/blob/eda4a2904a863b04f6ea511a4cf1e2bd4806ad35/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala — is it a regression, or does it seem expected to you?

[som-snytt]

Seems unexpected. The replicaManager is a member (ctor arg) of the enclosing class and the instance is getting assigned to the conflicting definition. That is only a bit weird.

[som-snytt]

Simple repro (or actually it looks slightly different) (probably won't be able to pursue it at the moment)

trait T
abstract class C(val r: T)

class D {
  val r = new C(null) {
    def f = r
  }
}

compiles previously but

pr10220.scala:6: error: recursive method f needs result type
  val r = new C(null) {
          ^
pr10220.scala:7: warning: reference to r is ambiguous;
it is both defined in the enclosing class D and available in the enclosing anonymous class as value r (defined in class C)
Since 3, symbols inherited from a superclass no longer shadow symbols defined in an outer scope.
To continue using the symbol from the superclass, write `this.r`.
Or use `-Wconf:msg=legacy-binding:s` to silence this warning.
    def f = r
            ^
1 warning
1 error

Maybe Lukas knows how to do the alias probe less forcefully. Almost said alien probe.

[lrytz]

Thank you for the minimization @som-snytt!

Maybe Lukas knows how to do the alias probe less forcefully

Hmm, didn't find a way so far... But it's probably a regression we can live with?

This fails to compile before my change (recursive method f needs result type):

trait T
abstract class C(val r: T)
class D {
  val r = new C(null) {
    def f = D.this.r
  }
}

[som-snytt]

Regressions are pretty easy to live with. They don't snore and always clean the sink when they are done in the morning.

I argued for erroring by default, so I would welcome more regressions. [Autocomplete suggests "terrorizing".]

If there is a place for -Ylegacy-whatever, it would be just to exclude this corner case. Possibly if a codebase contains this idiom, possibly only in tests, there could be a bunch of them. But extra options incur as much user wrath as regressions, perhaps more because of decision fatigue.

[lrytz]

@som-snytt I assume top-level is excluded here because that's what's implemented in Scala 3 (scala/scala3#8622). In Scala 3, the following compiles and new X resolves to C.this.X

package p {
  trait T { class X }
  class C extends T {
    def test = new X
  }
  class X
}

Changing package p to object p makes new X ambiguous. Do you see a good reason for that? cc @odersky

[som-snytt]

If T has a member def name: String = "John Doe", then just where it can be inherited, introducing a different definition of name might be in conflict.

Packagings are not classes and don't suffer from extension, so I should be able to write top-level def name = "Gloria Mundi" without concern about confusing anyone.

But this is ambiguous:

    trait T {
      class X
      def name: String = "John Doe"
    }
    class C extends T {
      def test = new X
      def result = name
    }
    class X
    //def name = "Gloria Mundi"
    val name: T = new T {
      def result = name
    }

I expected it to prefer the inherited name and not the enclosing definition which is a top-level def.

15 |      def result = name
   |                   ^^^^
   |                   Reference to name is ambiguous,
   |                   it is both defined in package object t10220$package
   |                   and inherited subsequently in anonymous class Object with p.T {...}

I don't think they actually got rid of package objects...

[lrytz]

So you expect an inherited symbol takes precedence over an outer definition in the same source file, if that outer definition is package-owned? Can you say why? I don't yet see what difference it makes if the outer symbol is top-level or not.

I should be able to write top-level def name = "Gloria Mundi" without concern about confusing anyone

What is the difference in that regard between package p { def name = "..." } and object p { def name = "..."}?

[lrytz]

For the record, I'm on this topic because I'm testing #10297, and I see warnings that didn't show up in 2.13. These new warnings seem helpful to me.

[som-snytt]

I guess the proof is in the pudding, or in golf it's the putting.

I have only a rationalization: in package p, I know nothing is inherited in this scope (because we do not package object p extends T). Once someone defines a class, the folks working in that mine must manage that namespace, where stuff is inherited.

I agree there is regularity in seeing object p as just a way to "associate" some definitions, and I think they speak of "package membership", but if the regularity were borne out, then we would love package object p extends T. You would be asking, Why is that different from object p extends T?

But all that is just a rationalization.

What are some examples of pernicious shadowing of top-level defs? I can't wait to see.

[lrytz]

An example was:

package p

class Code extends Parent {
  def impl = s"The $CommonName"
}

case class CommonName(some: Thing)

A second file:

package p
trait Parent {
  val CommonName = "something"
}

It's maybe clear here that we're not intending to refer to the synthetic case class companion, but this could still be a head scratcher for someone looking at the first file. One could for example do a search in the editor for CommonName and land on the case class.

In this case I moved CommonName to a separate file instead of changing the reference.

[som-snytt]

Aha, that does seem lintable, at least with CommonName in the same file. Conversely, it loses precedence when moved to a different file, but the reference in Parent does not.

I just realized this feature is not released until 2.13.11. Let me take a moment to clear my fever-addled brain.

I notice toString is not exempt.

Also I was about to wonder if the use case is really anon classes, esp SAM types. The user expectation is more obviously, "I am focused on implementing that one method, and probably unaware of inheriting anything."

[lrytz]

CommonName in the same file

Yes, I mean to warn only if the outer definition is in the same compilation unit.

I notice toString is not exempt.
if the use case is really anon classes

Those are very good points. I certainly saw it pop up a lot with anonymous classes, and one would expect toString to resolve in the current object. Hmm. Since this is a spec change and not an impl detail (a lint rule), we need to have general and hopefully simple rules. An exception for toString wouldn't work...

[lrytz]

I think we can wait to see if toString really is problematic. So far (Scala 3, my tests of the new Scala 2 warning) it didn't seem so.

[SethTisue]

the "under -Xsource:3" part happened in the followup PR, #10339

[SethTisue]

@lrytz actually maybe I'm confused and maybe the edit to the issue title I just did is wrong? if so, can you change it back?

and what is the appropriate text to include in the 2.13.11 release notes to cover both this PR and #10339? can you suggest an appropriate edit over at scala/scala-dev#843 ?

[som-snytt]

@SethTisue I took a shot and trivially proposed using the title, which covers both PRs.