Skip to content

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tkr-sh
Copy link
Contributor

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

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
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 Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. 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 Area: Documentation for any part of the project, including the compiler, standard library, and tools label May 13, 2025
@tkr-sh
Copy link
Contributor Author

tkr-sh commented May 27, 2025

r? @hkBst

@rustbot
Copy link
Collaborator

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.

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.

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

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

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

Choose a reason for hiding this comment

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

  1. I think that people understand start_bound > end_bound because R: RangeBounds<usize>, RangeBounds which has those 2 methods. (start_bound & end_bound)
  2. 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

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

Choose a reason for hiding this comment

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

@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 Jun 15, 2025
@alex-semenyuk
Copy link
Member

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

@rustbot

This comment has been minimized.

@tgross35
Copy link
Contributor

Please no emojis in commit messages

@tkr-sh
Copy link
Contributor Author

tkr-sh commented Aug 10, 2025

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 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 10, 2025
@tkr-sh
Copy link
Contributor Author

tkr-sh commented Aug 10, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

6 participants