Use syn/quote for codegen

[blinsay]

First of all, thanks for maintaining this crate. It's been really easy to get started with and to use .

I had some ideas for changing how code was generated for services, but wanted to make it easier to change things before I started messing around. I borrowed the syn/quote/prettyplease approach from tonic because I find the quasi-quoted codegen a lot easier to read than template strings to write! - here's an example of some heavy quote usage in tonic-build.

For this PR I tried to keep the structure of lib.rs mostly the same. I'm happy to break it up a bit or get a little more disciplined about idents and types if y'all are interested but I didn't want to presume too much.

[tclem]

👋🏻 @blinsay, thanks for the patch!

I had some ideas for changing how code was generated for services

Can you say a little more about what you'd like to change?

I do appreciate that moving away from the very utilitarian printf code gen is probably a good idea long term. I'm a bit hesitant to spend time on that though without some other benefit, mostly based on my own limited time and other higher priority things demanding attention.

I'd also like to carefully vet each of the 4 new dependencies (even if this is a build time crate) as that can have a pretty big impact on our usage of this library. Adding those means we take on the treadmill of updates, security patches, license compliance, etc (in perpetuity) and that also adds up.

Let's talk larger motivations to see if all this effort is worth it!

[blinsay]

Can you say a little more about what you'd like to change?

I had two semi-related ideas I wanted to mess with:

• I tried defining two proto services in the same package. That generates two constants named SERVICE_FQN and two functions named router, which causes compilation issues. There's a couple of ways it'd be possible to namespace those declarations and I wanted to see if any of them would feel right.

• I've been finding the router registration a bit tedious, and wanted to play with a default way to register a twirp service by just passing in an implementation. An API something like twirp::Router::new().nest_service(my_server_impl) would rule.

I'd also like to carefully vet each of the 4 new dependencies (even if this is a build time crate) as that can have a pretty big impact on our usage of this library. Adding those means we take on the treadmill of updates, security patches, license compliance, etc (in perpetuity) and that also adds up.

Totally get it!

twirp-build already depends on three of the four indirectly - syn is a direct dependency of prost, and quote and proc-macro2 are direct dependencies of prost-derive.

prettyplease isn't pulled in anywhere yet, but is also part of the dtonaly cinematic universe. Tonic depending on it made me feel comfortable suggesting it get pulled in here.

[blinsay]

also wanted to mention: I see the lint failure, but was waiting to clean it up until y'all have made a decision.

[tclem]

• I tried defining two proto services in the same package.

Oh yeah, this doesn't work at all. Big 👍🏻 to fixing that.

• I've been finding the router registration a bit tedious

Agree. We some extra internal code for this as well to register various internal layers and even non-twirp endpoints.

These both sound like excellent motivations. I also had a closer look at the diff and I really like how much this quota approach cleans things up. So much easier to read and maintain. I'm 👍🏻 to moving this forward and getting it merged. Ping me when the lints are green (I might have to approve to get CI to run again) and I'll have a more thorough read-through.

Thanks for the contribution!

[blinsay]

@tclem I'm passing clippy locally, seems like you need to kick something to get CI to run here.

Let me know if you like that expect message - happy to change it to whatever.

[tclem]

@blinsay for some reason I'm unable to get CI to run again (we've hit some bug and I've asked another internal team to take a look). In an attempt to work around: do you mind pushing an empty commit (git commit --allow-empty ...) or amending and force pushing again? We need the pull request synchronize event to actually fire.

[blinsay]

@tclem I tried amending and force-pushing and it looks like we're back where we started :(

[tclem]

But now this nice button re-appeared, so we're unblocked! Thanks!

[tclem]

OK, the new codegen fails in our usage of twirp-rs. I'm not 100% why. It's a little hard to tell from the error message (maybe we can improve that?). We have a lot of protos and at least 2 projects are failing to codegen.

I just changed our dependency to: twirp-build = { git = "https://github.com/github/twirp-rs.git", rev = "83992af8c5003188d3df359598624ff170c7f54b" } (I did the main twirp crate as well, but that shouldn't matter) and tried to build (I wanted to look at the new generated output).

  thread 'main' panicked at /Users/tclem/.cargo/git/checkouts/twirp-rs-cac7260611caa640/83992af/crates/twirp-build/src/lib.rs:140:53:
  generated an invalid token stream: Error("expected identifier or integer")
  stack backtrace:
     0: rust_begin_unwind
               at /rustc/4eb161250e340c8f48f66e2b929ef4a5bed7c181/library/std/src/panicking.rs:692:5
     1: core::panicking::panic_fmt
               at /rustc/4eb161250e340c8f48f66e2b929ef4a5bed7c181/library/core/src/panicking.rs:75:14
     2: core::result::unwrap_failed
               at /rustc/4eb161250e340c8f48f66e2b929ef4a5bed7c181/library/core/src/result.rs:1704:5
     3: core::result::Result<T,E>::expect
     4: <twirp_build::ServiceGenerator as prost_build::ServiceGenerator>::generate
     5: prost_build::code_generator::CodeGenerator::push_service
     6: prost_build::code_generator::CodeGenerator::generate
     7: prost_build::config::Config::generate
     8: prost_build::config::Config::compile_fds
     9: prost_build::config::Config::compile_protos
    10: build_script_build::main
    11: core::ops::function::FnOnce::call_once
  note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[tclem]

Oh, it's just that you can't define multiple rpcs... Here's a branch demonstrating the failure. We should probably improve our example and tests to include this.

[blinsay]

Ope, nice find. The fix was pretty silly. I'm not sure how it compiled in the first place to be honest.

Also added a new commit with a second rpc method defined on the Haberdasher API.

[tclem]

OK. Here's the next issue I run into. Do you mind adding a bit more debugging info to the error message (I believe this is coming from as_path) and I'll run it again. This particular project has way too many protos for me to guess at what the problem is...

  thread 'main' panicked at /Users/tclem/.cargo/git/checkouts/twirp-rs-cac7260611caa640/8b3c6e1/crates/twirp-build/src/lib.rs:19:10:
  twirp-build generated invalid Rust. this is a bug in twirp-build, please file an issue: Error("expected identifier")

[blinsay]

I pushed a hacky branch that hopefully helps. I didn't add it to this branch because I feel like we can do better, but hopefully that gets us enough info keep making progress and I'll think about a better way to do error reporting.

[tclem]

That helped! One problem was actually in the prost generated code; the project was missing a new include! in the right nested module to reference some new types (but the changes on this branch seemed to hide that error entirely).

I didn't read your "hacky branch" in detail, but you actually fixed whatever the codegen issue is here. I can still reproduce the error on 8b3c6e1 (even after fixing the include! issue), but baa5278 codegens and compiles just fine.

[blinsay]

Awesome. I switched from parsing input/output types as syn::Path to syn::Type - I cargo-culted syn::Path from tonic but in hindsight that doesn't make much sense. Let me mess around with error reporting and push something better here.

[blinsay]

Ok, I'm happier enough with that for you to review it. I ran with the pre-parsing stuff, which still calls panic if we can't convert prost's String method types to syn::Types, but now the error message points at the protobuf package.Service/method directly.

[tclem]

@blinsay code looks great, thank you for improving the error messages! I was able to validate by forcing some errors (rpcs that reference messages that aren't defined).

I'm also able to fully upgrade to this branch on our primary projects that depend on twirp-rs with no issues now 🎉

[CleanCut]

This PR was included in v0.8.0, released today.