New lint: pure functions without `#[must_use]`

[llogiq]

I often see rust PRs that mark pure functions as #[must_use], which makes sense, as discarding their results will more often than not be in error. As #[must_use] is now available for user code, we may want to lint functions that should have them.

To make this lintable at all, we need to first define "pure":

A pure function has no mutable argument and does not mutate global state.

Arguments that are mutable: &mut _, &Atomic*, &UnsafeCell, &Cell*, &RefCell*, &Mutex<_>, &RwLock<_>, &Sender, &Receiver

Mutating global state can be done via *Assign ops where self is static mut, direct assignment to a static mut or calling a method on a static mut that takes &mut self. For simplicity, it should suffice if we match those operations on anything that is not local to the body (or the args).

[flip1995]

I would also exclude functions that have an unsafe block anywhere in the body from this lint, since checking what's going on in there isn't reliably possible and there could always be some mutation going on.

[oli-obk]

Wrt unsafe, that feels a bit random. If you call another safe function that does the unsafe for you it won't be triggered.

[jplatte]

Arguments that are mutable: […]

I think for a lint like this to not produce lots of false-positives, it would need to be more clever than having a blacklist of mutable / interior-mutable types. Interior mutability is often not obvious from function signatures alone. There's gtk for example, where most / all widget types have .connect_* methods (see e.g. connect_clicked) that return a value that is almost always ignored, while being pure according to the rules above AFAICT.

[llogiq]

Agreed, I'd also mark callables as impure.

Even then the lint is likely to produce some false positives, but as a pedantic lint it might still be useful.

I'm thinking about omitting the lint for inner functions/methods, because the locality will make errors of that kind unlikely, so the attribute won't give too much benefit.

In other news, I'll be working on this.

[jplatte]

Not sure how much access clippy has to type information, but if "mutable arguments" could be implemented in terms of Freeze (which exists somewhere in the compiler if I read this issue correctly), that would probably drastically reduce (get rid of?) the potential for false-positives.

Agreed, I'd also mark callables as impure.

Why though? Higher-order functions are used most often in functional-style programming, which I'd say could in turn benefit most often from this lint.

[llogiq]

Because the argument may itself be impure, thus calling the function may have side effects by calling its argument.

[jplatte]

Yeah I guess as a conservative start, higher-order functions could be excluded altogether. Still I'm wondering if there isn't some way to reason about function arguments that would make it possible to apply this lint to something like this:

fn somefn<T, F>(mut f: F, mut val: T) -> u32
where
    F: FnMut(T) -> Option<T>,
{
    let mut count = 0;
    while let Some(v) = f(val) {
        val = v;
        count += 1;
    }

    count
}

Because while obviously this function could be used just for its side effects, that doesn't seem like something anybody would do. IMO this function's return value it what gives it meaning. And I think a human writer would most likely add #[must_use] here if they're aware of it, plus that its applicability is clear from just the signature. Maybe there are counter-examples with a similar (the same?) signature that would obviously be useful without their return value being used, but I haven't come up with one yet.