Not allowing to ignore Result as a return type

Following the tradition of Rust enforcing to actually deal with problems instead of ignoring them, does anyone think that ignored Results should be a compilation error in release mode?

Wouldn't this break backward compatibility?

2 Likes

Would this be the only case in which code compiles in debug mode and does not compile anymore in release mode? If yes, thatā€™d be a strong reason against such a suggestion IMO.

Also in general, preventing releases with warnings is something that e.g. CI is good for. There are lots of other warnings that you shouldnā€™t ignore either unless you want broken code. I donā€™t think a problem is being ā€œignoredā€ by Rust if thereā€™s a warning in place.

I like it when --release mode is all about optimizations and such and nothing more.

4 Likes

Did it never happen that new release of rust wasn't backward compatible?

There's already a #[must_use] warning for Result. You can turn it into a hard error using #![deny(must_use)] or even #![forbid(must_use)].

6 Likes

Yes, but it is up to a user and not compiler to enforce it. I'm suggesting that if compiler would enforce it it would be much better for the Rust language. Same way borrowing rules are enforced by compiler not left to the user to make sure he does only the right thing.

Prompting people to do the right thing and handle their errors is an admirable goal, but you'll never get to everyone, and some sort of balance must be struck. Changing #[must_use] to deny by default would be targeting those who are already ignoring compiler warnings. I'd personally expect a lot of .unwrap() (arguably an improvement), a lot of #![allow(must_use)] (making things worse), some let _ = ..., and a little bit of "hmm yeah, I really should do something different in the error condition here".

I could imagine it happening on an edition boundary, but I don't know that there's enough support for it. Probably there would be a lot of churn-based opposition.

As for making it a hard error and not a lint, there will still be a way around it. Doing the same thing for both Result variants is a valid use case, including doing nothing. If you kept pushing (e.g. disallow let _ = ... too), you'd induce a lot of annoyance and workarounds for diminishing returns. And being too pushy will backfire when people reach the conclusion that the compiler often cries wolf.

(One quick, project-wide way around it would be to stick on an older edition where it's not a hard error. Rust generally wants to encourage the opposite.)

5 Likes

As much as I appreciate your arguments and I see where you coming from, I personally believe that people are drawn to Rust for exactly that reason: The compiler will enforce correctness of their code and will not allow them to get sloppy/careless...

Those people aren't ignoring compiler warnings though.

I'm one of them -- that is, one of the reasons I'm drawn to Rust is that the compiler pushes me in the right direction and doesn't allow me to be too sloppy/careless, as you said. I don't ignore compiler warnings. So I personally wouldn't care too much care if the lint became deny only. It would force me to fix temporary "quick code" sooner rather than later, but it would have been fixed before committing anyway. I'd probably mutter "yeah yeah" a little more often but be fine with it.

However I also think making it a hard error and not a lint is past the point of balance.

9 Likes

I'm the same, I always setup cargo clippy -- -D warnings in CI, and I would personally care if the lint became a hard error. Using #[deny] in code is such a terrible practice that I have had to add --cap-lints=warn to my ~/.cargo/config to workaround people that want to make contributing to their projects a horrible experience. Go makes unused variables a hard error which makes prototyping in it a pain, you comment out one function call to see what happens without it, that results in a variable being unused so you comment out its definition, making another variable unused so you comment out that ....

The compiler should not stop you from running non-perfect code to see if your current tests pass, you will fix all warnings before committing anyway, so blocking the tests from running is just slowing down the feedback loop and making development more difficult, not meaningfully contributing to better code quality.

17 Likes

I'm talking only about release version. Not debug version. Surely it doesn't make sense to let unused variables and other "artefacts" from prototyping be in release version?

What does ā€œrelease versionā€ even mean? As I already stated, I like the --release flag as an easy way to turn on optimizations, nothing more. Itā€™s also used by e.g. cargo install some_crate, I guess. OTOH, when youā€™re developing or contributing to a rust library or binary project, then you might very well never actually call cargo build --release when releasing your code. Things are tested in debug mode and released as source code. The first person to run into a failed built might be the libraryā€™s user. On the other hand, you might want to run benchmarks on --release or you might have a big project where debug mode is just too slow to test out some use case of your prototype implementation of a program. In this case, youā€™ll compile a prototype, i.e. not a ā€œrelease versionā€, with --release in order to be able to run it properly. The prototype may be WIP and still have warnings.

If --release influenced whether (some or perhaps even all) warnings are denied, I think this would be super annoying. People who run into this during development would want to figure out how to disable this behavior with additional flags, and use that variant of the cargo build command for their testing purposes. Then the only people left that are affected are probably people that are using a library or installing something with cargo install. I.e. not the developers.

Warnings already can be turned into errors for actual ā€œreleasesā€. If you want to see errors instead of warnings, just pass the right command line arguments, as already discussed above. Thatā€™s the proper way to tell the compiler ā€œthis is a ā€˜release versionā€™, so please donā€™t allow any warnings.ā€ Of course this is pretty unnecessary for your local manual command line setup, since you can just, well... if you donā€™t want to ignore warnings, then just donā€™t ignore warnings; and if someone wants to ignore warnings, thereā€™s always going to be a way.

For something automated like continuous integration, where nobody might read the compiler output, denying warnings is already common practice; I donā€™t think thereā€™s anything to be gained here by changing any defaults. Setting up CI is already non-trivial; passing the right command line flags in order to not ignore warnings on the other hand is so straightforward, I doubt anyone will get this wrong unintentionally. I mean, some people will even enable clippy warnings for CI, or enforce rustfmt, or activate additional Rust warnings, or test older rustc versions, etc...

And even CI might be using non --release builds only, anyways.

15 Likes

Thatā€™s your response to 4 paragraphs of explanation on why the --release command line argument is something different to saying ā€œhey compiler, Iā€™m about to release my codeā€?

Itā€™s actually quite simple: just acknowledge something like cargo clippy -- -D warnings as the way to say ā€œhey compiler, Iā€™m about to release my codeā€. Although thatā€™s more like saying ā€œhey compiler, Iā€™m about to release my code and Iā€™m a CI toolā€, or ā€œhey compiler, Iā€™m about to release my code and Iā€™m a weird person thatā€™s going to ignore whatever you tell me as long as you donā€™t indicate an ERRORā€

20 Likes

cargo check doesn't even have a --release flag AFAIK, because that flag only affects code generation, and cargo check only checks for warnings and errors without generating any code.

cargo check does have a --release flag. The main difference between cargo check and cargo check --release is for code that uses #[cfg(debug_assertions)]. (Also, cargo check does generate code for proc macros and dev-dependencies.)

3 Likes

Thanks for correcting me. Though I believe debug_assertions can be enabled even in release builds.