From fd9ebe0ea563a02f42bd72792a2495e3aebdbabc Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Fri, 31 Jan 2025 22:29:36 -0800 Subject: [PATCH 1/5] Discourage default arg for extension receiver --- .../tools/dotc/reporting/ErrorMessageID.scala | 1 + .../dotty/tools/dotc/reporting/messages.scala | 9 ++++++++ .../dotty/tools/dotc/typer/RefChecks.scala | 22 +++++++++++-------- tests/warn/i12460.check | 20 +++++++++++++++++ tests/warn/i12460.scala | 7 ++++++ 5 files changed, 50 insertions(+), 9 deletions(-) create mode 100644 tests/warn/i12460.check create mode 100644 tests/warn/i12460.scala diff --git a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala index 25f2f879077e..449d0267808d 100644 --- a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala +++ b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala @@ -221,6 +221,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe case GivenSearchPriorityID // errorNumber: 205 case EnumMayNotBeValueClassesID // errorNumber: 206 case IllegalUnrollPlacementID // errorNumber: 207 + case ExtensionHasDefaultID // errorNumber: 208 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 fd85a65822eb..8b474313f950 100644 --- a/compiler/src/dotty/tools/dotc/reporting/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/messages.scala @@ -2514,6 +2514,15 @@ class ExtensionNullifiedByMember(method: Symbol, target: Symbol)(using Context) | |The extension may be invoked as though selected from an arbitrary type if conversions are in play.""" +class ExtensionHasDefault(method: Symbol)(using Context) + extends Message(ExtensionHasDefaultID): + def kind = MessageKind.PotentialIssue + def msg(using Context) = + i"""Extension method ${hl(method.name.toString)} should not have a default argument for its receiver.""" + def explain(using Context) = + i"""Although extensions are ordinary methods, they must be invoked as a selection. + |Therefore, the receiver cannot be omitted. A default argument for that parameter would never be used.""" + 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/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index 4dbeac7219c1..4c8b0fd25d4d 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -1163,22 +1163,20 @@ object RefChecks { * that are either both opaque types or both not. */ def checkExtensionMethods(sym: Symbol)(using Context): Unit = - if sym.is(Extension) && !sym.nextOverriddenSymbol.exists then + 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 explicit = Applications.stripImplicit(tp.stripPoly, wildcardOnly = true) 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 + val target = sym.info.explicit.firstParamTypes.head // required for extension method, the putative receiver + val methTp = sym.info.explicit.resultType // skip leading implicits and the "receiver" parameter def hidden = target.nonPrivateMember(sym.name) - .filterWithPredicate: - member => + .filterWithPredicate: member => member.symbol.isPublic && { val memberIsImplicit = member.info.hasImplicitParams val paramTps = if memberIsImplicit then methTp.stripPoly.firstParamTypes - else methTp.firstExplicitParamTypes + else methTp.explicit.firstParamTypes paramTps.isEmpty || memberIsImplicit && !methTp.hasImplicitParams || { val memberParamTps = member.info.stripPoly.firstParamTypes @@ -1190,7 +1188,13 @@ object RefChecks { } } .exists - if !target.typeSymbol.isOpaqueAlias && hidden + val receiverName = sym.info.explicit.firstParamNames.head + val num = sym.info.paramNamess.flatten.indexWhere(_ == receiverName) + val getterName = DefaultGetterName(sym.name.toTermName, num = num) + val getterDenot = sym.owner.info.member(getterName) + if getterDenot.exists + then report.warning(ExtensionHasDefault(sym), getterDenot.symbol.srcPos) + if !target.typeSymbol.isOpaqueAlias && !sym.nextOverriddenSymbol.exists && hidden then report.warning(ExtensionNullifiedByMember(sym, target.typeSymbol), sym.srcPos) end checkExtensionMethods diff --git a/tests/warn/i12460.check b/tests/warn/i12460.check new file mode 100644 index 000000000000..772121448fea --- /dev/null +++ b/tests/warn/i12460.check @@ -0,0 +1,20 @@ +-- [E208] Potential Issue Warning: tests/warn/i12460.scala:3:23 -------------------------------------------------------- +3 |extension (s: String = "hello, world") def invert = s.reverse.toUpperCase // warn + | ^ + | Extension method invert should not have a default argument for its receiver. + |--------------------------------------------------------------------------------------------------------------------- + | Explanation (enabled by `-explain`) + |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + | Although extensions are ordinary methods, they must be invoked as a selection. + | Therefore, the receiver cannot be omitted. A default argument for that parameter would never be used. + --------------------------------------------------------------------------------------------------------------------- +-- [E208] Potential Issue Warning: tests/warn/i12460.scala:5:17 -------------------------------------------------------- +5 |extension (using String)(s: String = "hello, world") def revert = s.reverse.toUpperCase // warn + | ^ + | Extension method revert should not have a default argument for its receiver. + |--------------------------------------------------------------------------------------------------------------------- + | Explanation (enabled by `-explain`) + |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + | Although extensions are ordinary methods, they must be invoked as a selection. + | Therefore, the receiver cannot be omitted. A default argument for that parameter would never be used. + --------------------------------------------------------------------------------------------------------------------- diff --git a/tests/warn/i12460.scala b/tests/warn/i12460.scala new file mode 100644 index 000000000000..ec2fdf4deed7 --- /dev/null +++ b/tests/warn/i12460.scala @@ -0,0 +1,7 @@ +//> using options -explain + +extension (s: String = "hello, world") def invert = s.reverse.toUpperCase // warn + +extension (using String)(s: String = "hello, world") def revert = s.reverse.toUpperCase // warn + +extension (s: String) def divert(m: String = "hello, world") = (s+m).reverse.toUpperCase // ok From f7d1bc23145ded3c7435031a30cefe92171e82a0 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Sat, 1 Feb 2025 12:26:20 -0800 Subject: [PATCH 2/5] Fix span of default getter --- compiler/src/dotty/tools/dotc/ast/Desugar.scala | 17 +++++++---------- tests/warn/i12460.check | 6 +++--- tests/warn/i12460.scala | 6 ++++-- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala index ec65224ac93d..51f64e67f9a9 100644 --- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala +++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala @@ -393,18 +393,14 @@ object desugar { rhs cpy.TypeDef(tparam)(rhs = dropInRhs(tparam.rhs)) - def paramssNoRHS = mapParamss(meth.paramss)(identity) { - vparam => - if vparam.rhs.isEmpty then vparam - else cpy.ValDef(vparam)(rhs = EmptyTree).withMods(vparam.mods | HasDefault) - } + def paramssNoRHS = mapParamss(meth.paramss)(identity): vparam => + if vparam.rhs.isEmpty then vparam + else cpy.ValDef(vparam)(rhs = EmptyTree).withMods(vparam.mods | HasDefault) def getterParamss(n: Int): List[ParamClause] = - mapParamss(takeUpTo(paramssNoRHS, n)) { - tparam => dropContextBounds(toMethParam(tparam, KeepAnnotations.All)) - } { - vparam => toMethParam(vparam, KeepAnnotations.All, keepDefault = false) - } + mapParamss(takeUpTo(paramssNoRHS, n)) + (tparam => dropContextBounds(toMethParam(tparam, KeepAnnotations.All))) + (vparam => toMethParam(vparam, KeepAnnotations.All, keepDefault = false)) def defaultGetters(paramss: List[ParamClause], n: Int): List[DefDef] = paramss match case ValDefs(vparam :: vparams) :: paramss1 => @@ -418,6 +414,7 @@ object desugar { .withMods(Modifiers( meth.mods.flags & (AccessFlags | Synthetic) | (vparam.mods.flags & Inline), meth.mods.privateWithin)) + .withSpan(vparam.rhs.span) val rest = defaultGetters(vparams :: paramss1, n + 1) if vparam.rhs.isEmpty then rest else defaultGetter :: rest case _ :: paramss1 => // skip empty parameter lists and type parameters diff --git a/tests/warn/i12460.check b/tests/warn/i12460.check index 772121448fea..98320b33ade7 100644 --- a/tests/warn/i12460.check +++ b/tests/warn/i12460.check @@ -8,10 +8,10 @@ | Although extensions are ordinary methods, they must be invoked as a selection. | Therefore, the receiver cannot be omitted. A default argument for that parameter would never be used. --------------------------------------------------------------------------------------------------------------------- --- [E208] Potential Issue Warning: tests/warn/i12460.scala:5:17 -------------------------------------------------------- +-- [E208] Potential Issue Warning: tests/warn/i12460.scala:5:37 -------------------------------------------------------- 5 |extension (using String)(s: String = "hello, world") def revert = s.reverse.toUpperCase // warn - | ^ - | Extension method revert should not have a default argument for its receiver. + | ^ + | Extension method revert should not have a default argument for its receiver. |--------------------------------------------------------------------------------------------------------------------- | Explanation (enabled by `-explain`) |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/tests/warn/i12460.scala b/tests/warn/i12460.scala index ec2fdf4deed7..0e37c6aa7de7 100644 --- a/tests/warn/i12460.scala +++ b/tests/warn/i12460.scala @@ -1,7 +1,9 @@ -//> using options -explain +//> using options -explain -Ystop-after:refchecks extension (s: String = "hello, world") def invert = s.reverse.toUpperCase // warn extension (using String)(s: String = "hello, world") def revert = s.reverse.toUpperCase // warn -extension (s: String) def divert(m: String = "hello, world") = (s+m).reverse.toUpperCase // ok +extension (s: String) + def divert(m: String = "hello, world") = (s+m).reverse.toUpperCase // ok + def divertimento(using String)(m: String = "hello, world") = (s+m).reverse.toUpperCase // ok From 6f925ce291b97a45bc7142d9848e5d6a3e921cc4 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Tue, 18 Feb 2025 10:37:50 -0800 Subject: [PATCH 3/5] Check HasDefaultParams, compute explicit info once --- .../dotty/tools/dotc/typer/RefChecks.scala | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index 4c8b0fd25d4d..d96b37dd3c55 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -1167,8 +1167,9 @@ object RefChecks { extension (tp: Type) def explicit = Applications.stripImplicit(tp.stripPoly, wildcardOnly = true) def hasImplicitParams = tp.stripPoly match { case mt: MethodType => mt.isImplicitMethod case _ => false } - val target = sym.info.explicit.firstParamTypes.head // required for extension method, the putative receiver - val methTp = sym.info.explicit.resultType // skip leading implicits and the "receiver" parameter + val explicitInfo = sym.info.explicit // consider explicit value params + val target = explicitInfo.firstParamTypes.head // required for extension method, the putative receiver + val methTp = explicitInfo.resultType // skip leading implicits and the "receiver" parameter def hidden = target.nonPrivateMember(sym.name) .filterWithPredicate: member => @@ -1188,12 +1189,14 @@ object RefChecks { } } .exists - val receiverName = sym.info.explicit.firstParamNames.head - val num = sym.info.paramNamess.flatten.indexWhere(_ == receiverName) - val getterName = DefaultGetterName(sym.name.toTermName, num = num) - val getterDenot = sym.owner.info.member(getterName) - if getterDenot.exists - then report.warning(ExtensionHasDefault(sym), getterDenot.symbol.srcPos) + if sym.is(HasDefaultParams) then + val getterDenot = + val receiverName = explicitInfo.firstParamNames.head + val num = sym.info.paramNamess.flatten.indexWhere(_ == receiverName) + val getterName = DefaultGetterName(sym.name.toTermName, num = num) + sym.owner.info.member(getterName) + if getterDenot.exists + then report.warning(ExtensionHasDefault(sym), getterDenot.symbol.srcPos) if !target.typeSymbol.isOpaqueAlias && !sym.nextOverriddenSymbol.exists && hidden then report.warning(ExtensionNullifiedByMember(sym, target.typeSymbol), sym.srcPos) end checkExtensionMethods From 97bceda3607c5fc7f948fdec36b486b259f2c259 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Tue, 18 Feb 2025 10:53:00 -0800 Subject: [PATCH 4/5] Massage the message --- .../src/dotty/tools/dotc/reporting/messages.scala | 6 ++++-- tests/warn/i12460.check | 12 ++++++++---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala index 8b474313f950..10368bee2a9e 100644 --- a/compiler/src/dotty/tools/dotc/reporting/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/messages.scala @@ -2520,8 +2520,10 @@ class ExtensionHasDefault(method: Symbol)(using Context) def msg(using Context) = i"""Extension method ${hl(method.name.toString)} should not have a default argument for its receiver.""" def explain(using Context) = - i"""Although extensions are ordinary methods, they must be invoked as a selection. - |Therefore, the receiver cannot be omitted. A default argument for that parameter would never be used.""" + i"""The receiver cannot be omitted when an extension method is invoked as a selection. + |A default argument for that parameter would never be used in that case. + |An extension method can be invoked as a regular method, but if that is the intended usage, + |it should not be defined as an extension.""" class TraitCompanionWithMutableStatic()(using Context) extends SyntaxMsg(TraitCompanionWithMutableStaticID) { diff --git a/tests/warn/i12460.check b/tests/warn/i12460.check index 98320b33ade7..ba44bed1a18f 100644 --- a/tests/warn/i12460.check +++ b/tests/warn/i12460.check @@ -5,8 +5,10 @@ |--------------------------------------------------------------------------------------------------------------------- | Explanation (enabled by `-explain`) |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - | Although extensions are ordinary methods, they must be invoked as a selection. - | Therefore, the receiver cannot be omitted. A default argument for that parameter would never be used. + | The receiver cannot be omitted when an extension method is invoked as a selection. + | A default argument for that parameter would never be used in that case. + | An extension method can be invoked as a regular method, but if that is the intended usage, + | it should not be defined as an extension. --------------------------------------------------------------------------------------------------------------------- -- [E208] Potential Issue Warning: tests/warn/i12460.scala:5:37 -------------------------------------------------------- 5 |extension (using String)(s: String = "hello, world") def revert = s.reverse.toUpperCase // warn @@ -15,6 +17,8 @@ |--------------------------------------------------------------------------------------------------------------------- | Explanation (enabled by `-explain`) |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - | Although extensions are ordinary methods, they must be invoked as a selection. - | Therefore, the receiver cannot be omitted. A default argument for that parameter would never be used. + | The receiver cannot be omitted when an extension method is invoked as a selection. + | A default argument for that parameter would never be used in that case. + | An extension method can be invoked as a regular method, but if that is the intended usage, + | it should not be defined as an extension. --------------------------------------------------------------------------------------------------------------------- From 3816ded1deaba4dd76e08cf75bd09a271c811557 Mon Sep 17 00:00:00 2001 From: Som Snytt Date: Tue, 18 Feb 2025 11:23:53 -0800 Subject: [PATCH 5/5] Revert prettification --- compiler/src/dotty/tools/dotc/ast/Desugar.scala | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/Desugar.scala b/compiler/src/dotty/tools/dotc/ast/Desugar.scala index 51f64e67f9a9..440664fd9f0a 100644 --- a/compiler/src/dotty/tools/dotc/ast/Desugar.scala +++ b/compiler/src/dotty/tools/dotc/ast/Desugar.scala @@ -393,14 +393,18 @@ object desugar { rhs cpy.TypeDef(tparam)(rhs = dropInRhs(tparam.rhs)) - def paramssNoRHS = mapParamss(meth.paramss)(identity): vparam => - if vparam.rhs.isEmpty then vparam - else cpy.ValDef(vparam)(rhs = EmptyTree).withMods(vparam.mods | HasDefault) + def paramssNoRHS = mapParamss(meth.paramss)(identity) { + vparam => + if vparam.rhs.isEmpty then vparam + else cpy.ValDef(vparam)(rhs = EmptyTree).withMods(vparam.mods | HasDefault) + } def getterParamss(n: Int): List[ParamClause] = - mapParamss(takeUpTo(paramssNoRHS, n)) - (tparam => dropContextBounds(toMethParam(tparam, KeepAnnotations.All))) - (vparam => toMethParam(vparam, KeepAnnotations.All, keepDefault = false)) + mapParamss(takeUpTo(paramssNoRHS, n)) { + tparam => dropContextBounds(toMethParam(tparam, KeepAnnotations.All)) + } { + vparam => toMethParam(vparam, KeepAnnotations.All, keepDefault = false) + } def defaultGetters(paramss: List[ParamClause], n: Int): List[DefDef] = paramss match case ValDefs(vparam :: vparams) :: paramss1 =>