I recently answered a Stack Overflow question where the poster wrote this bit of erroneous code:
let mut enter = String::new();
stdin().read_line(&mut enter).expect(r#"invalid key"#);
enter.trim().to_lowercase();
They expected it to convert enter to lowercase and couldn't figure out why their program didn't work. When I saw this I thought they must have ignored a warning, but no, it turns out to_lowercase has neither a #[must_use] attribute nor an associated Clippy warning.
This led me down a rabbit hole investigating the standard library's usage of the #[must_use] attribute. Overall, I'd say the usage is inconsistent. It's used a lot but also omitted a lot. The guidelines on when to add #[must_use] in the standard library don't set a clear policy on whether it should be used a lot or a little.
What do you think: Should it be used liberally on any function that has no use except for returning a value? Or should it be reserved only for error-prone functions that novices are likely to misuse?
As I see it, it could be helpful on any function that (a) has no mutable args; (b) has a non-unit return type; and (c) has no side effects. Side effects include things like mutating statics, spawning threads, and asserting preconditions.
These are essentially the rules the must_use_candidate lint uses, so... I ran Clippy on the standard library. It turned up nearly 800 matches. Yowza!
Adding that many annotations would be a lot of churn so I wanted to poll the community and see what you all thought should be done (if anything). I pinged some library team folks and they were on board with discussing this here. I'm willing to get into the weeds and work on PRs if there is general consensus.
Also, it's important to note that while it might be annoying to be on the receiving end of new #[must_use] warnings, adding them wouldn't break any stability guarantees.
To help guide the discussion I've extracted a set of examples and grouped them into rough categories.
Transformer "mimics"
Like the monsters that pose as treasure chests in Dark Souls and eat you when you get near, these are newbie traps. They sound like they might mutate their input argument but actually return a new value.
str::to_uppercase(self) -> String;
str::to_lowercase(self) -> String;
str::trim_left(&self) -> &str;
str::trim_right(&self) -> &str;
<[u8]>::to_ascii_uppercase(&self) -> Vec<u8>;
<[u8]>::to_ascii_lowercase(&self) -> Vec<u8>;
i32::swap_bytes(self) -> i32;
i32::rotate_left(self, n) -> i32;
i32::to_be(self) -> i32;
i32::to_le(self) -> i32;
Non-trap transformers
These methods also transform their input, but unlike the "mimics" above they don't trick anybody into thinking they might do it in place.
Option::copied(self) -> Option<T>;
Option::cloned(self) -> Option<T>;
Rc::into_raw(self) -> *const T;
Box::<str>::into_string(self) -> String;
String::into_boxed_str(self) -> Box<str>;
Pure functions
Pure functions have clear inputs and outputs. There's no point in calling them if you're going to ignore the result.
f32::is_nan(self) -> bool;
i32::is_negative(self) -> bool;
f32::sin_cos(self) -> (f32, f32);
slice::is_ascii(self) -> bool;
char::is_control(self) -> bool;
memchr(u8, &[u8]) -> Option<usize>;
memrchr(u8, &[u8]) -> Option<usize>;
Getters
Like the pure functions, why call a getter if you don't check the result? I can't think of a good reason.
into_keys is interesting because it does have the side effect of consuming the map. One could use it to drop the map. But that's a crazy use case, no? They ought to be calling drop.
str::len(&self) -> usize;
str::is_empty(&self) -> bool;
BTreeMap::keys(&self) -> Keys<K, V>;
BTreeMap::into_keys(self) -> Keys<K, V>;
Utf8Error::valid_up_to(&self) -> usize;
Formatter::width(&self) -> Option<usize>;
Used for panics?
It's plausible someone might write a[i] to cause a panic if the index is out of bounds. Is that a legitimate use case or should they get a must use warning?
Index::index(&self, Idx) -> &Self::Output;
Allocating constructors
It's wasteful to allocate a new object just to throw it away.
Vec::with_capacity(usize) -> Vec<T>;
Arc::downgrade(&Arc<T>) -> Weak<T>;
String::from_utf16_lossy(&[u16]) -> String;
Backtrace::capture() -> Backtrace;
mpsc::channel() -> (Sender<T>, Receiver<T>);
Non-allocating constructors
These constructors are cheap. If you don't save the return values it doesn't really cost you much. It's still wasteful, but only minorly so.
Vec::new() -> Vec<T>;
iter:empty() -> Empty<T>;
mem::zeroed() -> T;
str::from_utf_unchecked(&[u8]) -> &str;
MaybeUninit::uninit() -> MaybeUninit<T>;
Instant::now() -> Instant;