Reconsider return_self_not_must_use?

[dhardy]

Description

From #8071:

I ran the lint on 25 crates with lintcheck and got about 60 reports, the once I've looked at seem to be correct. upside_down_face

While I agree with the notion that the result of methods taking self argument and returning Self very likely should be used, I am less convinced that adding the lint without a period for consideration is appropriate. As noted above, it's high impact. In one project alone, I counted 71 affected methods. Given this, the new lint deserves extra consideration.

Mostly, this lint hits builder methods without side-effects where obtaining the output is the whole point.

#[must_use] was originally a tool for pointing out potentially dangerous behaviour, notably forgetting to check the result of a method returning Result<(), Error>. Using it to point out forgetting something harmless feels like abuse. Clippy is free to complain about generating unused values, but that's not what's happening here: it pushes the lint onto the compiler proper for users of the library using Clippy.

Regarding libstd, I see that std::ops::Add::add does have #[must_use] (since four years ago), and here there is justification: x + 1; looks quite similar to x += 1;. Similarly, fn f64::floor(self) -> Self has #[must_use] (users might assume that the method mutates its own state). In contrast, Option::filter, Option::map, etc. don't (users may abuse side-effects here, but it feels like bad style). Iterator does have #[must_use] on everything. OptionOptions::new has #[must_use] (even though it's a boring constructor). Rc::into_raw doesn't have #[must_use] but should (memory leak), though this wouldn't be picked up by this lint anyway. Duration::from_nanos does even though it's obviously a constructor. So the standard library is not entirely consistent but seems to err on the side of using #[must_use].

Back to this lint, I'm not sure what to think about it. It can't be called a style lint but does it actually prevent dangerous behaviour?

Version

No response

See also

• MCP for adding #[must_use] annotations throughout the standard library

Additional Labels

@rustbot label +C-question

[xFrednet]

Hey, thank you for your question! The lint is especially targeted towards instances like fn f64::floor(self) -> Self. The lint is in the suspicious category, as it is suspicious if Self is returned and not used. I believe it's fine having a new lint that triggers several warnings if they are all reasonable. The goal is not necessary to prevent dangerous behavior. Style lints should catch patterns that go against the Rust style. Now what this exactly is, is up to debate. With those lints, we're hoping that the community agrees with our evaluation.

I, personally, think the lint is reasonable, but I would only use it in real projects and not smaller tests and tinkering projects. We could move it to pedantic to have it allow-by-default. I believe that bigger projects that actively use Clippy also have clippy::pedantic set to warn.

Would you think that having it in a different category might be a better idea? (See README for the categories) Or did I misunderstand your question?

cc @GuillaumeGomez

[dhardy]

After reviewing the 71 cases mentioned above, using #[must_use] was reasonable in all cases. Perhaps I should run CI using both stable and nightly (permitting failures) to allow better handling of new lints.

As for categories, this lint is unusual in that it doesn't concern (possibly) incorrect code in the methods (or library) checked, but in users of those methods/library. None of the existing categories feel appropriate, though it borders on correctness, suspicious, pedantic and nursery (being new). Perhaps a new category aimed at libraries is most appropriate?

[GuillaumeGomez]

I used gtk-rs to develop this lint. Also, the "bigger" goal was to try to enforce the usage of #[must_use] as much as possible whenever it made sense. For a Builder type for example, the Builder type itself should be marked as #[must_use], and therefore none of its methods would trigger this lint.

As for the category, I didn't know which one was the most appropriate. As long as it warns by default, it's fine for me.

(Thanks @xFrednet for the ping!)

[xFrednet]

As for categories, this lint is unusual in that it doesn't concern (possibly) incorrect code in the methods (or library) checked, but in users of those methods/library. None of the existing categories feel appropriate, though it borders on correctness, suspicious, pedantic and nursery (being new). Perhaps a new category aimed at libraries is most appropriate?

Clippy tries to be general enough, that it applies to binary and library crates. The lint is only emitted for public items to only warn if they can be used by other modules. We have some lints which actually work the other way around that they don't trigger for public functions to avoid breaking the API of crates, these checks can be enabled with the avoid-breaking-exported-api configuration. I actually think that suspicious fits quite well, as the linted instances are suspicious but not directly wrong.

Perhaps I should run CI using both stable and nightly (permitting failures) to allow better handling of new lints.

We're trying to improve how we introduce new lints and have less false positives in them. It's always a big help if projects also run Clippy's nightly version and report bugs or make suggestions this way. Also thank you for asking the question 🙃

As for the category, I didn't know which one was the most appropriate. As long as it warns by default, it's fine for me.

This category seems the best to me if we want to have it warn by default, this also makes sense if the goal is to enforce more usage of #[must_use].

For a Builder type for example, the Builder type itself should be marked as #[must_use], and therefore none of its methods would trigger this lint.

I think this is actually an excellent point. Currently, it might not be obvious by the description and lint trigger that this is an option. I'll create a PR to clearly mention this option in the lint description. 🙃

[GuillaumeGomez]

I think this is actually an excellent point. Currently, it might not be obvious by the description and lint trigger that this is an option. I'll create a PR to clearly mention this option in the lint description. upside_down_face

Good idea! For the context: I actually discovered this "corner case" when testing the lint on gtk-rs.

[llogiq]

Should we perhaps extend the lint to find if a type has more than a certain number of Self returning methods and suggest the type be made #[must_use] instead?

[xFrednet]

We could do that by going through an impl block and then checking each function instead of each method like it's currently done. Now I've added a help message suggesting either adding the attribute to the function or type like this:

error: missing `#[must_use]` attribute on a method returning `Self`
  --> $DIR/return_self_not_must_use.rs:17:5
   |
LL | /     pub fn foo(&self) -> Self {
LL | |         Self
LL | |     }
   | |_____^
   |
   = help: consider adding the `#[must_use]` attribute to the method or `Self` type

I'm uncertain if it would be super beneficial to add more complicated logic when we could always have it like this 🤔

[xFrednet]

Done: #8209

[GuillaumeGomez]

@llogiq: I'm not sure if it's the right idea either because it includes an arbitrary check: at which point do we consider the type should be marked as #[must_use]? I think what @xFrednet is a good middle ground: we let people know that both possibilities exist and let them decide which one is the best to their situation.

@xFrednet I would reword it a bit:

help: consider adding the `#[must_use]` attribute to the method or directly on the `Self` type

EDIT: ah my comment arrived too late, I'll comment on the PR then.

[martinvonz]

My feeling is that warning makes sense only when self is consumed and Self implements Copy (like fn f64::floor(self) -> Self mentioned above).

I ran into this warning in a few places when I just upgraded Clippy for my project. Most cases were builders with signatures like fn set_foo(mut self, foo: &Foo) -> Self. It seems unlikely that you'd forget to use the result of such methods because they consume the argument you passed in unless they implement Copy.

Another case where I got the warning was for functions like fn create_child(&self, name: &str) -> Self. What makes the warning more relevant on such a function compared to a function that returns a different type? Shouldn't basically every function that takes a &self have #[must_use] in that case?

I don't know what I think about functions that take &mut self. Perhaps the warning makes sense there too.

Thoughts? I think I'll disable this lint for now (it will be the first one I ignore). Am I just missing something?

[GuillaumeGomez]

The issue is basically the same: if you take self (in any form) and return Self, it's very easy to overlook that self and happened to me a few times.