Skip to content

New challenge: Verify the safety of BinaryHeap functions - #747

Open
Kieran-Hulsman wants to merge 2 commits into
model-checking:mainfrom
Kieran-Hulsman:kieran/binary-heap-challenge
Open

Kieran-Hulsman wants to merge 2 commits into
model-checking:mainfrom
Kieran-Hulsman:kieran/binary-heap-challenge

Conversation

@Kieran-Hulsman

Copy link
Copy Markdown

Proposes a challenge to verify the safety of BinaryHeap: the Hole and sift functions, PeekMut, the TrustedLen implementations of its sorted iterators, and the safe methods built on them.

Adds the challenge document and its book entry.

Tracking issue: #746

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@Kieran-Hulsman
Kieran-Hulsman requested a review from a team as a code owner November 3, 2026 15:43
Comment thread doc/src/SUMMARY.md Resolved

@feliperodri feliperodri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a well-targeted challenge. BinaryHeap moves elements with ptr::read and ptr::copy_nonoverlapping at positions picked by user-supplied Ord, so it's real unsafe code whose safety depends on code the library doesn't control. Requiring safety for any Ord, including inconsistent ones, is what makes it worth verifying. I checked the targets against library/alloc/src/collections/binary_heap/mod.rs on main: every listed function exists, the seven listed unsafe functions are all the unsafe fns apart from from_raw_vec/as_mut_slice (which you deliberately leave as correctness-only), and every function with an unsafe block is on the list. Nothing here overlaps an existing challenge or harness.

What blocks approval: the number 31 is also used by #595, and the UB list leaves out the intrinsic preconditions that Hole relies on. I'd also like the scope of panicking Ord stated, since it's what Hole's Drop exists for. Details inline.

What would flip this to approval:

  1. Renumber if #595 merges first (file name, title, SUMMARY.md).
  2. Add "Invoking undefined behavior via compiler intrinsics" to the UB list.
  3. Say whether the proofs must cover an Ord that panics (unwinding through Hole::drop), or that this is out of scope.

Comment thread doc/src/challenges/0031-binary-heap.md Resolved
Comment thread doc/src/challenges/0031-binary-heap.md Resolved
Comment thread doc/src/challenges/0031-binary-heap.md Resolved
Comment thread doc/src/challenges/0031-binary-heap.md Resolved
Comment thread doc/src/challenges/0031-binary-heap.md Resolved
Comment thread doc/src/challenges/0031-binary-heap.md Resolved
Comment thread doc/src/challenges/0031-binary-heap.md Resolved
@feliperodri feliperodri added the Challenge Proposal Used to tag a challenge proposal PR / Issue label Nov 3, 2026

@feliperodri feliperodri left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, all four points are addressed: the intrinsics item is in the UB list, panicking Ord is explicitly out of scope, the other IntoIter unsafe impls are explicitly excluded, and the Description and Assumptions sections match the template. I re-checked the new text against binary_heap/mod.rs on main and it matches the code.

Approving. The one thing left is the number: #595 also uses 31, so whichever merges second takes the next free number (file name, title and SUMMARY.md), as you said you'd do.

@feliperodri

Copy link
Copy Markdown
Member

@DiuDiu777 could you help us with a review on this challenge proposal as well?

This branch has not been deployed

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

Labels

Challenge Proposal Used to tag a challenge proposal PR / Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants