Derive Macro for typespec_client_core::http::Model

[analogrelay]

This PR adds a new typespec_derive crate. This crate provides a derive macro for the Model type that replaces the json_model and xml_model declarative macros I added in #1724 . The generated code is essentially the same. Instead of two separate macros to choose formats, you can use #[typespec(format = "xml")] to change the format to xml (and other formats in the future. Primarily to support testing inside the crate, I also added support for an attribute to set the crate that contains Model, based on serde's equivalent.

I was going to have the azure_core crate re-exports this macro, like it re-exports the Model trait. However, there's a problem. You need to reference typespec_client_core in order to get the Model trait in scope. We re-export that trait from azure_core but the generated code won't use that, it'll use typespec_client_core. That can be overridden with a #[typespec(crate = "azure_core")] attribute, but that would require clients that don't reference typespec_client_core directly to put that attribute on every model type.

Instead, I'm re-exporting it from typespec_client_core. This means that service client crates now need to add a reference to typespec_client_core directly, but I think that's reasonable. It's not like crates can avoid being aware of typespec, or it's related libraries.

There are some alternative approaches we could take, but I'm not a huge fan of those. For example, we could make the macro reference Model from azure_core and move it to an azure_derive crate, but that means it's no longer available in the unbranded libraries. Or we could do something even funkier and find a way to "re-wrap" the proc_macro defined in typespec_derive in a new macro in azure_derive that sets the crate. That feels really clunky and difficult to achieve. So, I've stuck with this approach for now.

[heaths]

Do we even need to re-export it from typespec_client_core? I do that with Error and friends at

azure-sdk-for-rust/sdk/typespec/typespec_client_core/src/error/mod.rs

Line 11 in c1065e8

pub use typespec::error::*;

but I've been rethinking that. Is there any advantage? typespec is a dependency and, conceptually, a package deal with typespec_client_core already. The idea for re-exporting from azure_core was trying to hide the "typespec"iness, though rust-analyzer seems to know better when suggesting to import types.

@analogrelay, @RickWinter, @JeffreyRichter, @LarryOsterman thoughts about re-exporting typespec types from typespec_client_core? I'm leaning against it since you're already in "typespec" land and it's just more to maintain. I still think - for now, at least - we re-export types moved from azure_core from azure_core, but I at least want to move them around like with typespec and typespec_client_core so we better segment protocols and have fewer submodules people have to hunt through.

[analogrelay]

Do we even need to re-export it from typespec_client_core?

We don't need to, but it does provide one advantage. I've noticed that generally when you see a deriveable trait at path foo::bar::SomeTrait, the derive macro is also placed at the same path. This is valid because the derive macro is actually called something like model_derive and the ability to call it as #[derive(Model)] is syntactic sugar. This makes it so you only need to import the one "symbol" and you get both the trait and the derive macro in scope:

use serde::Deserialize;
use typespec_client_core::http::Model;

#[derive(Model, Deserialize)]
pub struct MyModel {}

If we don't re-export the derive macro next to the trait (which is actually where I started in an earlier iteration of this branch), then you have to import them separately. Now, if you're just deriving the trait, you don't actually have to import the trait, so you can do this:

use serde::Deserialize
use typespec_derive::Model;

#[derive(Model, Deserialize)]
pub struct MyModel {}

If you later want to use Model as a trait, you end up with some duplication (which the compiler should handle fine but looks odd):

use serde::Deserialize
use typespec_derive::Model;
use typespec_client_core::http::Model;

#[derive(Model, Deserialize)]
pub struct MyModel {}

fn do_something(m: impl Model) {}

The trick is that the symbol Model in #[derive(Model)] needs to resolve to the derive macro, not the trait.

thoughts about re-exporting typespec types from typespec_client_core?

In general, I'm against re-exporting unless there are specific reasons to do it. But I've also lived a life forever tainted by having to work on NuGet dependency algorithms, so I like to avoid hiding dependencies as much as possible 😅 .

I will say, the layering felt backwards to me when I saw it. I initially assumed that typespec was a superset of typespec_client_core and contained and re-exported all the types from that (and a future typespec_server_core?). So, there's definitely some potential for confusion there already.

My inclination is to start with as few re-exports as possible and see what customers think. It's generally an additive change to re-export a type elsewhere after you've shipped it, but a breaking change to stop re-exporting a type.

[heaths]

typespec is the "core" because:

1. It will serve as the root namespace when https://rust-lang.github.io/rfcs/3243-packages-as-optional-namespaces.html is fully implemented.
2. We may one day have a typespec_server_core for writing services.

As for re-exports, I may just end up removing most. I still want to keep them in azure_core for now, though, even if rearranged after the first beta (no point in breaking things now). I'm still holding onto hope that, to @RickWinter's pleasure, I can figure out someway to get rust-analyzer to recommend, e.g., using azure_core::Result instead of typespec::error::Result or even typespec::Result.

It does make me want to revisit preludes. We opted against them since they seem to be falling out of favor. Probably won't change our minds, though.

[analogrelay]

My personal preference here would be to just drop the re-exports here and have client code reference typespec_client::Response, typespec_client::Model, etc. but I'm open to whatever. I do think re-exporting the derive macro is valuable though, for the reasons I described above. Procedural Macros are required to be in a separate crate, so we're forced to put it in typespec_derive, but as I noted above I think it's very helpful to have the macro made available at the same path as the trait.

[heaths]

This is looking great but while I see nothing truly blocking, I expect enough changes I want to look at this again.

[analogrelay]

I believe all the feedback has been resolved. In addition, I've added tests that validate compilation errors. I was originally looking at compiletest_rs for this (which is an extraction of the compiletest utility used in rustc itself to test errors) but ran into some issues getting dependencies right. I realized it was actually fairly easy to just have a Cargo project that produced all the expected errors and then write a standard integration test that ran cargo build and validated the results against baseline files we check in. It'll be a little bit harder to review changes than with compiletest, but it works.

[heaths]

There are some build failures. Rebase on feature/track2 as well, which should fix one of them that @hallipr fixed and I merged just now. You can also update .vscode/cspell.json from within VSCode or easy enough manually. Or inline it with // cspell:ignore <word> if not universal. VSCode has a cSpell extension I highly recommend installing, especially if you write documentation. learn.microsoft.com requires it, for example.

[analogrelay]

I'm hitting a strange difference between the GitHub and AzDO builds. The GitHub build uses cargo test --all to run tests (as a reminder, --all is not the same as --all-features, it is equivalent to --workspace and builds all packages in the workspace, from here on I'll use --workspace to make that distinction clear). The AzDO build iterates through each package and runs cargo test --doc on each.

I don't understand why, but there's a difference in enabled features between these two options when running rustdoc. When running doctests through cargo test --workspace the rustdoc invocation looks like this (irrelevant options removed):

   Doc-tests typespec_client_core
     Running `/home/ashleyst/.rustup/toolchains/1.76.0-x86_64-unknown-linux-gnu/bin/rustdoc ... --cfg 'feature="default"' --cfg 'feature="derive"' --cfg 'feature="http"' --cfg 'feature="json"' --cfg 'feature="reqwest"' --cfg 'feature="reqwest_gzip"' --cfg 'feature="reqwest_rustls"' --cfg 'feature="tokio"' --cfg 'feature="tokio_sleep"' --cfg 'feature="xml"' --error-format human`

Note that --cfg 'feature="derive"' is present

When building the package directly, without --workspace, the rustdoc invocation looks like this (again, irrelevant options removed):

   Doc-tests typespec_client_core
     Running `/home/ashleyst/.rustup/toolchains/1.76.0-x86_64-unknown-linux-gnu/bin/rustdoc --cfg 'feature="default"' --cfg 'feature="http"' --cfg 'feature="json"' --cfg 'feature="reqwest"' --cfg 'feature="reqwest_gzip"' --cfg 'feature="reqwest_rustls"' --cfg 'feature="tokio"' --cfg 'feature="tokio_sleep"' --error-format human`

Note that this time --cfg 'feature="derive"' is absent.

This impacts the doctests for Response::deserialize_body and Response::deserialize_body_into. Those tests use the Model trait and the derive macro. When built with the derive feature, they can import use typespec_client_core::http::Model and get both the trait and the derive macro. When built without the derive feature, they have to also import use typespec_derive::Model to get the derive macro.

Unfortunately, if built with the derive feature, they cannot import the derive macro because it creates an ambiguity (the derive macro is imported from both typespec_client_core and typespec_derive).

For now, what I've done is just to fix the doctest. I can conditionally import typespec_derive::Model only when the derive feature is not present, which fixes the build in both cases. But I'm still unclear as to why the builds have different sets of features.