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.

[xFrednet]

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.

Even if it's very unlikely, would it be wrong to add the #[must_use] attribute in those cases? 🙃

[dhardy]

My 2c:

• fn create_child(&self, name: &str) -> Self is "obviously" a constructor, therefore has no more need of #[must_use] than new (which isn't linted)
• fn set_foo(mut self, foo: &Foo) -> Self is poor naming convention: "setters" usually modify self. I have many similar methods, e.g. see TextButton: with_color naming convention more obviously conveys usage, while on_push is still not completely clear... both, of course should carry #[must_use] (to do... on_push is not covered by this lint, but maybe could be since ret value is re-parametrisation of base type)

[martinvonz]

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.

Even if it's very unlikely, would it be wrong to add the #[must_use] attribute in those cases? 🙃

No, but it also wouldn't be wrong to add it on pretty much every function taking &self. I just think it is too annoying to have to add it in these places.

[martinvonz]

• fn set_foo(mut self, foo: &Foo) -> Self is poor naming convention: "setters" usually modify self.

Ah, good point (I should fix that in my project). However, the point still stands if the function is called with_foo, I think, unless we want Clippy to treat with_ functions differently.

[camsteffen]

I think it is very subjective whether or not adding #[must_use] is worth the effort. For instance if I am creating an API that will only be used a few times within a project where I have complete control of the code, I probably won't worry about it and would be bothered to get warnings about it. And there's an argument that the "must-usedness" of a function is sometimes so extremely obvious that #[must_use] feels over-cautious. Basically I think #[must_use] is an optional feature and there is nothing wrong with choosing not to use it. So I guess I'd lean towards pedantic for the category.

[GuillaumeGomez]

It's the point of this lint: it's not subjective and induced mistakes on some API usage. If it's not emitting a warning by default, the number of people using it by default will be very low. Important to be noted: it doesn't warn if the item isn't publicly exported, so in your case, it should be fine.

If you don't want/care about #[must_use], simply ignore the lint on your crate I guess?

[martinvonz]

It's the point of this lint: it's not subjective and induced mistakes on some API usage.

Can you share some examples? I think the only example I've seen was f64::floor(self). Are there examples whereSelf doesn't implement Copy?

[GuillaumeGomez]

I'm not sure if personal experience count. My logic is as follow: if it happened to me, it might happen to others. To expand a bit more: constructors aren't impacted, types having #[must_use] (mostly builders) aren't impacted either. That leaves very few possibilities. If you really don't want to add these #[must_use] (even if they make sense?), then simply ignore the lint. I still think it'll positively impact most crates and allow to avoid some cases like I did.

[martinvonz]

I'm not sure if personal experience count. My logic is as follow: if it happened to me, it might happen to others.

Can you link to the function you're thinking of? If it's not open-source, can you describe the case?

If you really don't want to add these #[must_use] (even if they make sense?), then simply ignore the lint.

I have done that but it's the first Clippy warning I have disabled.

I'd be all for enabling the lint by default if it applied only to Copy types. Maybe I'm wrong about that and there are cases where it's easy to forget to use the result in other cases too, but I haven't seen any examples yet (and I can't think of any either).

[GuillaumeGomez]

I don't have the code anymore (it's been years now...). But I can describe it. It was a wrapper for a C library pointer which was returning a copy of the array reversed.

pub struct HandleWrapper(*mut c_void);

impl HandleWrapper {
    pub fn reverse(&mut self) -> Self {}
}

It took me a while to understand that it wasn't modifying my self instance but returning another type. Read the doc I guess? :p

[martinvonz]

Interesting, thanks for sharing.

So what do you think about making this lint trigger only when self is passed as a mutable reference or as a Copy-able value (consumed)?

[GuillaumeGomez]

In my case, it could have been a &self since it's simply wrapping a pointer and I'm not sure it was modifying it. It also wouldn't solve the builder-pattern:

struct Builder;

impl Builder {
    pub fn set_this(self, x: u32) -> Self {}
    pub fn set_that(self, x: u32) -> Self {}
}

It's really tricky overall, which is why we reached this decision.

To quote the end of your commit message:

I don't think I've ever forgotten to use the result of those.

Wouldn't it be better to have the compiler have your back in case you forget? It's all about enforcing good usage of an API through the compiler so error aren't uncovered at the execution time.

[martinvonz]

Wouldn't it be better to have the compiler have your back in case you forget?

It typically already does. If I forgot to use the return value, the code would look something like this:

let builder = ThingBuilder::new();
builder.set_this(42);
# Compilation error saying something like "'builder' was moved on line 2"
let thing = builder.build();

Right?

[GuillaumeGomez]

If your builder doesn't implement Copy, then yes.

[ratmice]

I guess I worry that false positives in this case, can mean not just 'disable this clippy warning', but if you follow clippy's advice, can actually have 2nd order effects by introducing a warning downstream, anyone found a false positive in a popular library?

[GuillaumeGomez]

I tried it on gtk-rs and sysinfo, no false positive there. Curious to hear from others.

[martinvonz]

The first project I looked at was Clap. They addressed this new lint by adding #[must_use] annotations in clap-rs/clap#3251. As you might suspect, I feel like those were not actually useful. Maybe @epage can tell us if Clap has had reports of users forgetting to use the result of those functions?

[GuillaumeGomez]

I think they are useful. The name of these methods don't really make you expect that it's actually returning something. But that's the root of our disagreement. 😄

[martinvonz]

I think my argument has been unrelated to naming. I'm arguing that the compiler already tells you if you mess up, thanks to Rust's ownership rules. (Again, I agree with having this lint for &mut self and cases where Self is Copy.)

[epage]

User expectations have a large impact on usefuless. Most of those clap fixes were on types where the user primarily interacts with it through the must_use functions, leading to an expectation of it and lowering the value. I guess it would help for someone new to clap who is more used to &mut builders. String is an easy counter example where some users use a mix of &self, &mut self, and -> Self. &self is important in this because if a user primarily interacts with &self, they won't have expectations for how mutations are done. Back to that clap PR, one of the must_use was done on clap::Error::format. That is a case where it is more useful because a user won't have expectations for whether that function is &mut self or -> Self.

Overall, I would say this lint will be useful even if there are some cases where the must_use is more obvious.

[martinvonz]

@epage, how do you imagine users "who is more used to &mut builders" might use the API incorrectly? The only way I can think of is like this:

let mut app = App::new("git");
app.arg(App::new("commit"));
let matches = app.get_matches();

However, that fails to compile on line 3 because app was moved on line 2.

In the case of clap::Error::format, its signature is fn format(mut self, app: &mut App) -> Self. Since that consumes self, you would already get a compilation error if you tried to use the error afterwards.

Anyway, I just wanted to make sure you understood my argument. I think you all do by now, so I'll stop nagging :)

[camsteffen]

I believe this is resolved since the lint was moved to pedantic.