Skip to content

Multi output problem with delambdafied compilation #4809

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

Jump to bottom
Merged
merged 4 commits into from
Nov 13, 2015
Merged

Multi output problem with delambdafied compilation #4809

merged 4 commits into from
Nov 13, 2015

Conversation

wpopielarski
Copy link
Contributor

User code compilation with -Ybackend:GenBCode -Ydelambdafy:method
fails for projects with multiple output directories.
The problem has its root in a fact that some lambdaClass symbols
the associatedFile field is not set. It can be done in Delambdafy.scala
(makeAnonymousClass method) and is working for following lambda
examples:

package acme

object Delambdafy {
  type -->[D, I] = PartialFunction[D, I]

  def main(args: Array[String]): Unit = {
    val result = List(1, 2, 4).map { a =>
      val list = List("1", "2", "3").map { _ + "test" }
      list.find { _ == a.toString + "test" }
    }
    lazy val _foo = foo(result) {
      case x::xs if x isDefined => x.get.length
      case _ => 0
    }
    lazy val bar: Int => Int = {
      case 2 => 13
      case _ =>
        val v = List(1).map(_ + 42).head
    v + 1
    }
  }
  def foo(b: List[Option[String]])(a: List[Option[String]] => Int): Int =
  a(b)
}

but is NOT working for following lambda:

package acme

object Delambdafy {
  type -->[D, I] = PartialFunction[D, I]

  def main(args: Array[String]): Unit = {
    lazy val _baz = baz {
      case 1 =>
        val local = List(1).map(_ + 1)
    local.head
    }
  }
  def baz[T](f: Any --> Any): Any => Any = f
}

so that's why source of compilation unit is used to determine output
directory in case when source file is not found for symbol.

@scala-jenkins scala-jenkins added this to the 2.11.8 milestone Oct 21, 2015
@dragos
Copy link
Contributor

dragos commented Oct 27, 2015

The CLA checker is off. @wpopielarski has signed the CLA according to my checks :)

@lrytz
Copy link
Member

lrytz commented Oct 27, 2015

/synch

dragos
dragos reviewed Oct 27, 2015
View reviewed changes
src/compiler/scala/tools/nsc/backend/jvm/BytecodeWriters.scala
@@ -25,6 +25,9 @@ trait BytecodeWriters {
def outputDirectory(sym: Symbol): AbstractFile =
settings.outputDirs outputDirFor enteringFlatten(sym.sourceFile)

def outputDirectory(cunitSource: scala.reflect.internal.util.SourceFile): AbstractFile =
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the fully qualified name, SourceFile is just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be done

@retronym
Copy link
Member

This change also needs to be applied to this method in ByteCodeWriters:

  def getFile(sym: Symbol, clsName: String, suffix: String): AbstractFile =
    getFile(outputDirectory(sym), clsName, suffix)

I'm happy enough with the defensive/conservative approach in 2.11.x.

For 2.12, I think we can just simplify this to always base this on the compilation unit, rather than than Symbol#associatedFile. @wpopielarski Perhaps you can try that approach and report back whether there are any cases where currentUnit.file doesn't work where the old approach did?

I agree with @dragos : we should not emit compiler warnings here. You can use devWarning to emit logging that will show up as warnings under -Xdev only.

It is probably a good idea to set associatedFile in delambdafy, as you tried. Just add lambdaClass.associatedFile = currentUnit.source.file after

val lambdaClass = pkg newClassSymbol(name, originalFunction.pos, FINAL | SYNTHETIC) addAnnotation SerialVersionUIDAnnotation

And I think both test cases would work.

@retronym
Copy link
Member

I know this code predates your patch, but I'm not a fan of catching Throwable to recover from failures here. This catches OutOfMemoryError etc.

@retronym
Copy link
Member

I'd also like to see a test case for this change.

@wpopielarski
Copy link
Contributor Author

Thanks guys for advices. I implement them with pleasure :).

2015-10-29 2:23 GMT+01:00 Jason Zaugg notifications@github.com:

I'd also like to see a test case for this change.

—
Reply to this email directly or view it on GitHub
#4809 (comment).

@wpopielarski
Copy link
Contributor Author

@retronym in Delambdafy.scala I wanted to set source file with funOwner.sourceFile but this looks like be empty for TermSymbol (see my example in description). Getting it from compilation unit should solve problem.
About catching exception, it is exactly how it's implemented now. There is just try/catch block which returns null turned to error with assertion in single/multiple output decision few lines later. I will check if this is common than this part throws exceptions as normal execution path.

@wpopielarski
Multi output problem with delambdafied compilation
153c70b 
User code compilation with -Ybackend:GenBCode -Ydelambdafy:method
fails for projects with multiple output directories.
The problem has its root in a fact that some `lambdaClass` symbols
the `associatedFile` field is not set. It can be done in Delambdafy.scala
(`makeAnonymousClass` method) and is working for following lambda
examples:
{{{
package acme

object Delambdafy {
  type -->[D, I] = PartialFunction[D, I]

  def main(args: Array[String]): Unit = {
    val result = List(1, 2, 4).map { a =>
      val list = List("1", "2", "3").map { _ + "test" }
      list.find { _ == a.toString + "test" }
    }
    lazy val _foo = foo(result) {
      case x::xs if x isDefined => x.get.length
      case _ => 0
    }
    lazy val bar: Int => Int = {
      case 2 => 13
      case _ =>
        val v = List(1).map(_ + 42).head
	v + 1
    }
  }
  def foo(b: List[Option[String]])(a: List[Option[String]] => Int): Int =
  a(b)
}
}}}
but is NOT working for following lambda:
{{{
package acme

object Delambdafy {
  type -->[D, I] = PartialFunction[D, I]

  def main(args: Array[String]): Unit = {
    lazy val _baz = baz {
      case 1 =>
        val local = List(1).map(_ + 1)
	local.head
    }
  }
  def baz[T](f: Any --> Any): Any => Any = f
}
}}}
so that's why source of compilation unit is used to determine output
directory in case when source file is not found for symbol.
@wpopielarski
Allows to propagate fatal errors when output folder not found.
65b60c8
@wpopielarski
Sets source for newly created lambda class
9688625 
This source is then used to figure out output folder for
compilation product.
@wpopielarski wpopielarski changed the title Multi output problem with delambdafied compilation [WIP] Multi output problem with delambdafied compilation Nov 5, 2015
@wpopielarski wpopielarski force-pushed the delambdafy-multiple-outputs branch from 999280e to 9688625 Compare November 5, 2015 10:17
@wpopielarski wpopielarski changed the title [WIP] Multi output problem with delambdafied compilation Multi output problem with delambdafied compilation Nov 6, 2015
@retronym
Copy link
Member

LGTM. Thanks for adding the test case.

retronym added a commit that referenced this pull request Nov 13, 2015
@retronym
Merge pull request #4809 from wpopielarski/delambdafy-multiple-outputs
0c597f0 
Multi output problem with delambdafied compilation
@retronym retronym merged commit 0c597f0 into scala:2.11.x Nov 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@retronym retronym

Labels
None yet
Projects
None yet
Milestone
2.11.8
Development

Successfully merging this pull request may close these issues.
5 participants
@wpopielarski @dragos @lrytz @retronym @scala-jenkins