Record dependencies on macro arguments #1163
Conversation
| base = file("."), | ||
| id = "macro", | ||
| aggregate = Seq(macroProvider, macroClient), | ||
| settings = Defaults.defaultSettings ++ defaultSettings |
There was a problem hiding this comment.
Note: Usage of Defaults.defaultSettings is going to be deprecated in 0.13.2
|
So this looks extra super cool! I'm going to have to delay this until 0.13.3 though. I have way too many issues to resolve for 0.13.2 right now (with auto-plugins), but I'll get back to you next week after the 0.13.2-RC1 is out and 0.13 is ok to merge into again. |
|
(ping @xeno-by) |
| import Compat._ | ||
|
|
||
| locally { | ||
| import global._ // this is where MEA lives in 2.10.x |
There was a problem hiding this comment.
Can we import just MEA from Global? Global has more than 1600 members, we don't need to pull all of them here.
There was a problem hiding this comment.
Unfortunately this won't compile with Scala 2.8 and 2.9 since MacroExpansionAttachment appeared in 2.10.
There was a problem hiding this comment.
Ah, yes. This is the way to go then. Could you just expand the comment in the code explaining why do we need a wildcard import?
|
Great job, @Duhemm! |
|
@jsuereth: don't worry about this PR, I'll take care of it and schedule for sbt 0.13.3 release. |
|
Indeed. This looks very promising! I'm glad you managed to figure out the maze of compatibility hacks we have here. 👍 Have you considered expanding the package macro
object ClientNested {
Provider.printTree(Provider.printTree(Foo.str))
}Also, given the fact that you modify both https://github.com/sbt/sbt/blob/0.13/compile/interface/src/test/scala/xsbt/DependencySpecification.scala The code samples you are going to use in those tests should look similar to what you have in the Just in case you it's not clear what's the point of testing (seemingly) the same thing twice: scripted tests are functional tests and the specifications for |
|
Hi guys, Does anybody know what happened with the last build ? It seems to have timed out, but I don't see how the changes between 5f6b06a and d76b4ea may have caused this ? I've had to modify |
|
Yeah, it's transient to you, see the error message: It's quite possible there's a rare deadlock in the test listener, which would have caused this. It's the first time I've seen this message, but something I can pay attention to. |
| case _ => () | ||
|
|
||
| def handleTreeNode(node: Tree): Unit = { | ||
| def handleMacroExpansion(original: Tree): Unit = original.foreach(handleTreeNode) |
There was a problem hiding this comment.
Can we have a test of a nested macro expansion, i.e. a macro application that expands into another macro application?
|
Ok. I think we just missing test for nested macro applications we'll be done with the code. Once the last patch (for nested macro applications) is submitted we should look into squashing most of the commits. E.g. 14ff4b0 and c9be404 squashed reduce to empty commit as the latter one cancels the former. All small commits that fix typos, reduce use of reflection, etc. should be squashed as well. |
|
Hi, I just added this test and updated the comments you mentioned, is everything ok ? |
|
Yes, looks good. Could you rebase the branch and squash most of the commits? I think we would like to keep commits for the following events:
That would give us 5 commits in total. |
Add a test which shows the problem of not properly capturing dependencies of macro arguments.
Macros take arguments as trees and return some other trees; both of them have dependencies but we see trees only after expansion and recorded only those dependencies. This commit solves this problem by looking into the attachments of the trees that are supposed to contain originals of macro expansions and recording dependencies of the macro before its expansion.
It was not possible to make `ScalaCompilerForUnitTesting` compile several
files in different runs, which means that it was not possible to compile
and use a macro in a test case, since macros cannot be used in the same
compilation run that defines them.
This commit allows a test case to provide multiple grouped snippets of
code that will be compiled in separate runs.
For instance :
List(Map(<snippet A>, <snippet B>), Map(<snippet C>))
Here, <snippet A> and <snippet B> will be compiled together, and then
<snippet C> will be compiled, and will be able to use symbols defined
in <snippet A> or <snippet B>.
|
It looks like I managed to split the commits ! Thank you for your help ! |
Add a unit test which checks whether we capture dependencies introduced by arguments to macros. Those dependencies are special because macros get expanded during type checking and arguments to macros are not visible during regular tree walk.
|
@jsuereth: since this PR is shaping very nicely I'd like to get it in sbt 0.13.2. It's a very nice piece of work and improves significantly macro handling in name hashing. It's safe because it affects name hashing only. The old incremental compilation algorithm is unaffected. WDYT? |
|
@Duhemm: you need to update one more commit :) |
Add test analogous to source-dependencies/macro-arg-dep but check if dependencies of nested macro applications are handled properly. Nested macro applications are tricky because we have to look into original (before macro expansion) trees recursively. This test verifies that.
|
LGTM |
|
@jsuereth: ping on merging this into 0.13.2 |
|
\o/ |
Record dependencies on macro arguments
|
I'm merging this into 0.13.2. @gkossakowski Could you add an entry into https://github.com/sbt/sbt/blob/0.13/src/sphinx/Community/Changes.rst to summarize this enhancement, plz? |
It has been reported in sbt#1237 that stack overflows may occur during the extraction of used names (and later of dependencies between files). This problem has been introduced by sbt#1163, which was about recording the dependencies of macro arguments. When a macro is expanded, the compiler attaches the tree before expansion to the tree representing the expanded macro. As of Scala 2.11-RC3, some macros have themselves attached as original tree, which caused the same macro to be inspected over and over until a stack overflow. This commit solves this problem by making sure that the original of a macro expansion will be inspected if and only if it is different from the expanded tree. Fixes sbt#1237
It has been reported in sbt/sbt#1237 that stack overflows may occur during the extraction of used names (and later of dependencies between files). This problem has been introduced by sbt/sbt#1163, which was about recording the dependencies of macro arguments. When a macro is expanded, the compiler attaches the tree before expansion to the tree representing the expanded macro. As of Scala 2.11-RC3, some macros have themselves attached as original tree, which caused the same macro to be inspected over and over until a stack overflow. This commit solves this problem by making sure that the original of a macro expansion will be inspected if and only if it is different from the expanded tree. Fixes sbt/sbt#1237
It has been reported in sbt/sbt#1237 that stack overflows may occur during the extraction of used names (and later of dependencies between files). This problem has been introduced by sbt/sbt#1163, which was about recording the dependencies of macro arguments. When a macro is expanded, the compiler attaches the tree before expansion to the tree representing the expanded macro. As of Scala 2.11-RC3, some macros have themselves attached as original tree, which caused the same macro to be inspected over and over until a stack overflow. This commit solves this problem by making sure that the original of a macro expansion will be inspected if and only if it is different from the expanded tree. Fixes sbt/sbt#1237 Rewritten from sbt/zinc@7ee54a9
(from this discussion in sbt-dev).
This pull request solves this problem by looking into the attachments of the trees that are supposed to contain originals of macro expansions and recording dependencies of the macro before its expansion.