Want to extend `clear_with_drain` to other types of containers

[bluthej]

I implemented clear_with_drain for Vec, and then I had a look at #9059 and saw that manual_retain applies to a bunch of containers (BTW I admire your work on that lint @kyoto7250 :clap:).

So I'm thinking clear_with_drain should probably be extended in a similar fashion to apply to the collections in std that have both a clear and a drain method, i.e.:

• String
• HashSet
• HashMap
• VecDeque
• BinaryHeap

What do you think?

Also note that the documentations for Vec, VecDeque and String explicitly mention that drain with a full range will clear the container, "like clear() does". Does this change anything regarding the category proposed originally in #9339? Style was suggested, but that would mean the examples in the standard library would get warnings :sweat_smile:. Maybe Pedantic is more suitable?

@rustbot label +C-enhancement

[llogiq]

Sure, go for it!

[bluthej]

@rustbot claim

[bluthej]

I got most of it working, but I'm running into an issue regarding Strings.

I can't get it to work in that case because my call to is_type_diagnostic_item does not identify Strings. I looked into is_diagnostic_item and got this debug info:

&cx.tcx.diagnostic_items(adt.did().krate).name_to_id = {
    "BinaryHeap": DefId(5:781 ~ alloc[3758]::collections::binary_heap::BinaryHeap),
    "BTreeEntry": DefId(5:1212 ~ alloc[3758]::collections::btree::map::entry::Entry),
    "LinkedList": DefId(5:3477 ~ alloc[3758]::collections::linked_list::LinkedList),
    "cstring_type": DefId(5:7080 ~ alloc[3758]::ffi::c_str::CString),
    "Vec": DefId(5:6600 ~ alloc[3758]::vec::Vec),
    "Cow": DefId(5:688 ~ alloc[3758]::borrow::Cow),
    "ToString": DefId(5:5549 ~ alloc[3758]::string::ToString),
    "Arc": DefId(5:5705 ~ alloc[3758]::sync::Arc),
    "BTreeSet": DefId(5:3057 ~ alloc[3758]::collections::btree::set::BTreeSet),
    "box_new": DefId(5:296 ~ alloc[3758]::boxed::{impl#0}::new),
    "ToOwned": DefId(5:679 ~ alloc[3758]::borrow::ToOwned),
    "BTreeMap": DefId(5:1329 ~ alloc[3758]::collections::btree::map::BTreeMap),
    "vec_macro": DefId(5:5 ~ alloc[3758]::macros::vec),
    "VecDeque": DefId(5:4219 ~ alloc[3758]::collections::vec_deque::VecDeque),
    "Rc": DefId(5:4722 ~ alloc[3758]::rc::Rc),
    "format_macro": DefId(5:6 ~ alloc[3758]::macros::format),
}

As you can see, the String type is absent from the list. Should it be added? Or should I find another way to identify Strings?

I found another source (collection_is_never_read) that is trying to do the same thing as I am with is_type_diagnostic_item, so I am wondering if it actually works in that lint. I couldn't find any use of a String in the test for collection_is_never_read, so I suspect it might not work either.

[schubart]

Author of collection_is_never_read here. You are right, it gives a false negative for String. Thank you for mentioning this here!

Interestingly, collection_is_never_read does work for Option, which (like String) is not mentioned in your debug output above.

[bluthej]

So I performed the same debug test inside of collection_is_never_read with an Option and the list is totally different. In that case, Option is indeed in the list, which explains why it works. I assume it's a matter of context, but as of right now I haven't figured out where this list is actually defined or why it is context dependent...

Note that I'm getting two different lists in the same test, depending on the expression that is being checked by collection_is_never_read. I get the same list I have in clear_with_drain for all the types currently in the test file, and the list below for an Option. So it seems to be connected to the expression, not to which check I'm performing.

Any help on that would be appreciated :slightly_smiling_face:

&cx.tcx.diagnostic_items(adt.did().krate).name_to_id = {
    "FromResidual": DefId(2:3168 ~ core[9d00]::ops::try_trait::FromResidual),
    "assert_ne_macro": DefId(2:6 ~ core[9d00]::macros::assert_ne),
    // ...
    // Skipped items
    // ...
    "mir_set_discriminant": DefId(2:29856 ~ core[9d00]::intrinsics::mir::SetDiscriminant),
--> "Option": DefId(2:48673 ~ core[9d00]::option::Option),  <--
    "Debug": DefId(2:9011 ~ core[9d00]::fmt::Debug),
    // ...
    // Skipped items
    // ...
    "mir_make_place": DefId(2:29865 ~ core[9d00]::intrinsics::mir::__internal_make_place),
    "Alignment": DefId(2:49672 ~ core[9d00]::fmt::Alignment),
}

[llogiq]

There is a String LangItem. Perhaps that might be of help for both lints?

[bluthej]

@llogiq I guess we can do that, it's just a little unfortunate that we have to use lang items for Strings and diagnostic items for the other types.

[llogiq]

Ok, I'm currently on mobile, but it shouldn't be a big problem to add a String diagnostic item in a Rust PR. That would still mean we'd need the LangItem until the next sync, but it's a start.

[bluthej]

@schubart would you like to fix collection_is_never_read in a separate PR? Otherwise I'd be happy to fix it while I'm at it but I'm not sure it's good practice to be mixing a dev with a fix.

Either way is fine by me, just let me know :)

[schubart]

Please give it go! Thanks very much.