Under -Xsource:3, include top-level symbols from same file in outer ambiguity warning

[lrytz]

Also reverts the spec change of c212347, which I believe was wrong: inherited and outer definitions still have the same precedence. What changed is shadowing a few paragraphs below. The new spec is as suggested by @odersky (scala/scala3#17033 (comment)).

[som-snytt]

I don't disagree with your result (remove the global exclusion, and forgive aliases).

But it seems to me that shadowing is defined in terms of precedence.

Inherited shadowed outer def because they were same precedence.

Now, inherited cannot shadow outer def because its precedence is too low.

However, as with imports, we identify aliases to reduce ambiguity. (That language got added to the end of the section to explain the implementation.)

Therefore, I think adding a rule is less elegant. However, it's difficult to care much, since there are few consumers of the spec, and the name binding section, in particular, doesn't really help any users. Maybe it helps a few engineers communicate? I would compare it to the lexical definition of placeholder syntax, which is precise but not explanatory for users.

Maybe after a good night's sleep, I would see which example is not explained as precedence. It's worth adding that the spec need not reflect the implementation (which may calculate precedence the same but add a special rule to disqualify certain shadowings).

[som-snytt]

I see it follows dotty.

Only needs a test tweak, which illustrates the new feature nicely. Nobody runs jvm tests locally!

[SethTisue]

@lrytz there's a partest failure; does it just need a checkfile update...?

[lrytz]

This seems good to go, the dotty PR was approved by Martin (though not merged yet).

[SethTisue]

@lrytz If it warns by default, should it error on -Xsource:3? In accordance with our usual way of ratcheting up the pressure, if the user seems to be asking for it?

I'm also curious if the less aggressive alternative of don't-warn-by-default, warn-on--Xsource:3 was considered.

[som-snytt]

@SethTisue it errors on -Xsource:3, e.g. skalac -Xsource:3 test/files/neg/t11921b.scala

I think the nature of the (buggy spec) behavior is so subtle and pernicious that warn by default is called for.

I am too lazy-busy at the moment to track down a discussion.

It's also annoying to fix, in the absence of -rewrite. Do we warn earlier for annoying fixes, or defer for the same reason?

Like with early defs, if you hit the warning, you're likely to hit it often because of a coding pattern or style.

[lrytz]

We could consider deferring the warning by default until 2.13.12 (hint smarter/dotty@378928b)

[SethTisue]

Lukas reminds me that the most common dubious positive is #10220 (comment)

I discussed this with Lukas just now. I think you can reasonably argue this either way, the let's-go-slow way or the let's-be-bold way.

I'm okay with the let's-go-slow way. Perhaps the pattern we warn is sometimes pernicious, but not always, and it's a longstanding behavior.

I merged the Scala 3 PR, and we can see how that goes once users have it. And if Lukas adjust this PR so it doesn't warn except under -Xsource:3, that will pilot the change with an early-adopter crowd. Then for 2.13.12 we could consider taking it up a notch.

So Lukas, unless you have second thoughts, please go ahead and adjust.

[SethTisue]

@lrytz spec change part adds “It is an error if...”, but it's only an error in Scala 3, isn't it?

[som-snytt]

"It is an error when ..."

Note that it remains an error, even if the compiler is unable or unwilling to report it.

Edit: I just got what I meant.

On the spec wording, I've previously noted (maybe for the spec change that was reverted) that we don't have versioning for the spec. Since the spec is not a learning guide and doesn't change often, I think stating the truth of the spec is more important than a transient implementation limitation (which is due to social rather than technical constraints).

Also on the spec change, I still don't understand why it's not a precedence change. The new error happens because the inherited can't shadow the definition, by normal rules. Also unfortunate, albeit less tragic, is restoring the undefined notion of "local" definition.

[lrytz]

Not entirely sure if the spec should mention the ambiguity. It will probably never become a hard error on Scala 2, but I agree that's due to practical concerns.

I still don't understand why it's not a precedence change

After looking at it again I think you're right. By making definitions higher precedence they are no longer shadowed by inherited members, which is the goal.

I was concerned it could lead to the opposite, that code would compile that was previously ambiguous. That a definition is now picked for a reference that used to be ambiguous. But this isn't the case, as definitions are always in an inner scope compared to inherited, so they were preferred befroe.

[som-snytt]

I vaguely recall an old party game where you suffix every assertion with, "...in bed." This is the same game, except it's, "...under -Xsource:3."

I don't know why my previous edit lowered the precedence of packaged symbols. (Was I testing something a long time ago with different code?)

I verified, if it needed verifying, that def f(): p.q.C and it's ambiguous (as always) in separate files. I'm not necessarily convinced that it shouldn't be ambiguous as shown, i.e., I think it should be ambiguous because it's directly analogous to the ambiguity due to inheritance, but as they say, "Nobody codes like that." (Same result as if Test were in q, but I pushed it down a scope just to be doubly sure; it shouldn't matter whether "by a package clause" means the current package or an enclosing one.)

package p {
  package q {
    class C
  }
}

package p {
  class C
  package q {
    package r {
      class Test {
        def f() = new C()  // C is both defined in p and visible subsequently in p.q?
      }
    }
  }
}

I see that my view here is governed by my understanding of precedence rather than the "special rule" formulation which singles out inheritance as the problematic form of visibility.

[som-snytt]

Updating now so that I can experience the post-modern outer ambiguity.