-
Notifications
You must be signed in to change notification settings - Fork 34
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
Replace macros with codegen - POC/WIP #632
Conversation
Created a compiler plugin (not an sbt plugin) to generate managed sources from the @service definitions (here @serviceCodegen to preserve the originals) without using macros but by reusing most of the macro's implementations. Note that this requires a 2-subproject setup so that the app's compilation can see the sources generated by the protocol's compilation. This is shown in example/codegen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @L-Lavigne , this is definitely a great step forward in the project! I left some minor code formatting suggestions and a couple of questions, one of them related to the two-steps code generation stages that we will here.
Probably, we'll want to provide the compiler plugin automatically when you are including the sbt-mu-idl-gen
sbt plugin in the project. Would it make sense?
.settings( | ||
scalacOptions ++= Seq( | ||
// recompile whenever plugin changes (in IDEA this might require using sbt shell for imports & builds) | ||
"-Jdummy" + (`common`/Compile/packageBin).value.lastModified, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"-Jdummy" + (`common`/Compile/packageBin).value.lastModified, | |
"-Jdummy" + (`common` / Compile / packageBin).value.lastModified, |
scalacOptions ++= Seq( | ||
// recompile whenever plugin changes (in IDEA this might require using sbt shell for imports & builds) | ||
"-Jdummy" + (`common`/Compile/packageBin).value.lastModified, | ||
"-P:service-generator:outputDir=" + (Compile/sourceManaged).value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"-P:service-generator:outputDir=" + (Compile/sourceManaged).value | |
"-P:service-generator:outputDir=" + (Compile / sourceManaged).value |
.settings(noPublishSettings) | ||
.settings(moduleName := "mu-rpc-example-codegen-app") | ||
.settings( | ||
Compile/sourceGenerators += Def.task(((`example-codegen-protocol`/Compile/sourceManaged).value ** "*.scala").get) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compile/sourceGenerators += Def.task(((`example-codegen-protocol`/Compile/sourceManaged).value ** "*.scala").get) | |
Compile / sourceGenerators += Def.task( | |
((`example-codegen-protocol` / Compile / sourceManaged).value ** "*.scala").get) |
private lazy val serviceFilter = new FilterTreeTraverser({ | ||
case md: MemberDef => | ||
findAnnotation(md.mods, "serviceCodeGen").isDefined // TODO: rename to "service" | ||
case x => false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case x => false | |
case _ => false |
parsedOptions.getOrElse( | ||
optionName, | ||
throw new IllegalArgumentException( | ||
s"Plugin $name missing option $optionName.\n${optionsHelp.get}")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid this Option.get
?
} | ||
} | ||
|
||
case class HttpOperation(operation: Operation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll want to split this into client and server classes for better readability.
Would it make sense to split the Http part too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely! Currently we have a monolithic straight-up copy of the current macro output but there's lots of improvements we can make so it becomes more user-friendly.
Also moving off macros might help resolve the issue that led us to put the HTTP stuff in there in the first place, where we couldn't get two processors for the same macro applied from different classes.
|
||
@message case class Hello(words: String) | ||
|
||
@serviceCodeGen(Protobuf) trait ExampleService[F[_]] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how the code generation would behave when we would be generating code in two different stages:
- From IDL to Scala code (scala files with
@serviceCodeGen
) (sbt-mu-idl-gen) @serviceCodeGen
to the expanded version of the Scala Code (scala compiler plugin)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried it yet but I suppose the SBT plugin generates the @service
-annotated sources from IDL, and then the compiler plugin generates the service infrastructure from those sources afterwards.
We could explore combining these two steps into one, however I don't think that would work since the developer needs the intermediate @service
-annotated traits in order to implement their methods.
The compiler plugin processes the |
Note that the CI build is failing because the generated sources are either not created by the example protocol or not seen by the example app. It Works on my Machine (™) so I'll investigate what happened there. |
@L-Lavigne CI is failing because you need to |
That should be triggered by the root project's compile though, and it should trigger compilation of Still not sure why this is the case. Locally it works although I do recall needing a full project re-import in IDEA at some point. I don't see previous build state being much of an issue for CI though. |
This doesn't compile for me when using sbt console and running The reason is that the task at https://github.com/higherkindness/mu/pull/632/files#diff-fdc3abdfd754eeb24090dbd90aeec2ceR496 is evaluated before the plugin generates ExampleServiceObject.scala. You can see this, if you use:
The list of files is empty and is printed before the compilation of The example works fine this way:
But it doesn't seem to be a good solution. |
Just a follow-up, is this PR still in progress? It would be nice if the macro can be replaced by the codegen because it is very hard to intelliJ to resolve those macros. |
Closing. We did end up going with srcgen, but not quite like this. |
This PR introduces a work-in-progress compiler plugin (not an sbt plugin) to generate "managed" sources from the
@service
definitions (here@serviceCodegen
, to preserve the originals) without using macros but by reusing most of the existing macro implementation. Note that this requires a 2-subproject setup so that the app's compilation can see the sources generated by the protocol's compilation. This is shown inexample/codegen
.Please refer to #631 for the rationale behind this and the pros & cons of this approach.
How it works: Compiling
example/codegen/app
should trigger compilation ofexample/codegen/protocol
which will generate the object in the protocol module'starget/scala-2.12/src_managed
for the app module to refer to. The generator is in thecommon
module'sServiceGenerator.scala
and basically wraps a modified copy of theinternal
module'sserviceImpl
inside a compiler plugin instead of a macro.Note the following current limitations and remaining TODOs:
common
project (where the protocol keywords are) instead of depending on it, because of the limitations described in Better support for compiler plugins that have dependencies sbt/sbt#2255. We should move it to a different module and implement a workaround based on sbt-assembly.@serviceCodegen
to preserve the existing@service
macro's functionality; the goal if we adopt this approach is to replace the@service
implementation.${service}Object
and contains everything the macro currently generates in the service companion; we'll want to split this into client and server classes for better readability.example/codegen
package to match the example modules, but we'll change it to the actual service's package.Comments, questions and ideas welcome!