From d947fbfc73b0f8eaa3f44062baab17c9fc8f1645 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Fri, 19 May 2023 15:47:03 -0700 Subject: [PATCH] Warn if extension is hidden by member of receiver --- .../tools/dotc/core/SymDenotations.scala | 2 +- .../tools/dotc/reporting/ErrorMessageID.scala | 1 + .../dotty/tools/dotc/reporting/messages.scala | 11 ++ .../dotty/tools/dotc/typer/Applications.scala | 32 ++--- .../dotty/tools/dotc/typer/RefChecks.scala | 62 ++++++++- .../src/dotty/tools/dotc/typer/Typer.scala | 14 +-- .../tools/dotc/printing/PrintingTest.scala | 2 +- .../dotty/tools/scripting/ScriptTestEnv.scala | 4 +- .../src/tests/implicitConversions.scala | 8 +- .../src/tests/inheritedMembers1.scala | 1 + tests/warn/i16743.check | 84 +++++++++++++ tests/warn/i16743.scala | 119 ++++++++++++++++++ tests/warn/i9241.scala | 5 +- 13 files changed, 309 insertions(+), 36 deletions(-) create mode 100644 tests/warn/i16743.check create mode 100644 tests/warn/i16743.scala diff --git a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala index 14ba05568735..f2519995f11b 100644 --- a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala +++ b/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 */ diff --git a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala index 6011587a7100..33f5dcf1b1f5 100644 --- a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala +++ b/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 diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala index 484bc88c0983..bcdf65873008 100644 --- a/compiler/src/dotty/tools/dotc/reporting/messages.scala +++ b/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.""" + 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" diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 82f4c89ae203..072e2b5ccc51 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/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 { @@ -1589,22 +1605,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 index 173d5e6b1f7e..3a4c0dd24acb 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/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 } diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 0b05bcd078ff..581820918762 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/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` diff --git a/compiler/test/dotty/tools/dotc/printing/PrintingTest.scala b/compiler/test/dotty/tools/dotc/printing/PrintingTest.scala index 2c970e93f573..63ee5c54fab7 100644 --- 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 index ebae5bfca6be..1db92d5415b4 100644 --- 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 index 720eab1ccb1a..c3051e653663 100644 --- a/scaladoc-testcases/src/tests/implicitConversions.scala +++ b/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 -} \ No newline at end of file +} diff --git a/scaladoc-testcases/src/tests/inheritedMembers1.scala b/scaladoc-testcases/src/tests/inheritedMembers1.scala index d8fa44607e5e..561e50ceaec2 100644 --- 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/warn/i16743.check b/tests/warn/i16743.check new file mode 100644 index 000000000000..a81b322e8016 --- /dev/null +++ b/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. + | + | 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. + | + | 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. + | + | 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. + | + | 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. + | + | 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. + | + | 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. + | + | 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. + | + | 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. + | + | 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. + | + | 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. + | + | 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. + | + | longer explanation available when compiling with `-explain` diff --git a/tests/warn/i16743.scala b/tests/warn/i16743.scala new file mode 100644 index 000000000000..4c9c99cf30d0 --- /dev/null +++ b/tests/warn/i16743.scala @@ -0,0 +1,119 @@ + +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 + def end: Int = 42 + def at(n: Int) = n + def w(n: Int): Int = 42 + n + def x(n: Int): Int = 42 + n + def y(n: Int): Int = u + n + def y(s: String): String = s + u + +extension (_t: T) + def t = 27 // warn + def f(i: Int): String = String.valueOf(i) + def g(x: String)(i: Int): String = x*i // warn + def h(x: String): String = x // warn + def i(x: Any, y: Int): String = (x.toString)*y + def j(x: Any, y: Int): String = (x.toString)*y // warn + def k(x: String): String = x // warn + def l(using String): String = summon[String] + def m(using String): String = "m" + summon[String] // warn + def n(using String): String = "n" + summon[String] // warn + def o: String = "42" // warn + def u: Int = 27 // warn + def v(d: Double) = 3.14 + def end(n: Int): Int = 42 + n + def at: Int = 42 // warn + def w(using String)(n: String): Int = (summon[String] + n).toInt + def x(using String)(n: Int): Int = summon[String].toInt + n // warn + def y(using String)(s: String): String = s + summon[String] // warn + +// 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 // nowarn see Quote below + +// extension on opaque type matches member of underlying type +object Dungeon: + opaque type IArray[+T] = Array[? <: T] + object IArray: + extension (arr: IArray[Byte]) def length: Int = arr.asInstanceOf[Array[Byte]].length +trait DungeonDweller: + extension (arr: Dungeon.IArray[Byte]) def length: Int = 42 // nowarn + def f[A <: Byte](x: Dungeon.IArray[A]) = x.length +trait SadDungeonDweller: + def f[A](x: Dungeon.IArray[A]) = 27 // x.length // just to confirm, length is not a member + +trait Quote: + type Tree <: AnyRef + given TreeMethods: TreeMethods + trait TreeMethods: + extension (self: Tree) + def length(): Int +class QuotesImpl extends Quote: + type Tree = String + given TreeMethods: TreeMethods with + extension (self: Tree) + def length(): Int = self.length() // nowarn Tree already has a member with the same name. + +class Result: + def apply(using String): String = s"result ${summon[String]}" + +class Depends: + type Thing = String + def thing: Thing = "" +object Depending: + extension (using depends: Depends)(x: depends.Thing) + def y = 42 + def length() = 42 // nowarn see Quote above + def f(using d: Depends) = d.thing.y + def g(using d: Depends) = d.thing.length() + +@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.k("hi")) // no, implicit is either omitted (supplied implicitly) or explicitly (using foo) + println(x.l) // usual, invokes member + println("L"+x.l(using "x")) // explicit, member doesn't check, try extension + println(x.m(using "x")) // same idea as previous, except member takes no implicits or any params + println(x.m(2)) // member checks by adapting result + println(x.n) // Result + println(x.n.apply) // apply Result with given + println(x.n(using "x")) // apply Result explicitly, not extension + println(x.end(2)) + println(x.at(2)) + println { + val p = x.at + p(2) + } + println { + given String = "42" + x.w("27") + } diff --git a/tests/warn/i9241.scala b/tests/warn/i9241.scala index ed1db2df0c8e..cbc8a0698d18 100644 --- a/tests/warn/i9241.scala +++ b/tests/warn/i9241.scala @@ -22,9 +22,12 @@ final class Baz private (val x: Int) extends AnyVal { } extension (x: Int) + @annotation.nowarn def unary_- : Int = ??? + @annotation.nowarn def unary_+[T] : Int = ??? def unary_!() : Int = ??? // warn + @annotation.nowarn def unary_~(using Int) : Int = ??? end extension @@ -40,4 +43,4 @@ extension (using Int)(x: Byte) def unary_+[U] : Int = ??? def unary_!() : Int = ??? // warn def unary_~(using Int) : Int = ??? -end extension \ No newline at end of file +end extension