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

Fix (and test) codegen issues w/ types that conflict with Rust types inc Result #705

Merged
merged 11 commits into from
Dec 2, 2024

Conversation

anelson
Copy link
Contributor

@anelson anelson commented Nov 30, 2024

I ran into #568 when trying to use typify to generate Rust bindings for the Model Context
Protocol
. Anthropic have published a JSON Schema specification for this protocol, but as soon as I tried to codegen Rust bindings for it, I ran into the aforementioned issue.

I added the entire MCP JSON schema to typify/tests/schemas/model-context-protocol.json, which as expected immediately caused the schemas test to fail. Fixing this was a relatively simple matter of finding every place in the codegen that referred to the Rust std::result::Result type as simply Result, and modify the codegen to use the fully-qualified ::std::result::Result type name instead.. That was enough to make the MCP schema work right.

But then I looked a bit closer and realized there were some other Rust built-in types that were being referenced in such a way as to cause either errors during codegen or generation of code that doesn't compile.

So I made a diabolical test schema rust-collisions.json that is truly cursed. It defines a bunch of types that collide with Rust built-in types and keywords, and made the schemas test explode with such force that there may have been inquiries from the neighbors. Upon reassuring them that that's just how the Rust compiler expresses affection, I made some more codegen changes to resolve all of the issues that my cursed schema exposed, by using fully-qualified type paths ::std::.... for every built-in Rust type that is referenced in the generated code.

I've tried to make the changes minimal, and to my knowledge none of the changes I have made would be breaking from the perspective of any user of the generated Rust code.

I had to modify several existing tests in typify-impl that broke because they were expecting the non-canonicalized versions of Rust standard types. Pay particular attention to the hack I put in test_util::validate_output_impl which is not something I'm particularly proud of but I think it retains the spirit of the test.

Closes #568

@anelson anelson changed the title Fix (and test) codegen issues w/ Rust types inc Result Fix (and test) codegen issues w/ types that conflict with Rust types inc Result Nov 30, 2024
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

Thanks for this. The short of it seems to be to prefix std types with their full path. Any other subtlety beyond the testing kludge?

@@ -373,12 +373,12 @@ pub(crate) fn generate_serde_attr(
let default_fn = match (state, &prop_type.details) {
(StructPropertyState::Optional, TypeEntryDetails::Option(_)) => {
serde_options.push(quote! { default });
serde_options.push(quote! { skip_serializing_if = "Option::is_none" });
serde_options.push(quote! { skip_serializing_if = "std::option::Option::is_none" });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we prefix with :: ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, yes we should. There's also a std::vec::Vec right below it which also needs to be fully qualified.

@@ -17,7 +17,7 @@ schemars = "0.8.21"
semver = "1.0.23"
serde = "1.0.215"
serde_json = "1.0.133"
syn = { version = "2.0.89", features = ["full"] }
syn = { version = "2.0.89", features = ["full", "visit-mut"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this new feature only needed for tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch, I moved that feature to the dev-dependencies.

@@ -23,6 +23,8 @@ fn test_schemas() {
}

fn validate_schema(path: std::path::PathBuf) -> Result<(), Box<dyn Error>> {
println!("Testing the processing of schema: {}", path.display());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to leave this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did mean to since I found it helpful when troubleshooting, but I'll remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it was useful feel free to leave it

&& !path.segments.is_empty()
&& path.segments[0].ident == "std"
{
// Fun additional hack to keep you on your toes:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check the history on that? It might have been during a previous round of prefixing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right, it was done in #647.

The tests that have the fully-qualified type paths in them don't seem wrong or irrelevant. It's not a very common way to declare Rust types outside of proc macros and other codegen, but it's perfectly valid.

I dislike the fact that a special-case is needed, and if anyone ever decides to extend those tests with a field with a fully-qualified type other than HashMap<String, String> then the tests will fail in a way that's probably going to be surprising.

On the other hand, it's actually not a realistic invariant that Rust -> JSON RPC -> Rust transform must always exactly reproduce the input Rust AST. Codegen needs to fully-qualify types that would not normally be fully-qualified. So another solution might be to run the decanonicalization pass on both actual and expected. Now that I think of it, that seems like the better approach. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the idea of running it on both actual and expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just pushed def2173 which does just that. It's definitely better that way.

@anelson
Copy link
Contributor Author

anelson commented Dec 1, 2024

Thanks for this. The short of it seems to be to prefix std types with their full path. Any other subtlety beyond the testing kludge?

Yes that's pretty much the whole fix. A few words to describe, and only +28,792 −13,699 lines to implement ;)

@anelson anelson requested a review from ahl December 1, 2024 18:06
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

  • how do we handle testing
  • are there other functional changes (and if so, can we revert?)

model-context-protocol.json is large; is it adding value beyond what we get from rust-collisions?

Can you please take a look at rustdoc before and after? I don't recall if we'll see simplified or fully qualified types. The latter would be fine... but perhaps distracting / ugly.

Thanks.

// Decanonicalize the types generated by typify so that we can compare them to the original
// Rust types' ASTs that definitely do not use canonicalized std types.
let actual = decanonicalize_std_types(actual);

// Make sure they match.
if let Err(err) = expected.syn_cmp(&actual, ignore_variant_names) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

would another approach be to change the implementation of impl SynCompare for TypePath to be fuzzier i.e. to identify ::std::...::Foo as equivalent to Foo?

Alternatively, I'm inclined towards changing the tests to just use the fully qualified type name.

What do you think of these options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would another approach be to change the implementation of impl SynCompare for TypePath to be fuzzier i.e. to identify ::std::...::Foo as equivalent to Foo?

Yes I suppose you could change the SynCompare impl to strip path components any time it sees ::std:: before the comparison. My personal preference is to not do that, since it's more spooky action at a distance, while this impl is very explicit about what's happening.

Alternatively, I'm inclined towards changing the tests to just use the fully qualified type name.

That would certainly work, but if anyone adds new test cases will they remember that the types have to be fully-qualified, and if they forget will the resulting test failure make it obvious what their mistake was?

It's your codebase, your call, but speaking in terms of my personal preference, I prefer the explicit decanonicalization approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I suppose you could change the SynCompare impl to strip path components any time it sees ::std:: before the comparison. My personal preference is to not do that, since it's more spooky action at a distance, while this impl is very explicit about what's happening.

Fair enough.

That would certainly work, but if anyone adds new test cases will they remember that the types have to be fully-qualified, and if they forget will the resulting test failure make it obvious what their mistake was?

I think so.

It's your codebase, your call, but speaking in terms of my personal preference, I prefer the explicit decanonicalization approach.

Let's go with your approach.

e.to_string(),
)
})
Self::try_from(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer we keep this as parse() -- I have some ambivalence about the use of To/From in these cases.

Are there other instances (I may have missed) where there's a functional change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No that should be the only one. I've reverted this in a subsequent commit.

@anelson
Copy link
Contributor Author

anelson commented Dec 2, 2024

  • how do we handle testing

Sorry, I don't understand what you're asking. I added two additional schema test cases to exercise the problem and verify the fix. They produce code that compiles and passes all existing tests for correctness. Is there some additional testing you have in mind?

  • are there other functional changes (and if so, can we revert?)

I will revert the change to the Deserialize impl for String types. There are no other such changes that I can see.

model-context-protocol.json is large; is it adding value beyond what we get from rust-collisions?

It isn't needed to reproduce the problem with types named Result, no. I added it initially to reproduce that bug, but I kept it there since it seems useful to have as a test case another real-world JSON schema, in case of regressions. You already have the Github and Vega schemas, so I figured there was precedent. I can remove it if you disagree.

Can you please take a look at rustdoc before and after? I don't recall if we'll see simplified or fully qualified types. The latter would be fine... but perhaps distracting / ugly.

Sure.

I ran cargo doc -p example-build from this branch with my codegen changes.

This is the doc for example_build::builder::Veggie:

example_build::builder::Veggie docs

Looks fine to me. The fully-qualified types are simplified in the output. And yet the actual codegen output is still using the canonicalized output:

example_build::builder::Veggie Rust struct

@ahl
Copy link
Collaborator

ahl commented Dec 2, 2024

  • how do we handle testing

Sorry, I don't understand what you're asking. I added two additional schema test cases to exercise the problem and verify the fix. They produce code that compiles and passes all existing tests for correctness. Is there some additional testing you have in mind?

I should have been clearer; I was summarizing my code review feedback.

I will revert the change to the Deserialize impl for String types. There are no other such changes that I can see.

Thanks!

model-context-protocol.json is large; is it adding value beyond what we get from rust-collisions?

It isn't needed to reproduce the problem with types named Result, no. I added it initially to reproduce that bug, but I kept it there since it seems useful to have as a test case another real-world JSON schema, in case of regressions. You already have the Github and Vega schemas, so I figured there was precedent. I can remove it if you disagree.

I'm just a little reluctant to extend testing time (etc.) if there's not a ton of value; please think it over and do what you think is best.

Can you please take a look at rustdoc before and after? I don't recall if we'll see simplified or fully qualified types. The latter would be fine... but perhaps distracting / ugly.

Sure.

I ran cargo doc -p example-build from this branch with my codegen changes.

Thanks for checking on that!

@anelson
Copy link
Contributor Author

anelson commented Dec 2, 2024

I just noticed in my comment #705 (comment), I just happened to pick at random one of the generated builder structs, and there is a String that is not canonicalized!

I looked into it further, it appears that schemas.rs never enabled generating struct builders! I've just made another change to enable struct builders for all schemas. As soon as I did that, the tests started to fail when generating builders for my cursed schema, so then I fixed the struct builder generator output to properly canonicalize String.

Note that as a result of this, the generated code for the test case merged-schemas compiles with warnings:


WARNINGS:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
warning: unused variable: `value`
    --> tests/schemas/merged-schemas.rs:1825:13
     |
1825 |             value: Unsatisfiable2,
     |             ^^^^^ help: if this is intentional, prefix it with an underscore: `_value`
     |
     = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `value`
    --> tests/schemas/merged-schemas.rs:1831:17
     |
1831 |         fn from(value: super::Unsatisfiable2) -> Self {
     |                 ^^^^^ help: if this is intentional, prefix it with an underscore: `_value`

warning: unused variable: `value`
    --> tests/schemas/merged-schemas.rs:1846:13
     |
1846 |             value: Unsatisfiable3,
     |             ^^^^^ help: if this is intentional, prefix it with an underscore: `_value`

warning: unused variable: `value`
    --> tests/schemas/merged-schemas.rs:1852:17
     |
1852 |         fn from(value: super::Unsatisfiable3) -> Self {
     |                 ^^^^^ help: if this is intentional, prefix it with an underscore: `_value`

warning: 4 warnings emitted
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

STDERR:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
warning: unused variable: `value`
    --> /Users/rupert/sources/typify/typify/tests/schemas/merged-schemas.rs:1825:13
     |
1825 |             value: Unsatisfiable2,
     |             ^^^^^ help: if this is intentional, prefix it with an underscore: `_value`
     |
     = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `value`
    --> /Users/rupert/sources/typify/typify/tests/schemas/merged-schemas.rs:1831:17
     |
1831 |         fn from(value: super::Unsatisfiable2) -> Self {
     |                 ^^^^^ help: if this is intentional, prefix it with an underscore: `_value`

warning: unused variable: `value`
    --> /Users/rupert/sources/typify/typify/tests/schemas/merged-schemas.rs:1846:13
     |
1846 |             value: Unsatisfiable3,
     |             ^^^^^ help: if this is intentional, prefix it with an underscore: `_value`

warning: unused variable: `value`
    --> /Users/rupert/sources/typify/typify/tests/schemas/merged-schemas.rs:1852:17
     |
1852 |         fn from(value: super::Unsatisfiable3) -> Self {
     |                 ^^^^^ help: if this is intentional, prefix it with an underscore: `_value`
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

I'm not sure if this is considered acceptable for the generated code or not. It does not appear to be caused by any of my changes, it's just coming to light now that I've enabled generating builder structs in the test case. To confirm this, I checked out main, added .with_struct_builder(true) to the schemas.rs test, and re-ran the tests; I get the same warnings. It seems like it's an issue outside the scope of this PR but maybe you disagree.

@anelson
Copy link
Contributor Author

anelson commented Dec 2, 2024

I'm just a little reluctant to extend testing time (etc.) if there's not a ton of value; please think it over and do what you think is best.

I did some testing (all on my local M1 Pro MBP).

main branch (without any of my test changes)

Right after a cargo clean:

cargo test -p typify  136.85s user 17.49s system 357% cpu 43.130 total

Re-running cargo test (so no rebuild, just re-running the already-built tests):

cargo test -p typify  4.89s user 3.62s system 73% cpu 11.540 total

this branch with my changes

Right after a cargo clean:

cargo test -p typify  140.43s user 17.88s system 279% cpu 56.583 total

Re-running cargo test (so no rebuild, just re-running the already-built tests):

cargo test -p typify  5.67s user 3.98s system 73% cpu 13.050 total

this branch with my changes, but with the MCP schema removed

Right after a cargo clean:

cargo test -p typify  135.65s user 17.23s system 348% cpu 43.851 total

Re-running cargo test (so no rebuild, just re-running the already-built tests):

cargo test -p typify  5.30s user 4.03s system 72% cpu 12.929 total

Key findings:

  • The changes increase clean build time by 31.2% (+13.5s)
  • The MCP schema accounts for most of this increase (12.7s)
  • Test re-runs are marginally slower (+13.1%, or 1.5s)

So I don't think the MCP schema is providing enough value to justify the increase in build/test time. I'll remove it.

@anelson anelson requested a review from ahl December 2, 2024 18:30
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

some suggestions regarding comments; feel free to consider the spirit and not the literal wording

typify-impl/src/test_util.rs Outdated Show resolved Hide resolved
typify-impl/src/test_util.rs Outdated Show resolved Hide resolved
Comment on lines +1493 to +1499
::std::string::String::deserialize(deserializer)?
.parse()
.map_err(|e: self::error::ConversionError| {
<D::Error as ::serde::de::Error>::custom(
e.to_string(),
)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

rustfmt is happy with this change in indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm surprised as well, but I've run cargo fmt before committing. I just re-ran it again to make sure; no files were changed.

@@ -64,7 +64,8 @@ fn validate_schema(path: std::path::PathBuf) -> Result<(), Box<dyn Error>> {
"std",
typify::CrateVers::Version("1.0.0".parse().unwrap()),
None,
),
)
.with_struct_builder(true),
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this to improve test coverage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See, see my comment #705 (comment) for a detailed explanation. There was one place left in the codegen where String was used, and all of the tests passed because that one place was in the codegen for optional struct builders. So I enabled that for all of the schema tests, the tests started to fail, then I fixed that one remaining use of a simple String type and verified that the tests passed. I left this in place because it seems like these tests are meant to exercise the code generation in its entirety.

@anelson anelson requested a review from ahl December 2, 2024 19:12
anelson and others added 10 commits December 2, 2024 20:15
I ran into oxidecomputer#568 when trying to use typify to
generate Rust bindings for the [Model Context
Protocol](https://spec.modelcontextprotocol.io).  The Anthropic have
published a (JSON Schema specification for this
protocol)[https://github.com/modelcontextprotocol/specification/blob/main/schema/schema.json],
but as soon as I tried to codegen Rust bindings for it, I ran into the
aforementioned issue.

I added the entire MCP JSON schema to
`typify/tests/schemas/model-context-protocol.json`, which as expected
immediately caused the `schemas` test to fail.  Fixing this was a
relatively simple matter of finding every place in the codegen that
referred to the Rust `std::result::Result` type as simply `Result`.
That was enough to make the MCP schema work right.

But then I looked a bit closer and realized there were some other Rust
built-in types that were being referenced in such a way as to cause
either errors during codegen or generation of code that doesn't compile.

So I made a diabolical test schema `rust-collisions.json` that is truly
cursed.  It defines a bunch of types that collide with Rust built-in
types and keywords, and made the `schemas` test explode with such
force that there may have been inquiries from the neighbors.  Upon
reassuring them that that's just how the Rust compiler expresses
affection, I made some more codegen changes to resolve all of the
issues that my cursed schema exposed.

I've tried to make the changes minimal, and to my knowledge none of the
changes I have made would be breaking from the perspective of any user
of the generated Rust code.

I had to modify several existing tests in `typify-impl` that broke
because they were expecting the non-canonicalized versions of Rust
standard types.  Pay particular attention to the hack I put in
`test_util::validate_output_impl` which is not something I'm
particularly proud of but I think it retains the spirit of the test.

Closes oxidecomputer#568
Remove some `dbg!` that I left in by accident.  Revert a couple of
unintnetional changes to the indentation of the JSON in the test.
Based on a PR comment from @ahl.  Remove some ugly special-case
handling, and unconditionally decanonicalize all `std`-prefixed types in
both the generated code as well as the original types' ASTs.  With this
in place, we can expect the two ASTs to match exactly, which some of the
tests assert.
This will result in a lot of other changes in the generated code; those changes are in a separate commit to make code review easier.
This was in the codegen for struct builders, which `schemas.rs` do not enable by default so it didn't fail with my cursed schema.  This change enables generating struct builders for *all* of the test cases in `schemas.rs`, and verifies that the generated code properly compiles now that the remaining `String` is canonicalized.

This also changes all of the generated tests but that will be in a separate commit for easier reviewing.
See the prior commit, Fix (and test) a missing fully-qualified `String` use.

This commit is just the changes to the automatically-generated test outputs.
The bug this originally reproduced is also reproduced by the `rust-collisions.json` schema which is much simpler.  Having the MCP schema slow down clean builds by 31% on my M1Pro MBP, so it's not worth it.
Co-authored-by: Adam Leventhal <adam.leventhal@gmail.com>
@anelson anelson force-pushed the fix/mcp-json-schema-fixes branch from 8e75438 to a0cdeff Compare December 2, 2024 19:16
Copy link
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

one fix, merge Cargo.toml, and we'll merge

Comment on lines 107 to 111
/// Our code generation logic, always canonicalizes
/// any standard type (eg, `Option` is output as `::std::option::Option`), to avoid potential
/// conflicts with types in the JSON schema with conflicting names like `Option`. Unfortunately,
/// this complicates the test cases that start with a Rust type that implements `JsonSchema`, use
/// that to generate a JSON schema for that type, then use typify to generate a Rust binding for
/// that type, and expects that the Rust AST for the source type and the generated type are exactly
/// the same.
/// this complicates the test cases that validate that a round-trip from Rust to JSON back to Rust
/// are exactly the same.
Copy link
Collaborator

Choose a reason for hiding this comment

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

wrap, remove comma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rewrapped and removed the extraneous comma.

I rebased against upstream main to resolve the conflict with Cargo.toml.

Hopefully this is good to go now...

@ahl
Copy link
Collaborator

ahl commented Dec 2, 2024

Thanks so much for this contribution! Greatly appreciated!

@ahl ahl merged commit f409d37 into oxidecomputer:main Dec 2, 2024
4 checks passed
@anelson anelson deleted the fix/mcp-json-schema-fixes branch December 3, 2024 08:17
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.

Objects fields named Result generate code that (generally) won't compile
2 participants