Forbid panic! in production code

[YohDeadfall]

Related

Part of #7997.

What

The pull request forbids any usage of panic! outside of tests, but I found that there are some cases that I don't know how to resolve. As an example:

https://github.com/rerun-io/rerun/blob/abc5f78cbd4a952a91cbf8993c4ad1961d7ae3bc/crates/store/re_types_core/src/arrow_zip_validity.rs#L196-L202

For the follow up, I suppose, the ResultExt should be used to produce an error, right?

https://github.com/rerun-io/rerun/blob/abc5f78cbd4a952a91cbf8993c4ad1961d7ae3bc/crates/store/re_types_core/src/lib.rs#L160-L176

[YohDeadfall]

Even with the proposed changes the build script should terminate with the correct status code, but without panic! and when it's desired. See rust-lang/cargo#15038. So, my thoughts is that the build script should always use Result to report issues back or abort execution directly without panic!.

[YohDeadfall]

Arguably the advantage with panicing is that it's easier to pinpoint the error here. Haven't worked with cargo::error yet, could you post a before/after example for a given error?

Here are a few examples of the output with the corresponding code. Note that Result<T, E> depending on the variant is translated into exit(0) or exit(1), so not different from the first two examples.

cargo::error and zero exit code

use std::process::exit;

fn main() {
    println!("cargo::error=cargo:error");
}

use std::process::exit;

fn main() {
    println!("cargo::error=cargo:error and exit");
    exit(0);
}

error: [email protected]: cargo:error and abort
error: build script logged errors

cargo::error with non-zero exit code

use std::process::exit;

fn main() {
    println!("cargo::error=cargo:error and exit");
    exit(1);
}

error: [email protected]: cargo:error and abort
error: failed to run custom build command for `cargo-err v0.1.0 (/home/yoh/src/cargo-err)`

Caused by:
  process didn't exit successfully: `/home/yoh/src/cargo-err/target/debug/build/cargo-err-090802dea66ce06a/build-script-build` (exit status: 1)
  --- stdout
  cargo::error=cargo:error and abort

cargo::error and abort

use std::process::exit;

fn main() {
    println!("cargo::error=cargo:error and exit");
    exit(1);
}

error: [email protected]: cargo:error and abort
error: failed to run custom build command for `cargo-err v0.1.0 (/home/yoh/src/cargo-err)`

Caused by:
  process didn't exit successfully: `/home/yoh/src/cargo-err/target/debug/build/cargo-err-090802dea66ce06a/build-script-build` (signal: 6, SIGABRT: process abort signal)
  --- stdout
  cargo::error=cargo:error and abort

panic!

use std::process::exit;

fn main() {
    panic!("cargo::error=cargo:error");
}

error: failed to run custom build command for `cargo-err v0.1.0 (/home/yoh/src/cargo-err)`

Caused by:
  process didn't exit successfully: `/home/yoh/src/cargo-err/target/debug/build/cargo-err-090802dea66ce06a/build-script-build` (exit status: 101)
  --- stderr

  thread 'main' panicked at build.rs:2:5:
  cargo::error=cargo:error and panic!
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---

If you want to abort execution of the script as early as possible, then I would recommend to use cargo::error immediately followed exit(0). It should be a macro in order to format arguments as panic! does. Then if rust-lang/cargo#15038 is implemented, it's a single place to change the exit code.

[Wumpf]

Thank you for the detailed overview! The cargo error variants look indeed nicer, I figure we should use those whenever there's an error in user command or user environment 👍 . For everything else (i.e. implementation errors of build.rs) we want the callstack, those should panic

[YohDeadfall]

@Wumpf, are you fine that the current implementation tries to grab as many errors as possible or should the build script stop on the first one? If you want to stop ASAP, then I need to modify my code a bit as I proposed.

[Wumpf]

generally I think it's nice to see all errors at once - too often sometimes they only show up in some circumstances (e.g. ci) and then it's nice to have all of them. That is unless there's too many non-sensical ones caused by the first. Tbh not sure how it playes out with the ones here. I'd just leave it as-is for now and then exit earlier if there's too much spam 🤷

[Wumpf]

looks like there's some missed here. I think these should also have case-by-case allow because they're again just debug assertions https://github.com/rerun-io/rerun/actions/runs/17211880873/job/48833402796?pr=10941

[YohDeadfall]

Yeah, there are two places that should be patched with expect, and I though that I did that. Turns out that I didn't, and now I know what terminal was killed due to memory pressure of running rust-analyzers.

[Wumpf]

I figure that's a huge drawback of printing with cargo::error (+ exit(0)) - you have to be 100% certain that you're inside a build.rs which as demonstrated isn't always that clear

[Wumpf]

ci still doesn't run through https://github.com/rerun-io/rerun/actions/runs/17221558702/job/48887551457?pr=10941

[YohDeadfall]

It's caused by rust-lang/rust-clippy#15564. I will remove expect for now as clippy cannot see panic inside const.

[YohDeadfall]

@Wumpf, I polished the pull request taking into account the provided information. Therefore, all cargo:error usage is in re_build_tools as it's referenced by build.rs. In other build crates I made panic! allowed like in tests as it's not production code. The production crates use expect where needed.

[Wumpf]

ci is still not happy. You can replicate its set of checks by running pixi run rs-check --skip individual_crates docs_slow (that's very lengthy though, you may want to pick out smaller subsets for iterating)

[YohDeadfall]

ci is still not happy.

Yeah, I will come back to it and fix that as I get a bit of spare time. I still remember about the pull requests, it's not abandoned, just a startup I'm working for prepares for a conference and lots of thing have to be ready before that.

[Wumpf]

putting this to draft for tracking purposes