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(utoipa-gen): remove unnecessary allocation with to_string in expanded code #982

Merged

Conversation

CBenoit
Copy link
Contributor

@CBenoit CBenoit commented Jul 29, 2024

Taking something that is a string and converting it to a string using to_string() kind of misses the point of why we are doing the conversion in the first place, and also fails to document this to the reader. to_owned() fully captures the reason that a conversion is required at a particular spot: borrowed (&str) to owned (String).

Some people enable this clippy lint in order to enforce this style:

- https://rust-lang.github.io/rust-clippy/master/index.html#/str_to_string

Code generated by utoipa macro is triggering this lint. To avoid that, this patch is replacing the to_string() usages in generated code by to_owned().

This patch is not changing any other to_string() because the utoipa project itself does not need to adhere to this style. The goal is just to reduce friction for downstream consumers.

Here is how it shows up without this fix:

warning: `to_string()` called on a `&str`
   --> devolutions-gateway/src/api/webapp.rs:271:1
    |
271 | / #[utoipa::path(
272 | |     post,
273 | |     operation_id = "SignSessionToken",
274 | |     tag = "WebApp",
...   |
286 | |     ),
287 | | )]
    | |__^
    |
    = help: consider using `.to_owned()`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string
    = note: this warning originates in the attribute macro `utoipa::path` (in Nightly builds, run with -Z macro-backtrace for more info)

There is no lint for the opposite ("use to_string() instead of to_owned()), so it seems safe to apply this fix.

Taking something that is a string and converting it to a string
using `to_string()` kind of misses the point of why we are doing the
conversion in the first place, and also fails to document this to the
reader. `to_owned()` fully captures the reason that a conversion is
required at a particular spot: borrowed (`&str`) to owned (`String`).

Some people enable this clippy lint in order to enforce this style:

- https://rust-lang.github.io/rust-clippy/master/index.html#/str_to_string

Code generated by utoipa macro is triggering this lint.
To avoid that, this patch is replacing the `to_string()` usages in
generated code by `to_owned()`.

This patch is not changing any other `to_string()` because the utoipa
project itself does not need to adhere to this style.
The goal is just to reduce friction for downstream consumers.
@juhaku
Copy link
Owner

juhaku commented Jul 31, 2024

Might be just useful to try not to actually use the .to_string() or to_owned() call in the first place. Mist likely it is relic from older times. But anyhow if it is necessary to have either one then I guess this change for the to_owned() is alright.

utoipa-gen/src/path.rs Outdated Show resolved Hide resolved
utoipa-gen/src/path.rs Outdated Show resolved Hide resolved
@CBenoit CBenoit changed the title fix(utoipa-gen): use to_owned instead of to_string in expanded code fix(utoipa-gen): remove unnecessary allocation with to_string in expanded code Aug 1, 2024
@CBenoit
Copy link
Contributor Author

CBenoit commented Aug 1, 2024

Sounds right! I removed the superfluous allocation.

Copy link
Owner

@juhaku juhaku left a comment

Choose a reason for hiding this comment

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

Great 💯

@juhaku juhaku merged commit 2088259 into juhaku:master Aug 5, 2024
8 checks passed
@CBenoit CBenoit deleted the to-owned-instead-of-to-string-in-generated-code branch August 5, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Released
Development

Successfully merging this pull request may close these issues.

2 participants