Make String a diagnostic item

[bluthej]

Proposal

It would be nice if String was a diagnostic item.

Background

I implemented the clear_with_drain Clippy lint for Vecs first, and then proposed to extend that to 5 other container types including Strings. However, in the process I realized that all my container types were diagnostic items except for Strings.

This led me to find a false negative in collection_is_never_read, which was also trying to check for Strings using is_type_diagnostic_item. The current workaround is to use the lang item for String, which means that this type gets a special treatment. I find this a little unfortunate and would like to be able to handle all these container types in the same way 🙂

@llogiq suggested to add a String diagnostic item here so I thought I would open the issue!

Ironically, the String type was the one mentioned as an example in @Manishearth's original proposal #39131, and it was again suggested in this comment. But apparently nobody needed this diagnostic item that bad!

Pros and cons

When working on Clippy lints, one expects the ability to check for such a prominent type using is_type_diagnostic_item, which can lead to false negatives (like I mentioned, even though this was also due to insufficient testing).

The only downside that I can think of is that there would be two different ways to check for the String type, so one can wonder which one one is supposed to use.

[jyn514]

I think this should be fixed in clippy's helper library. There's no reason for an item to be both a lang item and a diagnostic item.

[Manishearth]

I believe when I first proposed diagnostic items String wasn't a lang item and that has since changed.

Yes, for lang items we should just use the lang item.

[bluthej]

Oh I see. Thx for the feedback, I'll close this issue and see if we can find a better workaround on the Clippy side.