Warn if extension receiver already has member (#17543)

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

Author: smarter

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -1355,7 +1355,7 @@ object SymDenotations {
      *  @param inClass   The class containing the result symbol's definition
      *  @param site      The base type from which member types are computed
      *
-     *  inClass <-- find denot.symbol      class C { <-- symbol is here
+     *  inClass <-- find denot.symbol      class C { <-- symbol is here }
      *
      *                   site: Subtype of both inClass and C
      */

compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala

@@ -207,6 +207,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
   case MatchTypeLegacyPatternID // errorNumber: 191
   case UnstableInlineAccessorID // errorNumber: 192
   case VolatileOnValID // errorNumber: 193
+  case ExtensionNullifiedByMemberID // errorNumber: 194
 
   def errorNumber = ordinal - 1
 

compiler/src/dotty/tools/dotc/reporting/messages.scala

@@ -2451,6 +2451,17 @@ class SynchronizedCallOnBoxedClass(stat: tpd.Tree)(using Context)
         |you intended."""
 }
 
+class ExtensionNullifiedByMember(method: Symbol, target: Symbol)(using Context)
+  extends Message(ExtensionNullifiedByMemberID):
+  def kind = MessageKind.PotentialIssue
+  def msg(using Context) =
+    i"""Extension method ${hl(method.name.toString)} will never be selected
+       |because ${hl(target.name.toString)} already has a member with the same name and compatible parameter types."""
+  def explain(using Context) =
+    i"""An extension method can be invoked as a regular method, but if that is intended,
+       |it should not be defined as an extension.
+       |Although extensions can be overloaded, they do not overload existing member methods."""
+
 class TraitCompanionWithMutableStatic()(using Context)
   extends SyntaxMsg(TraitCompanionWithMutableStaticID) {
   def msg(using Context) = i"Companion of traits cannot define mutable @static fields"

compiler/src/dotty/tools/dotc/typer/Applications.scala

@@ -346,6 +346,22 @@ object Applications {
     val flags2 = sym1.flags | NonMember // ensures Select typing doesn't let TermRef#withPrefix revert the type
     val sym2 = sym1.copy(info = methType, flags = flags2) // symbol not entered, to avoid overload resolution problems
     fun.withType(sym2.termRef)
+
+  /** Drop any leading implicit parameter sections */
+  def stripImplicit(tp: Type, wildcardOnly: Boolean = false)(using Context): Type = tp match {
+    case mt: MethodType if mt.isImplicitMethod =>
+      stripImplicit(resultTypeApprox(mt, wildcardOnly))
+    case pt: PolyType =>
+      pt.derivedLambdaType(pt.paramNames, pt.paramInfos,
+          stripImplicit(pt.resultType, wildcardOnly = true))
+            // can't use TypeParamRefs for parameter references in `resultTypeApprox`
+            // since their bounds can refer to type parameters in `pt` that are not
+            // bound by the constraint. This can lead to hygiene violations if subsequently
+            // `pt` itself is added to the constraint. Test case is run/enrich-gentraversable.scala.
+        .asInstanceOf[PolyType].flatten
+    case _ =>
+      tp
+  }
 }
 
 trait Applications extends Compatibility {
@@ -1591,22 +1607,6 @@ trait Applications extends Compatibility {
       tp
   }
 
-  /** Drop any leading implicit parameter sections */
-  def stripImplicit(tp: Type, wildcardOnly: Boolean = false)(using Context): Type = tp match {
-    case mt: MethodType if mt.isImplicitMethod =>
-      stripImplicit(resultTypeApprox(mt, wildcardOnly))
-    case pt: PolyType =>
-      pt.derivedLambdaType(pt.paramNames, pt.paramInfos,
-          stripImplicit(pt.resultType, wildcardOnly = true))
-            // can't use TypeParamRefs for parameter references in `resultTypeApprox`
-            // since their bounds can refer to type parameters in `pt` that are not
-            // bound by the constraint. This can lead to hygiene violations if subsequently
-            // `pt` itself is added to the constraint. Test case is run/enrich-gentraversable.scala.
-        .asInstanceOf[PolyType].flatten
-    case _ =>
-      tp
-  }
-
   /** Compare owner inheritance level.
     *  @param    sym1 The first owner
     *  @param    sym2 The second owner

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

@@ -1033,8 +1033,7 @@ object RefChecks {
    *  surprising names at runtime. E.g. in neg/i4564a.scala, a private
    *  case class `apply` method would have to be renamed to something else.
    */
-  def checkNoPrivateOverrides(tree: Tree)(using Context): Unit =
-    val sym = tree.symbol
+  def checkNoPrivateOverrides(sym: Symbol)(using Context): Unit =
     if sym.maybeOwner.isClass
        && sym.is(Private)
        && (sym.isOneOf(MethodOrLazyOrMutable) || !sym.is(Local)) // in these cases we'll produce a getter later
@@ -1100,6 +1099,55 @@ object RefChecks {
 
   end checkUnaryMethods
 
+  /** Check that an extension method is not hidden, i.e., that it is callable as an extension method.
+   *
+   *  An extension method is hidden if it does not offer a parameter that is not subsumed
+   *  by the corresponding parameter of the member with the same name (or of all alternatives of an overload).
+   *
+   *  For example, it is not possible to define a type-safe extension `contains` for `Set`,
+   *  since for any parameter type, the existing `contains` method will compile and would be used.
+   *
+   *  If the member has a leading implicit parameter list, then the extension method must also have
+   *  a leading implicit parameter list. The reason is that if the implicit arguments are inferred,
+   *  either the member method is used or typechecking fails. If the implicit arguments are supplied
+   *  explicitly and the member method is not applicable, the extension is checked, and its parameters
+   *  must be implicit in order to be applicable.
+   *
+   *  If the member does not have a leading implicit parameter list, then the argument cannot be explicitly
+   *  supplied with `using`, as typechecking would fail. But the extension method may have leading implicit
+   *  parameters, which are necessarily supplied implicitly in the application. The first non-implicit
+   *  parameters of the extension method must be distinguishable from the member parameters, as described.
+   *
+   *  If the extension method is nullary, it is always hidden by a member of the same name.
+   *  (Either the member is nullary, or the reference is taken as the eta-expansion of the member.)
+   */
+  def checkExtensionMethods(sym: Symbol)(using Context): Unit = if sym.is(Extension) then
+    extension (tp: Type)
+      def strippedResultType = Applications.stripImplicit(tp.stripPoly, wildcardOnly = true).resultType
+      def firstExplicitParamTypes = Applications.stripImplicit(tp.stripPoly, wildcardOnly = true).firstParamTypes
+      def hasImplicitParams = tp.stripPoly match { case mt: MethodType => mt.isImplicitMethod case _ => false }
+    val target = sym.info.firstExplicitParamTypes.head // required for extension method, the putative receiver
+    val methTp = sym.info.strippedResultType // skip leading implicits and the "receiver" parameter
+    def hidden =
+      target.nonPrivateMember(sym.name)
+      .filterWithPredicate:
+        member =>
+        val memberIsImplicit = member.info.hasImplicitParams
+        val paramTps =
+          if memberIsImplicit then methTp.stripPoly.firstParamTypes
+          else methTp.firstExplicitParamTypes
+
+        paramTps.isEmpty || memberIsImplicit && !methTp.hasImplicitParams || {
+          val memberParamTps = member.info.stripPoly.firstParamTypes
+          !memberParamTps.isEmpty
+          && memberParamTps.lengthCompare(paramTps) == 0
+          && memberParamTps.lazyZip(paramTps).forall((m, x) => x frozen_<:< m)
+        }
+      .exists
+    if !target.typeSymbol.denot.isAliasType && !target.typeSymbol.denot.isOpaqueAlias && hidden
+    then report.warning(ExtensionNullifiedByMember(sym, target.typeSymbol), sym.srcPos)
+  end checkExtensionMethods
+
   /** Verify that references in the user-defined `@implicitNotFound` message are valid.
    *  (i.e. they refer to a type variable that really occurs in the signature of the annotated symbol.)
    */
@@ -1233,8 +1281,8 @@ class RefChecks extends MiniPhase { thisPhase =>
 
   override def transformValDef(tree: ValDef)(using Context): ValDef = {
     if tree.symbol.exists then
-      checkNoPrivateOverrides(tree)
       val sym = tree.symbol
+      checkNoPrivateOverrides(sym)
       checkVolatile(sym)
       if (sym.exists && sym.owner.isTerm) {
         tree.rhs match {
@@ -1246,9 +1294,11 @@ class RefChecks extends MiniPhase { thisPhase =>
   }
 
   override def transformDefDef(tree: DefDef)(using Context): DefDef = {
-    checkNoPrivateOverrides(tree)
-    checkImplicitNotFoundAnnotation.defDef(tree.symbol.denot)
-    checkUnaryMethods(tree.symbol)
+    val sym = tree.symbol
+    checkNoPrivateOverrides(sym)
+    checkImplicitNotFoundAnnotation.defDef(sym.denot)
+    checkUnaryMethods(sym)
+    checkExtensionMethods(sym)
     tree
   }
 

compiler/src/dotty/tools/dotc/typer/Typer.scala

@@ -2568,17 +2568,17 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
     vdef1.setDefTree
   }
 
-  def typedDefDef(ddef: untpd.DefDef, sym: Symbol)(using Context): Tree = {
-    def canBeInvalidated(sym: Symbol): Boolean =
+  private def retractDefDef(sym: Symbol)(using Context): Tree =
+    // it's a discarded method (synthetic case class method or synthetic java record constructor or overridden member), drop it
+    val canBeInvalidated: Boolean =
       sym.is(Synthetic)
       && (desugar.isRetractableCaseClassMethodName(sym.name) ||
          (sym.owner.is(JavaDefined) && sym.owner.derivesFrom(defn.JavaRecordClass) && sym.is(Method)))
+    assert(canBeInvalidated)
+    sym.owner.info.decls.openForMutations.unlink(sym)
+    EmptyTree
 
-    if !sym.info.exists then
-      // it's a discarded method (synthetic case class method or synthetic java record constructor or overriden member), drop it
-      assert(canBeInvalidated(sym))
-      sym.owner.info.decls.openForMutations.unlink(sym)
-      return EmptyTree
+  def typedDefDef(ddef: untpd.DefDef, sym: Symbol)(using Context): Tree = if !sym.info.exists then retractDefDef(sym) else {
 
     // TODO: - Remove this when `scala.language.experimental.erasedDefinitions` is no longer experimental.
     //       - Modify signature to `erased def erasedValue[T]: T`

compiler/test/dotty/tools/dotc/printing/PrintingTest.scala

@@ -25,7 +25,7 @@ import java.io.File
 class PrintingTest {
 
   def options(phase: String, flags: List[String]) =
-    List(s"-Xprint:$phase", "-color:never", "-classpath", TestConfiguration.basicClasspath) ::: flags
+    List(s"-Xprint:$phase", "-color:never", "-nowarn", "-classpath", TestConfiguration.basicClasspath) ::: flags
 
   private def compileFile(path: JPath, phase: String): Boolean = {
     val baseFilePath  = path.toString.stripSuffix(".scala")

compiler/test/dotty/tools/scripting/ScriptTestEnv.scala

@@ -217,8 +217,10 @@ object ScriptTestEnv {
 
     def toUrl: String = Paths.get(absPath).toUri.toURL.toString
 
+    // Used to be an extension on String
     // Treat norm paths with a leading '/' as absolute (Windows java.io.File#isAbsolute treats them as relative)
-    def isAbsolute = p.norm.startsWith("/") || (isWin && p.norm.secondChar == ":")
+    //@annotation.nowarn // hidden by Path#isAbsolute
+    //def isAbsolute = p.norm.startsWith("/") || (isWin && p.norm.secondChar == ":")
   }
 
   extension(f: File) {

scaladoc-testcases/src/tests/implicitConversions.scala

@@ -6,7 +6,9 @@ given Conversion[A, B] with {
   def apply(a: A): B = ???
 }
 
-extension (a: A) def extended_bar(): String = ???
+extension (a: A)
+  @annotation.nowarn
+  def extended_bar(): String = ???
 
 class A {
   implicit def conversion(c: C): D = ???
@@ -45,7 +47,7 @@ class B {
 class C {
   def extensionInCompanion: String = ???
 }
-
+@annotation.nowarn // extensionInCompanion
 object C {
   implicit def companionConversion(c: C): B = ???
 
@@ -70,4 +72,4 @@ package nested {
   }
 
   class Z
-}
+}

scaladoc-testcases/src/tests/inheritedMembers1.scala

@@ -2,6 +2,7 @@ package tests
 package inheritedMembers1
 
 
+/*<-*/@annotation.nowarn/*->*/
 class A
 {
     def A: String

tests/warn/i16743.check

@@ -0,0 +1,84 @@
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:30:6 --------------------------------------------------------
+30 |  def t = 27 // warn
+   |      ^
+   |      Extension method t will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:32:6 --------------------------------------------------------
+32 |  def g(x: String)(i: Int): String = x*i // warn
+   |      ^
+   |      Extension method g will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:33:6 --------------------------------------------------------
+33 |  def h(x: String): String = x // warn
+   |      ^
+   |      Extension method h will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:35:6 --------------------------------------------------------
+35 |  def j(x: Any, y: Int): String = (x.toString)*y // warn
+   |      ^
+   |      Extension method j will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:36:6 --------------------------------------------------------
+36 |  def k(x: String): String = x // warn
+   |      ^
+   |      Extension method k will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:38:6 --------------------------------------------------------
+38 |  def m(using String): String = "m" + summon[String] // warn
+   |      ^
+   |      Extension method m will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:39:6 --------------------------------------------------------
+39 |  def n(using String): String = "n" + summon[String] // warn
+   |      ^
+   |      Extension method n will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:40:6 --------------------------------------------------------
+40 |  def o: String = "42" // warn
+   |      ^
+   |      Extension method o will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:41:6 --------------------------------------------------------
+41 |  def u: Int = 27 // warn
+   |      ^
+   |      Extension method u will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:44:6 --------------------------------------------------------
+44 |  def at: Int = 42 // warn
+   |      ^
+   |      Extension method at will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:46:6 --------------------------------------------------------
+46 |  def x(using String)(n: Int): Int = summon[String].toInt + n // warn
+   |      ^
+   |      Extension method x will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E194] Potential Issue Warning: tests/warn/i16743.scala:47:6 --------------------------------------------------------
+47 |  def y(using String)(s: String): String = s + summon[String] // warn
+   |      ^
+   |      Extension method y will never be selected
+   |      because T already has a member with the same name and compatible parameter types.
+   |
+   | longer explanation available when compiling with `-explain`