Warn on bad extensions of aliases

[RoccoMathijn]

Fixes #22233

[RoccoMathijn]

Hi @som-snytt! Would you like to have a look at this since you opened the issue and the change is in code that you authored?

[KacperFKorban]

Looks great! Thanks for the contribution.

I added a small nitpick comment, but you can ignore it if you don't like it, since it's purely a personal preference/opinion.

[KacperFKorban]

Sorry, I got used to doing reviews for people that can merge by themselves, so the call for action is very unclear 😅 , so either:

1. push a change to the branch,
2. or answer my comment

[som-snytt]

I'm suspicious about aliases that are not effectively final. I'm taking a look now.

[som-snytt]

This is the use case I was thinking of, but it is covered by the previous change, not to warn on overrides.

It's already covered in tests/warn/i16743.scala although I've made it look slightly different.

extension (s: String) def length = 42 // warn

import compiletime.*

trait T:
  def t = "t"

trait Quote:
  type Tree
  given TreeMethods = deferred
  trait TreeMethods:
    extension (self: Tree) def t: String

class QuotesImpl extends Quote:
  type Tree = T
  override given TreeMethods:
    extension (self: Tree) def t: String = self.t

[som-snytt]

Otherwise LGTM!

[som-snytt]

probably a check file is not needed, as the text is vetted by the original test.

what I would do is move tests/pos/ext-override.scala to warns.

[RoccoMathijn]

I removed the check file 👍

Also I moved tests/pos/ext-override.scala to the warn but actually I don't understand the reasoning. The tests in warn all give a warning right? In this test case there are no warnings. Could you elaborate?

[som-snytt]

@RoccoMathijn they allow warn tests to have zero warnings, which is useful. I think (IMHO) tests about lack of warning should go in warn, while testing "does this compile" remain in pos. A warn test can still have -Werror to announce that no warns are expected (so you don't have to scan the test code).

[KacperFKorban]

@som-snytt I was thinking about a similar example:

//> using scala 3.nightly

class Animal {
  def show: String = "Animal"
  def show1: String = "Animal"
}
class Dog extends Animal

abstract class A:
  type X
  extension (a: X) def show: String

class B extends A:
  override type X = Animal
  extension (d: X) override def show: String = "Dog"

class C:
  type X = Animal
  extension (d: X) def show1: String = "Dog" // no warn on nightly, warn now

@main
def main =
  given B {}
  val dog = new Dog
  println(dog.show)
  given C {}
  println(dog.show1)

This change is closer to being correct IMO. Though, I'm not sure if we should also warn on B#show, since this one also will never be used. Though even if that's the case, I think that it would be an unrelated bug.

[som-snytt]

@KacperFKorban yes. dog.show is a good one, I'll use that for all future simple examples.

The justification for not warning at the override in B is "don't warn for something the user can do nothing about".

Some -Wunused are silenced by ???, so another choice would be to warn normally but not if the RHS is ???. Also the override extension could be marked @unused. I agree that would be a future enhancement, a Spree for another day.

Another example for overriding is that -Wunused:params avoids warning if the signature is "fixed" by a superclass, because maybe you can't change it; but IIRC I dropped that heuristic and instead just require @unused if that is intended. The same logic could apply to B here: warn by default but provide easy explicit escape hatches.

[som-snytt]

I forgot https://github.com/scala/scala3/pull/21191/files
and #21190

in case it is relevant.

[KacperFKorban]

Looks Good! Thanks!