[SUPERSEDED] Inherited members can induce ambiguity

[som-snytt]

The language specification is clarified so that an inherited class member cannot shadow a local definition.

The following reference m is ambiguous:

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

The previous behavior is available under -Ylegacy-binding.

Fixes scala/bug#11921

[som-snytt]

I forgot whether ci: requires a space or no space.

[som-snytt]

We don't have a convention for the spec change, which applies to Scala 3 source.

It would still be nice to have a page listing what behaviors are controlled by -Xsource (or the implicit search flag); most are restrictions or minor syntax? and therefore not spec-worthy.

[som-snytt]

This is an edge case where -Xsource:3 is actually useful.

[lrytz]

LGTM, thank you! Just the spec change should be revised.

[som-snytt]

I pushed -Ylegacy-binding, so that users get the improved behavior for free, but can opt-out if their legacy code base is littered with problematic inheritance.

[som-snytt]

I was wondering if it would pass bootstrap on the first try. I didn't expect more sbt fragility:

/tmp/sbt_3faddf00/xsbt/GlobalHelpers.scala:19: error: reference to global is ambiguous;
it is both defined in trait GlobalHelpers and available as value global in class Compat
  import global._
         ^

This is no doubt because the sbt code is never seen, but slinks by in a dark alley, brokering its shady deals.

Actually, this is just an ordinary member, but it looks like it is fooled by the self type! I'll add a test.

[som-snytt]

The update approximates WWDD at scala/scala3#8622

Follow-up for bootstrapping to follow.

Some edits are not entirely boring:

[error] .../scala/src/compiler/scala/reflect/quasiquotes/Parsers.scala:95:31: reference to global is ambiguous;
[error] it is both defined in class Parser and available as value global in class ParserTreeBuilder
[error]           case other       => global.abort("unreachable")
[error]                               ^

where the local code odor is

import treeBuilder.{global => _, unit => _}

Another good one, which tree is intended? There is no guarantee that the tree in the completer is the tree we passed in. It would be nice if there were syntax for that, new C(tree' = tree).

    protected def finishSymbolMaybeExpandee(tree: Tree, annZippers: List[treeInfo.AnnotationZipper]): Unit = {
      val sym = tree.symbol
      unmarkWeak(sym)
      markMaybeExpandee(sym)
      sym.setInfo(new MaybeExpandeeCompleter(tree) {
        override def kind = s"maybeExpandeeCompleter for ${sym.accurateKindString} ${sym.rawname}#${sym.id}"
        override def maybeExpand(): Unit = {
          val companion = if (tree.isInstanceOf[ClassDef]) patchedCompanionSymbolOf(sym, context) else NoSymbol

An actual bug in a test:

  class OnceOnly extends IterableOnce[Int] {
    override def knownSize: Int = 1

    var iterated:Boolean = false

    override def iterator: Iterator[Int] = {
      new Iterator[Int] {
        assertFalse("Attempt to re-iterate!", iterated)
        iterated = true
        private var v = Option(42)
        override def hasNext: Boolean = v.nonEmpty && knownSize > 0
        override def next(): Int = {
          val res = v.get
          v = None
          res
        }
      }
    }
  }

  @Test
  def addAllTest2(): Unit = {
    val hs = HashSet.empty[Int]
    hs.addAll(new OnceOnly)
  }

That's my fault for not reviewing the contribution (which I suggested). It intends to enforce that the collection is not iterated twice. It's not even iterated once, because knownSize is the inherited -1, not the positive value in scope. The test should check that something was added.

[som-snytt]

You don't often get to edit pos/t0165.scala. Probably any edit nullifies the early test of a crasher. The charming package name is test3.

    object result extends LinkedHashMap[String,String] with ArrayResult {
      def current = result
    }

[som-snytt]

@lrytz This is why I was looking at this code recently. Could you comment on the idea that this was a spec bug and that fixing it finds a few code bugs without breaking legitimate code? Probably some Community Build experience will say more.

-Ylegacy-binding, while technically a forking option, follows in the path forged by other transition options such as -Yeta-expand-keeps-star and -Yinfer-by-name.

[lrytz]

This is not going to be a trivial change to push through a minor release, but because it's a clear improvement and it is likely to flag actual bugs, I think we should give it a try.

People will get compilation errors when upgrading Scala on code that previously compiled, so let's add a very specific message to the error. Something like this:

Since 2.13.11, symbols inherited by a superclass no longer take precedence over symbols defined in an outer scope.
Typically this is fixed by writing `this.coll`. Use `-Ylegacy-binding` to revert to the previous behavior.

[som-snytt]

That's a good idea, to mitigate the migration effort with an explanatory message.

I also would not be opposed to releasing 2.14 next 14.2 with default -Xsource:3 (!).

[lrytz]

Testing this change on a codebase that might or might not have similarities to https://github.com/morganstanley/optimus-cirrus. It would really help if the error could be skipped in case the compiler knows the two symbols are aliases.

Type example:

class C[T] { type TT = T }
object O { type TT = String
  class D extends C[TT] {
    def n(x: TT) = x  // now ambiguous
    def m(x: O.TT): this.TT = x  // but they are aliases
  }
}

Term example:

trait Context
class A[C <: Context](val c: C)
class B(val c: Context) { b =>
  val a = new A[c.type](c) {
    def m = c  // now ambiguous
    def t: this.c.type = b.c // but they are aliases
  }
}

What do you think?

[som-snytt]

That is a design goal, which I assume Dotty achieves.

[lrytz]

Scala 3.2.1 issues an ambiguity error for both of these examples. But if it's actually correct it would be helpful, and could be forward ported. The disambiguation could be extended to other cases, not only the new one added here.

[som-snytt]

Maybe take the last thing I said and assume the opposite. I remember seeing the dealiasing in dotty and made a mental note but did not follow up. I'll give it more thought or ponderance (made-up word).

[som-snytt]

Oh actually, just back from cooking lunch, I did test this or similar case, and was relieved that dotty didn't require me to notice the aliasing.

Edit: or I'm thinking of pos/t11921c.scala where package-level is an escape hatch.

[som-snytt]

"local"!

[som-snytt]

Tweaked the code comment and rebased.

@lrytz merge for parity with dotty? Then your de-aliasing improvement with de-this follows hard upon.

[lrytz]

@lrytz merge for parity with dotty? Then your de-aliasing improvement with de-this follows hard upon.

Let's wait and see, instead of merging a new error and later narrowing it down. I'm also conidering whether it should maybe be a warning (on by default), an error only under -Xsource:3.

[som-snytt]

subsumed and superseded by #10220