utkarshkukreti / markup.rs

A blazing fast, type-safe template engine for Rust.
Apache License 2.0
350 stars 14 forks source link

Clippy deny because of multiple "to_string" implementations #35

Closed friedemannsommer closed 9 months ago

friedemannsommer commented 9 months ago

Running clippy with the latest version (0.14.0) leads to an error because of "inherent_to_string_shadow_display".

Example code

markup::define! {
    Layout<'styles, Children: markup::Render>(
        inline_styles: &'styles str,
        children: Children,
    ) {
        @markup::doctype()
        html {
            head {
                meta[charset="utf-8"];
                meta[name="viewport", content="width=device-width, initial-scale=1, user-scalable=1"];
                title { "Example" }
                style {
                    @ inline_styles
                }
            }
            body {
                @ children
            }
        }
    }
}
Macro expansion ```rust pub struct Layout<'styles, Children: markup::Render> { pub inline_styles: &'styles str, pub children: Children, } impl<'styles, Children: markup::Render> Layout<'styles, Children> { #[inline] #[allow(unused)] pub fn to_string(&self) -> String { let mut string = String::with_capacity(324usize); let _ = ::markup::Render::render(self, &mut string); string } } impl<'styles, Children: markup::Render> ::markup::Render for Layout<'styles, Children> { fn render(&self, __writer: &mut impl std::fmt::Write) -> std::fmt::Result { let Layout { inline_styles, children, } = self; ::markup::Render::render(&(markup::doctype()), __writer)?; __writer.write_str("Example")?; ::markup::Render::render(&(children), __writer)?; __writer.write_str("")?; Ok(()) } } impl<'styles, Children: markup::Render> std::fmt::Display for Layout<'styles, Children> { #[inline] fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { ::markup::Render::render(self, fmt) } } ```
Relevant code sections ### `impl ... to_string(&self) -> String` https://github.com/utkarshkukreti/markup.rs/blob/eb4a8583c45562c40f45702daf7496a7f1349afb/markup-proc-macro/src/generate.rs#L53-L58 ### `impl Display for ...` https://github.com/utkarshkukreti/markup.rs/blob/eb4a8583c45562c40f45702daf7496a7f1349afb/markup-proc-macro/src/generate.rs#L67-L72
Versions * rustc - `1.73.0 (cc66ad468 2023-10-03)` * cargo - `1.73.0` * clippy - `0.1.73` * markup - `0.14.0`
utkarshkukreti commented 9 months ago

Thanks for the detailed report!

I found this issue on the clippy repo. The reason we have a manual to_string() is exactly the same as the creator of that issue explains - we have a faster to_string() implementation which overrides the slower fmt based implementation.

I think we should add a #![allow(clippy::inherent_to_string_shadow_display)] on the generated code.

Kijewski commented 9 months ago

Yes, IMO suppressing clippy::inherent_to_string_shadow_display is right action. Clippy does not take into account that the explicit implementation is there for a reason: to pre-allocate memory for the returned string.

friedemannsommer commented 9 months ago

I found this issue on the clippy repo. The reason we have a manual to_string() is exactly the same as the creator of that issue explains - we have a faster to_string() implementation which overrides the slower fmt based implementation.

I think we should add a #![allow(clippy::inherent_to_string_shadow_display)] on the generated code.

Sounds good to me.

utkarshkukreti commented 9 months ago

I pushed e5cdaaff1598ba337eb07b7ed055cbe24ecbc7bb, let's see if all the CI tests pass on the oldest Rust version we support.

utkarshkukreti commented 9 months ago

@friedemannsommer can you try out 0a31d3da565214e24a527b15c93dd0b02b6a9ae6 and let me know if it works for you? I'll make a release hopefully by tomorrow.

markup = { git = "https://github.com/utkarshkukreti/markup.rs", rev = "0a31d3da565214e24a527b15c93dd0b02b6a9ae6" }
friedemannsommer commented 9 months ago

@utkarshkukreti it works without any issue.

Thank you for the quick response and fix :+1:.

Macro expansion ```rust pub struct Layout<'styles, Children: markup::Render> { pub inline_styles: &'styles str, pub children: Children, } impl<'styles, Children: markup::Render> Layout<'styles, Children> { #[inline] #[allow(clippy::inherent_to_string_shadow_display)] #[allow(unused)] pub fn to_string(&self) -> String { let mut string = String::with_capacity(324usize); let _ = ::markup::Render::render(self, &mut string); string } } impl<'styles, Children: markup::Render> ::markup::Render for Layout<'styles, Children> { fn render(&self, __writer: &mut impl std::fmt::Write) -> std::fmt::Result { let Layout { inline_styles, children, } = self; ::markup::Render::render(&(markup::doctype()), __writer)?; __writer.write_str("Example")?; ::markup::Render::render(&(children), __writer)?; __writer.write_str("")?; Ok(()) } } impl<'styles, Children: markup::Render> std::fmt::Display for Layout<'styles, Children> { #[inline] fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { ::markup::Render::render(self, fmt) } } ```
Versions * rustc - `1.73.0 (cc66ad468 2023-10-03)` * cargo - `1.73.0` * clippy - `0.1.73` * markup - [`0a31d3da565214e24a527b15c93dd0b02b6a9ae6`](https://github.com/utkarshkukreti/markup.rs/tree/0a31d3da565214e24a527b15c93dd0b02b6a9ae6)
utkarshkukreti commented 9 months ago

0.15.0 has been published.