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.51k stars 1.55k forks source link

restriction lint: subcondition side-effects #2215

Open xd009642 opened 7 years ago

xd009642 commented 7 years ago

So disclaimer, I'm not sure if this is a necessary lint (anyone with input see my rust forum post https://users.rust-lang.org/t/evaluation-order-of-boolean-subconditions/13748). But I have the feeling that boolean subcondition execution order is probably not guaranteed like in C and C++. Therefore, if a user had side-effects in a subcondition program behaviour could change significantly due to something like a changed optimisation flag.

Below is some code which should be warned against if this lint is necessary. If this is an issue I wouldn't mind implementing the lint to help out :smile:

fn side_effect(i: &mut i32) -> bool {
    (*i) += 1;
    true
}

fn main() {
    let mut i  = 4;
    if false && side_effect(&mut i) {
        //Some code
    }
    println!("i is {}", i);
    if side_effect(&mut i) && false {
        //Some code
    }
    println!("i is {}", i);
}
oli-obk commented 7 years ago

Subconditions are ordered in c++, too and often this is required. E.g. foo.len() > 3 && foo[2] is guaranteed panic free.

oli-obk commented 7 years ago

For bit operations instead of short circuiting operations this makes sense though. I think we already have an issue for eval order unclearness

https://github.com/rust-lang-nursery/rust-clippy/issues/1193

https://github.com/rust-lang-nursery/rust-clippy/issues/277

xd009642 commented 7 years ago

Fair enough, I guess this is the issue with spending the day job doing MISRA C you forget what is paranoia and what is language spec :smile: I'm happy closing this issue if you think there's nothing that needs doing

oli-obk commented 7 years ago

I guess this is the issue with spending the day job doing MISRA C you forget what is paranoia and what is language spec

It never hurts to be paranoid. Please bring in your experience with MISRA-C by scrutinizing Rust code! We can really use an experienced eye from mission critical software development.

We could have a restriction lint that disallows side-effects in boolean chains. I think this could often result in a gain in readability. What do you think?

xd009642 commented 7 years ago

True true, when I have time I'll look over my copy of the MISRA standard and open an issue with warning lints related to MISRA violations/warnings. It's a good first step in getting the safety of rust onto things like automobiles etc :smile: