Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport "Warn if extension receiver already has member" to LTS #21052

Merged
merged 2 commits into from
Jul 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/SymDenotations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1349,7 +1349,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
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
case MatchTypeLegacyPatternID // errorNumber: 191 - unused in LTS
case UnstableInlineAccessorID // errorNumber: 192 - unused in LTS
case VolatileOnValID // errorNumber: 193
case ExtensionNullifiedByMemberID // errorNumber: 194

def errorNumber = ordinal - 1

Expand Down
11 changes: 11 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2425,6 +2425,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"
Expand Down
32 changes: 16 additions & 16 deletions compiler/src/dotty/tools/dotc/typer/Applications.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -1577,22 +1593,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
Expand Down
62 changes: 56 additions & 6 deletions compiler/src/dotty/tools/dotc/typer/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -952,8 +952,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
Expand Down Expand Up @@ -1017,6 +1016,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.)
*/
Expand Down Expand Up @@ -1150,8 +1198,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 {
Expand All @@ -1163,9 +1211,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
}

Expand Down
14 changes: 7 additions & 7 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2446,17 +2446,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`
Expand Down
2 changes: 1 addition & 1 deletion compiler/test/dotty/tools/dotc/printing/PrintingTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
4 changes: 3 additions & 1 deletion compiler/test/dotty/tools/scripting/ScriptTestEnv.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
8 changes: 5 additions & 3 deletions scaladoc-testcases/src/tests/implicitConversions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ???
Expand Down Expand Up @@ -45,7 +47,7 @@ class B {
class C {
def extensionInCompanion: String = ???
}

@annotation.nowarn // extensionInCompanion
object C {
implicit def companionConversion(c: C): B = ???

Expand All @@ -70,4 +72,4 @@ package nested {
}

class Z
}
}
1 change: 1 addition & 0 deletions scaladoc-testcases/src/tests/inheritedMembers1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package tests
package inheritedMembers1


/*<-*/@annotation.nowarn/*->*/
class A
{
def A: String
Expand Down
84 changes: 84 additions & 0 deletions tests/warn/i16743.check
Original file line number Diff line number Diff line change
@@ -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`
Loading