Skip to content

Enforce safety comments on alloc - #160941

Open
nia-e wants to merge 1 commit into
rust-lang:mainfrom
nia-e:alloc-safety-comments
Open

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

Conversation

@nia-e

@nia-e nia-e commented Aug 11, 2026 •
edited by rustbot
Loading

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 •
edited
Loading

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

JonathanBrouwer commented Aug 11, 2026

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

clarfonthey commented Aug 12, 2026

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.

clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/boxed/thin.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/boxed/thin.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/boxed/thin.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/binary_heap/mod.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/binary_heap/mod.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/binary_heap/mod.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/binary_heap/mod.rs
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/binary_heap/mod.rs
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/btree/mem.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/vec_deque/drain.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/vec_deque/mod.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/vec_deque/spec_extend.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/vec_deque/spec_extend.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/ffi/c_str.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/raw_vec/mod.rs
type Cap = core::num::niche_types::UsizeNoHighBit;

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

@clarfonthey clarfonthey Aug 12, 2026 •
edited by rustbot
Loading

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 •
edited
Loading

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!

clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/raw_vec/mod.rs
Comment on lines -892 to +894
// SAFETY: Precondition passed to caller
// SAFETY: Untriaged.
if let Some((ptr, layout)) = unsafe { self.current_memory(elem_layout) } {
// SAFETY: Untriaged.
// SAFETY: Precondition passed to caller

@clarfonthey clarfonthey Aug 12, 2026 •
edited by rustbot
Loading

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.

This one looked painful to figure out

View changes since the review

clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/vec/into_iter.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/vec/mod.rs Outdated
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/alloc.rs
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/alloc.rs
clarfonthey
clarfonthey reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/sync.rs Outdated
fn into_inner_with_allocator(this: Self) -> (NonNull<ArcInner<T>>, A) {
let this = mem::ManuallyDrop::new(this);
// SAFETY: Untriaged.
// SAFETY: Pointer is valid for reads and won't be double-dropped.

@clarfonthey clarfonthey Aug 12, 2026 •
edited by rustbot
Loading

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.

YORO? (You only read once.)

View changes since the review

@ChrisDenton

ChrisDenton commented Aug 12, 2026

Copy link
Copy Markdown
Member

Could you put a #![forbid(clippy::missing_safety_doc)] in lib.rs? I know it's technically redundant with the tidy lint but I've been trying to enable (or at least not ignore) more clippy lints so it'd be useful to me to have that tracked with clippy too.

@nia-e

nia-e commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

missing_safety_doc is for missing function safety docs ^^ i think that goes as a separate PR? this is just the safety comments on unsafe blocks and the scope is already slightly terrifyingly large lol

@nia-e

nia-e commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

that should address review comments 🫠

@rust-log-analyzer

This comment has been minimized.

Sign in to view
DanielEScherzer
DanielEScherzer reviewed Aug 12, 2026
View reviewed changes
Comment thread library/alloc/src/collections/btree/node.rs Outdated
Comment thread library/alloc/src/collections/btree/node.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/io/cursor.rs Outdated
Comment thread library/alloc/src/vec/mod.rs Outdated
@rust-bors

This comment has been minimized.

Sign in to view
@nia-e

nia-e commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

(presumably #160953)

it seems my children have come to haunt me

@nia-e
nia-e force-pushed the alloc-safety-comments branch from 0ad4d08 to 0c8e275 Compare August 13, 2026 00:47
@rustbot

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
@nia-e
tidy: enforce documented unsafe in alloc
2120557 
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 0c8e275 to 2120557 Compare August 19, 2026 19:20
@rustbot

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

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

Reviewers

@clarfonthey clarfonthey clarfonthey left review comments
+1 more reviewer
@DanielEScherzer DanielEScherzer DanielEScherzer left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@clarfonthey clarfonthey

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@nia-e @JonathanBrouwer @clarfonthey @ChrisDenton @rust-log-analyzer @rustbot @DanielEScherzer