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:

rerun/crates/store/re_types_core/src/arrow_zip_validity.rs

Lines 196 to 202 in abc5f78

	pub fn unwrap_required(self) -> I {
	if let Self::Required(i) = self {
	i
	} else {
	panic!("Could not 'unwrap_required'. 'ZipValidity' iterator has nulls.");
	}
	}

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

rerun/crates/store/re_types_core/src/lib.rs

Lines 160 to 176 in abc5f78

	#[cfg(debug_assertions)]
	Err(err) => {
	panic!(
	"failed to serialize data for {descriptor}: {}",
	re_error::format_ref(&err)
	)
	}
	#[cfg(not(debug_assertions))]
	Err(err) => {
	re_log::error!(
	%descriptor,
	"failed to serialize data: {}",
	re_error::format_ref(&err)
	);
	None
	}

[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!.

[Wumpf]

Thanks for taking a stab at removing some more panics & unwraps!

rerun/crates/store/re_types_core/src/arrow_zip_validity.rs

Lines 196 to 202 in abc5f78

	pub fn unwrap_required(self) -> I {
	if let Self::Required(i) = self {
	i
	} else {
	panic!("Could not 'unwrap_required'. 'ZipValidity' iterator has nulls.");
	}
	}

likely callsites need to be patched to remove the need for this method

rerun/crates/store/re_types_core/src/lib.rs

Lines 160 to 176 in abc5f78

	#[cfg(debug_assertions)]
	Err(err) => {
	panic!(
	"failed to serialize data for {descriptor}: {}",
	re_error::format_ref(&err)
	)
	}
	#[cfg(not(debug_assertions))]
	Err(err) => {
	re_log::error!(
	%descriptor,
	"failed to serialize data: {}",
	re_error::format_ref(&err)
	);
	None
	}

I think we should keep this, rationale see documentation of the method.
Similarly, I'm very much on the fence of removing unwrap_debug_or_log_error since this is used in cases where we should absolutely not error in debug builds but don't want to crash in release if we accidentally shipped anyways. They fullfill the same purpose as debug_assert. Let's document it like that (being a stand-in for debug_assert) and keep them, using the appropriate expect

So, my thoughts is that the build script should always use Result to report issues back or abort execution directly without panic!.

sure, why not, but I'd like to see the tradeoff in outputs first. Generally for buildscripts we really don't care all that much since a the build will fail the same either way - the prime motivator of avoiding panic/unwrap is that the sdk/viewer never crash, so that's not a concern here. 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?

[emilk]

Cool, I didn't know about this!

[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: cargo-err@0.1.0: 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: cargo-err@0.1.0: 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: cargo-err@0.1.0: 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

[Wumpf]

looks good now. Thanks again!

[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]

Practically all changes in 46cb497 are incorrect as they use cargo_error in libraries that are not necessarily part of build.rs.

• re_types_builder is invoked not as a build tool inside of build.rs, but rather as command line tool to build types. We typically run it with pixi run codegen
• re_build_info isn't a library exclusively used in build.rs, but rather to read out build info that was previously injected via a build.rs
• re_dev_tools is a command line tool used by various build & testing scripts

[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]

nice catch of dead code!

[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

[YohDeadfall]

@Wumpf, could you run the workflows on the pull request? I have some time to finish it, finally.