Skip to content

Fix #9772: Normalize value definitions #9836

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

Merged
merged 4 commits into from
Oct 15, 2020
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
41 changes: 27 additions & 14 deletions compiler/src/dotty/tools/dotc/ast/Desugar.scala
Original file line number Diff line number Diff line change
Expand Up @@ -147,24 +147,32 @@ object desugar {

// ----- Desugar methods -------------------------------------------------

/** Setter generation is needed for:
* - non-private class members
* - all trait members
* - all package object members
*/
def isSetterNeeded(valDef: ValDef)(using Context): Boolean = {
val mods = valDef.mods
mods.is(Mutable)
&& ctx.owner.isClass
&& (!mods.is(Private) || ctx.owner.is(Trait) || ctx.owner.isPackageObject)
}

/** var x: Int = expr
* ==>
* def x: Int = expr
* def x_=($1: <TypeTree()>): Unit = ()
*
* Generate the setter only for
* - non-private class members
* - all trait members
* - all package object members
* Generate setter where needed
*/
def valDef(vdef0: ValDef)(using Context): Tree = {
val vdef @ ValDef(name, tpt, rhs) = vdef0
val vdef @ ValDef(_, tpt, rhs) = vdef0
val mods = vdef.mods
val setterNeeded =
mods.is(Mutable)
&& ctx.owner.isClass
&& (!mods.is(Private) || ctx.owner.is(Trait) || ctx.owner.isPackageObject)
if (setterNeeded) {

val valName = normalizeName(vdef, tpt).asTermName
Copy link
Member

@bishabosha bishabosha Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps isSetterNeeded could be a default parameter for normalizeName to avoid calling it twice, and then swap the conditions in the Valdef branch (case vdef: ValDef if isSetterNeeded && name.isExtension), but if the performance isn't impacted that much this is minor

Copy link
Contributor Author

@tudorv91 tudorv91 Oct 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I considered that as well, but decided against it in order not to particularize the arg list of normalizeName with a param that is relevant to one branch. The second call would happen only if the valDef's name is extension, so I would expect it to be highly exceptional


if (isSetterNeeded(vdef)) {
// TODO: copy of vdef as getter needed?
// val getter = ValDef(mods, name, tpt, rhs) withPos vdef.pos?
// right now vdef maps via expandedTree to a thicket which concerns itself.
Expand All @@ -173,7 +181,7 @@ object desugar {
// The rhs gets filled in later, when field is generated and getter has parameters (see Memoize miniphase)
val setterRhs = if (vdef.rhs.isEmpty) EmptyTree else unitLiteral
val setter = cpy.DefDef(vdef)(
name = name.setterName,
name = valName.setterName,
tparams = Nil,
vparamss = (setterParam :: Nil) :: Nil,
tpt = TypeTree(defn.UnitType),
Expand Down Expand Up @@ -887,7 +895,7 @@ object desugar {
mdef.tparams.head.srcPos)
defDef(
cpy.DefDef(mdef)(
name = mdef.name.toExtensionName,
name = normalizeName(mdef, ext).toExtensionName,
tparams = ext.tparams ++ mdef.tparams,
vparamss = mdef.vparamss match
case vparams1 :: vparamss1 if mdef.name.isRightAssocOperatorName =>
Expand Down Expand Up @@ -934,8 +942,13 @@ object desugar {
report.error(IllegalRedefinitionOfStandardKind(kind, name), errPos)
name = name.errorName
}
if name.isExtensionName && (!mdef.mods.is(ExtensionMethod) || name.dropExtension.isExtensionName) then
report.error(em"illegal method name: $name may not start with `extension_`", errPos)
mdef match {
case vdef: ValDef if name.isExtension && isSetterNeeded(vdef) =>
report.error(em"illegal setter name: `extension_=`", errPos)
case memDef if name.isExtensionName && !mdef.mods.is(ExtensionMethod) =>
report.error(em"illegal name: $name may not start with `extension_`", errPos)
case _ =>
}
name
}

Expand Down
6 changes: 6 additions & 0 deletions compiler/src/dotty/tools/dotc/core/NameOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,12 @@ object NameOps {
else name.toTermName
}

/** Does the name match `extension`? */
def isExtension: Boolean = name match
case name: SimpleName =>
name.length == "extension".length && name.startsWith("extension")
case _ => false

/** Does this name start with `extension_`? */
def isExtensionName: Boolean = name match
case name: SimpleName => name.startsWith("extension_")
Expand Down
20 changes: 20 additions & 0 deletions tests/neg/illegal-extension.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
-- Error: tests/neg/illegal-extension.scala:2:6 ------------------------------------------------------------------------
2 | def extension_n: String = "illegal method" // error: illegal name: extension_n may not start with `extension_`
| ^^^^^^^^^^^
| illegal name: extension_n may not start with `extension_`
-- Error: tests/neg/illegal-extension.scala:4:6 ------------------------------------------------------------------------
4 | val extension_val = 23 // error: illegal name: extension_val may not start with `extension_`
| ^^^^^^^^^^^^^
| illegal name: extension_val may not start with `extension_`
-- Error: tests/neg/illegal-extension.scala:5:14 -----------------------------------------------------------------------
5 | private var extension = Nil // error: illegal setter name: `extension_=`
| ^^^^^^^^^
| illegal setter name: `extension_=`
-- Error: tests/neg/illegal-extension.scala:16:23 ----------------------------------------------------------------------
16 |extension (x: Any) def extension_foo: String = "foo" // error: illegal name: extension_foo may not start with `extension_`
| ^^^^^^^^^^^^^
| illegal name: extension_foo may not start with `extension_`
-- Error: tests/neg/illegal-extension.scala:9:6 ------------------------------------------------------------------------
9 | var extension = 1337 // error: illegal setter name: `extension_=`
| ^^^^^^^^^
| illegal setter name: `extension_=`
19 changes: 17 additions & 2 deletions tests/neg/illegal-extension.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,20 @@
trait A {
def extension_n: String = "illegal method" // error: illegal method name: extension_n may not start with `extension_`
def extension_n: String = "illegal method" // error: illegal name: extension_n may not start with `extension_`
type extension_type = Int // allowed because it's a type alias
val extension_val = 23 // error: illegal name: extension_val may not start with `extension_`
private var extension = Nil // error: illegal setter name: `extension_=`
}

extension (x: Any) def extension_foo: String = "foo" // error: illegal method name: extension_foo may not start with `extension_`
class B {
var extension = 1337 // error: illegal setter name: `extension_=`
}

class C {
private var extension = "OK" // allowed because it does not require a setter
}

extension (x: Any) def extension_foo: String = "foo" // error: illegal name: extension_foo may not start with `extension_`
extension (some: Any) def valid_extension_name: String = {
var extension = "foo" // `extension` name allowed because it doesn't require a setter
s"$extension bar"
}