clippy::return_self_not_must_use is overreaching

[rkuhn]

Using 1.59.0-beta.3, the following illustrates the point:

warning: missing `#[must_use]` attribute on a method returning `Self`
  --> libp2p-streaming-response/src/v2/behaviour.rs:32:5
   |
32 | /     pub fn with_request_timeout(self, request_timeout: Duration) -> Self {
33 | |         Self {
34 | |             request_timeout,
35 | |             ..self
36 | |         }
37 | |     }
   | |_____^
   |
   = note: `#[warn(clippy::return_self_not_must_use)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#return_self_not_must_use

I can see how a method that takes &self and produces Self may be misused (although I don’t think that is compelling enough to warrant a default lint), but the above method is not susceptible to programmer mistakes, thanks to Rust’s affine type system. If you deem this worthy of a fix, I may try my hand at it.

[matthiaskrgr]

@Manishearth can you move this to the clippy repo? Thanks!

[rkuhn]

Oops, sorry for putting it into the wrong place!

[Manishearth]

@matthiaskrgr no longer on core, I don't have admin powers anymore

@Mark-Simulacrum ?

[ratmice]

fwiw there is also seems like a dupe of #8197
Though I was skeptical of this lint in that other bug, I'm happy it was moved to pedantic in #8302
So I wonder if both should be closed?

[rkuhn]

I strongly disagree: #[must_use] is purely extraneous when using the builder pattern, since it doesn’t add any useful aspect to the API. And it is also incorrect when interpreted as an English phrase, since there is no reason why a builder must be used. The #[must_use] annotation has been part of the error handling story of Rust, which is a core aspect of language design, and as such I think it is a valid choice to say that ignoring errors should have a syntactic representation, softly enforced by a lint.

So, “pedantic” would be that this lint needs to be removed :-) (I’m a big fan of using English words as defined in the dictionary, please excuse my humour).

Perhaps we have different perspectives on what a “linter” is. To me, a linter is a useful tool only if it shows me places in my code that are very likely incorrect or where there is a compelling reason to use a different code pattern due to how the language and compiler are designed. If a tool forces me to spend many hours on bringing my projects in line with someone else’s coding taste after every update of my Rust toolchain, then it loses its utility — which is a shame since I’d really like to keep that first part. Having a good linter is of huge benefit to a programming community, and I wouldn’t want to see Rust gradually losing clippy by it losing its way.

---

To make my proposal more explicit:

• the lint should be changed to not trigger on methods that take self, since this is the hallmark of the builder pattern, where the lint has no use
• the lint should remain in the pedantic category afterwards, since the cases where it triggers are by far not of the same severity as ignoring errors

[ratmice]

We don't have different perspective on what a linter is, clippy surpassed this compelling reason argument for me a long time ago. I merely tripped over this lint in other project which does use it and felt like it was excessive and had ramifications for users who don't even use clippy. But if it is in pedantic so people have to enable it explicitly it surpassed my ability to care about it any more.

however keep up the good fight.

[PatchMixolydic]

Personally, having builders tagged as #[must_use] has saved me from many bugs due to my forgetfulness. This may seem redundant in the face of static typing, but I've encountered this primarily with builders that build side effects (such as rustc's DiagnosticBuilder). In this case, not calling emit can be especially bad since it can lead to ICEs.

On the other hand, adding #[must_use] to every single function is excessively noisy. A simpler way to do it is to simply tag the builder itself, which may be a decent improvement to this lint's suggestion:

#[must_use]
#[derive(Clone)]
pub struct DiagnosticBuilder<'a>(Box<DiagnosticBuilderInner<'a>>);

It's questionable if this would be desirable in all cases, though. In one of my projects, I've made the (questionable?) decision to give a non-builder struct builder methods:

use glam::{Quat, Vec3};

#[derive(Clone, Copy)]
pub struct Transform {
    position: Vec3,
    rotation: Quat,
    scale: Vec3,
}

impl Transform {
    pub const fn new() -> Self {
        Self {
            position: Vec3::ZERO,
            rotation: Quat::IDENTITY,
            scale: Vec3::ONE,
        }
    }

    pub const fn with_position(mut self, position: Vec3) -> Self {
        self.position = position;
        self
    }
}

Transform is expected to be used directly for all of your graphic transformation needs, so there's no Transform::build method to forget to call. As such, I don't see a reason for it to be #[must_use]. One could argue that creating a Transform and not using it is useless, but this tends to be the case for most other types, and suggesting to mark those as#[must_use] would cause immense amounts of churn.

Given all of this, I'm partial to demoting clippy::return_self_not_must_use to restriction, as it seems to be way too context-dependent for a standard lint category.

[martinvonz]

There's a bit more discussion about this on #8197. I'll summarize my view here as well:

I think the lint makes sense if Self is Copy and maybe if the argument is passed as mutable reference (&mut self). I don't think it makes sense in other cases.