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
from

Conversation

Projects
None yet
2 participants
@smithjessk
Collaborator

smithjessk commented Jul 6, 2016

Closes #24.

  • Stop canonicalization if parameters are by-name. This works even if the function has variable arguments
  • Add tests to cover these cases. These tests are inspired by / copied from Scala Async.
  • Correct the implementation of a test also from Scala Async

smithjessk added some commits Jun 2, 2016

@@ -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",

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

How come we need this dependency?

This comment has been minimized.

@smithjessk

smithjessk Jul 11, 2016

Collaborator

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

Removed it in 1387ecb.

@@ -8,7 +8,6 @@ import scala.language.experimental.macros
import scala.reflect.macros.whitebox.Context

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

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

This comment has been minimized.

@smithjessk
@@ -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

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

It is enough to say:

Detect by-name parameters, and prevent canonicalizing them.

This comment has been minimized.

@smithjessk
// should not be canonicalized.
// Maps to the parameter lists, saying whether or not they are by-name.
val byNameParams: immutable.Seq[immutable.Seq[Boolean]] = {

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

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

This comment has been minimized.

@smithjessk

smithjessk Jul 11, 2016

Collaborator

Done in 1bc8ded.

// Maps to the parameter lists, saying whether or not they are by-name.
val byNameParams: immutable.Seq[immutable.Seq[Boolean]] = {
// Check that `selector` has a non-trivial symbol. If it doesn't, we assume

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

What is a non-trivial symbol?

This comment has been minimized.

@smithjessk

smithjessk Jul 11, 2016

Collaborator

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.

val byNameParams: immutable.Seq[immutable.Seq[Boolean]] = {
// Check that `selector` has a non-trivial symbol. If it doesn't, we assume
// that there were no by-name parameters.
if (selector.symbol != null && selector.symbol != NoSymbol) {

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

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

This comment has been minimized.

@smithjessk

smithjessk Jul 11, 2016

Collaborator

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.

noRepeatedParamsSeq
}
} else {
immutable.Seq.fill(1, paramss(0).length)(false)

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

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

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

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.

This comment has been minimized.

@smithjessk

smithjessk Jul 11, 2016

Collaborator

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.

if (selector.symbol != null && selector.symbol != NoSymbol) {
val paramLists = selector.symbol.asMethod.paramLists
// This value does not contain repeated parameters.

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

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

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

} else {
immutable.Seq.fill(1, paramss(0).length)(false)
}
}

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

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.

This comment has been minimized.

@smithjessk

smithjessk Jul 11, 2016

Collaborator

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

This comment has been minimized.

@smithjessk

smithjessk Jul 26, 2016

Collaborator

Working on this change here.

This comment has been minimized.

@smithjessk

smithjessk Aug 2, 2016

Collaborator

Would the first condition disallow calling methods with repeated parameters?

@@ -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

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

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
}

This comment has been minimized.

@smithjessk

smithjessk Jul 11, 2016

Collaborator

By bynamess, do you mean byNameParams?

Also, where is cs defined?

This comment has been minimized.

@axel22

axel22 Aug 25, 2016

Member

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

test("nested await as bare expression") {
val c = async(coroutine { () =>
await(Future(await(Future("")).isEmpty))
val emptyString = await(Future(""))

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

Why this change?

This comment has been minimized.

@smithjessk

smithjessk Jul 11, 2016

Collaborator

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.

test("nested await in block") {
val c = async(coroutine { () =>
()
await(Future(await(Future("")).isEmpty))
val emptyString = await(Future(""))

This comment has been minimized.

@axel22

axel22 Jul 9, 2016

Member

Same here?

This comment has been minimized.

@smithjessk

smithjessk Jul 11, 2016

Collaborator

See #24.

@axel22

This comment has been minimized.

Member

axel22 commented Jul 9, 2016

Please also add an additional file to tests by-name-tests.scala, using the ScalaTest template from another test file, and add multiple unit tests for the functionality that you are implementing.

@smithjessk

This comment has been minimized.

Collaborator

smithjessk commented Jul 11, 2016

Yes! Will do.

To-do list for unit tests:

smithjessk added some commits Jul 11, 2016

Add tests to assert that coroutines cannot be passed as by-name param…
…eters

Note that these tests currently fail.
Add test of by-name parameter traversal
Assert that they are not lifted even when they surround a non by-name parameter.
@axel22

This comment has been minimized.

Member

axel22 commented Jul 22, 2016

Should I take another look?

@smithjessk

This comment has been minimized.

Collaborator

smithjessk commented Jul 26, 2016

I still have to go over a few of your comments. In the meantime, could you look at this? Thanks!

smithjessk added some commits Jul 26, 2016

Pass coroutine applications by-name in ByNameTest
Previously, the parameters passed by-name were unapplied coroutines.
@smithjessk

This comment has been minimized.

Collaborator

smithjessk commented Jul 26, 2016

To-do list for by-name analysis:

  • Use ByNameParam class as described here
@axel22

This comment has been minimized.

Member

axel22 commented Aug 20, 2016

Note: this branch has merge conflicts.

@smithjessk

This comment has been minimized.

Collaborator

smithjessk commented Aug 24, 2016

Yep- I'm still working on it. Do you mind taking a look at this comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment