rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.27k stars 1.52k forks source link

"this could sometimes panic" lint #959

Open oli-obk opened 8 years ago

oli-obk commented 8 years ago

minimal range analysis (lots of snippets like if x == 0 { return; } x -= 1; around, for example) to eliminate false positives (range analysis by itself can provide useful optimizations) the hardest thing to reason about is that v1.len() + v2.len() can't ever overflow if v1, v2: Vec<T> and size_of::<T>() > 0

idea by @eddyb https://botbot.me/mozilla/rust-internals/2016-05-27/?msg=66824497&page=3

eddyb commented 8 years ago

The context is #33905, for which I made a simple change to warn on all #[inline] or generic functions with overflow checks and then looked at the hundreds of results, which showed some interesting patterns.

@nagisa's dataflow framework should usable for tracking ranges based on comparisons, for cases where checks prevent overflows (e.g. if i < n { n - i }), without much difficulty.

Some of the "won't overflow only because you can't have more allocated bytes in total than isize::MAX" situations are in unsafe code deep in collection implementations, so most code could be free of false positives even without understanding that, but it's not a given.

oli-obk commented 8 years ago

Don't https://internals.rust-lang.org/t/mir-compiler-plugins-for-custom-mir-passes/3166/11?u=ker and https://github.com/rust-lang/rust/issues/34066 basically state that we should not be using MIR for linting?

nagisa commented 8 years ago

The AST passes never were stabilised explicitly either so what is said about the MIR passes was supposed to equally apply to the AST passes too. On Jun 6, 2016 1:48 PM, "Oliver Schneider" notifications@github.com wrote:

Don't https://internals.rust-lang.org/t/mir-compiler-plugins-for-custom-mir-passes/3166/11?u=ker and rust-lang/rust#34066 https://github.com/rust-lang/rust/issues/34066 basically state that we should not be using MIR for linting?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Manishearth/rust-clippy/issues/959#issuecomment-223926092, or mute the thread https://github.com/notifications/unsubscribe/AApc0h0Q-Xp2nBTCcuzWBfK02kUTOU4jks5qI_r5gaJpZM4IoeaJ .

eddyb commented 8 years ago

@oli-obk Well, that doesn't make MIR less stable than all of the currently unstable compiler internals all lints use. There might be a way to expose the CFG and pieces of the instructions in a less unstable manner, but IMO not having that yet shouldn't stop people for building incredibly powerful tools.

oli-obk commented 7 years ago

@Manishearth can we have a T-MIR label?

Manishearth commented 7 years ago

Done. you can add labels in https://github.com/Manishearth/rust-clippy/labels

oli-obk commented 7 years ago

thanks! I thought that was an owner-only-feature.

mcarton commented 7 years ago

FYI, the ability to register MIR passes was removed: https://github.com/rust-lang/rust/pull/40239.

oli-obk commented 7 years ago

That's not an issue. We can get the MIR from the tcx and the body_id.

oli-obk commented 7 years ago

Wrong button