Skip to content

Fix #2924: make TreeChecker.typedBlock iterative #3080

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.

Sign up for GitHub

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 5 commits into from
Sep 11, 2017
Merged

Fix #2924: make TreeChecker.typedBlock iterative #3080

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
60591f8
Fix #2924: make TreeChecker.typedBlock iterative
nicolasstucki Sep 6, 2017
82196a5
Make FirstTransform.reorder semi-tailrec
nicolasstucki Sep 7, 2017
02f2688
Improve FirstTransform reorder.
nicolasstucki Sep 8, 2017
7a3146a
Make withDefinedSyms iterative
nicolasstucki Sep 8, 2017
20391ec
Rewrite fold
nicolasstucki Sep 9, 2017
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
27 changes: 14 additions & 13 deletions compiler/src/dotty/tools/dotc/transform/FirstTransform.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,26 +92,27 @@ class FirstTransform extends MiniPhaseTransform with InfoTransformer with Annota
private def reorderAndComplete(stats: List[Tree])(implicit ctx: Context): List[Tree] = {
val moduleClassDefs, singleClassDefs = mutable.Map[Name, Tree]()

def reorder(stats: List[Tree]): List[Tree] = stats match {
def reorder(stats: List[Tree], revPrefix: List[Tree] = Nil): List[Tree] = stats match {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a rather complex algorithm. Whenever I see reversed lists my head starts to hurt. Can we simplify it using ListBuffers? Also, if we keep it we need more docs to explain it.

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify: I believe the original reorder algorithm was already close to the limit of tolerable complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking more about it, I believe the revPrefix idea is the best option. So OK to leave the code as is. Just document what revPrefix stands for. I.e. at any point:

reordered statements = revPrefix.reverse ++ reorder(stats, Nil)

Also, I'd not make revPrefix have a default parameter. To easy to make errors that way.

case (stat: TypeDef) :: stats1 if stat.symbol.isClass =>
if (stat.symbol is Flags.Module) {
moduleClassDefs += (stat.name -> stat)
singleClassDefs -= stat.name.stripModuleClassSuffix
val stats1r = reorder(stats1)
if (moduleClassDefs contains stat.name) stat :: stats1r else stats1r
revPrefix.reverse ::: (if (moduleClassDefs contains stat.name) stat :: stats1r else stats1r)
} else {
def stats1r = reorder(stats1)
val normalized = moduleClassDefs remove stat.name.moduleClassName match {
case Some(mcdef) =>
mcdef :: stats1r
case None =>
singleClassDefs += (stat.name -> stat)
stats1r
}
stat :: normalized
reorder(
stats1,
moduleClassDefs remove stat.name.moduleClassName match {
case Some(mcdef) =>
mcdef :: stat :: revPrefix
case None =>
singleClassDefs += (stat.name -> stat)
stat :: revPrefix
}
)
}
case stat :: stats1 => stat :: reorder(stats1)
case Nil => Nil
case stat :: stats1 => reorder(stats1, stat :: revPrefix)
case Nil => revPrefix.reverse
}

def registerCompanion(name: TermName, forClass: Symbol): TermSymbol = {
Expand Down
77 changes: 49 additions & 28 deletions compiler/src/dotty/tools/dotc/transform/TreeChecker.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,34 +149,42 @@ class TreeChecker extends Phase with SymTransformer {

def withDefinedSym[T](tree: untpd.Tree)(op: => T)(implicit ctx: Context): T = tree match {
case tree: untpd.DefTree =>
val sym = tree.symbol
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the original logic was complicated - that foldRightBN is sneaky because it mixes laziness with side-effects ! The changes remove the foldRightBN complexity but make everything else more complex. Proposal: make withDefinedSyms take a repeated argument and remove withDefinedSym. I.e.

   def withDefinedSyms[T](trees: untpd.Tree*)(op: => T)(implicit ctx: Context)

withDefinedSyms can step iteratively through all the trees, record the symbols found as defined in a ListBuffer or List and then execute op.

assert(isValidJVMName(sym.name.encode), s"${sym.name.debugString} name is invalid on jvm")
everDefinedSyms.get(sym) match {
case Some(t) =>
if (t ne tree)
ctx.warning(i"symbol ${sym.fullName} is defined at least twice in different parts of AST")
// should become an error
case None =>
everDefinedSyms(sym) = tree
}
assert(!nowDefinedSyms.contains(sym), i"doubly defined symbol: ${sym.fullName} in $tree")

if (ctx.settings.YcheckMods.value) {
tree match {
case t: untpd.MemberDef =>
if (t.name ne sym.name) ctx.warning(s"symbol ${sym.fullName} name doesn't correspond to AST: ${t}")
// todo: compare trees inside annotations
case _ =>
}
preWithDefinedSym(tree)
postWithDefinedSym(tree)(op)
case _ => op
}

private def preWithDefinedSym[T](tree: untpd.DefTree)(implicit ctx: Context): Unit = {
val sym = tree.symbol
assert(isValidJVMName(sym.name.encode), s"${sym.name.debugString} name is invalid on jvm")
everDefinedSyms.get(sym) match {
case Some(t) =>
if (t ne tree)
ctx.warning(i"symbol ${sym.fullName} is defined at least twice in different parts of AST")
// should become an error
case None =>
everDefinedSyms(sym) = tree
}
assert(!nowDefinedSyms.contains(sym), i"doubly defined symbol: ${sym.fullName} in $tree")

if (ctx.settings.YcheckMods.value) {
tree match {
case t: untpd.MemberDef =>
if (t.name ne sym.name) ctx.warning(s"symbol ${sym.fullName} name doesn't correspond to AST: ${t}")
// todo: compare trees inside annotations
case _ =>
}
}
}

nowDefinedSyms += tree.symbol
//ctx.echo(i"defined: ${tree.symbol}")
val res = op
nowDefinedSyms -= tree.symbol
//ctx.echo(i"undefined: ${tree.symbol}")
res
case _ => op
private def postWithDefinedSym[T](tree: untpd.DefTree)(op: => T)(implicit ctx: Context): T = {
val sym = tree.symbol
nowDefinedSyms += sym
//ctx.echo(i"defined: ${tree.symbol}")
val res = op
nowDefinedSyms -= sym
//ctx.echo(i"undefined: ${tree.symbol}")
res
}

def withDefinedSyms[T](trees: List[untpd.Tree])(op: => T)(implicit ctx: Context) =
Expand Down Expand Up @@ -411,8 +419,21 @@ class TreeChecker extends Phase with SymTransformer {
}
}

override def typedBlock(tree: untpd.Block, pt: Type)(implicit ctx: Context) =
withDefinedSyms(tree.stats) { super.typedBlock(tree, pt) }
override def typedBlock(tree: untpd.Block, pt: Type)(implicit ctx: Context) = {
var locally = List.empty[Symbol]
for (stat <- tree.stats) {
stat match {
case stat: untpd.DefTree =>
preWithDefinedSym(stat)
locally = stat.symbol :: locally
nowDefinedSyms += stat.symbol
case _ =>
}
}
val res = super.typedBlock(tree, pt)
nowDefinedSyms --= locally
res
}

override def typedInlined(tree: untpd.Inlined, pt: Type)(implicit ctx: Context) =
withDefinedSyms(tree.bindings) { super.typedInlined(tree, pt) }
Expand Down
Loading