Improve doc of some methods that take ranges #140983

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

Jump to bottom

Open

tkr-sh wants to merge 5 commits into rust-lang:master

from tkr-sh:master

Open

Improve doc of some methods that take ranges #140983

tkr-sh wants to merge 5 commits into rust-lang:master from tkr-sh:master

+16 −16

Conversation

tkr-sh

Copy link

Contributor

@tkr-sh tkr-sh commented May 13, 2025 •

edited

Loading

Some methods that were taking some range in parameter were a bit inconsistent / unclear in the panic documentation.

Here is the recap:

Replaced "start/end point" by "start/end bound" to be coherent with RangeBounds naming (it's also easier to understand I think)
Previously, it was written "[...] or if the end point is greater than the length of [...]", but this is not entirely true! Actually, you can have a start bound that is greater than the length, with an end bound that is unbounded and it will also panic. Therefore I think that "[...] one of the range bound is bounded and greater than the length of [...]" is better!
String methods weren't mentionning that the method panics if start_bound > end_bound but it actually does! It uses slice::range which panics when start > end. (https://doc.rust-lang.org/stable/src/alloc/string.rs.html#1932-1934, https://doc.rust-lang.org/stable/src/core/slice/index.rs.html#835-837).
You can also test it with:

struct MyRange;
impl std::ops::RangeBounds<usize> for MyRange {
 fn start_bound(&self) -> std::ops::Bound<&usize> {
 std::ops::Bound::Included(&3usize)
 }
 fn end_bound(&self) -> std::ops::Bound<&usize> {
 std::ops::Bound::Included(&1usize)
 }
}
fn main() {
 let mut s = String::from("I love Rust!");
 s.drain(MyRange); // panics!
}

@rustbot rustbot assigned ibraheemdev

May 13, 2025

@rustbot

Copy link

Collaborator

rustbot commented May 13, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
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

@rustbot rustbot added S-waiting-on-review T-libs labels

May 13, 2025

@tkr-sh

Copy link

Contributor Author

tkr-sh commented May 13, 2025

@rustbot label +A-docs

@rustbot rustbot added the A-docs label

May 13, 2025

hkBst

hkBst reviewed

May 15, 2025

View reviewed changes

library/alloc/src/collections/vec_deque/mod.rs Outdated Show resolved Hide resolved

hkBst

hkBst reviewed

May 15, 2025

View reviewed changes

library/alloc/src/collections/vec_deque/mod.rs Show resolved Hide resolved

@tkr-sh

Copy link

Contributor Author

rustbot commented May 27, 2025

Failed to set assignee to hkBst: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

hkBst

hkBst approved these changes

May 27, 2025

View reviewed changes

Copy link

Member

@hkBst hkBst left a comment

Choose a reason for hiding this comment

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

I'm happy with this.

tgross35

tgross35 reviewed

May 28, 2025

View reviewed changes

library/alloc/src/collections/vec_deque/mod.rs Outdated Show resolved Hide resolved

ibraheemdev

ibraheemdev requested changes

Jun 15, 2025

View reviewed changes

library/alloc/src/collections/vec_deque/mod.rs Outdated

Comment on lines 1461 to 1462

/// Panics if the range has `start_bound > end_bound`, or, a range bound is

/// bounded and greater than the length of the deque.

Copy link

Member

@ibraheemdev ibraheemdev Jun 8, 2025

Choose a reason for hiding this comment

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

I'm not sure about the start_bound and end_bound variables here because they aren't parameters. I also find "a range bound" confusing and wasn't sure what it was referring to at first. I would prefer something like this.

Suggested change

/// Panics if the range has `start_bound > end_bound`, or, a range bound is

/// bounded and greater than the length of the deque.

/// Panics if the starting bound is greater than the end bound, or if any of the range

/// bounds are greater than the length of the deque.

Copy link

Member

@hkBst hkBst Jun 16, 2025

Choose a reason for hiding this comment

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

The salient part is that an unbounded range bound is fine, but a bounded one must be in-bounds.

Copy link

Contributor Author

@tkr-sh tkr-sh Jun 16, 2025

Choose a reason for hiding this comment

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

I think that people understand start_bound > end_bound because R: RangeBounds<usize>, RangeBounds which has those 2 methods. (start_bound & end_bound)
Improve doc of some methods that take ranges #140983 (comment) . Also, a range can have a Bound which is unbounded (which is fine, as pointed by @hkBst.), so I think that this new definition is less precise,

Copy link

Member

@ibraheemdev ibraheemdev Jun 16, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

I think that's fine, an unbounded range wouldn't be considered "a range bound greater than the length of the deque". If you feel being more precise is needed then something like "if the range is bounded on either end past the length of the deque" would work, but I find the current wording confusing.

Copy link

Contributor Author

@tkr-sh tkr-sh Aug 10, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

52f0b6a & 5c2a36e

@rustbot rustbot added S-waiting-on-author and removed S-waiting-on-review labels

Jun 15, 2025

@alex-semenyuk

Copy link

Member

alex-semenyuk commented Aug 10, 2025

@tkr-sh
Thanks for your contribution.
Form wg-triage. Any updates on this PR?

@rustbot

This comment has been minimized.

tkr-sh added 4 commits

August 10, 2025 10:39

@tkr-sh


 📝 docs: improve doc of some methods w/ ranges

2bd5fcc

@tkr-sh


 ✏️ fix: one of the range "s/bound/bounds"

14cf0d2

@tkr-sh


 🚚 refactor: rename from "one of the" to "a"

f21ed03

@tkr-sh


 ♻️ refactor: reword

52f0b6a

@tkr-sh tkr-sh force-pushed the master branch from a8efc51 to 52f0b6a Compare

August 10, 2025 10:40

@tkr-sh


 ✏️ fix: typo

5c2a36e

@tgross35

Copy link

Contributor

tgross35 commented Aug 10, 2025

Please no emojis in commit messages

@tkr-sh

Copy link

Contributor Author

tkr-sh commented Aug 10, 2025 •

edited

Loading

hello @alex-semenyuk !
I just commited changes that should make this PR approved I think.
Sorry for the delay btw.

@rustbot review

@rustbot

Copy link

Collaborator

rustbot commented Aug 10, 2025

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

@rustbot rustbot added S-waiting-on-review and removed S-waiting-on-author labels

Aug 10, 2025

@tkr-sh

Copy link

Contributor Author

tkr-sh commented Aug 10, 2025 •

edited

Loading

@tgross35 is there a guideline in the rustc dev book that says to not do that ? If not, I don't see any reason why I should not do it.

@ibraheemdev

Copy link

Member

ibraheemdev commented Aug 18, 2025 •

edited

Loading

Could you please squash the commits? This looks good after that's done.

@tgross35

Copy link

Contributor

tgross35 commented Aug 18, 2025

@tgross35 is there a guideline in the rustc dev book that says to not do that ? If not, I don't see any reason why I should not do it.

Ideally everything like that would be documented, but we assume that if things aren't mentioned then existing conventions will be followed. This is where reviewers come in :)

@ibraheemdev ibraheemdev added S-waiting-on-author and removed S-waiting-on-review labels

Aug 19, 2025

Labels

A-docs S-waiting-on-author T-libs

6 participants

@tkr-sh @rustbot @alex-semenyuk @tgross35 @ibraheemdev @hkBst

Improve doc of some methods that take ranges #140983

Are you sure you want to change the base?

Improve doc of some methods that take ranges #140983

Conversation

@tkr-sh tkr-sh commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 13, 2025

Uh oh!

tkr-sh commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

tkr-sh commented May 27, 2025

Uh oh!

rustbot commented May 27, 2025

Uh oh!

@hkBst hkBst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@ibraheemdev ibraheemdev Jun 8, 2025

Choose a reason for hiding this comment

Uh oh!

@hkBst hkBst Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

@tkr-sh tkr-sh Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

@ibraheemdev ibraheemdev Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@tkr-sh tkr-sh Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-semenyuk commented Aug 10, 2025

Uh oh!

This comment has been minimized.

tgross35 commented Aug 10, 2025

Uh oh!

tkr-sh commented Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 10, 2025

Uh oh!

tkr-sh commented Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibraheemdev commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgross35 commented Aug 18, 2025

Uh oh!

Uh oh!

@tkr-sh tkr-sh commented May 13, 2025 •

edited

Loading

@ibraheemdev ibraheemdev Jun 16, 2025 •

edited

Loading

@tkr-sh tkr-sh Aug 10, 2025 •

edited

Loading

tkr-sh commented Aug 10, 2025 •

edited

Loading

tkr-sh commented Aug 10, 2025 •

edited

Loading

ibraheemdev commented Aug 18, 2025 •

edited

Loading