Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive ambiguity warning #12816

Closed
lrytz opened this issue Jun 30, 2023 · 3 comments · Fixed by scala/scala#10456
Closed

False positive ambiguity warning #12816

lrytz opened this issue Jun 30, 2023 · 3 comments · Fixed by scala/scala#10456
Assignees
Milestone

Comments

@lrytz
Copy link
Member

lrytz commented Jun 30, 2023

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
@lrytz lrytz self-assigned this Jun 30, 2023
@som-snytt
Copy link

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
Copy link
Member Author

lrytz commented Jul 3, 2023

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants