Skip to content

Potential documentation gap in impl Index<Range<usize>> for [T] #121568

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
pezcore opened this issue Feb 24, 2024 · 3 comments
Open

Potential documentation gap in impl Index<Range<usize>> for [T] #121568

pezcore opened this issue Feb 24, 2024 · 3 comments
Assignees
Labels
A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@pezcore
Copy link

pezcore commented Feb 24, 2024

Location

Index::index

Summary

In Rust, if you attempt to index a slice or array with a "backwards" Range<usize> , the program panics:

let arr = [0,1,2,3,4,5,6];
let slice = &arr[4..1]; // panic: slice index starts at 4 but ends at 1 

This behavior is unsurprising, but not obvious: a reasonable alternative behavior would be to just return the empty slice &[]. This is what python does.

The documentation for Index::index states that the indexing operation "May panic if the index is out of bounds", but doesn't define what it means to be "out of bounds". I'd argue that in this example, the Range 4..1 is actually not out-of-bounds for an array of size 7 because both 4 and 1 are less than 7 and therefore, by the letter of the documentation, the panic behavior here is wrong and returning the empty slice &[] is a more reasonable behavior to expect than panicking.

I feel like the documentation should make explicitly clear that this panics if either of the following are true:

  1. range.end > slice.len()
  2. range.end < range.start

Also the documentation should make clear that this does panic, not just that it might, because surely this panic is not UB.

@pezcore pezcore added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Feb 24, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 24, 2024
@clubby789
Copy link
Contributor

clubby789 commented Feb 24, 2024

Implementors of the trait might or might not panic on third party types, so 'may' is technically correct. The documentation for the implementations themselves could say 'will'

@pezcore
Copy link
Author

pezcore commented Feb 24, 2024

"may panic" is correct on the Index<Idx> level, but on the Index<Range<usize>> level of specificity , the documentation should say that it does panic and define the exact set of conditions that panic.

@jieyouxu jieyouxu added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 26, 2024
@JaniM
Copy link
Contributor

JaniM commented Mar 31, 2024

@rustbot claim

This is a weird one. It is very difficult to find this specific implementation from the docs (having to jump through SliceIndex). We can add documentation for each SliceIndex impl, but I feel like thwre'd be value in rethinking the discoverability of this.

JaniM added a commit to JaniM/rust that referenced this issue Mar 31, 2024
Implementation note: The most probable place for users to find
the documentation is at https://doc.rust-lang.org/std/slice/trait.SliceIndex.html

On that page, documentation added to specific methods will not
be visible. As such, I opted to add the comments to the impl blocks
directly.

Helps with rust-lang#121568.
JaniM added a commit to JaniM/rust that referenced this issue Mar 31, 2024
Implementation note: The most probable place for users to find
the documentation is at https://doc.rust-lang.org/std/slice/trait.SliceIndex.html

On that page, documentation added to specific methods will not
be visible. As such, I opted to add the comments to the impl blocks
directly.

Helps with rust-lang#121568.
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Mar 31, 2024
…trieb

doc: describe panic conditions for SliceIndex implementations

Implementation note: The most probable place for users to find the documentation is at https://doc.rust-lang.org/std/slice/trait.SliceIndex.html

On that page, documentation added to specific methods will not be visible. As such, I opted to add the comments to the impl blocks directly.

Helps with rust-lang#121568.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 31, 2024
Rollup merge of rust-lang#123271 - JaniM:janim/sliceindex-doc, r=Nilstrieb

doc: describe panic conditions for SliceIndex implementations

Implementation note: The most probable place for users to find the documentation is at https://doc.rust-lang.org/std/slice/trait.SliceIndex.html

On that page, documentation added to specific methods will not be visible. As such, I opted to add the comments to the impl blocks directly.

Helps with rust-lang#121568.
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 T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants