Skip to content

Document nonzero invariant on NonZero::unchecked_add#158598

Closed
Manishearth wants to merge 1 commit into
rust-lang:mainfrom
Manishearth:nonzero-docs
Closed

Document nonzero invariant on NonZero::unchecked_add#158598
Manishearth wants to merge 1 commit into
rust-lang:mainfrom
Manishearth:nonzero-docs

Conversation

@Manishearth

Copy link
Copy Markdown
Member

Safety bug identified during agentic unsafe Rust audit.

This is kind of an obvious invariant but it's technically missing from the docs.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2026
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 30, 2026
@rustbot

rustbot commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from 6 candidates

@rustbot

This comment has been minimized.

@rustbot

rustbot commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@jhpratt

jhpratt commented Jun 30, 2026

Copy link
Copy Markdown
Member

This actually seems correct? It starts with a nonzero value and you're adding an unsigned value, so zero inherently cannot occur without wrapping — which is already called out as UB.

@ds84182

ds84182 commented Jun 30, 2026

Copy link
Copy Markdown

Since overflow cannot occur, unchecked_add is modeled as infinite precision integer addition with the caveat that "your house may explode" if the result exceeds a certain limit. Mathematically there are no non-negative integers that can be added to a positive integer that results in 0.

So I think this can be left to the developer's common sense.

Also the type is named NonZero.

@Manishearth

Copy link
Copy Markdown
Member Author

Aha, I was looking through this macro to try and understand this code and didn't realize this macro was only applying to unsigned types. Thanks!

Also the type is named NonZero.

From a safety review perspective I always prefer explicitly documented invariants.

You'd be surprised how many people incorrectly call MaybeUninit::assume_init() despite it being in the name, it's by far the biggest safety goof I see.

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2026
@jhpratt

jhpratt commented Jun 30, 2026

Copy link
Copy Markdown
Member

I definitely see that a fair amount as well. I'm not opposed to explicitly documenting it, but I don't think it's particularly necessary given the current wording 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants