getting rid of LabelDef

[Adriaan Moors]

Hi,

(I'm just reporting, I don't intend to act on this before 2.10.1.)

To skip to the questions hidden in this message, see the (??)

I propose to excise LabelDef. It's horribly broken and may burst at any time.

labels can't reliably be re-typechecked (even just type checking them is icky: have a look at typedLabelDef..),

nor duplicated, since they don't have ValDefs for their arguments. 

I think this is irregularity from other definitions is due to the following. There are two ways to execute the body of a labeldef:

  - control flow "flows into" it, which means we need to come up with values for its arguments

    this is why a labeldef's arguments aren't actually definitions: they don't bind new symbols,

    they indicate the symbols that should be used to initialize their arguments in this scenario

  - however, you can also jump to a label and specify values for its arguments,

    in this scenario, the arguments should be interpreted as binding symbols, and we should pass the supplied values

finally, control flow just "flows out" of a labeldef

DefDefs can do all this, but cleaner:

- replace a LabelDef by a DefDef (add the LABEL flag to its symbol) and call the method immediately after its definition

  (note that you must explicitly supply arguments instead of "borrowing" symbols from other defs)

- you must explicitly implement fall-through by calling the next label-method if desired (i.e., in the pattern matcher)

- in absence of a next-label-call, at the end of the method execution, control flow will return to where the method was called from;

  this is equivalent to jumping to the "next label" that is implicitly right after the original call to the method

  (I think this means the semantics are okay for use in the tailcalls transform, since all calls are by definition in tail position,

   so that returning to right after the call to the label-method will also return from the tailcall-transformed method)

- (??) transformations that rewrite method calls need to consider the jumpy semantics of label-methods

  are there any deal breakers here? they only transformation i can think of is tailcalls, which will have to be updated to the new scheme anyway

to get the same bytecode as we currently get from labeldefs and jumps to them:

- the backend should inline a method with flag LABEL at its first call-site (which must be right after the definition),

  and emit a label for use at subsequent call-sites

- never emit method definitions in the bytecode for label-methods (thus, they need not be lifted)

this would make life easier in:

- specialization and tree-duplication/retyping in general

- continuations (selectiveanf currently has a pretty broken idea of what labels do)

- anything that deals with trees, as there will now be one fewer case to consider

note that virtpatmat already emits labeldefs that handle fall-through explictly, while loops and tailcalls will work seamlessly

downsides/impact:

- as already mentioned: tailcalls, while loops

- (??) lambdalift should not lift label-methods (label-methods should always be nested in normal methods)

- (??) are other transformations affected?

cheers

adriaan

[Grzegorz Kossakowski]

On 13 April 2012 11:27, Adriaan Moors <adriaa...@epfl.ch> wrote:

Hi,

(I'm just reporting, I don't intend to act on this before 2.10.1.)

That's the only thing I don't agree with in this e-mail. Ship it in 2.10 please, please.

LabelDefs were a major hurdle for me when working Scala+GWT. I never could really get semantics of LabelDefs. After a lot of reverse-engineering I got to the point of understanding them the way you described. However, I never felt I won't be surprised by yet another 'fun' interaction between LabelDefs, symbols and rest of trees.

I don't know about things before cleanup but refactoring you describe would be fairly straightforward to do in the backend.

--
Grzegorz Kossakowski

[Paul Phillips]

On Fri, Apr 13, 2012 at 11:07 AM, Grzegorz Kossakowski
<grzegorz.k...@gmail.com> wrote:
> LabelDefs were a major hurdle for me when working Scala+GWT. I never could
> really get semantics of LabelDefs. After a lot of reverse-engineering I got
> to the point of understanding them the way you described. However, I never
> felt I won't be surprised by yet another 'fun' interaction between
> LabelDefs, symbols and rest of trees.

I can't even bear to consider how many of my "lost 1000 hours" in the
pattern matcher swirled around them. There's nothing quite like
trying to fix something which doesn't work right, where you have to
examine usages which are known to work correctly to know how these are
supposed to work, except there aren't any.

[Adriaan Moors]

On Fri, Apr 13, 2012 at 12:07 PM, Grzegorz Kossakowski <grzegorz.k...@gmail.com> wrote:

(I'm just reporting, I don't intend to act on this before 2.10.1.)

That's the only thing I don't agree with in this e-mail. Ship it in 2.10 please, please

It's not that I don't want to, but I'm afraid to think how much time I'll have to sink into making it work fully.

I'm way behind schedule as it is, partly due to how broken labeldef support is (I've had to work on specialize, uncurry, selectiveanf, tailcalls,... just to get those to work with the more "complicated" labeldef trees that virtpatmat generates).

adriaan

[martin odersky]

I agree that label defs are evil. Let's discuss around Scala Days about what time budget is needed to remove them, and when would be the logical time to do it. Since it is an internal change, it could foreseeably be done between Scala Days and the first RC, but it should not delay the RC significantly, and it should not add a lot of risk. 

Cheers

 - Martin

--
Martin Odersky
Prof., EPFL and Chairman, Typesafe
PSED, 1015 Lausanne, Switzerland
Tel. EPFL: +41 21 693 6863
Tel. Typesafe: +41 21 691 4967

[Jason Zaugg]

On Fri, Apr 13, 2012 at 5:56 PM, martin odersky <martin....@epfl.ch> wrote:
> I agree that label defs are evil. Let's discuss around Scala Days about what
> time budget is needed to remove them, and when would be the logical time to
> do it. Since it is an internal change, it could foreseeably be done between
> Scala Days and the first RC, but it should not delay the RC significantly,
> and it should not add a lot of risk.

A meta observation: if you were to leave that change to 2.11, you'd be
left in the awkward position of breaking the public API of Trees [1].

Perhaps you need to reserve the right to break this part of the API at
each major revision of the compiler, rather than going through a
deprecation cycle.

-jason

[1] https://github.com/scala/scala/blob/master/src/library/scala/reflect/api/Trees.scala#L438

[Daniel Sobral]

On Fri, Apr 13, 2012 at 13:05, Jason Zaugg <jza...@gmail.com> wrote:
> On Fri, Apr 13, 2012 at 5:56 PM, martin odersky <martin....@epfl.ch> wrote:
>> I agree that label defs are evil. Let's discuss around Scala Days about what
>> time budget is needed to remove them, and when would be the logical time to
>> do it. Since it is an internal change, it could foreseeably be done between
>> Scala Days and the first RC, but it should not delay the RC significantly,
>> and it should not add a lot of risk.
>
> A meta observation: if you were to leave that change to 2.11, you'd be
> left in the awkward position of breaking the public API of Trees [1].
>
> Perhaps you need to reserve the right to break this part of the API at
> each major revision of the compiler, rather than going through a
> deprecation cycle.

This occurred to me as well. Macros that do anything more than reify
code are bound to break much more often than normal code is. It's
unreasonable to expect _generated code_ (well, generated ASTs) to
remain stable even at the bug fix level.

Perhaps the expectations that macro writers can -- and cannot -- have
should be clearly enunciated.

--
Daniel C. Sobral

I travel to the future all the time.

[martin odersky]

That's a good point, and a reason for doing it now. 

We will have to set expectations regarding Tree API, but I fear that no matter what we do it will be pretty hard to change once we have published it.

Before we release 2.10, we should have a review of Trees and decide what else should be changed before we freeze.

Fortunately, Trees have been historically pretty stable. 

 - Martin

[Paul Phillips]

On Fri, Apr 13, 2012 at 6:16 PM, martin odersky <martin....@epfl.ch> wrote:
> We will have to set expectations regarding Tree API, but I fear that no
> matter what we do it will be pretty hard to change once we have published
> it.
>
> Before we release 2.10, we should have a review of Trees and decide what
> else should be changed before we freeze.
>
> Fortunately, Trees have been historically pretty stable.

I'm reasonably happy with Trees, happy for me anyway. But I always have a list.

// Weird "tree-like" not-Trees which always demand special handling
// and could be better integrated

case class ImportSelector(name: Name, namePos: Int, rename: Name,
renamePos: Int)
case class Modifiers(flags: Long, privateWithin: Name, annotations: List[Tree])
sealed abstract class AnnotationInfo // and its buddies, *AnnotArg etc.

// Trees which encode too many disparate things

ValDef: parameters, vals, vars, fields, self-types, ...

// Trees which are clunky

// it being divorced from constructor arguments and desugared so early
// is a constant source of pain and confusion
case class New(tpt: Tree) extends TermTree

// Given that there's a whole dedicated node called "UnApply", why do
// I have to use this ungodly structure to "UnApply" things.
case class UnApply(fun: Tree, args: List[Tree]) extends TermTree

object UnapplyPattern {
private object UnapplySeq {
def unapply(x: UnApply) = x match {
case UnApply(
Apply(TypeApply(Select(qual, nme.unapplySeq), List(tpt)), _),
List(ArrayValue(_, elems))) =>
Some((qual.symbol, tpt, elems))
case _ =>
None
}
}

def apply(x: UnApply): Pattern = x match {
case UnapplySeq(ListModule, tpt, elems) =>
ListExtractorPattern(x, tpt, elems)
case _ =>
ExtractorPattern(x)
}
}

// trees which feel hacky or which I think should be justified
case class Star(elem: Tree)
case class AssignOrNamedArg(lhs: Tree, rhs: Tree) extends TermTree
case class ReferenceToBoxed(ident: Ident) extends TermTree

// trees which could have clearer names

// How about "TypeRepresentation" or something, so we don't
// have to say "Not to be confused with 'TypeTree'..."
trait TypTree extends Tree

// It would be nice to have names for TypeApply and AppliedTypeTree
// which gave one a better chance to intuit the distinction.
case class AppliedTypeTree(tpt: Tree, args: List[Tree])
case class TypeApply(fun: Tree, args: List[Tree])

// This tree sounds less like what it is with each passing day
case class ApplyDynamic(qual: Tree, args: List[Tree])

// On the compiler side, but... I hope "TypeTreeWithDeferredRefCheck"
// is with us always, so when the right day arrives I can roll out
// "HackTreeContainingMysteryClosure" in good conscience.
case class TypeTreeWithDeferredRefCheck()(val check: () => TypeTree)
extends TypTree

[Mirko Stocker]

On Sat, Apr 14, 2012 at 09:35, Paul Phillips <pa...@improving.org> wrote:
> case class ImportSelector(name: Name, namePos: Int, rename: Name,
> renamePos: Int)
> case class Modifiers(flags: Long, privateWithin: Name, annotations: List[Tree])

Yes, please! In scala-refactoring, I internally have an
ImportSelectorTree and ModifierTree. I also have a NameTree, and
there's a SuperConstructorCall and SelfTypeTree but these are less
problematic, they just make a few things easier for me, but the
ImportSelectorTree would be great to have.

Cheers,

Mirko

--
Mirko Stocker | m...@misto.ch
Work: http://ifs.hsr.ch | http://infoq.com
Personal: http://misto.ch | http://twitter.com/m_st

[Paul Phillips]

On Sat, Apr 14, 2012 at 10:28 AM, Mirko Stocker <m...@misto.ch> wrote:
> I also have a NameTree

I keep meaning to add that. I did recently put qualifier on RefTree
so you can handle Ident and Select uniformly (on Ident, qualifier is
always EmptyTree.)

[Mirko Stocker]

On Sat, Apr 14, 2012 at 11:32, Paul Phillips <pa...@improving.org> wrote:
> On Sat, Apr 14, 2012 at 10:28 AM, Mirko Stocker <m...@misto.ch> wrote:
>> I also have a NameTree
>
> I keep meaning to add that.

For me, the biggest pain with names is finding their position in the
source code. If you're courageous, here's the code:

http://www.assembla.com/code/scala-refactoring/git/nodes/master/org.scala-refactoring.library/src/main/scala/scala/tools/refactoring/common/PimpedTrees.scala#ln144

> I did recently put qualifier on RefTree
> so you can handle Ident and Select uniformly (on Ident, qualifier is
> always EmptyTree.)

Cool, that could be useful.

[Paul Phillips]

On Sat, Apr 14, 2012 at 11:47 AM, Mirko Stocker <m...@misto.ch> wrote:
> For me, the biggest pain with names is finding their position in the
> source code. If you're courageous, here's the code:
>
> http://www.assembla.com/code/scala-refactoring/git/nodes/master/org.scala-refactoring.library/src/main/scala/scala/tools/refactoring/common/PimpedTrees.scala#ln144

Ha ha, don't you know I've already done about the same thing. At least twice.

In current master, grotesque code starts around here:

https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/interpreter/IMain.scala#L500

When I tried to fix the xml positions I broke the IDE and had to revert it:

commit 07fab88cee
Author: Paul Phillips <pa...@improving.org>
Date: 10 months ago

Reverts r25036, "Altered the positioning of XML...

Reverts r25036, "Altered the positioning of XML literal trees" because
the IDE did not like it. No review.

commit 92a2fd5397
Author: Paul Phillips <pa...@improving.org>
Date: 11 months ago

More adjustments to repl parsing to accomodate ...

More adjustments to repl parsing to accomodate inaccurately positioned
parse trees. No review.

commit 94e1965b64
Author: Paul Phillips <pa...@improving.org>
Date: 11 months ago

Altered the positioning of XML literal trees.

with the earliest position later than the earliest character in the
source code, which was breaking some repl logic. No review.

And then there is the substantial rewrite here:

https://github.com/paulp/scala/tree/topic/positions

Which takes the rather more appealing position (no pun intended) of
adding a "sourceCode" method to Position, so you can get the source
defined by the range without having a PhD in special casing.
https://github.com/paulp/scala/blob/topic/positions/src/compiler/scala/tools/nsc/util/Position.scala#L319