#[no_panic] math

[BenjaminBrienen]

Objective

bevy_math being no_panic would be cool.

Solution

Add #[no_panic] to all pub fns.

Testing

It compiles

[Victoronz]

Hmm, while a cool library, I'm not sure whether we actually want to use this.
Whether it allows compilation is very dependent on optimization settings. Do we actually test all of them?
Because panic detection happens at link time, any cargo build of this library alone and cargo check even within a binary will not trigger compilation failures.
Having compilation fail for users in debug mode, or because of some specific compilation setting, or some other edge case seems like a strong downside to me if we can't test for it in CI.

[bushrat011899]

Perhaps #[no_panic] should be #[cfg_attr(...)]'ed behind a feature flag, and an explicit "test the no_panic stuff" CI task should be added? Similar to the way we're currently testing no_std compliance.

[mweatherley]

I agree with @Victoronz and I also question what value this actually brings to Bevy users. It would be one thing if no_panic were actually a top-level module declaration that statically guaranteed panic-freeness of the public API (outside of marked exceptions or whatever), and if it were implemented using MIR by the Rust compiler itself, but that it is not.

I would much prefer if we just:

• enforced writing panicking documentation on methods that do so with good reason, and
• actually fixed the one panic in bevy_math users have actually complained about.

[BenjaminBrienen]

It brings value to bevy engine developers, ideally. It points out where documentation is missing or where an algorithm might be improved.

[mockersf]

I'm against including this for everyone. It could maybe be an optional dependency with a dedicated step in CI to check it. It depends if the bevy_math people are interested enough in this.

And it's not possible to merge this PR with this title

[Victoronz]

It brings value to bevy engine developers, ideally. It points out where documentation is missing or where an algorithm might be improved.

The problem is that users will experience the errors, not us.
What is a user supposed to do when their project fails to compile because of a no_panic error in bevy_math?
They will have no reliable way of fixing it themselves.
no_panic will reject code it cannot prove doesn't contain a panic, not just code that definitely panics.

If users report us compilation failures, we won't have a reliable way to fix them either, aside from just removing no_panic again.

A cursory look at the crate shows me debug_assert! usage.
This panics, so should it be stripped?
In release mode, adding to a maxed out integer will not panic, it will silently overflow. In debug, a panicking check is compiled in instead. This is the case with various other operations we might want to use. I guess this can be reconciled with a cfg_attr(no_panic, not(debug_assertions)).

Even if default compilation doesn't link them in, some non-default linker, codegen backend, dynamic linking, or compilation behavior might.

I personally do not think preventing panics from being compiled in is worth obstructing day to day Rust usage.

[BenjaminBrienen]

Based on the feedback, I was going to put it behind a config with a check in CI that uses that feature.

[bushrat011899]

Based on the feedback, I was going to put it behind a config with a check in CI that uses that feature.

I think this is a good idea. no_panic in principle is a really useful tool, even if it isn't perfect. Having it as a cfg option gives us the best of both worlds IMO.

[bushrat011899]

Much better! Minor suggestion around making no_panic an optional dependency. I also realised we can probably test the const functions by wrapping them in a non-const function and asserting that wrapper is #[no_panic].

Which then leads me to wonder, maybe a future PR could refactor all the no_panic stuff into tests instead, leaving no_panic as a dev-dependency. Outside scope for this PR IMO.

[BenjaminBrienen]

Much better! Minor suggestion around making no_panic an optional dependency. I also realised we can probably test the const functions by wrapping them in a non-const function and asserting that wrapper is #[no_panic].

Which then leads me to wonder, maybe a future PR could refactor all the no_panic stuff into tests instead, leaving no_panic as a dev-dependency. Outside scope for this PR IMO.

I like the idea of having both; I wish it didn't require code duplication, though.

[BenjaminBrienen]

cargo build -p bevy_math -F check_no_panic is successful, but cargo test -p bevy_math -F check_no_panic is not:

  = note: bevy_math-0c5176ee38e2aff9.bevy_math.4257f652f436454c-cgu.04.rcgu.o : error LNK2019: unresolved external symbol ␍
          ␍
          ERROR[no-panic]: detected panic in function `diameter`␍
           referenced in function _ZN98_$LT$bevy_math..primitives..dim2..Circle..diameter..__NoPanic$u20$as$u20$core..ops..drop..Drop$GT$4drop17h1fbedd7a08eee6b5E␍
          bevy_math-0c5176ee38e2aff9.bevy_math.4257f652f436454c-cgu.12.rcgu.o : error LNK2001: unresolved external symbol ␍
          ␍
          ERROR[no-panic]: detected panic in function `diameter`␍
          ␍
          bevy_math-0c5176ee38e2aff9.bevy_math.4257f652f436454c-cgu.04.rcgu.o : error LNK2019: unresolved external symbol ␍
          ␍
          ERROR[no-panic]: detected panic in function `closest_point`␍
           referenced in function _ZN103_$LT$bevy_math..primitives..dim2..Circle..closest_point..__NoPanic$u20$as$u20$core..ops..drop..Drop$GT$4drop17ha1760fcbcf30bfbdE␍
          bevy_math-0c5176ee38e2aff9.bevy_math.4257f652f436454c-cgu.07.rcgu.o : error LNK2001: unresolved external symbol ␍

[BenjaminBrienen]

Honestly, no_panic just isn't good enough for this to make any sense.