Emit #[must_use] in Generate new assist

[lnicola]

Closes #11736

[lnicola]

bors r+

[bors]

Build succeeded:

• Rust (macos-latest)
• Rust (ubuntu-latest)
• Rust (windows-latest)
• TypeScript (ubuntu-latest)
• TypeScript (windows-latest)

[matklad]

Hm, I would say it's better to not do this -- this adds to noise, and IDE noise is something which we should proactively fight.

That is, I don't think this #[must_use] brings any significant benefit: in rust-analyzer, we have about 500 fn new, and I don't think we've ever missed must uses on them.

At the same time, #[must_use] certainly brings some amount of noise to every instance where it is used.

Now, if you are something like stdlib, it makes sense to #[must_use] public APIs, but even in case of, eg, liballoc, it seems that only about have of the news are tagged with #[must_use], probably because even there a significant chunk of them are just not public API.

[lnicola]

Maybe it should be configurable? See also https://twitter.com/m_ou_se/status/1481684384096923652 for context.

I don't think the #[must_use]s are doing much, but IDEs are very important learning tools, so it feels right to nudge the user into following the language idioms.

[matklad]

Interestingly, one of my specific concerns here is that new users would a) be forced to learn about using #[must_use] earlier than necessary, b) can be confused about the extent #[must_use] should be used (eg, they can believe that one should always #[must_use], while even for stdlib it's more like only a (public) half of the cases).

I do agree that education is a major goal of an IDE, but we should be mindful of contextlessness, which can lead to confusion.

I would say, for educational purposes:

• we should complete #[must_use] attribute
• maaayeb we should have a 💡 for pub functions which look like they can be #[must_use].

[lnicola]

CC @Walther

[matklad]

🤔 given the stats for std (<50% of new functions are must use), I am inclined to say that we probably don't even need config here.

[lnicola]

What about the others (len, is_empty, is_variant)?

[HKalbasi]

And any emission or suggestion of #[must_use] on functions should be disabled when return type is already #[must_use], which I think this PR doesn't consider?

[lnicola]

Okay, I'll file a revert PR tomorrow morning.

[matklad]

What about the others (len, is_empty, is_variant)?

Similarlish picture of only must-using the public API

λ rg -F 'fn is_empty(' -B 3
src/env.rs
820-    fn len(&self) -> usize {
821-        self.inner.len()
822-    }
823:    fn is_empty(&self) -> bool {
--
861-    fn len(&self) -> usize {
862-        self.inner.len()
863-    }
864:    fn is_empty(&self) -> bool {

src/sys_common/wtf8.rs
536-    }
537-
538-    #[inline]
539:    pub fn is_empty(&self) -> bool {

src/sys_common/process.rs
113-    fn len(&self) -> usize {
114-        self.iter.len()
115-    }
116:    fn is_empty(&self) -> bool {

src/sys/unix/process/process_common.rs
510-    fn len(&self) -> usize {
511-        self.iter.len()
512-    }
513:    fn is_empty(&self) -> bool {

src/sys/sgx/waitqueue/mod.rs
140-        WaitQueue { inner: UnsafeList::new() }
141-    }
142-
143:    pub fn is_empty(&self) -> bool {

src/sys/sgx/waitqueue/unsafe_list.rs
47-        }
48-    }
49-
50:    pub fn is_empty(&self) -> bool {

src/sys/windows/process.rs
874-    fn len(&self) -> usize {
875-        self.iter.len()
876-    }
877:    fn is_empty(&self) -> bool {

src/process.rs
1080-    fn len(&self) -> usize {
1081-        self.inner.len()
1082-    }
1083:    fn is_empty(&self) -> bool {

src/ffi/os_str.rs
755-    #[stable(feature = "osstring_simple_functions", since = "1.9.0")]
756-    #[must_use]
757-    #[inline]
758:    pub fn is_empty(&self) -> bool {

src/io/cursor.rs
257-    /// assert!(buff.is_empty());
258-    /// ```
259-    #[unstable(feature = "cursor_remaining", issue = "86369")]
260:    pub fn is_empty(&self) -> bool {

src/os/unix/net/ancillary.rs
438-    /// Returns `true` if the ancillary data is empty.
439-    #[must_use]
440-    #[unstable(feature = "unix_socket_ancillary_data", issue = "76915")]
441:    pub fn is_empty(&self) -> bool {

src/collections/hash/set.rs
223-    /// ```
224-    #[inline]
225-    #[stable(feature = "rust1", since = "1.0.0")]
226:    pub fn is_empty(&self) -> bool {

src/collections/hash/map.rs
540-    /// ```
541-    #[inline]
542-    #[stable(feature = "rust1", since = "1.0.0")]
543:    pub fn is_empty(&self) -> bool {

Also, there's a general guideline of composability: we don't need to write everything immediately, we should make it easy to compose a sequence of actions into the desired result. In this sense, the fact that we have completion for attributes is more or less it.

Though, it'd be cool to invent some kind of more contextual/interractive UI, which allows you to tweak things like visibility, naming, and attirbutes after you generated the baseline fn.

[lnicola]

I think pattern here is that the attributes were added for public APIs, leaving the private ones unchanged. They might or might not get the attributes if they were written from scratch today.

I wonder if @m-ou-se or the libs team would have an opinion on whether #[must_use] would ideally be applied to user code, including private methods.