4 ms·

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

tialaramex 5y 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 5y 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 5y 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 5y 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 5y 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.

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 5y 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 5y 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 5y 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 5y 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.