-
Notifications
You must be signed in to change notification settings - Fork 2.1k
chore: Disallow reserve() in clippy to prevent panics
#22386
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -703,6 +703,10 @@ impl StringViewArrayBuilder { | |
| if self.in_progress.capacity() < required_cap { | ||
| self.flush_in_progress(); | ||
| let to_reserve = (length as usize).max(self.next_block_size() as usize); | ||
| #[expect( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thought here on the exemption rationale. The bounded block size helps avoid overflow issues, but it does not prevent allocation failure. It would be helpful to clarify that this is an intentional tradeoff for an infallible API path, or possibly reference a future follow-up for a fallible builder approach. That would make it clearer that this is a narrow exception rather than a general safe-use pattern for the lint. |
||
| clippy::disallowed_methods, | ||
| reason = "StringView's block size bounds growth, so reserve cannot overflow capacity arithmetically. This hot loop intentionally avoids the extra `try_reserve` checks. It remains subject to allocator failure/OOM, which must be managed externally." | ||
| )] | ||
| self.in_progress.reserve(to_reserve); | ||
| } | ||
|
|
||
|
|
@@ -730,6 +734,10 @@ impl StringViewArrayBuilder { | |
| if self.in_progress.capacity() < required_cap { | ||
| self.flush_in_progress(); | ||
| let to_reserve = (length as usize).max(self.next_block_size() as usize); | ||
| #[expect( | ||
| clippy::disallowed_methods, | ||
| reason = "StringView's block size bounds growth, so reserve cannot overflow capacity arithmetically. This hot loop intentionally avoids the extra `try_reserve` checks. It remains subject to allocator failure/OOM, which must be managed externally." | ||
| )] | ||
| self.in_progress.reserve(to_reserve); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale on
#[expect(clippy::disallowed_methods)]mentions that block sizing prevents capacity overflow, butVec::reservecan still fail due to allocation failure. If this path is intentionally kept infallible because the surrounding API cannot return aResult, it would help to call that out explicitly in the reason. Otherwise, it may be worth tracking a follow-up for a fallibleStringViewbuilder path usingtry_reserveso allocation failures can be propagated cleanly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review -- this is a good point.
I believe using the unsafe API here is an intentional performance tradeoff, I've updated the doc to make it clear: