scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
230 stars 21 forks source link

False positive ambiguity warning #12816

Closed lrytz closed 12 months ago

lrytz commented 1 year ago
trait U { def m: Int = 0 }
package object p extends U

package p {
  trait T { def m = 1 }
  class C extends T { def t = m }
}

With 2.13.11 and -Xsource:3, this emits a false ambiguity warning:

➜ sandbox sc A.scala -Xsource:3 -deprecation
A.scala:4: warning: package object inheritance is deprecated (https://github.com/scala/scala-dev/issues/441);
drop the `extends` clause or use a regular object instead
package object p extends U
                 ^
A.scala:8: warning: reference to m is ambiguous;
it is both defined in the enclosing trait U and inherited in the enclosing class C as method m (defined in trait T)
In Scala 2, symbols inherited from a superclass shadow symbols defined in an outer scope.
Such references are ambiguous in Scala 3. To continue using the inherited symbol, write `this.m`.
Or use `-Wconf:msg=legacy-binding:s` to silence this warning.
  class C extends T { def t = m }
                              ^
2 warnings
som-snytt commented 1 year ago

I'm not sure why it's a false positive. But I've forgotten how the spec of the new ambiguity was worded.

I think the intuition is that if x is available at the point of reference, but adding an inherited member x to a superclass changes the referent, then it's a true positive.

The definition is available by virtue of packaging (albeit in the same unit).

By my formulation, that inherited and packaged have the same precedence, then it's a false positive. But I would be inclined to tweak that, to say inherited is lower precedence than all defs local to the unit. (Inherited members can come from anywhere, all the more reason to distrust them.)

In different units, it should not warn, so that's clearly a false positive.

lrytz commented 12 months ago

As you say, the reported example warns when trait U and package object p are in a different file, which is certainly not intended. That's how I found it.

not sure why it's a false positive

My intuition to warn was not "adding an inherited x changes the referent", but instead "there is a non-inherited definition x in an outer scope in the same file, but the reference x doesn't resolve to that".

So this does not warn, even though extends U changes the reference T:

trait P { trait T }
trait U { trait T }
object O extends P {
  trait C extends U {
    def t: T // no warn (good)
  }
}

This next one doesn't warn, maybe that's a bug? Anyway this is a corner case, package objects are typically defined in separate files, which should not warn.

package object p { def m = 0 }
package p {
  trait T { def m = 1 }
  class C extends T { def t = m } // no warn (unclear?)
}

About the spec, we have something in comments: https://github.com/scala/scala/blob/v2.13.11/spec/02-identifiers-names-and-scopes.md?plain=1#L20-L27


Below both T are inherited, so they have the same precedence. The one in the inner scope shadows the outer one.

trait P { trait T }
trait U { trait T }
object O extends P {
  trait C extends U {
    def t: T // no warn (good)
  }
}

Here is again the original example of this report.

I'm not sure if p.m is made available by packaging or by inheritance (it's sort of both). In the first case it would have higher precedence than T.m and cause an ambiguity. Again, it's a corner case, as package objects are typically in separate files.

trait U { def m: Int = 0 }
package object p extends U

package p {
  trait T { def m = 1 }
  class C extends T { def t = m }
}

Finally, I guess here O.T is "local", so it has higher precedence than the inherited T, so it's ambiguous.

trait U { trait T }
object O {
  trait T
  trait C extends U {
    def t: T
  }
}

Confusing to me is that "local" in the spec for precedence (does it have a definition?) is not the same as "local" in section 4 (https://github.com/scala/scala/blob/v2.13.11/spec/04-basic-declarations-and-definitions.md?plain=1#L26-L30) 🤷‍♂️

som-snytt commented 12 months ago

I remember one of my edits removed the "local" before it was restored.

My question, which you answered nicely, is perhaps about what "local" means. The other part of the intuition is that you can see it. What you can see is more intentional than what is concealed (by virtue of a separate file).

I bet ten or 15 years ago, you did not expect this would be the conversation in 2023.