Skip to content

[Guideline] Add do not divide by 0 #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vapdrs
Copy link
Contributor

@vapdrs vapdrs commented Jun 10, 2025

Closes #131

Copy link

netlify bot commented Jun 10, 2025

Deploy Preview for scrc-coding-guidelines ready!

Name Link
🔨 Latest commit 899d826
🔍 Latest deploy log https://app.netlify.com/projects/scrc-coding-guidelines/deploys/685c00be8336c600095c5bbc
😎 Deploy Preview https://deploy-preview-132--scrc-coding-guidelines.netlify.app
📱 Preview on mobile
Toggle QR Code...

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
Copy link
Collaborator

felix91gr commented Jun 11, 2025

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

  1. 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

Copy link
Collaborator

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vapdrs added 2 commits June 24, 2025 17:23
As stated there is no compliant way to do this, so no example should be
present.
While the guideline does not strictly apply to this example, it is a
good suggestion for what to do instead.
@vapdrs vapdrs requested review from felix91gr and PLeVasseur June 27, 2025 18:09
Copy link
Collaborator

@PLeVasseur PLeVasseur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


.. guideline:: Do not divide by 0
:id: gui_kMbiWbn8Z6g5
:category: Mandatory
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:category: Mandatory
:category: mandatory

Should be lowercase, see here in conf.py.

:status: draft
:release: latest
:fls: fls_Q9dhNiICGIfr
:decidability: Undecidable
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:decidability: Undecidable
:decidability: undecidable

Should be lowercase, see here in conf.py.

:release: latest
:fls: fls_Q9dhNiICGIfr
:decidability: Undecidable
:scope: System
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:scope: System
:scope: system

Should be lowercase, see here in conf.py.

:status: draft

There is no compliant way to perform integer division by zero. A checked division will prevent any
division by zero from happening. The programmer can then handle the returned Option.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out how we can use one of the nice bits of the coding guidelines extension to link to the Rust standard library.

Suggested change
division by zero from happening. The programmer can then handle the returned Option.
division by zero from happening. The programmer can then handle the returned :std:``std::option::Option``.

(This was shamelessly stolen from the FLS extension)

@@ -81,3 +81,44 @@ Expressions
}

fn with_base(_: &Base) { ... }

.. guideline:: Do not divide by 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@PLeVasseur
Copy link
Collaborator

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Coding Guideline]: Do not divide by 0
4 participants