4 ms·

another related lifetime footgun involving these RAII guard objects is that the two following lines are not the same: let _ = mutex.lock(); // un-used, un-

another related lifetime footgun involving these RAII guard objects is that the two following lines are not the same: let _ = mutex.lock(); // un-used, un-named 'hole' variable let _a = mutex.lock(); // un-used, but named 'hole' variable You might use this when the lock is essentially virtual, guarding something that isn't a data object. The first line will immediately drop the lockguard, while the second will leave it existing until the end of the scope. It's fairly subtle that this is the case. Where I actually ran into this was with a profiling timer object that was returning suspiciously short timings. But it could just as easily have been a critical section of some sort.

estebank 4y ago
I thought we had a deny by default lint for dropping a Mutex due to a _ binding. If we don't, definitely should.
- tialaramex 4y ago
  This https://godbolt.org/z/rx3h1rxW7 https://godbolt.org/z/rx3h1rxW7 does not even warn you that we're not actually locking the mutex. There's a correct warning if call lock() without using the result, because it has the must-use annotation, but if we throw it into _ the compiler is satisfied even though that's definitely not what a Mutex guard is for.
  - estebank 4y ago
    On the upper righ side, click on tools and select clippy https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=04f076ee51f96a17a4f35ead257dd773 https://play.rust-lang.org/?version=stable&mode=debug&editio... It will produce the following output: error: non-binding let on a synchronization lock --> src/main.rs:5:5 | 5 | let _ = m.lock(); | ^^^^^^^^^^^^^^^^^ | = note: `#[deny(clippy::let_underscore_lock)]` on by default = help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_lock Clippy is a third party tool, but highly recommended because of things like this. I would like to see this lint uplifted into rustc.
    - tialaramex 4y ago
      Firstly, sincerely thanks for telling me how to add Clippy, that would have been a good idea in this context and now I'll remember it's there. Secondly, yes, I think should be uplifted. I'd even argue for default deny because I can't think of a reason you would want this behaviour and yet not find this more explicit phrasing more maintainable: let guard = m.lock(); Mutex::unlock(guard); // We don't hold the guard, just touch and go [I'm assuming anybody who actually wants this would prefer Mutex::unlock() over drop() because it says what's going on, and if they care that they can't write this in stable their effort would be appreciated pushing Mutex::unlock stabilisation rather than blocking a default-deny]
      - stormbrew 4y ago
        tbh I would use (and I think have used) drop() for this just because I know drop exists and what it does but Mutex::unlock is something I'd only ever know about if I went and scanned the whole Mutex page on rustdoc. I like the idea of these sorts of "drop-with-semantics" functions, but in practice until/unless they add a way to `impl !Drop` (or maybe `#[deprecated]` on a trait impl so you can add a deprecation note on your `impl Drop`) on a type, I'm not sure they're worth it.
- stormbrew 4y ago
  you're right, there is a clippy lint for it on a mutex in particular. Tbh I think it might be good to generalize it somehow? Because I think there are other cases it could bite you than just mutexes. But obviously the whole point of doing that in the first place is usually to avoid a must-use warning, so it can't be completely general. Also, imo, that's probably a clippy lint that should be promoted to a built-in.

estebank 4y ago

I thought we had a deny by default lint for dropping a Mutex due to a _ binding. If we don't, definitely should.

tialaramex 4y ago
This https://godbolt.org/z/rx3h1rxW7 https://godbolt.org/z/rx3h1rxW7 does not even warn you that we're not actually locking the mutex. There's a correct warning if call lock() without using the result, because it has the must-use annotation, but if we throw it into _ the compiler is satisfied even though that's definitely not what a Mutex guard is for.
- estebank 4y ago
  On the upper righ side, click on tools and select clippy https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=04f076ee51f96a17a4f35ead257dd773 https://play.rust-lang.org/?version=stable&mode=debug&editio... It will produce the following output: error: non-binding let on a synchronization lock --> src/main.rs:5:5 | 5 | let _ = m.lock(); | ^^^^^^^^^^^^^^^^^ | = note: `#[deny(clippy::let_underscore_lock)]` on by default = help: consider using an underscore-prefixed named binding or dropping explicitly with `std::mem::drop` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_lock Clippy is a third party tool, but highly recommended because of things like this. I would like to see this lint uplifted into rustc.
  - tialaramex 4y ago
    Firstly, sincerely thanks for telling me how to add Clippy, that would have been a good idea in this context and now I'll remember it's there. Secondly, yes, I think should be uplifted. I'd even argue for default deny because I can't think of a reason you would want this behaviour and yet not find this more explicit phrasing more maintainable: let guard = m.lock(); Mutex::unlock(guard); // We don't hold the guard, just touch and go [I'm assuming anybody who actually wants this would prefer Mutex::unlock() over drop() because it says what's going on, and if they care that they can't write this in stable their effort would be appreciated pushing Mutex::unlock stabilisation rather than blocking a default-deny]
    - stormbrew 4y ago
      tbh I would use (and I think have used) drop() for this just because I know drop exists and what it does but Mutex::unlock is something I'd only ever know about if I went and scanned the whole Mutex page on rustdoc. I like the idea of these sorts of "drop-with-semantics" functions, but in practice until/unless they add a way to `impl !Drop` (or maybe `#[deprecated]` on a trait impl so you can add a deprecation note on your `impl Drop`) on a type, I'm not sure they're worth it.
stormbrew 4y ago
you're right, there is a clippy lint for it on a mutex in particular. Tbh I think it might be good to generalize it somehow? Because I think there are other cases it could bite you than just mutexes. But obviously the whole point of doing that in the first place is usually to avoid a must-use warning, so it can't be completely general. Also, imo, that's probably a clippy lint that should be promoted to a built-in.