Skip to content

Fix == for value classes #4490

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 8 commits into from
Jun 28, 2018
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
36 changes: 22 additions & 14 deletions compiler/src/dotty/tools/dotc/transform/AccessProxies.scala
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ abstract class AccessProxies {
if (passReceiverAsArg(accessor.name))
(argss.head.head.select(accessed), tps.takeRight(numTypeParams), argss.tail)
else
(ref(TermRef(cls.thisType, accessed)), tps, argss)
(if (accessed.isStatic) ref(accessed) else ref(TermRef(cls.thisType, accessed)),
tps, argss)
val rhs =
if (accessor.name.isSetterName &&
forwardedArgss.nonEmpty && forwardedArgss.head.nonEmpty) // defensive conditions
Expand All @@ -70,6 +71,11 @@ abstract class AccessProxies {
def accessorNameKind: ClassifiedNameKind
def needsAccessor(sym: Symbol)(implicit ctx: Context): Boolean

def ifNoHost(reference: RefTree)(implicit ctx: Context): Tree = {
assert(false, "no host found for $reference with ${reference.symbol.showLocated} from ${ctx.owner}")
reference
}

/** A fresh accessor symbol */
private def newAccessorSymbol(owner: Symbol, name: TermName, info: Type, pos: Position)(implicit ctx: Context): TermSymbol = {
val sym = ctx.newSymbol(owner, name, Synthetic | Method, info, coord = pos).entered
Expand Down Expand Up @@ -125,18 +131,14 @@ abstract class AccessProxies {
def useAccessor(reference: RefTree)(implicit ctx: Context): Tree = {
val accessed = reference.symbol.asTerm
var accessorClass = hostForAccessorOf(accessed: Symbol)
if (!accessorClass.exists) {
val curCls = ctx.owner.enclosingClass
transforms.println(i"${curCls.ownersIterator.toList}%, %")
ctx.error(i"illegal access to protected ${accessed.showLocated} from $curCls",
reference.pos)
accessorClass = curCls
if (accessorClass.exists) {
val accessorName = accessorNameKind(accessed.name)
val accessorInfo =
accessed.info.ensureMethodic.asSeenFrom(accessorClass.thisType, accessed.owner)
val accessor = accessorSymbol(accessorClass, accessorName, accessorInfo, accessed)
rewire(reference, accessor)
}
val accessorName = accessorNameKind(accessed.name)
val accessorInfo =
accessed.info.ensureMethodic.asSeenFrom(accessorClass.thisType, accessed.owner)
val accessor = accessorSymbol(accessorClass, accessorName, accessorInfo, accessed)
rewire(reference, accessor)
else ifNoHost(reference)
}

/** Replace tree with a reference to an accessor if needed */
Expand All @@ -156,6 +158,12 @@ object AccessProxies {
/** Where an accessor for the `accessed` symbol should be placed.
* This is the closest enclosing class that has `accessed` as a member.
*/
def hostForAccessorOf(accessed: Symbol)(implicit ctx: Context): Symbol =
ctx.owner.ownersIterator.findSymbol(_.derivesFrom(accessed.owner))
def hostForAccessorOf(accessed: Symbol)(implicit ctx: Context): Symbol = {
def recur(cls: Symbol): Symbol =
if (!cls.exists) NoSymbol
else if (cls.derivesFrom(accessed.owner)) cls
else if (cls.companionModule.moduleClass == accessed.owner) accessed.owner
else recur(cls.owner)
recur(ctx.owner)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import core.Flags._
import core.Decorators._
import MegaPhase.MiniPhase
import ast.Trees._
import config.Printers.transforms

/** Add accessors for all protected accesses. An accessor is needed if
* according to the rules of the JVM a protected class member is not accesissible
Expand Down Expand Up @@ -56,6 +57,14 @@ class ProtectedAccessors extends MiniPhase {
val insert = new Insert {
def accessorNameKind = ProtectedAccessorName
def needsAccessor(sym: Symbol)(implicit ctx: Context) = ProtectedAccessors.needsAccessor(sym)

override def ifNoHost(reference: RefTree)(implicit ctx: Context): Tree = {
val curCls = ctx.owner.enclosingClass
transforms.println(i"${curCls.ownersIterator.toList}%, %")
ctx.error(i"illegal access to protected ${reference.symbol.showLocated} from $curCls",
reference.pos)
reference
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import ExtensionMethods._, TreeExtractors._, ValueClasses._
* For a value class V defined as:
* class V(val underlying: U) extends AnyVal
* we avoid unnecessary allocations:
* new V(u1) == new V(u2) => u1 == u2
* new V(u1) == new V(u2) => u1 == u2 provided V does not redefine `equals`
* (new V(u)).underlying() => u
*/
class VCElideAllocations extends MiniPhase with IdentityDenotTransformer {
Expand All @@ -27,7 +27,9 @@ class VCElideAllocations extends MiniPhase with IdentityDenotTransformer {
// new V(u1) == new V(u2) => u1 == u2
// (We don't handle != because it has been eliminated by InterceptedMethods)
case BinaryOp(NewWithArgs(tp1, List(u1)), op, NewWithArgs(tp2, List(u2)))
if (tp1 eq tp2) && (op eq defn.Any_==) && isDerivedValueClass(tp1.typeSymbol) =>
if (tp1 eq tp2) && (op eq defn.Any_==) &&
isDerivedValueClass(tp1.typeSymbol) &&
!defn.Any_equals.overridingSymbol(tp1.typeSymbol.asClass).exists =>
// == is overloaded in primitive classes
u1.equal(u2)

Expand Down
8 changes: 4 additions & 4 deletions compiler/src/dotty/tools/dotc/typer/Inliner.scala
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ object Inliner {
ctx.error("Implementation restriction: cannot use private constructors in inline methods", tree.pos)
tree // TODO: create a proper accessor for the private constructor
}
else if (AccessProxies.hostForAccessorOf(tree.symbol).exists) useAccessor(tree)
else tree
else useAccessor(tree)
case _ =>
tree
}
override def ifNoHost(reference: RefTree)(implicit ctx: Context): Tree = reference
}

/** Fallback approach if the direct approach does not work: Place the accessor method
Expand Down Expand Up @@ -124,7 +124,7 @@ object Inliner {
if needsAccessor(tree.symbol) && tree.isTerm && !tree.symbol.isConstructor =>
val (refPart, targs, argss) = decomposeCall(tree)
val qual = qualifier(refPart)
inlining.println(i"adding receiver passing inline accessor for $tree -> (${qual.tpe}, $refPart: ${refPart.getClass}, [$targs%, %], ($argss%, %))")
println(i"adding receiver passing inline accessor for $tree/$refPart -> (${qual.tpe}, $refPart: ${refPart.getClass}, [$targs%, %], ($argss%, %))")

// Need to dealias in order to cagtch all possible references to abstracted over types in
// substitutions
Expand All @@ -142,7 +142,7 @@ object Inliner {
def addQualType(tp: Type): Type = tp match {
case tp: PolyType => tp.derivedLambdaType(tp.paramNames, tp.paramInfos, addQualType(tp.resultType))
case tp: ExprType => addQualType(tp.resultType)
case tp => MethodType(qualType :: Nil, tp)
case tp => MethodType(qualType.simplified :: Nil, tp)
}

// Abstract accessed type over local refs
Expand Down
2 changes: 2 additions & 0 deletions compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import core._
import Types._, Contexts._, Flags._, Symbols._, Annotations._, Trees._, NameOps._
import Decorators._
import Variances._
import NameKinds._
import util.Positions._
import rewrite.Rewrites.patch
import config.Printers.variances
Expand Down Expand Up @@ -134,6 +135,7 @@ class VarianceChecker()(implicit ctx: Context) {
def skip =
!sym.exists ||
sym.is(PrivateLocal) ||
sym.name.is(InlineAccessorName) || // TODO: should we exclude all synthetic members?
sym.is(TypeParam) && sym.owner.isClass // already taken care of in primary constructor of class
tree match {
case defn: MemberDef if skip =>
Expand Down
Loading