[Guideline] Add do not divide by 0

[vapdrs]

Closes #131

[netlify]

✅ Deploy Preview for scrc-coding-guidelines ready!

Name	Link
🔨 Latest commit	9dabe78
🔍 Latest deploy log	https://app.netlify.com/projects/scrc-coding-guidelines/deploys/68794453786cb20008f7e394
😎 Deploy Preview	https://deploy-preview-132--scrc-coding-guidelines.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[felix91gr]

Hi @vapdrs! I have already sent you a message in Zulip, but here seems like a better place to do so.

I come to add a couple of things to this guideline :)

1. There is a Clippy lint (actually a couple of lints, but this one is the main one) that one can enable to catch these kinds of operations: arithmetic_side_effects. This is a fairly general lint that, if enabled using warn or deny, lints any arithmetic expression that either overflows or panics (division and modulo by 0 will do the latter).

2. The aforementioned lint should indicate to the user that there are arithmetic operations in the stdlib that can be used to guarantee well-defined behavior. In the case of division by zero, it will suggest using operations such as checked_div, which outputs an Option<T> that the user must then handle properly. The None result indicates overflow, underflow or division by zero.

There are other such operations for division, such as checked_div_rem_euclid for when a remainder is desired.

And for other arithmetic operations, there are quite a few functions one can use to avoid Undefined Behavior: https://doc.rust-lang.org/std/?search=checked

3. A complement to the two resources above is the NonZero type. One uses NonZero to enclose a value that is known, by construction, to not equal zero.

This combines rather well with Option, as in Option<NonZero>, since the compiler can do some memory layout optimization due to the fact that the value being enclosed by NonZero has one bit-pattern that is known to not be possible (the 000...000 pattern)

I will review the PR shortly :3

[PLeVasseur]

Thanks for opening this up @vapdrs! Could you check the comments I left?

[PLeVasseur]

Thanks for pulling this together @vapdrs! I left a few suggestions based on how conf.py works together with Sphinx Needs as well as a way to use our coding guidelines extension. Could you take a look?

[PLeVasseur]

Asking a bit of a practical question to folks. Does the combination of:

• mandatory
• undecidable

put too great of a burden on reviewers / auditors or is this "just the way it is and should be" given the nature of writing safety-critical code?

Tagging @AlexCeleste and @rcseacord for their thoughts as well.

[vapdrs]

Just a bit of rationale for why I chose that combination. I used "mandatory" as a stand in for "very important" as a reason it goes above and beyond "required".

I would not want to mandate that no safety-critical programmer could never used unchecked arithmetic, the decidable subset version of this rule, but I do want to mandate that they do not take an action (divide by zero) that would lead to a panic.

[felix91gr]

Well, "do not divide by zero" is undecidable in the sense that statically knowing if the b in a / b is 0 is undecidable.

However, the guideline is not impossible to comply with since the user themselves can check (if b != 0 etc) but also because there are APIs that we can use for this, like the NonZero<T> struct and the checked_div and checked_rem functions.

[vapdrs]

That is true, there are more clever ways to enforce this in Rust.

I would be fine mandating that any division must use checked functions, or have a NonZero<T> divisor (which elides any performance penalty from checking). This would then kick the can down the road, where as now your problem is don't use NonZero<T>::new_unchecked and cause UB.

[PLeVasseur]

However, the guideline is not impossible to comply with since the user themselves can check

Definitely, I agree. Perhaps this is an instance of where we need to make use of the Exceptions section to list the sorts of suggestions you made. What do you two think?

like the NonZero<T>

TIL about NonZero<T>. Super cool.

[vapdrs]

The rule already shouldn't apply to this, it is specified as applying to:

unsigned integer or two’s complement division

which is the "built-in" rules of the ArithmeticExpression. Using NonZero<T> goes through core::ops::Div::div which is already outside the scope of this guideline. I will clarify that to make it more explicit though.

[PLeVasseur]

I'm realizing now that perhaps the issue template having uppercase options may have been to blame for them being made uppercase in this PR. Was that the case for you @vapdrs?

I've opened #143 to fix this 👍