Skip to content

Enforce safety comments on alloc - #160941

Open
nia-e wants to merge 2 commits into
rust-lang:mainfrom
nia-e:alloc-safety-comments
Open

Enforce safety comments on alloc#160941
nia-e wants to merge 2 commits into
rust-lang:mainfrom
nia-e:alloc-safety-comments

Conversation

@nia-e

@nia-e nia-e commented Aug 11, 2026

Copy link
Copy Markdown
Member

View all comments

cc @clarfonthey, @workingjubilee. Following up from #160824. I spent a painfully long time going over the comments and ensuring there's nothing too awful ^^

r? clarfonthey

@rustbot rustbot added A-tidy Area: The tidy tool S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 11, 2026
@nia-e

nia-e commented Aug 11, 2026

Copy link
Copy Markdown
Member Author

This will break a lot of ppl's CI ^^

@bors rollup=never

@JonathanBrouwer

JonathanBrouwer commented Aug 11, 2026

Copy link
Copy Markdown
Member

I'm not convinced rollup=never actually helps. This kind of PR is likely to have soft conflicts (i.e. someone adds an unsafe block between this PR being opened and merged), but rollup=never doesn't help detecting those. In fact, it makes them worse because soft conflicts may get detected in PR CI/try jobs of rollups and not waste queue time. My primary recommendation to help with this would be to reduce the time between PR open and r+, and rebase before r+ if this is more than a few days

@JonathanBrouwer

Copy link
Copy Markdown
Member

@bors rollup=iffy p=1 likely to soft conflict
p=1 does help because it makes you get into rollups earlier reducing time between pr open & merge

@nia-e

nia-e commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

...i completely forgot to actually push the commit with all my manual work. apologies 🫠

@clarfonthey

Copy link
Copy Markdown
Contributor

Oh, I didn't know that you were actually going through and adding comments to these. I'd, probably want to do an actual review in that case.

Comment thread library/alloc/src/boxed/thin.rs Outdated
Comment thread library/alloc/src/boxed/thin.rs Outdated
Comment thread library/alloc/src/boxed/thin.rs Outdated
Comment thread library/alloc/src/collections/binary_heap/mod.rs Outdated
Comment thread library/alloc/src/collections/binary_heap/mod.rs Outdated
Comment thread library/alloc/src/collections/binary_heap/mod.rs Outdated
Comment thread library/alloc/src/collections/binary_heap/mod.rs
Comment thread library/alloc/src/collections/binary_heap/mod.rs
Comment thread library/alloc/src/collections/btree/mem.rs Outdated
Comment thread library/alloc/src/collections/vec_deque/drain.rs Outdated
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
Comment thread library/alloc/src/collections/vec_deque/spec_extend.rs Outdated
Comment thread library/alloc/src/collections/vec_deque/spec_extend.rs Outdated
Comment thread library/alloc/src/ffi/c_str.rs Outdated
type Cap = core::num::niche_types::UsizeNoHighBit;

// SAFETY: Untriaged.
// SAFETY: 0 *definitely* is less than isize::MAX.

@clarfonthey clarfonthey Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But how can you be sure?

View changes since the review

@nia-e nia-e Aug 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

easy: isize was defined before 2025 and therefore there were affordable computers around that had to address more than 0 bytes of memory!

@nia-e

nia-e commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

per this week's libs meeting, we (a) do want this! and (b) we should go for ignore-tidy-undocumented-unsafe instead of a blank untriaged comment

@clarfonthey

Copy link
Copy Markdown
Contributor

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2026
@rust-bors

This comment has been minimized.

@clarfonthey

Copy link
Copy Markdown
Contributor

(apologies for merging some conflicting PRs. wouldn't mind helping rebase if this is getting difficult to manage)

nia-e added 2 commits August 28, 2026 23:56
safety comments in alloc

this was pain and it's not even half i think

oepsje woepsje
@nia-e
nia-e force-pushed the alloc-safety-comments branch from 75cc497 to 483873e Compare August 28, 2026 21:56
@rustbot

rustbot commented Aug 28, 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.

@nia-e

nia-e commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

think this is done and mergeable now. rebased on main, so it shouldn't conflict much i hope?

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2026
@clarfonthey

Copy link
Copy Markdown
Contributor

Okay, that was painful, but reviewed.

@bors try

@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Aug 29, 2026
@clarfonthey

Copy link
Copy Markdown
Contributor

Recommended merge command assuming try passes:

r=clarfonthey p=1 note="Affects entire alloc crate, can easily go stale"

@clarfonthey

Copy link
Copy Markdown
Contributor

Also fine with you just rebasing this yourself and merging in case of conflicts, assuming you mark new blocks as ignored instead of trying to determine what their comments should be.

@rust-bors

rust-bors Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 8e834e4 (8e834e4af0546ff9e43f5ddca26d6ea6c9305778)
Base parent: 6bb812b (6bb812b714cce8e08864b7b569303f25ab08eaad)

@clarfonthey

Copy link
Copy Markdown
Contributor

@bors r+ p=1 note="Affects entire alloc crate, can easily go stale"

@rust-bors

rust-bors Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 483873e has been approved by clarfonthey

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2026
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Aug 29, 2026
…rfonthey

Enforce safety comments on `alloc`

cc @clarfonthey, @workingjubilee. Following up from rust-lang#160824. I spent a painfully long time going over the comments and ensuring there's nothing too awful ^^

r? clarfonthey
JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Aug 29, 2026
…rfonthey

Enforce safety comments on `alloc`

cc @clarfonthey, @workingjubilee. Following up from rust-lang#160824. I spent a painfully long time going over the comments and ensuring there's nothing too awful ^^

r? clarfonthey
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 29, 2026
…rfonthey

Enforce safety comments on `alloc`

cc @clarfonthey, @workingjubilee. Following up from rust-lang#160824. I spent a painfully long time going over the comments and ensuring there's nothing too awful ^^

r? clarfonthey
rust-bors Bot pushed a commit that referenced this pull request Aug 29, 2026
Rollup of 7 pull requests

Successful merges:

 - #160941 (Enforce safety comments on `alloc`)
 - #161975 (Abort instead of unwinding out of an inconsistent BTreeMap::split_off)
 - #157968 (Preserve ambiguous glob lint for subtrait method picks)
 - #161514 (rework handling of doc attributes on macro calls)
 - #161944 (rustdoc: Take into account edition information for keyword highlighting)
 - #161974 (remove rustc_never_type_options attr remnants)
 - #161978 (Move rustdoc ui doctests into the right testsuite)

Failed merges:

 - #161702 (Use `drop_guard` in some places in {core,alloc,std})
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-tidy Area: The tidy tool S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

7 participants