Better dependencies for macros, future of the scalahost compiler plugin

[Martin Duhem]

Hi,

For the last week, we've been working on :

• Scalahost, a compiler plugin that tracks the symbols touched during the expansion of a macro 
• Providing a mean to record dependencies that come from macro expansions in sbt
• Making the two work together

Would you be interested in integrating this in sbt ?

What is Scalahost (https://github.com/scalareflect/scalahost) ?

Scalahost is a compiler plugin for scalac 2.11.0. Its job is to register all the symbols that are touched during the expansion of a macro, and attach them to the expanded macro. Using this information, we can deduct the dependencies between the expanded macro and everything that has been involved in its expansion.

Scalahost adds an attachment to the expanded macro. This attachment is a Map[String, Any], so that we can extract it without using an additional library or reflection. 

For instance, the problems illustrated by these scripted tests are solved using scalahost : 

• https://github.com/Duhemm/sbt/tree/sbt-with-scalahost-2/sbt/src/sbt-test/source-dependencies/macro-scalahost-1
  
• https://github.com/Duhemm/sbt/tree/sbt-with-scalahost-2/sbt/src/sbt-test/source-dependencies/macro-scalahost-2
  
• https://github.com/Duhemm/sbt/tree/sbt-with-scalahost-2/sbt/src/sbt-test/source-dependencies/macro-scalahost-3
  

Unfortunately, scalahost suffers from some limitations and won't work in some cases. With some macros, scalahost will fail during the expansion. (I am sorry I don't know the macro engine well enough to be able to explain from where this limitation comes from, maybe Eugene can elaborate ?)

What has been done regarding the dependencies from macro expansions ?

Dependencies that come from macro expansions require special handling (please see this discussion). Registering them with dependencies by inheritance would make debugging sbt really complicated. Therefore, I added a new "bucket" where dependencies that come from macro expansions should be registered. The logic required to invalidate macro clients has been implemented too.

I am still modifying the existing tests so that they compile with this modification and making sure that I didn't break anything, but it looks like it's working nicely.

The diff can be found here : https://github.com/Duhemm/sbt/compare/sbt-macro-dependencies

What is needed to make scalahost work with sbt ?

Only a few lines of code. Here is the diff that shows how to get the symbols touched during a macro expansion.

What do you think ?

Even though it is not the universal solution, scalahost can still be helpful in many cases, and using the information it provides requires very little addition in sbt. Do you think that this is worth integrating in sbt ?

What do you think of the new dependencies from macro expansions thing ? Obviously, without scalahost this is not very useful, but this will come in handy when we have a more complete support of macros in sbt. 

[Grzegorz Kossakowski]

Hi Martin!

Thanks for you write up! Your branch looks pretty good already.

There are some issues, though. Boolean flags are rarely great solution and signature of `addExternalDep` method is a good example of that. Somebody could call addExternalDep with inherited=true, fromMacro=true parameters but it makes little sense and it's not supported properly. I know that you just extended what was already there but I think we should look into refactoring how dependency tracking is implemented.

You probably noticed that adding tracking of a new kind of dependency (like your "fromMacro") is quiet involved at the moment. The reason is that dependency kind is hardcoded into method signatures like `addExternalDep`. We should look into abstractacting over specific dependency kind. I'll look into that tomorrow to see if I can propose something to you.

Other than that, the implementation looks good. Whether to include the changes into sbt is a bit separate discussion. Ideally, if we manage to refactor dependency tracking code so adding new kinds of dependencies becomes trivial, we'll have easier time deciding this. Smaller changes are always easier to get through because they are easier to review, revert, maintain, etc.

--
You received this message because you are subscribed to the Google Groups "sbt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sbt-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sbt-dev/d82b63c8-5037-44ee-b347-dca70ee9e52e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
Grzegorz Kossakowski

Scalac hacker at Typesafe

twitter: @gkossakowski

github: @gkossakowski

[Grzegorz Kossakowski]

Hi Martin!

I've implemented a rough sketch of abstracting of specific dependency kind. See: https://github.com/gkossakowski/sbt/commit/62fddc2c95ac64253ce4d9a0eb3f4852770ffaef

The code doesn't compile yet (the refactoring is not complete) but it should give you an idea where I want to go with this.

Only when working on listing all dependency kinds I realized that your implementation is incomplete. You track only external dependencies (on things coming from another subproject). However, a macro expansion might introduce both external and internal dependencies.

It can be seen that you missed some cases by looking at your change to `binaryDependency`: https://github.com/Duhemm/sbt/compare/sbt-macro-dependencies#diff-a2b129460f05548355e1345594eafcd8R111

You can see that you touched just one case out of many in that method.

I think that adding full implementation would be a pain in the butt given current code structure. You'd have to modify a ton of tricky code. I think you should refactor the code so adding a new dependency kind becomes straightforward and only then work on macro-specific dependency kinds.

Let me know whether the refactoring I outlined in the commit above makes sense to you.

[Eugene Burmako]

Just wanted to chime in about "scalahost suffers from some limitations and won't work in some cases. With some macros, scalahost will fail during the expansion".

Scalahost injects its own implementation of scala.reflect.macros.Context that wraps the compiler-provided context and universe and registers all APIs that have been called. Unfortunately some popular macros cast universe to scala.reflect.internal.SymbolTable (or, even worse, to scala.tools.nsc.Global) in order to access internal APIs and that will fail if the macro context is a proxy.

That's a problem at the moment, but it should become less and less of a problem as time passes by, because in Scala 2.11 we have codified all internal APIs that people accessed through casts as public methods on `c.internal._`.

On 10 May 2014 18:47, Martin Duhem <martin...@gmail.com> wrote:

--

[Grzegorz Kossakowski]

On 14 May 2014 12:08, Eugene Burmako <eugene....@epfl.ch> wrote:

Just wanted to chime in about "scalahost suffers from some limitations and won't work in some cases. With some macros, scalahost will fail during the expansion".

Scalahost injects its own implementation of scala.reflect.macros.Context that wraps the compiler-provided context and universe and registers all APIs that have been called. Unfortunately some popular macros cast universe to scala.reflect.internal.SymbolTable (or, even worse, to scala.tools.nsc.Global) in order to access internal APIs and that will fail if the macro context is a proxy.

That's a problem at the moment, but it should become less and less of a problem as time passes by, because in Scala 2.11 we have codified all internal APIs that people accessed through casts as public methods on `c.internal._`.

Oh, makes sense now.

Long term the only thing we have to worry about is to make sure that people don't need to cast to internals. It seems like we are getting there, though.

[Josh Suereth]

This is great work guys!   Super excited to see stuff like this added in a backward compatible/testable way.  I realize it's twice as hard, but the ability to try out both name-hashing *AND* macro-support without having to bump major scala/sbt versions is a huge win.

Keep up the good work.

--

You received this message because you are subscribed to the Google Groups "sbt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sbt-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sbt-dev/CAD%2BExrKjr50uurLyXLGGc0rT72sSYgr8aqiAmiMyFgc4ghWnBg%40mail.gmail.com.

[Martin Duhem]

Hi guys !

I've started working on this, and have a question.

What are the deprecated methods that I should introduce ? So far, I only deprecated the old methods in /compile/inc/src/main/scala/sbt/inc/Analysis.scala. Should I also deprecate the ones in /compile/inc/src/main/scala/sbt/inc/Relations.scala ?

There is a scripted test (source-dependencies/import-class-name-hashing) that presents a very strange behavior : sometimes it fails, sometimes it succeeds without any changes. Here is the output of sbt (shortened at the bottom of the page)

Apart from source-dependencies/import-class-name-hashing, all source-dependencies/* and unit tests seem to pass. 

What I've done can be found here : https://github.com/Duhemm/sbt/compare/abstract-over-dependency-kind

Would you prefer that I directly open a pull request to make the discussion easier to follow as I continue working on this ?

[Grzegorz Kossakowski]

On 16 May 2014 13:02, Martin Duhem <martin...@gmail.com> wrote:

Hi guys !

I've started working on this, and have a question.

What are the deprecated methods that I should introduce ? So far, I only deprecated the old methods in /compile/inc/src/main/scala/sbt/inc/Analysis.scala. Should I also deprecate the ones in /compile/inc/src/main/scala/sbt/inc/Relations.scala ?

Yes. We have to provide deprecated overloads for all public methods in all traits that are exposed by Analysis object.

The reason is that `compile` returns Analysis object so it's a public API.

 

There is a scripted test (source-dependencies/import-class-name-hashing) that presents a very strange behavior : sometimes it fails, sometimes it succeeds without any changes. Here is the output of sbt (shortened at the bottom of the page)

That's strange indeed. We'll need more logging to see what's going on.

Add `logLevel in compile := Level.Debug` to the build.sbt file in that test. It should print the log from incremental compiler when the test fails.

 

Apart from source-dependencies/import-class-name-hashing, all source-dependencies/* and unit tests seem to pass. 

Great news!

 

What I've done can be found here : https://github.com/Duhemm/sbt/compare/abstract-over-dependency-kind

Would you prefer that I directly open a pull request to make the discussion easier to follow as I continue working on this ?

Sounds good to me. Let's continue our discussion in a PR.

[Martin Duhem]

I just updated the gist (https://gist.github.com/Duhemm/d58d6217bc47afef2815#file-debug-fail) with the additional logging and submitted the pull request.

[Grzegorz Kossakowski]

It looks like dependencies are not recorded correctly anymore.

Adding `incOptions := incOptions.value.withRelationsDebug(true)` should confirm that as incremental compiler will dump extracted dependencies after each step. It's puzzling that it doesn't always fail, though.

--

You received this message because you are subscribed to the Google Groups "sbt-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sbt-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/sbt-dev/2d66f367-672b-4d2b-9fb6-a03a42662645%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.