Skip to content
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

SI-8245 Fix regression in interplay between lazy val, return #3488

Merged
merged 1 commit into from Feb 9, 2014

Conversation

retronym
Copy link
Member

@retronym retronym commented Feb 8, 2014

In 4c86dbb / SI-6358, synthesis of lazy val accessors trees was
moved into the typer phase (in MethodSynthesis). Before that point,
the symobl for the accessor was created early, but the tree was
not. This led to crashes in intervening phases (extensionmethods)
as changeOwner calls didn't catch the smuggled symbol.

Moving the accessor generation forward, however, brought a problem:
we now introduce a DefDef around the RHS of the lazy val, but we're
not actually guaranteed that the body has already been typechecked.
If it happened to be typechecked for the purposes of return type
inference, we'll pick up the typechecked tree from transformed:

// LazyValGetter#derivedTree
val rhs1 = transformed.getOrElse(rhs0, rhs0)

But if the method had an explicit return type (which must always
be the case if it contains a return!), rhs0 will be untyped.

This leads to, e.g.:

def foo(o: Option[Int]): Int = {
  lazy val i = o.getOrElse(return -1)
  i + 1
}

def foo(o: Option[Int]): Int = {
  lazy <artifact> var i$lzy: Int = _;
  <stable> <accessor> lazy def i: Int = {
    i$lzy = o.getOrElse(return -1);
    i$lzy
  };
  i.+(1)
};

When this is typechecked, the return binds to the closest enclosing
DefDef, lazy def i. This commit changes Context#enclMethod to
treat DefDefs as transparent.

enclMethod is only used in one other spot that enforces the
implementation restriction that "module extending its companion class
cannot use default constructor arguments".

Review by @gkossakowski

In 4c86dbb / SI-6358, synthesis of lazy val accessors trees was
moved into the typer phase (in MethodSynthesis). Before that point,
the symobl for the accessor *was* created early, but the tree was
not. This led to crashes in intervening phases (extensionmethods)
as `changeOwner` calls didn't catch the smuggled symbol.

Moving the accessor generation forward, however, brought a problem:
we now introduce a DefDef around the RHS of the lazy val, but we're
not actually guaranteed that the body has already been typechecked.
If it happened to be typechecked for the purposes of return type
inference, we'll pick up the typechecked tree from `transformed`:

    // LazyValGetter#derivedTree
    val rhs1 = transformed.getOrElse(rhs0, rhs0)

But if the method had an explicit return type (which must *always*
be the case if it contains a `return`!), `rhs0` will be untyped.

This leads to, e.g.:

    def foo(o: Option[Int]): Int = {
      lazy val i = o.getOrElse(return -1)
      i + 1
    }

    def foo(o: Option[Int]): Int = {
      lazy <artifact> var i$lzy: Int = _;
      <stable> <accessor> lazy def i: Int = {
        i$lzy = o.getOrElse(return -1);
        i$lzy
      };
      i.+(1)
    };

When this is typechecked, the `return` binds to the closest enclosing
`DefDef`, `lazy def i`. This commit changes `Context#enclMethod` to
treat `DefDef`s as transparent.

`enclMethod` is only used in one other spot that enforces the
implementation restriction that "module extending its companion class
cannot use default constructor arguments".
c.enclMethod = if (isDefDef) c else enclMethod

// SI-8245 `isLazy` need to skip lazy getters to ensure `return` binds to the right place
c.enclMethod = if (isDefDef && !owner.isLazy) c else enclMethod
Copy link
Member

Choose a reason for hiding this comment

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

Can this break people who rely on enclosingMethod in macros?

Copy link
Member Author

Choose a reason for hiding this comment

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

2.11.0-M7

scala> def impl(c: Context) = {println(c.enclosingMethod); import c.universe._; q"()"}
warning: there were 2 deprecation warning(s); re-run with -deprecation for details
impl: (c: reflect.macros.Context)c.universe.Literal

scala> def enc: Unit = macro impl
defined term macro enc: Unit

scala> def foo = { lazy val x = enc }
def foo = {
  lazy <artifact> val x$lzy = enc;
  ()
}
foo: Unit

scala> def foo: Int = { lazy val x = enc; 0 }
def foo: Int = {
  lazy <artifact> val x$lzy = enc;
  0
}
foo: Int

This PR:

scala> def foo: Int = { lazy val x = enc; 0 }
def foo: Int = {
  lazy <artifact> val x$lzy = enc;
  0
}
foo: Int

scala> def foo = { lazy val x = enc }
def foo = {
  lazy <artifact> val x$lzy = enc;
  ()
}
foo: Unit

Copy link
Member

Choose a reason for hiding this comment

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

It's good that we have deprecated enclosingXXX...

@gkossakowski
Copy link
Member

LGTM.

gkossakowski added a commit that referenced this pull request Feb 9, 2014
SI-8245 Fix regression in interplay between lazy val, return
@gkossakowski gkossakowski merged commit 73784d7 into scala:master Feb 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants