Clippy deny because of multiple "to_string" implementations #35

friedemannsommer · 2023-11-22T17:48:10Z

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

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("<html><head><meta")?;
        let __value = "utf-8";
        if ::markup::RenderAttributeValue::is_none(&__value)
            || ::markup::RenderAttributeValue::is_false(&__value)
        {
        } else if ::markup::RenderAttributeValue::is_true(&__value) {
            __writer.write_str(" charset")?;
        } else {
            __writer.write_str(" charset=\"")?;
            ::markup::Render::render(&(__value), __writer)?;
            __writer.write_str("\"")?;
        }
        __writer.write_str("><meta")?;
        let __value = "viewport";
        if ::markup::RenderAttributeValue::is_none(&__value)
            || ::markup::RenderAttributeValue::is_false(&__value)
        {
        } else if ::markup::RenderAttributeValue::is_true(&__value) {
            __writer.write_str(" name")?;
        } else {
            __writer.write_str(" name=\"")?;
            ::markup::Render::render(&(__value), __writer)?;
            __writer.write_str("\"")?;
        }
        let __value = "width=device-width, initial-scale=1, user-scalable=1";
        if ::markup::RenderAttributeValue::is_none(&__value)
            || ::markup::RenderAttributeValue::is_false(&__value)
        {
        } else if ::markup::RenderAttributeValue::is_true(&__value) {
            __writer.write_str(" content")?;
        } else {
            __writer.write_str(" content=\"")?;
            ::markup::Render::render(&(__value), __writer)?;
            __writer.write_str("\"")?;
        }
        __writer.write_str("><title>Example</title><style>")?;
        ::markup::Render::render(&(inline_styles), __writer)?;
        __writer.write_str("</style></head><body>")?;
        ::markup::Render::render(&(children), __writer)?;
        __writer.write_str("</body></html>")?;
        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`

markup.rs/markup-proc-macro/src/generate.rs

Lines 53 to 58 in eb4a858

    
           pub fn to_string(&self) -> String { 
        
               let mut string = String::with_capacity(#size_hint); 
        
               // Ignoring the result because writing to a String can't fail. 
        
               let _ = ::markup::Render::render(self, &mut string); 
        
               string 
        
           }

`impl Display for ...`

markup.rs/markup-proc-macro/src/generate.rs

Lines 67 to 72 in eb4a858

    
           impl #impl_generics std::fmt::Display for #name #ty_generics #where_clause { 
        
               #[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 - 0.14.0

The text was updated successfully, but these errors were encountered:

utkarshkukreti · 2023-11-22T18:16:12Z

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 · 2023-11-22T18:20:13Z

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 · 2023-11-22T18:26:34Z

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.

See #35.

utkarshkukreti · 2023-11-22T18:30:37Z

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

utkarshkukreti · 2023-11-22T19:59:48Z

@friedemannsommer can you try out 0a31d3d 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 · 2023-11-22T20:10:47Z

@utkarshkukreti it works without any issue.

Thank you for the quick response and fix 👍.

Macro expansion

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("<html><head><meta")?;
        let __value = "utf-8";
        if ::markup::RenderAttributeValue::is_none(&__value)
            || ::markup::RenderAttributeValue::is_false(&__value)
        {
        } else if ::markup::RenderAttributeValue::is_true(&__value) {
            __writer.write_str(" charset")?;
        } else {
            __writer.write_str(" charset=\"")?;
            ::markup::Render::render(&(__value), __writer)?;
            __writer.write_str("\"")?;
        }
        __writer.write_str("><meta")?;
        let __value = "viewport";
        if ::markup::RenderAttributeValue::is_none(&__value)
            || ::markup::RenderAttributeValue::is_false(&__value)
        {
        } else if ::markup::RenderAttributeValue::is_true(&__value) {
            __writer.write_str(" name")?;
        } else {
            __writer.write_str(" name=\"")?;
            ::markup::Render::render(&(__value), __writer)?;
            __writer.write_str("\"")?;
        }
        let __value = "width=device-width, initial-scale=1, user-scalable=1";
        if ::markup::RenderAttributeValue::is_none(&__value)
            || ::markup::RenderAttributeValue::is_false(&__value)
        {
        } else if ::markup::RenderAttributeValue::is_true(&__value) {
            __writer.write_str(" content")?;
        } else {
            __writer.write_str(" content=\"")?;
            ::markup::Render::render(&(__value), __writer)?;
            __writer.write_str("\"")?;
        }
        __writer.write_str("><title>Example</title><style>")?;
        ::markup::Render::render(&(inline_styles), __writer)?;
        __writer.write_str("</style></head><body>")?;
        ::markup::Render::render(&(children), __writer)?;
        __writer.write_str("</body></html>")?;
        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

utkarshkukreti · 2023-11-23T07:52:07Z

0.15.0 has been published.

utkarshkukreti added a commit that referenced this issue Nov 22, 2023

add an #[allow] attribute to generated code for false positive in clippy

e5cdaaf

See #35.

utkarshkukreti closed this as completed Nov 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clippy deny because of multiple "to_string" implementations #35

Clippy deny because of multiple "to_string" implementations #35

friedemannsommer commented Nov 22, 2023

`impl ... to_string(&self) -> String`

`impl Display for ...`

utkarshkukreti commented Nov 22, 2023

Kijewski commented Nov 22, 2023

friedemannsommer commented Nov 22, 2023 •

edited

Loading

utkarshkukreti commented Nov 22, 2023 •

edited

Loading

utkarshkukreti commented Nov 22, 2023

friedemannsommer commented Nov 22, 2023

utkarshkukreti commented Nov 23, 2023

Clippy deny because of multiple "to_string" implementations #35

Clippy deny because of multiple "to_string" implementations #35

Comments

friedemannsommer commented Nov 22, 2023

Example code

impl ... to_string(&self) -> String

impl Display for ...

utkarshkukreti commented Nov 22, 2023

Kijewski commented Nov 22, 2023

friedemannsommer commented Nov 22, 2023 • edited Loading

utkarshkukreti commented Nov 22, 2023 • edited Loading

utkarshkukreti commented Nov 22, 2023

friedemannsommer commented Nov 22, 2023

utkarshkukreti commented Nov 23, 2023

`impl ... to_string(&self) -> String`

`impl Display for ...`

friedemannsommer commented Nov 22, 2023 •

edited

Loading

utkarshkukreti commented Nov 22, 2023 •

edited

Loading