add to_string_trait_impl lint

[andrewbanchich]

closes #12076

changelog: [to_string_trait_impl]: add lint for direct ToString implementations

[rustbot]

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

[llogiq]

There are some unrelated tests that get errors because of the lint. This can be worked around by adding suitable #![allow(..)] annotations, then re-blessing the resulting changes.

I do wonder though if this should really be a style lint. If there's any type that one may want to convert to a string, but not for displaying, linting that would constitute a false positive without a way to distinguish from a true one.

[matthiaskrgr]

If I have a type that already implements to_string as well as Display but both in different ways (on purpose), will this lint? Seems like this would be a false positive then.

[andrewbanchich]

@matthiaskrgr that wouldn't be possible since Display automatically implements ToString. trying to impl both would be an error due to conflicting impls.

[matthiaskrgr]

struct A {}

impl std::fmt::Display for A {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
        writeln!(f, "AAAAAH")?;
        Ok(())
    }
}

impl A {
    fn to_string(&self) -> String {
        String::from("oh no")
    }
}

fn main() {
    let a = A {};
    assert_eq!(a.to_string(), format!("{}", a));
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2b630f06f701276154489b12f623423a

[andrewbanchich]

There are some unrelated tests that get errors because of the lint. This can be worked around by adding suitable #![allow(..)] annotations, then re-blessing the resulting changes.

ah, i'll push a fix soon

I do wonder though if this should really be a style lint. If there's any type that one may want to convert to a string, but not for displaying, linting that would constitute a false positive without a way to distinguish from a true one.

i could be missing some context, but the docs sound like there isn't much room for nuance (maybe that's an issue with the docs). is there a good reason one would want to separate conversion logic from displaying logic in this case? users can still impl From<Point> for String if they want to abstract conversions.

[andrewbanchich]

@matthiaskrgr ah, sorry, i misunderstood what you meant. no this will not lint that since it only checks for impl ToString. i can add a test for that if you'd like.

[andrewbanchich]

@llogiq there are a bunch of errors that aren't from this lint. maybe something broke in master?

e.g.

error: unnecessary use of `to_vec`
  --> tests/ui/unnecessary_to_owned.rs:138:13
   |
LL |     let _ = IntoIterator::into_iter([std::path::PathBuf::new()][..].to_vec());
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `[std::path::PathBuf::new()]

edit: weird... master passes all tests... how did this tostring_impl lint cause unrelated lints to trigger?

[andrewbanchich]

@llogiq i added allows for the tests triggered by this new lint, but still not sure why seemingly unrelated lints are triggering now

@matthiaskrgr added a test to show a direct to_string() impl doesn't trigger this

[y21]

You added #[allow] attributes to some of the tests, which shifted the line numbers and so the output from clippy no longer matches with the .stderr file of unnecessary_to_owned. You need to re-bless the tests (cargo uibless) to have it update the expected output.

[andrewbanchich]

got it. thanks @y21

fixed

[andrewbanchich]

ah, actually, looks like there are other areas. will fix those soon.

[andrewbanchich]

hmm, now failures are for the doc comment examples, even though they have no_run 😕 cargo test passed when i ran it locally.

[TennyZhuang]

will to_string_impl be a better name?

[y21]

hmm, now failures are for the doc comment examples, even though they have no_run 😕 cargo test passed when i ran it locally.

no_run tests are still compiled (just not run) ^^

I think to see the error that happens in CI you need to run cargo test in the clippy_lints directory

[andrewbanchich]

@TennyZhuang not sure if there's a formal rule for this, but personally i prefer the current name since ToString has no space and this is not linting direct to_string() function impls.

[andrewbanchich]

all ci checks are passing now @llogiq

[TennyZhuang]

I can’t find a convention rule for clippy, but generally according to rust naming convention, it’s preferred to add an underscore when an UpperCamelCase naming have to be used in a snake_case context.

BTW I can find both styles in clippy.

• clippy::derive_ord_xor_partial_ord
• clippy::derive_partial_eq_without_eq

and

• clippy::filetype_is_file

[andrewbanchich]

hmm, true. on one hand that is for rust code, not clippy names (as you said), but on the other hand it might make sense to try to adhere to that.

what do we think of to_string_trait_impl?

[y21]

that wouldn't be possible since Display automatically implements ToString. trying to impl both would be an error due to conflicting impls.

#12122 (comment)

Note that this is only true without #![feature(specialization)] (example)

The ToString impl for all T: Display is marked as a default fn and can be specialized on. For example, the stdlib has a specialized impl ToString for String because there's no reason for .to_string() on a String to go through heavy fmt machinery (it only needs to be cloned).

Is this an acceptable false positive, considering that specialization is an incomplete nightly feature? 🤔

[andrewbanchich]

that wouldn't be possible since Display automatically implements ToString. trying to impl both would be an error due to conflicting impls.

#12122 (comment)

Note that this is only true without #![feature(specialization)] (example)

The ToString impl for all T: Display is marked as a default fn and can be specialized on. For example, the stdlib has a specialized impl ToString for String because there's no reason for .to_string() on a String to go through heavy fmt machinery (it only needs to be cloned).

Is this an acceptable false positive, considering that specialization is an incomplete nightly feature? 🤔

i'll let others weigh in since i have only a surface level understanding of the mechanics from reading about specialization , but i'd think that users can / should use it to impl a specialized Display instead of ToString. i'd guess std chose to specialize ToString instead of Display since it's the one doing the blanket impl for T: Display.

[andrewbanchich]

actually, thinking about it some more, i don't think my assumption about std's ToString specialization makes sense. wouldn't we just let users add an allow for that since it seems like a reasonable edge case? i.e. isn't it normal for users to mark false positives with allows?

[andrewbanchich]

@TennyZhuang updated name to to_string_trait_impl

[llogiq]

Again, I'm a bit worried that this may cause needless noise at some projects, so I'm going to run a lintcheck first.

Otherwise this should be fine.

[andrewbanchich]

@llogiq any results from the check?

[llogiq]

Sorry, I was quite busy, I'll see if I can run it this evening.

[bors]

☔ The latest upstream changes (presumably #12160) made this pull request unmergeable. Please resolve the merge conflicts.

[andrewbanchich]

@llogiq I'll fix these conflicts once the lintcheck is done

[llogiq]

Sorry it took me so long, but the lintcheck came back negative, which is good. So r=me after a rebase.

[andrewbanchich]

No worries! Thanks. I'll rebase soon.

[andrewbanchich]

all set @llogiq

[llogiq]

Thank you!

@bors r+

[bors]

📌 Commit 6d76d14 has been approved by llogiq

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6d76d14 with merge ea073bd...

[bors]

💔 Test failed - checks-action_test

[llogiq]

Another MacOS Runner failure... 🙃

@bors retry

[bors]

⌛ Testing commit 6d76d14 with merge 79f10cf...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 79f10cf to master...