Warn if extension is hidden by member of receiver

som-snytt · som-snytt · commit 46d6a586c07c · 2024-01-11T08:00:59.000-08:00
diff --git a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala
@@ -1346,6 +1346,7 @@ object SymDenotations {
      *  inClass <-- find denot.symbol      class C { <-- symbol is here
      *
      *                   site: Subtype of both inClass and C
+     *  } <-- balance the brace
      */
     final def matchingDecl(inClass: Symbol, site: Type, name: Name = this.name)(using Context): Symbol = {
       var denot = inClass.info.nonPrivateDecl(name)
diff --git a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala
@@ -206,6 +206,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
   case PureUnitExpressionID // errorNumber: 190
   case MatchTypeLegacyPatternID // errorNumber: 191
   case UnstableInlineAccessorID // errorNumber: 192
+  case ExtensionNullifiedByMemberID // errorNumber: 193
 
   def errorNumber = ordinal - 1
 
diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala
@@ -2429,6 +2429,15 @@ 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"Suspicious extension ${hl(method.name.toString)} is already a member of ${hl(target.name.toString)}"
+  def explain(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.
+       |It can be called as a regular method, but should probably be defined that way."""
+
 class TraitCompanionWithMutableStatic()(using Context)
   extends SyntaxMsg(TraitCompanionWithMutableStaticID) {
   def msg(using Context) = i"Companion of traits cannot define mutable @static fields"
diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala
@@ -345,6 +345,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 {
@@ -1554,22 +1570,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
diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala
@@ -985,8 +985,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
@@ -1048,6 +1047,28 @@ object RefChecks {
 
   end checkUnaryMethods
 
+  /** Check that an extension method is not hidden, i.e., that it is callable.
+   *
+   *  An extension method is hidden if it does not offer a parameter that is not subsumed
+   *  by the corresponding parameter of the member (or of all alternatives of an overload).
+   *
+   *  If the member has no parameters and the extension method has only implicit parameters,
+   *  then warn that the extension is shadowed unless called with explicit arguments.
+   */
+  def checkExtensionMethods(sym: Symbol)(using Context): Unit = if sym.is(Extension) then
+    extension (tp: Type) def firstExplicitParamTypes = Applications.stripImplicit(tp.stripPoly, wildcardOnly = true).firstParamTypes
+    val target = sym.info.firstParamTypes.head // required for extension method
+    if !target.typeSymbol.denot.isAliasType && !target.typeSymbol.denot.isOpaqueAlias then
+      val paramTps = sym.denot.info.resultType.firstExplicitParamTypes
+      val hidden =
+        target.nonPrivateMember(sym.name)
+        .filterWithPredicate:
+          _.info.firstExplicitParamTypes
+           .lazyZip(paramTps)
+           .forall((m, x) => x frozen_<:< m)
+        .exists
+      if hidden then report.warning(ExtensionNullifiedByMember(sym, target.typeSymbol), sym.srcPos)
+
   /** 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.)
    */
@@ -1181,8 +1202,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)
       if (sym.exists && sym.owner.isTerm) {
         tree.rhs match {
           case Ident(nme.WILDCARD) => report.error(UnboundPlaceholderParameter(), sym.srcPos)
@@ -1193,9 +1214,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
   }
 
diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala
@@ -2543,17 +2543,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 bailDefDef(sym: Symbol)(using Context): Tree =
+    // it's a discarded method (synthetic case class method or synthetic java record constructor), drop it
+    val canBeInvalidated: Boolean =
       sym.is(Synthetic)
       && (desugar.isRetractableCaseClassMethodName(sym.name) ||
         (sym.isConstructor && sym.owner.derivesFrom(defn.JavaRecordClass)))
+    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), 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 bailDefDef(sym) else {
 
     // TODO: - Remove this when `scala.language.experimental.erasedDefinitions` is no longer experimental.
     //       - Modify signature to `erased def erasedValue[T]: T`
@@ -2576,7 +2576,8 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
       case untpd.TypeDefs(tparams) => tparams
     }
 
-    // Register GADT constraint for class type parameters from outer to inner class definition. (Useful when nested classes exist.) But do not cross a function definition.
+    // Register GADT constraint for class type parameters from outer to inner class definition.
+    // (Useful when nested classes exist.) But do not cross a function definition.
     if sym.flags.is(Method) then
       rhsCtx.setFreshGADTBounds
       ctx.outer.outersIterator.takeWhile(!_.owner.is(Method))
diff --git a/compiler/test/dotty/tools/dotc/printing/PrintingTest.scala b/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")
diff --git a/compiler/test/dotty/tools/scripting/ScriptTestEnv.scala b/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) {
diff --git a/scaladoc-testcases/src/tests/implicitConversions.scala b/scaladoc-testcases/src/tests/implicitConversions.scala
@@ -45,7 +45,7 @@ class B {
 class C {
   def extensionInCompanion: String = ???
 }
-
+@annotation.nowarn // extensionInCompanion
 object C {
   implicit def companionConversion(c: C): B = ???
 
@@ -70,4 +70,4 @@ package nested {
   }
 
   class Z
-}
+}
diff --git a/scaladoc-testcases/src/tests/inheritedMembers1.scala b/scaladoc-testcases/src/tests/inheritedMembers1.scala
@@ -2,6 +2,7 @@ package tests
 package inheritedMembers1
 
 
+/*<-*/@annotation.nowarn/*->*/
 class A
 {
     def A: String
diff --git a/tests/neg/i16743.check b/tests/neg/i16743.check
@@ -0,0 +1,60 @@
+-- [E193] Potential Issue Error: tests/neg/i16743.scala:26:6 -----------------------------------------------------------
+26 |  def t = 27 // error
+   |      ^
+   |      Suspicious extension t is already a member of T
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E193] Potential Issue Error: tests/neg/i16743.scala:28:6 -----------------------------------------------------------
+28 |  def g(x: String)(i: Int): String = x*i // error
+   |      ^
+   |      Suspicious extension g is already a member of T
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E193] Potential Issue Error: tests/neg/i16743.scala:29:6 -----------------------------------------------------------
+29 |  def h(x: String): String = x // error
+   |      ^
+   |      Suspicious extension h is already a member of T
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E193] Potential Issue Error: tests/neg/i16743.scala:31:6 -----------------------------------------------------------
+31 |  def j(x: Any, y: Int): String = (x.toString)*y // error
+   |      ^
+   |      Suspicious extension j is already a member of T
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E193] Potential Issue Error: tests/neg/i16743.scala:32:6 -----------------------------------------------------------
+32 |  def k(x: String): String = x // error
+   |      ^
+   |      Suspicious extension k is already a member of T
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E193] Potential Issue Error: tests/neg/i16743.scala:33:6 -----------------------------------------------------------
+33 |  def l(using String): String = summon[String] // error: can't be called implicitly
+   |      ^
+   |      Suspicious extension l is already a member of T
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E193] Potential Issue Error: tests/neg/i16743.scala:34:6 -----------------------------------------------------------
+34 |  def m(using String): String = "m" + summon[String] // error
+   |      ^
+   |      Suspicious extension m is already a member of T
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E193] Potential Issue Error: tests/neg/i16743.scala:35:6 -----------------------------------------------------------
+35 |  def n(using String): String = "n" + summon[String] // error
+   |      ^
+   |      Suspicious extension n is already a member of T
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E193] Potential Issue Error: tests/neg/i16743.scala:36:6 -----------------------------------------------------------
+36 |  def o: String = "42" // error
+   |      ^
+   |      Suspicious extension o is already a member of T
+   |
+   | longer explanation available when compiling with `-explain`
+-- [E193] Potential Issue Error: tests/neg/i16743.scala:37:6 -----------------------------------------------------------
+37 |  def u: Int = 27 // error
+   |      ^
+   |      Suspicious extension u is already a member of T
+   |
+   | longer explanation available when compiling with `-explain`
diff --git a/tests/neg/i16743.scala b/tests/neg/i16743.scala
@@ -0,0 +1,70 @@
+
+//> using options -Werror
+
+trait G
+given G = new G { override def toString = "mygiven" }
+given String = "aGivenString"
+
+trait T:
+  def t = 42
+  def f(x: String): String = x*2
+  def g(x: String)(y: String): String = (x+y)*2
+  def h(x: Any): String = x.toString*2
+  def i(x: Any, y: String): String = (x.toString+y)*2
+  def j(x: Any, y: Any): String = (x.toString+y.toString)
+  def k(using G): String = summon[G].toString
+  def l(using G): String = summon[G].toString
+  def m: String = "mystring"
+  def n: Result = Result()
+  def o: Int = 42
+  def u: Int = 42
+  def u(n: Int): Int = u + n
+  def v(n: Int): Int = u + n
+  def v(s: String): String = s + u
+
+extension (_t: T)
+  def t = 27 // error
+  def f(i: Int): String = String.valueOf(i)
+  def g(x: String)(i: Int): String = x*i // error
+  def h(x: String): String = x // error
+  def i(x: Any, y: Int): String = (x.toString)*y
+  def j(x: Any, y: Int): String = (x.toString)*y // error
+  def k(x: String): String = x // error
+  def l(using String): String = summon[String] // error: can't be called implicitly
+  def m(using String): String = "m" + summon[String] // error
+  def n(using String): String = "n" + summon[String] // error
+  def o: String = "42" // error
+  def u: Int = 27 // error
+  def v(d: Double) = 3.14
+
+// deferred extension is defined in subclass
+trait Foo:
+  type X
+  extension (x: X) def t: Int
+
+trait Bar extends Foo:
+  type X = T
+  extension (x: X) def t = x.t
+
+// extension on opaque type matches member of underlying type
+opaque type IArray[+T] = Array[? <: T]
+object IArray:
+  extension (arr: IArray[Byte]) def length: Int = arr.asInstanceOf[Array[Byte]].length
+
+class Result:
+  def apply(using String): String = s"result ${summon[String]}"
+
+@main def test() =
+  val x = new T {}
+  println(x.f(42)) // OK!
+  //println(x.g("x")(42)) // NOT OK!
+  println(x.h("hi")) // member!
+  println(x.i("hi", 5)) // OK!
+  println(x.j("hi", 5)) // member!
+  println(x.k)
+  println(x.l(using "x"))
+  println(x.l)
+  println(x.m(using "x"))
+  println(x.m(2))
+  println(x.n)          // OK just checks
+  println(x.n(using "x"))  // also just checks
diff --git a/tests/warn/i9241.scala b/tests/warn/i9241.scala
@@ -22,6 +22,7 @@ final class Baz private (val x: Int) extends AnyVal {
 }
 
 extension (x: Int)
+  @annotation.nowarn
   def unary_- : Int = ???
   def unary_+[T] : Int = ???
   def unary_!() : Int = ??? // warn

Original file line number	Diff line number	Diff line change
`@@ -1346,6 +1346,7 @@ object SymDenotations {`
`1346`	`1346`	`* inClass <-- find denot.symbol class C { <-- symbol is here`
`1347`	`1347`	`*`
`1348`	`1348`	`* site: Subtype of both inClass and C`
	`1349`	`+ * } <-- balance the brace`
`1349`	`1350`	`*/`
`1350`	`1351`	`final def matchingDecl(inClass: Symbol, site: Type, name: Name = this.name)(using Context): Symbol = {`
`1351`	`1352`	`var denot = inClass.info.nonPrivateDecl(name)`
Original file line number	Diff line number	Diff line change
`@@ -217,8 +217,10 @@ object ScriptTestEnv {`
`217`	`217`
`218`	`218`	`def toUrl: String = Paths.get(absPath).toUri.toURL.toString`
`219`	`219`
	`220`	`+ // Used to be an extension on String`
`220`	`221`	`// Treat norm paths with a leading '/' as absolute (Windows java.io.File#isAbsolute treats them as relative)`
`221`		`- def isAbsolute = p.norm.startsWith("/") \|\| (isWin && p.norm.secondChar == ":")`
	`222`	`+ //@annotation.nowarn // hidden by Path#isAbsolute`
	`223`	`+ //def isAbsolute = p.norm.startsWith("/") \|\| (isWin && p.norm.secondChar == ":")`
`222`	`224`	`}`
`223`	`225`
`224`	`226`	`extension(f: File) {`
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ class B {`
`45`	`45`	`class C {`
`46`	`46`	`def extensionInCompanion: String = ???`
`47`	`47`	`}`
`48`		`-`
	`48`	`+@annotation.nowarn // extensionInCompanion`
`49`	`49`	`object C {`
`50`	`50`	`implicit def companionConversion(c: C): B = ???`
`51`	`51`
`@@ -70,4 +70,4 @@ package nested {`
`70`	`70`	`}`
`71`	`71`
`72`	`72`	`class Z`
`73`		`-}`
	`73`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,7 @@ package tests`
`2`	`2`	`package inheritedMembers1`
`3`	`3`
`4`	`4`
	`5`	`+/<-/@annotation.nowarn/->/`
`5`	`6`	`class A`
`6`	`7`	`{`
`7`	`8`	`def A: String`