Warn if extension receiver already has member

[som-snytt]

Best effort to warn if defining extension method for a type which has a non-private member of the same name and a subsuming signature.

Excludes opaque types (for defining forwarders to a public API) and other alias types (where the type member may override an abstract type member).

Fixes #16743

[sjrd]

@jchyb ping for triage. IIUC you're issue supervisor this week?

[smarter]

Oops, looks like I completely missed this. I think this is fine as-is since it shouldn't have any false positives but I left a comment with a possible improvement to avoid false negatives.

[smarter]

There's a few situations where this check wouldn't be enough:

1. The method defined in the class is overloaded (other.exists will return false).

2. The parameters in the extension definition are subtypes of the one in the primary definition:

class C { def foo(x: Any) = 0 }
extension (c: C) def foo(x: Int) = 1 // uncallable

3. Only the first (if any) parameter list matches:

class C { def foo(x: Int)(y: String) = 0 }
extension (c: C) def foo(x: Int)(y: Int) = 1 // uncallable

class C { def foo: Int = 0 }
extension (c: C) def foo: String = "" // uncallable

4. The primary definition might have a type parameter or implicit/using clause:

class C { def foo(using Int) = 0 }
extension (c: C) def foo = 1 // uncallable

5. The extension definition might have a type parameter or implicit/using clause in its prefix:

class C { def foo = 0 }
extension (using Int)(c: C) def foo = 1 // uncallable

6. Borderline (which we may or may not want to warn about): the extension definition might have a type parameter or implicit/using clause in its suffix:

class C { def foo = 0 }
extension (c: C) def foo(using Int) = 1 // only callable by explicitly passing `(using ...)`

I think this is solvable by doing something like (using Applications.stripImplicit):

Suggested change

	val other = paramTpt.tpe.nonPrivateMember(name)
	if other.exists && sym.denot.info.resultType.matches(other.info) then
	def firstExplicitParamTypes(tp: Type) = stripImplicit(tp.stripPoly, wildcardOnly = true).firstParamTypes
	val extFirst = firstExplicitParamTypes(sym.denot.info)
	val matching =
	paramTpt.tpe.nonPrivateMember(name)
	.filterWithPredicate: clsMeth =>
	val clsFirst = firstExplicitParamTypes(clsMeth.info)
	extFirst.lazyZip(clsFirst).forall(_ frozen_<:< _)
	.nonEmpty

But I haven't actually tried it.

[som-snytt]

Thanks! I was bored and in need of stimulation. If I'm able to work through the cases, I'm sorry in advance that I will request your re-review.

[som-snytt]

case 7 is case 6 but where c.foo(using x) typechecks with native member returning { def apply(using X) }. I had to refresh my knowledge, and the reference is not current or precise (as it mentions only when m is missing, not when m is present but expression doesn't typecheck). I'll experiment now with your code suggestion.

[som-snytt]

Ha, it errored on an old line of mine. The test almost covered it: member m: String hides extension m(i: Int) because m(42) is String#apply, whereas m: Int does not hide.

Result type does not drive extension search, so m: String always hides nullary extension.

For non-nullary extension and nullary member, consider apply in result.

The existing test case n was only about nullary member and extension with implicit params. We need a word, "implicitary", for that.

[som-snytt]

Moved the check to refchecks, where it is more at home.

Added some test code and conditions, learned something about extension methods and something about dotty internals!

Edit: previously, assumed incorrectly that all scaladoc test failures were spurious.

Error:  -- [E193] Potential Issue Error: /home/runner/work/dotty/dotty/scaladoc-testcases/src/tests/implicitConversions.scala:9:21 
Error:  9 |extension (a: A) def extended_bar(): String = ???
Error:    |                     ^
Error:    |                Suspicious extension extended_bar is already a member of A
Error:    |
Error:    | longer explanation available when compiling with `-explain`
Error:  one error found

where there is an extension method of the same name on a different type in A. I slapped a nowarn on it.

I was hoping to get out for some noontime exercise, but local dotty says you can't annotate an extension! I'm learning so much about extensions today! I guess the annotation goes on the def.

[som-snytt]

Already missed the narrow window to claim that message ID.

[smarter]

Otherwise LGTM.

[som-snytt]

Followed up on previous review.

[smarter]

Otherwise LGTM, thanks for your work!

[som-snytt]

Last chance for this PR to grab an error ID before other PRs overtake it. #20143

[smarter]

🥇

[soronpo]

This PR creates false positives under 3.5.0-RC7. Need to minimize.

[soronpo]

I'm sorry, I phrased it incorrectly. These are not false positives, but they create warnings for definitions I use as workaround for lack of imports in an extension method block.

Here is an example:

class Foo(val someField: Int)

extension (foo: Foo)
  def someField = foo.someField
  def bar1 = 1 + someField
  def bar2 = 2 + someField
  def bar3 = 3 + someField

Since I cannot apply import foo.someField commonly in the extension method block, this concept has been a useful hack.
@som-snytt What do you think about excluding the specific pattern of def <fieldName> = extended.<fieldName> from the warning?

Or alternatively, should we just enable the original extended value fields within each of the extension method's scope?