Disallow code usage with a custom `clippy.toml`
34 points by schneems
34 points by schneems
Update: you can also use this technique to disallow unwrap()!
FWIW, Clippy has a built-in/dedicated lint for this called unwrap_used.
IDK if you clicked the link, but it's topical. I would love to get a glimpe of that cloudflare clippy.toml. Good callout, I'll add a link to that note.
Am I reading this correctly that it's allow by default https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_used? I wonder why. I also find it odd that the default value of allow-unwrap-in-tests is false (I unwrap like wild in tests).
IDK if you clicked the link, but it's topical.
Yeah, I'm aware.
I also find it odd that the default value of allow-unwrap-in-tests is false (I unwrap like wild in tests).
Personally I would find this surprising if it were the default behavior, since if that were the case and I asked for unwrap to be banned by enabling the lint, but then for whatever reason it didn't trigger in some contexts.
I wonder why.
Because choosing the set of default-enabled lints requires striking a delicate balance between efficacy and not annoying people too much. Personally, I watch every Rust toolchain release for new non-default-enabled lints and maintain a big list of extra lints that I enable for all my projects here: https://gitlab.computer.surgery/charles/nix-rust-quickstart/-/blob/0ad7ffd0181fdfce1c1b28f84521a29c608a0007/template/Cargo.toml.liquid#L1-81.
Personally I would find this surprising if it were the default behavior, since if that were the case and I asked for unwrap to be banned by enabling the lint, but then for whatever reason it didn't trigger in some contexts.
I see that, I sometimes treat unit tests as a REPL to prototype code, it would be weird if something worked there but not in the main program (if I didn't know about this setting).
delicate balance between efficacy and not annoying people too much
Agreed, reading the rationale in the linked lint it mentions "dirty" code, which seems surprising to optimize for. I do write fast-and-dirty rust for things like advent of code, but that's the exception and not the rule (personally).
maintain a big list of extra lints
Maybe it makes sense to have a class of lints that is default, but borderline. Like: When it fails, have it also tell you how to change the lint behavior in the error output. Like lint state maybe
Checking path_facts v0.2.1 (/Users/rschneeman/Documents/projects/path_facts)
error: use of a disallowed method `std::env::set_current_dir`
--> src/path_facts.rs:395:9
|
395 | std::env::set_current_dir(temp.path()).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: Use `crate::test_help::SetCurrentDirTempSafe` to safely set the current directory for tests
= help: This is a `maybe` lint, to remove this message add `disallow_set_current_dir = allow` in your
= clippy.toml file. If you want to remove this message and keep the lint set
= `disallow_set_current_dir = deny`
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#disallowed_methods
= note: `-D clippy::disallowed-methods` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::disallowed_methods)]`
(but not sure the actual utility here)
Why do people avoid unwrap? I see many codebases where it's religiously avoided even when panicking would be the best option.
You can still panic, expect() with a message about the conditions that were violated for the panic to happen, this communicates: Panic is intentional, and 'here are conditions that must be upheld to avoid this panic:
result.expect("Regex checked by clippy");
Best practice is to use result and communicate to the caller "this may error." then thread that all the way up and panic at the top level. But there are other cases where a panic is appropriate, or if a check is performed external to compilation.
This might be useful if the fatal condition must be seen by a user of the software, but for panics that are programming errors, surely the stack trace is the most useful thing to have?
surely the stack trace is the most useful thing to have?
Coming from ruby, this was difficult to get used to (not having them in errors by default). If you do a good job with making real error enums and properly nesting them, it's usually unambiguous where the error came from once it bubbles to the top. I still miss my backtraces, but I've learned to live without having them for the most part.
There is a way to add backtraces to errors, it's just not the default https://doc.rust-lang.org/std/backtrace/index.html. Depending on the usage and scenario other options are available as well such as using instrument to capture otel traces which would give you much more information than a simple stack trace from the point of the panic.
must be seen by a user of the software
For me it's more about communication with the programmer. MOST rust code doesn't panic, so if I'm calling something without a Result return, I assume it is infallible. In the example I gave before with expect() the panic is entirely local (i.e. the programmer made a bad regex and it wasn't caught with clippy), but in most real world scenarios, there's external dependencies or assumptions (such as the size of a configuration file). The way you hint to your end-user-programmer "you might need to take care about setting up the right conditions for me to not fail" is by forcing them to handle the error cases.
I would rather see an explicit panic! with a useful error and a reason why we’re panicking. Unable to parse features file $fileName is much better than thread fl2_worker_thread panicked: called Result::unwrap() on an Err value.
Ah, but that's not the use of unwrap() that people advocate for. There is (hopefully) widespread agreement that that's an inappropriate place for a .unwrap() in production code.
The use of .unwrap() that people advocate for is in situations where it can never be triggered. Like, for all possible inputs to your program (including the config file, etc.), the .unwrap() won't be invoked. When it does happen, then, it's because there's a bug in your program, and the state is likely corrupt, and you need to stop execution now and get a developer to look into what happened.
This is analogous to using myvec[i]: it will panic if i is out of bounds, so you should only to call it when you're certain that i will be in bounds. Always using myvec.get(i), even when you're certain that i is in bounds, would be overzealous.
Now, even given this, you still want to minimize the number of unwraps in your code. Design your data structures so that invariants are enforced by the type system, make illegal states unrepresentable, all that. But you'll likely still have a sprinkling of unreachable .unwraps() around, and that's OK.
I usually put these into the Cargo.toml where you can also configure other lints https://doc.rust-lang.org/cargo/reference/manifest.html#the-lints-section. This is nice as you can also inherit the lint configuration in a workspace.
If you use std::sync primitives like Mutex do you allow unwrap() or force expect() on every lock()?
Personally I always .expect("lock should not be poisoned") in that case.
The official docs suggest propagating the panic for a poisoned mutex. I prefer using an expect though https://doc.rust-lang.org/std/sync/struct.Mutex.html
Most usage of a mutex will simply unwrap() these results, propagating panics among threads to ensure that a possibly invalid invariant is not witnessed.
Depending on the scenario, you can recover from a poisoned mutex, it means the state in your mutex might be unexpected or invalid but sometimes that's okay (like if the data wasn't important anyway https://github.com/schneems/path_facts/blob/5a5c9a28e525567f616012dcd0186e05c49faac9/src/test_help.rs#L15 )
Can I use this on my entire depgraph? Asking cause I'm currently working on a project using nrf-softdevice, which, due to the way it uses interrupts, requires that all code use its critical section implementation. If i use any libraries that directly call the cortex_m package's implementation, it is liable to break (citation: https://github.com/embassy-rs/nrf-softdevice?tab=readme-ov-file#interrupts)
So far I haven't found a way to guarantee this doesn't happen
Thank you for the link!
P. S. Just to show some visible support to the Rust community I want to simply say that a "null pointer exception" can happen in any even triple safe language. Rust is amazing and is doing a great job bringing us collectively to a safer place 🙏