Skip to content

Properly interpolate #[openapi]-annotation with TokenStreams#48

Merged
DenuxPlays merged 3 commits intoProbablyClem:mainfrom
domenicquirl:bugfix/token-stream-string-matching
Feb 10, 2025
Merged

Properly interpolate #[openapi]-annotation with TokenStreams#48
DenuxPlays merged 3 commits intoProbablyClem:mainfrom
domenicquirl:bugfix/token-stream-string-matching

Conversation

@domenicquirl
Copy link

Replaces the String-based interpolation of the auto-discovered tokens into the manually annotated attributes with quoting via TokenStreams.

What this PR fixes

When bumping to utoipa v5 / utoipauto v0.2, I noticed I was getting an error in my IDE (VS Code) on the #[utoipauto] annotation. Everything built fine when running tests / just building with cargo, but the macro crashed by unwrapping an Err when run via rust-analyzer.

I was able to narrow the issue down to your handling of the original #[openapi] annotations on the struct deriving OpenAPI: utoipauto does its discovery and then pieces together the automatically discovered components with the existing annotations like modifiers, tags, etc. To do this, update_openapi_macro_attributes takes the whole attribute, converts it to a string via to_token_stream and then tries to strip #[openapi( at the start and )] at the end of the attribute, inserting everything in between into the newly generated annotation that is created as the output of the macro, containing also the automatically discovered paths.

With cargo, this works because the rustc proc-macro server is relatively exact in its to_string() representation of the attribute:

#[openapi(modifiers(&Addon1, &Addon2),\ninfo(title = \"Title\", version = \"v1\",),\nvariables((\"var\" =\n(description = \"Description\")),)),))]

(I've left out a bunch of stuff but you get the idea.)

Compare this to the to_string() representation we get when running under rust-analyzer:

# [openapi (modifiers (& Addon1 , & Addon2) , info (title = \"Title\" , version = \"v1\" ,) , variables ((\"var\" = (description = \"Description\")) ,)) ,))

As you can see, rust-analyzer's proc-macro server inserts additional whitespace in between the tokens inside that make up the token stream, which causes the substrings to no longer match and not be removed, and therefore causes the macro to output something like #[openapi(paths(...), #[openapi(modifiers(...))] )] containing first the auto-discovered paths, followed by the entire original attribute, which fails to re-parse as a TokenStream because attributes are not valid inside attributes.

Given that the impl Display for TokenStream explicitly cautions against matching against its output because it isn't stable by saying

Note: the exact form of the output is subject to change, e.g. there might be changes in the whitespace used between tokens. Therefore, you should not do any kind of simple substring matching on the output string (as produced by to_string) to implement a proc macro, because that matching might stop working if such changes happen. Instead, you should work at the TokenTree level, e.g. matching against TokenTree::Ident, TokenTree::Punct, or TokenTree::Literal.

I'm gonna suggest that this should be considered a bug in utoipauto.

Changes made to fix this issue

The PR changes the interpolation inside update_openapi_macro_attributes to work on TokenStreams instead of Strings. To do this, I had to create a larger change than I'd like, because I had to go back to file_utils::exctract_module_name_from_path to return an actual Path (this also fixes a bug where hyphens were only replaced with underscores in the crate name, but not module names, which for some reason existed in one of the tests), such that discover_from_file can now parse_module_items as Paths, such that update_openapi_macro_attributes can interpolate those paths into the original macro input on the level of TokenStreams / quote. This necessitated also rewriting the extract_ and remove_ functions in attribute_utils to work on syn's Attribute/Meta.

I've ran the changes in the PR against your test suite, including the acceptance tests, and further verified the macro output is unchanged / correct in a regular build in the project I was having the issue in originally. I'll leave a comment for some test changes I had to make as a review.

@domenicquirl
Copy link
Author

(Had to fix a doc test I missed.)

@DenuxPlays
Copy link
Collaborator

Thank you for your contribution.

I am gonna have look at it at the weekend and do some testing.

@DenuxPlays
Copy link
Collaborator

Hey I tested it.

It works great and actually fixes: #46

I just have to make a code review.
Hopefully I find time in like 2 hours or something like that.

Just as an update

Copy link
Collaborator

@DenuxPlays DenuxPlays left a comment

Choose a reason for hiding this comment

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

Just one comment but it looks great.

But in the extract_* functions I am not sure if we should use nested for loops or if we should work with iterators.
Ofcourse both work so it's just a preferene.

But I had something like this in my mind:

fn extract_schemas(nested_attributes: &Punctuated<Meta, Token![,]>) -> TokenStream {
    nested_attributes
        .iter()
        .find_map(|meta| {
            let Meta::List(list) = meta else { return None };
            if !list.path.is_ident("components") {
                return None;
            }
            
            let nested = list
                .parse_args_with(Punctuated::<Meta, Token![,]>::parse_terminated)
                .expect("Expected a list of attributes inside components(...)!");

            nested.iter().find_map(|meta| {
                let Meta::List(list) = meta else { return None };
                if list.path.is_ident("schemas") {
                    Some(list.tokens.clone())
                } else {
                    None
                }
            })
        })
        .unwrap_or_else(TokenStream::new)
}

Maybe @ProbablyClem can say what he wants as I don't really care.
Both work and are readable.

@DenuxPlays DenuxPlays added bug Something isn't working enhancement New feature or request and removed enhancement New feature or request labels Feb 9, 2025
@domenicquirl
Copy link
Author

Addressed your comments and just went ahead with changing the extract_ methods to iterators. It's a bit longer but less indented, so that's nice at least. I also pulled together schemas and responses into a single function as they only differ in the final key / ident that they are looking for.

Happy to hear that this actually fixes the other issue as well ☺️

@DenuxPlays
Copy link
Collaborator

LGTM

Thank you for your contribution.

FYI:
I would like to put this pr through a test phase together with: #47
So this will probably be in the release 0.3.0-alpha.1

@DenuxPlays DenuxPlays merged commit 067fa42 into ProbablyClem:main Feb 10, 2025
7 checks passed
@domenicquirl domenicquirl deleted the bugfix/token-stream-string-matching branch February 10, 2025 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

(Rust-Analyzer) Issue with rust macro utoipauto. Panic on simple structure

2 participants