rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.57k stars 12.47k forks source link

Rust should warn for unused "pure" objects #127685

Open VorpalBlade opened 1 month ago

VorpalBlade commented 1 month ago

Code

use std::path::PathBuf;

fn main() {
    let mut buf = PathBuf::new();
    buf.push("/something");
}

Current output

Compiling playground v0.0.1 (/playground)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.64s
     Running `target/debug/playground`

Desired output

Warning: buf is written to but never read.

Rationale and extra context

Here push is a side effect free operation, it only modifies memory belonging to the object. With some sort of attribute or other annotation it should be possible to tell rustc about this, allowing more warnings about unused objects to be produced. The standard library should use such attributes where appropriate (various methods on String, str, PathBuf, etc).

Do note that for generic types such as Vec<T> this might be tricky, as T would also have to be side effect free (e.g. warning about Vec<i32>::push is good, Vec<MutexGuard>::push is probably not), I'm not sure about the best way to handle that, or if it should just be punted to the future.

Other cases

No response

Rust Version

❯ rustc --version --verbose
rustc 1.79.0 (129f3b996 2024-06-10)
binary: rustc
commit-hash: 129f3b9964af4d4a709d1383930ade12dfe7c081
commit-date: 2024-06-10
host: x86_64-unknown-linux-gnu
release: 1.79.0
LLVM version: 18.1.7

Anything else?

Having a warning for this would have saved me time in the profiler (yes, in a hot loop left over (otherwise refactored) pathbuf construction was actually causing a noticable slowdown in my program).

eggyal commented 1 month ago

Here push is a side effect free operation, it only modifies memory belonging to the object.

Except it may cause the buffer to reallocate, which will affect the allocator and memory pages and operating system and goodness knows what else...

the8472 commented 1 month ago

It seems like it would require something close to a proper effects system to get any decent false-positive and false-negative rates. push isn't even pure, it has heap access and non-termination . But there's more territory between an IO effect and non-termination/heap-allocation: accessing global state. By letting things escape into global state we lose local reasoning whether something will ultimately affect IO.

And beyond effects of a type's own functions, the state of a local object that never escapes can still feed into control flow which can then affect other things. So there'd have be a lot of things that have to be considered "uses" by default. E.g. any unwrap() call would make a thing used, since the unwinding is observable.

VorpalBlade commented 1 month ago

Except it may cause the buffer to reallocate, which will affect the allocator and memory pages and operating system and goodness knows what else...

Yes, and running even entirely pure code on the CPU will slightly age the silicone, and consume power etc. Yet that is fine to optimise away. Why isn't the memory allocator okay to optimise away? Besides, I'm suggesting a lint here, not changes to the optimiser.

No programmer ever had said "hurray, the compiler didn't optimise away / warn about useless work being done".

And beyond effects of a type's own functions, the state of a local object that never escapes can still feed into control flow which can then affect other things. So there'd have be a lot of things that have to be considered "uses" by default. E.g. any unwrap() call would make a thing used, since the unwinding is observable.

Absolutely. unwrap should clearly not be annotated as being side effect free. This would have to be opt-in on specific functions. I'm in two minds about if it should be transitive (kind of like const functions can only call other const functions). On one hand that helps correctness. On the other hand there could be "interior IO mutation" that can't be observed from the outside, and if it is just used for diagnostics we may want to allow that.

Though my understanding is that this already exists (to some extent) in C++ with GCC specific attributes, where you annotate a type as __attribute__((warn_unused)) (see docs). This tells GCC that the type's constructors/destructors are side effect free. This wouldn't work for the current example obviously, since push is not a "constructor"-ish function (also rust has associated functions with a naming conventions, not constructors like C++ does).

However, perhaps some inspiration can be drawn from how does is done in G++ / Clang++?