Skip to content

Fix #3917: Properly desugar Ident #3925

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 3 commits into from
Jan 30, 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
21 changes: 3 additions & 18 deletions compiler/sjs/backend/sjs/JSCodeGen.scala
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,7 @@ class JSCodeGen()(implicit ctx: Context) {
throw new FatalError(s"Assignment to static member ${sym.fullName} not supported")
val genRhs = genExpr(rhs)
val lhs = lhs0 match {
case lhs: Ident => desugarIdent(lhs).getOrElse(lhs)
case lhs: Ident => desugarIdent(lhs)
case lhs => lhs
}
lhs match {
Expand Down Expand Up @@ -850,21 +850,6 @@ class JSCodeGen()(implicit ctx: Context) {
}
} // end of genStatOrExpr()

// !!! DUPLICATE code with DottyBackendInterface
private def desugarIdent(i: Ident): Option[Select] = {
i.tpe match {
case TermRef(prefix: TermRef, name) =>
Some(tpd.ref(prefix).select(i.symbol))
case TermRef(prefix: ThisType, name) =>
Some(tpd.This(prefix.cls).select(i.symbol))
/*case TermRef(NoPrefix, name) =>
if (i.symbol is Method) Some(This(i.symbol.topLevelClass).select(i.symbol)) // workaround #342 todo: remove after fixed
else None*/
case _ =>
None
}
}

private def qualifierOf(fun: Tree): Tree = fun match {
case fun: Ident =>
fun.tpe match {
Expand Down Expand Up @@ -907,7 +892,7 @@ class JSCodeGen()(implicit ctx: Context) {
val sym = tree.fun.symbol

val fun = tree.fun match {
case fun: Ident => desugarIdent(fun).getOrElse(fun)
case fun: Ident => desugarIdent(fun)
case fun => fun
}

Expand Down Expand Up @@ -1571,7 +1556,7 @@ class JSCodeGen()(implicit ctx: Context) {
implicit val pos = tree.pos

val fun = tree.fun match {
case fun: Ident => desugarIdent(fun).get
case fun: Ident => desugarIdent(fun).asInstanceOf[Select]
case fun: Select => fun
}
val receiver = fun.qualifier
Expand Down
12 changes: 4 additions & 8 deletions compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala
Original file line number Diff line number Diff line change
Expand Up @@ -444,16 +444,12 @@ class DottyBackendInterface(outputDirectory: AbstractFile, val superCallsMap: Ma
def desugarIdent(i: Ident): Option[tpd.Select] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not get rid of the Option here since the backend expects this signature

var found = desugared.get(i.tpe)
if (found == null) {
i.tpe match {
case TermRef(prefix: TermRef, _) =>
found = tpd.ref(prefix).select(i.symbol)
case TermRef(prefix: ThisType, _) =>
found = tpd.This(prefix.cls).select(i.symbol)
case TermRef(NoPrefix, _) =>
if (i.symbol is Flags.Method) found = This(i.symbol.topLevelClass).select(i.symbol) // workaround #342 todo: remove after fixed
tpd.desugarIdent(i) match {
case sel: tpd.Select =>
desugared.put(i.tpe, sel)
found = sel
case _ =>
}
if (found != null) desugared.put(i.tpe, found)
}
if (found == null) None else Some(found)
}
Expand Down
30 changes: 23 additions & 7 deletions compiler/src/dotty/tools/dotc/ast/tpd.scala
Original file line number Diff line number Diff line change
Expand Up @@ -826,14 +826,13 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
*/
def becomes(rhs: Tree)(implicit ctx: Context): Tree =
if (tree.symbol is Method) {
val setr = tree match {
case Ident(_) =>
val setter = tree.symbol.setter
assert(setter.exists, tree.symbol.showLocated)
ref(tree.symbol.setter)
case Select(qual, _) => qual.select(tree.symbol.setter)
val setter = tree.symbol.setter
assert(setter.exists, tree.symbol.showLocated)
val qual = tree match {
case id: Ident => desugarIdentPrefix(id)
case Select(qual, _) => qual
}
setr.appliedTo(rhs)
qual.select(setter).appliedTo(rhs)
}
else Assign(tree, rhs)

Expand Down Expand Up @@ -1012,5 +1011,22 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
val encoding = ctx.settings.encoding.value
if (file != null && file.exists) new SourceFile(file, Codec(encoding)) else NoSource
}

/** Desugar identifier into a select node. Return the tree itself if not possible */
def desugarIdent(tree: Ident)(implicit ctx: Context): Tree = {
val qual = desugarIdentPrefix(tree)
if (qual.isEmpty) tree
else qual.select(tree.symbol)
}

/** Recover identifier prefix (e.g. this) if it exists */
def desugarIdentPrefix(tree: Ident)(implicit ctx: Context): Tree = tree.tpe match {
case TermRef(prefix: TermRef, _) =>
ref(prefix)
case TermRef(prefix: ThisType, _) =>
This(prefix.cls)
case _ =>
EmptyTree
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import core.Symbols._
import core.Types._
import typer.ConstFold
import ast.Trees._
import Simplify.desugarIdent

/** Various constant folding.
*
Expand Down Expand Up @@ -182,22 +181,19 @@ import Simplify.desugarIdent
case _ => false
}
case t1: Ident =>
desugarIdent(t1) match {
case Some(t) =>
val t2i = t2 match {
case t2: Ident => desugarIdent(t2).getOrElse(t2)
case _ => t2
}
isSimilar(t, t2i)
case None => t1.symbol eq t2.symbol
Copy link
Contributor Author

@allanrenucci allanrenucci Jan 30, 2018

Choose a reason for hiding this comment

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

I think the logic was wrong here. If t2 is a select then it is not enough to compare the symbols

t2 match {
case t2: Ident =>
t1.symbol eq t2.symbol
case _ => // Select
isSimilar(t2, t1)
}
case t1: Select => t2 match {
case t2: Select =>
(t1.symbol eq t2.symbol) &&
isSimilar(t1.qualifier, t2.qualifier)
case t2: Ident => desugarIdent(t2) match {
case Some(t2) => isSimilar(t1, t2)
case None => false
case t2: Select => isSimilar(t1, t2)
case _ => false
}
case _ => false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ class Devalify extends Optimisation {
readingOnlyVals(qual)

case t: Ident if !t.symbol.is(Mutable | Method) && !t.symbol.info.dealias.isInstanceOf[ExprType] =>
desugarIdent(t).forall(readingOnlyVals)
desugarIdent(t) match { case s: Select => readingOnlyVals(s); case _ => true }

case t: This => true
// null => false, or the following fails devalify:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,9 @@ class DropNoEffects(val simplifyPhase: Simplify) extends Optimisation {
cpy.Block(bl)(stats2, expr2)

case t: Ident if !t.symbol.is(Method | Lazy) && !t.symbol.info.isInstanceOf[ExprType] || effectsDontEscape(t) =>
desugarIdent(t) match {
case Some(t) if !(t.qualifier.symbol.is(JavaDefined) && t.qualifier.symbol.is(Package)) => t
case _ => EmptyTree
}
val prefix = desugarIdentPrefix(t)
if (!prefix.isEmpty && !prefix.symbol.is(allOf(JavaDefined, Package))) t
else EmptyTree

case app: Apply if app.fun.symbol.is(Label) && !app.tpe.finalResultType.derivesFrom(defn.UnitClass) =>
// This is "the scary hack". It changes the return type to Unit, then
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import core.Flags._
import core.TypeApplications.noBounds
import ast.Trees._
import transform.SymUtils._
import Simplify.desugarIdent
import dotty.tools.dotc.ast.tpd

/** Inline case class specific methods using desugarings assumptions.
Expand Down Expand Up @@ -109,10 +108,9 @@ class InlineCaseIntrinsics(val simplifyPhase: Simplify) extends Optimisation {
def receiver(t: Tree): Type = t match {
case t: Apply => receiver(t.fun)
case t: TypeApply => receiver(t.fun)
case t: Ident => desugarIdent(t) match {
case Some(t) => receiver(t)
case _ => NoType
}
case t: Ident =>
val prefix = desugarIdentPrefix(t)
prefix.tpe.widenDealias
case t: Select => t.qualifier.tpe.widenDealias
}

Expand Down
20 changes: 3 additions & 17 deletions compiler/src/dotty/tools/dotc/transform/localopt/Simplify.scala
Original file line number Diff line number Diff line change
Expand Up @@ -149,29 +149,15 @@ class Simplify extends MiniPhase with IdentityDenotTransformer {

object Simplify {
import tpd._
// TODO: This function is duplicated in jvm/DottyBackendInterface.scala, let's factor these out!
def desugarIdent(i: Ident)(implicit ctx: Context): Option[Select] = {
i.tpe match {
case TermRef(prefix: TermRef, _) =>
Some(ref(prefix).select(i.symbol))
case TermRef(prefix: ThisType, _) =>
Some(This(prefix.cls).select(i.symbol))
case _ => None
}
}

/** Is this tree mutable, or java.lang.System.{in, out, err}? These three
* System members are the only static final fields that are mutable.
* See https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5.4
*/
@tailrec def isEffectivelyMutable(t: Tree)(implicit ctx: Context): Boolean = t match {
def isEffectivelyMutable(t: Tree)(implicit ctx: Context): Boolean = t match {
case _ if t.symbol.is(Mutable) => true
case s: Select => s.symbol.owner == defn.SystemModule
case i: Ident =>
desugarIdent(i) match {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not understand why desugaring was needed here because it doesn't change the symbol of the tree

case Some(ident) => isEffectivelyMutable(ident)
case None => false
}
case _: Select | _: Ident =>
t.symbol.owner == defn.SystemModule
case _ => false
}

Expand Down
27 changes: 27 additions & 0 deletions tests/pos/i3917.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
class A {
var a = false
}

object B {
var b = false
}

class C {
var c = false
}

object C extends A {
def test = {
a = true
C.a = true
this.a = true
C.this.a = true

import B._
b = true

val c0 = new C
import c0._
c = true
}
}