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

Fix AST transformations of by-name parameters #27

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
aae630a
Partially implement reflection test from Scala Async
smithjessk Jun 2, 2016
647d1ea
Merge branch 'master' of https://github.com/storm-enroute/coroutines …
smithjessk Jun 3, 2016
c354504
Copy method definitions into canonicalized AST
smithjessk Jun 6, 2016
20ce237
Copy declarations and identifiers from method definitions into proper…
smithjessk Jun 6, 2016
4136755
Merge branch 'master' of https://github.com/storm-enroute/coroutines …
smithjessk Jun 6, 2016
a9a7885
Remove checks that asserted equality between unit types
smithjessk Jun 6, 2016
e07ca52
Make some progress on properly transforming by-name parameters
smithjessk Jun 11, 2016
36ef738
Remove infinite looping introduced in the previous commit
smithjessk Jun 15, 2016
23bf279
Merge master
smithjessk Jun 23, 2016
c2f47df
Re-add tests that were accidentally removed in previous commit
smithjessk Jun 23, 2016
db52258
Don't canonicalize nested methods
smithjessk Jul 5, 2016
9ade443
Fix failing tests involving by-name parameters
smithjessk Jul 5, 2016
1e78d94
Remove failing and commented-out tests
smithjessk Jul 5, 2016
48c1c85
Remove tests and changes copied from add-async-tests
smithjessk Jul 5, 2016
c35b6b7
Document and simplify canonicalization of function application
smithjessk Jul 5, 2016
c476e7d
Clean up and document by-name parameter transformation
smithjessk Jul 6, 2016
bd1c75a
Fix implementation of test from Scala Async
smithjessk Jul 6, 2016
1387ecb
Remove dependency on scala-compiler
smithjessk Jul 11, 2016
1bc8ded
Clean up by-name modifications in AstCanonicalization
smithjessk Jul 11, 2016
06e28eb
Remove commented-out code
smithjessk Jul 11, 2016
2b576cf
Add tests to assert that coroutines cannot be passed as by-name param…
smithjessk Jul 11, 2016
ffd0ea7
Add repeated parameters test to ByNameTest
smithjessk Jul 11, 2016
28adad9
Add test of by-name parameter traversal
smithjessk Jul 12, 2016
b2983ad
Add another by-name parameter test
smithjessk Jul 12, 2016
570221f
Pass coroutine applications by-name in ByNameTest
smithjessk Jul 26, 2016
fa55257
Run NestedContextValidator on by-name parameters
smithjessk Jul 26, 2016
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
1 change: 1 addition & 0 deletions project/Build.scala
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ object CoroutinesBuild extends MechaRepoBuild {
"org.scalatest" % "scalatest_2.11" % "2.2.6" % "test",
"org.scala-lang.modules" %% "scala-parser-combinators" % "1.0.2",
"org.scala-lang" % "scala-reflect" % "2.11.4",
"org.scala-lang" % "scala-compiler" % "2.11.4",
Copy link
Member

Choose a reason for hiding this comment

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

How come we need this dependency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added it while experimenting with different solutions. Forgot to take it out.

Removed it in 1387ecb.

"org.scala-lang.modules" % "scala-async_2.11" % "0.9.5" % "bench"
)
case _ => Nil
Expand Down
73 changes: 69 additions & 4 deletions src/main/scala/org/coroutines/AstCanonicalization.scala
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import scala.language.experimental.macros
import scala.reflect.macros.whitebox.Context



/** Transforms the coroutine body into three address form with restricted control flow
Copy link
Member

Choose a reason for hiding this comment

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

Always 3 lines between the import section and the first top-level section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

* that contains only try-catch statements, while loops, if-statements, value and
* variable declarations, pattern matches, nested blocks and function calls.
Expand Down Expand Up @@ -90,7 +89,7 @@ trait AstCanonicalization[C <: Context] {
val localvarname = TermName(c.freshName("x"))
val decls = List(
q"var $localvarname = null.asInstanceOf[Boolean]",
q"""
q"""
..$conddecls
if ($condident) {
..$thendecls
Expand Down Expand Up @@ -122,6 +121,48 @@ trait AstCanonicalization[C <: Context] {
(decls, q"$localvarname")
case q"$selector[..$tpts](...$paramss)" if tpts.length > 0 || paramss.length > 0 =>
// application

// We first need to see which parameters, if any, are by-name. Those that are
Copy link
Member

Choose a reason for hiding this comment

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

It is enough to say:

Detect by-name parameters, and prevent canonicalizing them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

// should not be canonicalized.

// Maps to the parameter lists, saying whether or not they are by-name.
val byNameParams: immutable.Seq[immutable.Seq[Boolean]] = {
Copy link
Member

Choose a reason for hiding this comment

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

It should be sufficient to use just Seq[Seq[Boolean]].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 1bc8ded.

// Check that `selector` has a non-trivial symbol. If it doesn't, we assume
Copy link
Member

Choose a reason for hiding this comment

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

What is a non-trivial symbol?

Copy link
Collaborator Author

@smithjessk smithjessk Jul 11, 2016

Choose a reason for hiding this comment

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

You're right: non-trivial isn't the best way to describe what I meant.

Some methods threw errors when I called asMethod on them. These checks are my way of preventing those exceptions.

I labeled the problematic functions trivial. Pretty sure they were lambdas.

// that there were no by-name parameters.
if (selector.symbol != null && selector.symbol != NoSymbol) {
Copy link
Member

Choose a reason for hiding this comment

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

When is the selector.symbol equal to null or NoSymbol?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Related to #27 (comment).

I think lambdas were problematic. There might have been more cases, though. I should have documented this more heavily at the time.

val paramLists = selector.symbol.asMethod.paramLists

// This value does not contain repeated parameters.
Copy link
Member

Choose a reason for hiding this comment

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

Note that repeated by name parameters are not allowed in Scala.

Also, the paramss value might not contain parameters with default values.

val noRepeatedParamsSeq = paramLists.map { currentParamList =>
currentParamList.map { param => param.asTerm.isByNameParam }
}

// Check that parameters were both passed in to and requested by the method.
if (paramss.length > 0 && noRepeatedParamsSeq.length > 0) {
// Initially, we assume that no parameters were repeated.
var paramsSeq = noRepeatedParamsSeq

/** If the number of passed-in parameters is greater than the number of
* parameters needed by the method, then we assume that there are repeated
* parameters.
*
* This problem is fixed by appending the last value in
* `noRepeatedParamsSeq` to `paramsSeq`. This is done until the number of
* parameters are equal.
*/
while (paramss(0).length > paramsSeq(0).length) {
val newHead = paramsSeq(0) :+ paramsSeq(0).last
val newTail = paramsSeq.tail
paramsSeq = newHead :: newTail
}
paramsSeq
} else {
noRepeatedParamsSeq
}
} else {
immutable.Seq.fill(1, paramss(0).length)(false)
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that if there is no symbol information, all the parameters are considered non-by-name?

Copy link
Member

@axel22 axel22 Jul 9, 2016

Choose a reason for hiding this comment

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

Should it not be smth like:

for (params <- paramss) yield
  for (p <- params) yield false

Otherwise, with the current code you'd end up having only the first parameter list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

About the lack of symbol information: yes, that's what I intended.

About the first parameter list issue: you're right. I think I assumed that, if there was no appropriate symbol information, there would only be one parameter list.

Changed this in 1bc8ded.

}
}
Copy link
Member

Choose a reason for hiding this comment

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

You should not inspect the method symbol and match the parameter trees to the method type information to figure out which is which - you're reproducing complex typechecker logic by doing that, and not covering all the edge cases.

Instead, see this:

http://stackoverflow.com/questions/29757584/handling-by-name-parameters-in-scala-macro

The ByNameParamClass is a dummy class that denotes that a parameter is by-name. Unfortunately, this dummy class is only used to wrap the type of the by-name parameter, but it does not appear in any way in an actual application. This is a flaw in the reflection API, as it does not expose enough information.

We should have the following behavior:

  1. If the lengths of the argument lists at the callsite is equal to the length of the parameter lists at the method definition, AND, the respective argument types exactly match respective parameter types, then use that information to figure out what is by-name and what is not. This already covers a majority of cases.
  2. Otherwise, do not try to patch the argument lists, and immediately report an error - such a method application cannot be used inside a coroutine definition. Workaround for users is to introduce some sort of bridge method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can do! After I write unit tests, I will change my implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Working on this change here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would the first condition disallow calling methods with repeated parameters?

val (rdecls, newselector) = selector match {
case q"$r.$method" =>
val (rdecls, rident) = canonicalize(r)
Expand All @@ -130,10 +171,34 @@ trait AstCanonicalization[C <: Context] {
(Nil, q"$method")
}
for (tpt <- tpts) disallowCoroutinesIn(tpt)
val (pdeclss, pidents) = paramss.map(_.map(canonicalize).unzip).unzip

Copy link
Member

@axel22 axel22 Jul 9, 2016

Choose a reason for hiding this comment

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

Could you do just:

val (pdeclss, pidents) = {
  val cs = for ((params, bynames) <- paramss.zip(bynamess)) yield {
    for ((p, byname) <- params.zip(bynames)) yield {
      if (byname) (List(), p) else canonicalize(p)
    }
  }
  cs.unzip.unzip
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By bynamess, do you mean byNameParams?

Also, where is cs defined?

Copy link
Member

Choose a reason for hiding this comment

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

By bynamess, I meant byNameParams, yes.
cs is defined in my snippet, in the second line.

// Canonicalize parameters that are not by-name.
val paramsByNameUnmodified = {
val modifiedParamLists = mutable.Seq.fill[
List[(List[c.universe.Tree], c.universe.Tree)]](paramss.length)(null)

// Canonicalize each parameter list.
for (i <- 0 until paramss.length) {
val modifiedParams = mutable.Seq.fill[
(List[c.universe.Tree], c.universe.Tree)](paramss(i).length)(null)
for (j <- 0 until modifiedParams.length) {
if (byNameParams(i)(j)) {
modifiedParams(j) = (List(q""), paramss(i)(j))
} else {
modifiedParams(j) = canonicalize(paramss(i)(j))
}
}
modifiedParamLists(i) = modifiedParams.toList
}
modifiedParamLists.toList
}
val pdeclss =
paramsByNameUnmodified.map((_.map(tuple => tuple._1)))
.flatten.flatten.filter{ decl => decl != q"" }
val pidents = paramsByNameUnmodified.map((_.map(tuple => tuple._2)))
val localvarname = TermName(c.freshName("x"))
val localvartree = q"val $localvarname = $newselector[..$tpts](...$pidents)"
(rdecls ++ pdeclss.flatten.flatten ++ List(localvartree), q"$localvarname")
(rdecls ++ pdeclss ++ List(localvartree), q"$localvarname")
case q"$r[..$tpts]" if tpts.length > 0 =>
// type application
for (tpt <- tpts) disallowCoroutinesIn(tpt)
Expand Down
36 changes: 28 additions & 8 deletions src/test/scala/org/coroutines/async-await-tests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import scala.annotation.unchecked.uncheckedVariance
import scala.concurrent._
import scala.concurrent.duration._
import scala.concurrent.ExecutionContext.Implicits.global
import scala.language.{ reflectiveCalls, postfixOps }
import scala.util.Success


Expand Down Expand Up @@ -237,15 +236,14 @@ class AsyncAwaitTest extends FunSuite with Matchers {
val fut = AsyncAwaitTest.async(coroutine { () =>
Option(new ParamWrapper("value!")) match {
case Some(valueHolder) =>
AsyncAwaitTest.await(Future(doAThing(valueHolder)))
AsyncAwaitTest.await(doAThing(valueHolder))
case None =>
None
}
})

val result = Await.result(fut, 5 seconds)
assert(result.asInstanceOf[Future[ParamWrapper[String]]].value ==
Some(Success(None)))
assert(result == None)
}

// Source: https://git.io/vr7NW
Expand Down Expand Up @@ -309,25 +307,47 @@ class AsyncAwaitTest extends FunSuite with Matchers {
assert(result == 103)
}

// Source: https://git.io/vrFj3
/** Source: https://git.io/vrFj3
* Modified so that there is no coroutine passed as a by-name parameter. For more
* information, see https://git.io/vKkM5.
*/
test("nested await as bare expression") {
val c = async(coroutine { () =>
await(Future(await(Future("")).isEmpty))
val emptyString = await(Future(""))
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Future.apply takes a by-name parameter, but coroutines can't be passed by-name. See #24.

I will add some unit tests to assert that this check happens.

await(Future(emptyString.isEmpty))
})
val result = Await.result(c, 5 seconds)
assert(result == true)
}

// Source: https://git.io/vrAnM
/** Source: https://git.io/vrAnM
* Modified so that there is no coroutine passed as a by-name parameter. For more
* information, see https://git.io/vKkM5.
*/
test("nested await in block") {
val c = async(coroutine { () =>
()
await(Future(await(Future("")).isEmpty))
val emptyString = await(Future(""))
Copy link
Member

Choose a reason for hiding this comment

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

Same here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #24.

await(Future(emptyString.isEmpty))
})
val result = Await.result(c, 5 seconds)
assert(result == true)
}

/** Source: https://git.io/vrAlJ
* Modified so that there is no coroutine passed as a by-name parameter. For more
* information, see https://git.io/vKkM5.
*/
test("by-name expressions aren't lifted") {
def foo(ignored: => Any, b: Int) = b
val c = async(coroutine { () =>
val innerValue = await(Future(1))
await(Future(foo(???, innerValue)))
})
val result = Await.result(c, 5 seconds)
assert(result == 1)
}

// Source: https://git.io/vrhTe
test("named and default arguments respect evaluation order") {
var i = 0
Expand Down