Skip to content
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

Use scalameta for codegen #205

Merged
merged 11 commits into from Jan 7, 2020
Merged

Use scalameta for codegen #205

merged 11 commits into from Jan 7, 2020

Conversation

cb372
Copy link
Member

@cb372 cb372 commented Jan 3, 2020

I find this easier to work with than the existing printer, which is basically doing fancy string concatenation, because scalameta gives us a nice AST with which to model the code. It also provides quasiquotes, which are very convenient, and it indents the code correctly for us.

(Unfortunately we end up losing a lot of the type information when we do a catamorphism, and we have to add it back in using casting. I haven't found a nice way around this. Open to suggestions.)

The codegen takes a stream type constructor as an argument, allowing the sbt plugin to choose between FS2 and Monix for streaming more cleanly than it does currently. It also includes an import higherkindess.mu.rpc.protocol._ statement so we don't have to add the import in the sbt plugin.

If people are happy with the scalameta approach, I suggest we remove higherkindness.skeuomorph.mu.print and update the Mu sbt plugin to use the new scalameta-based codegen. In future we could also migrate the OpenAPI codegen to scalameta. In this PR I've kept higherkindness.skeuomorph.mu.print but added a @deprecated annotation.

I find this easier to work with than the existing printer, which is
basically doing fancy string concatenation, because scalameta gives us a
nice AST with which to model the code. It also provides quasiquotes,
which are very convenient, and it indents the code correctly for us.

(Unfortunately we end up losing a lot of the type information when we
do a catamorphism, and we have to add it back in using casting. I
haven't found a nice way around this. Open to suggestions.)

If people are happy with this approach, I suggest we remove
`higherkindness.skeuomorph.mu.print` and update the Mu sbt plugin to use
the new scalameta-based codegen. In future we could also migrate the
OpenAPI codegen to scalameta.
Copy link
Member

@BenFradet BenFradet left a comment

Choose a reason for hiding this comment

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

the scalameta approach does look cleaner to me than the current printer. It's a shame for the casting though :(

val codegen: higherkindness.skeuomorph.mu.Protocol[Mu[MuF]] => String = {
p: higherkindness.skeuomorph.mu.Protocol[Mu[MuF]] =>
val tree = higherkindness.skeuomorph.mu.codegen.protocol(p, streamCtor)
//println(tree.structure)
Copy link
Member

Choose a reason for hiding this comment

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

🎨

streamCtor: (Type, Type) => Type.Apply
)(implicit T: Basis[MuF, T]): Pkg = {
val packageName = protocol.pkg.getOrElse("proto")
val packageTerm = packageName.parse[Term].get.as[Term.Ref]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with the scalameta API but is the get safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope! Need to have a think about error handling. Maybe the whole thing should return an Either. That would also mean we could make the casts slightly more palatable by making them return an Either instead of throwing an exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Everything now returns Either[String, ?].

@cb372 cb372 requested a review from pepegar January 3, 2020 19:17

val expected = expectation(ct, useIdiom)

(actual.clean must beEqualTo(expected.clean)) :| s"""
Copy link
Member Author

Choose a reason for hiding this comment

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

Rather than stripping whitespaces and doing string comparison, we can parse the expected output into a scalameta tree and compare trees instead.

Everything now returns an Either[String, ?]
This is to check that the `knownCoproductTypes` optimization is being
applied correctly.

Also removed the test for printing a protocol, as I'm about to deprecate
that feature.
This is so it will be executed even if you call `codegen.schema`
directly instead of calling it via `codegen.protocol`.
@cb372 cb372 changed the title Proof-of-concept using scalameta for codegen Use scalameta for codegen Jan 6, 2020
Forgot it was also used to import things like the names of serialization
types, compression types, etc. It's easier, cleaner and less error-prone
to import the package once than remember to fully qualify everything in
that package.
@cb372
Copy link
Member Author

cb372 commented Jan 6, 2020

I'm pretty happy with this now. I've requested a review from @pepegar as he wrote the original printer. @juanpedromoreno Do you want to have a look too?

Copy link
Member

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

👌


val avroSchema: Schema = new Schema.Parser().parse(definition)

val toMuSchema: Schema => Mu[MuF] =
scheme.hylo(transformAvro[Mu[MuF]].algebra, fromAvro)

val printSchemaAsScala: Mu[MuF] => String =
muprint.schema.print _
codegen.schema(_).right.get.syntax
Copy link
Member

Choose a reason for hiding this comment

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

I know this is an example, but, does it make sense if we show something like this? (taking into account that Either is right-biased):

codegen.schema(_).map(_.syntax)

@cb372
Copy link
Member Author

cb372 commented Jan 7, 2020

@pepegar I'm going to merge this, but feel free to review later if you have time. I'm happy to make another PR to address any comments.

@pepegar
Copy link
Member

pepegar commented Jan 7, 2020

Sure thing, sorry @cb372 , I reviewed today mornign but didn't submit the approved! Great job :)

@cb372 cb372 merged commit a74e917 into master Jan 7, 2020
@cb372 cb372 deleted the code-gen-with-scalameta branch January 7, 2020 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants