From 46d6a586c07cd161eee33450c6ea09195a54dea4 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 | 1 + .../tools/dotc/reporting/ErrorMessageID.scala | 1 + .../dotty/tools/dotc/reporting/messages.scala | 9 +++ .../dotty/tools/dotc/typer/Applications.scala | 32 ++++----- .../dotty/tools/dotc/typer/RefChecks.scala | 35 ++++++++-- .../src/dotty/tools/dotc/typer/Typer.scala | 17 ++--- .../tools/dotc/printing/PrintingTest.scala | 2 +- .../dotty/tools/scripting/ScriptTestEnv.scala | 4 +- .../src/tests/implicitConversions.scala | 4 +- .../src/tests/inheritedMembers1.scala | 1 + tests/neg/i16743.check | 60 ++++++++++++++++ tests/neg/i16743.scala | 70 +++++++++++++++++++ tests/warn/i9241.scala | 1 + 13 files changed, 203 insertions(+), 34 deletions(-) create mode 100644 tests/neg/i16743.check create mode 100644 tests/neg/i16743.scala diff --git a/compiler/src/dotty/tools/dotc/core/SymDenotations.scala b/compiler/src/dotty/tools/dotc/core/SymDenotations.scala index b1e85f2b4f90..ea707f49fd46 100644 --- 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 index f5e7f9d44f56..c95f6ea691ba 100644 --- 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 index 4c7cf003cbc9..eed2c436d3c8 100644 --- 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 index 004b21ce4fb5..6daf66151dd1 100644 --- 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 index fb692446f1df..3f13b6e85f98 100644 --- 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 index 1303b64cbd12..12412772dfdf 100644 --- 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 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..f12b8ad11801 100644 --- 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 -} \ 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/neg/i16743.check b/tests/neg/i16743.check new file mode 100644 index 000000000000..a46d2045249f --- /dev/null +++ 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 new file mode 100644 index 000000000000..0b9eed435501 --- /dev/null +++ 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 index ed1db2df0c8e..661dda242a72 100644 --- 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