Skip to content
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

Tracking Issue for slice_range #76393

Open
2 of 4 tasks
dylni opened this issue Sep 5, 2020 · 31 comments
Open
2 of 4 tasks

Tracking Issue for slice_range #76393

dylni opened this issue Sep 5, 2020 · 31 comments
Labels
A-slice Area: `[T]` C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@dylni
Copy link
Contributor

dylni commented Sep 5, 2020

The feature gate for the issue is #![feature(slice_range)].

This issue currently tracks the following API:

pub mod slice {
    pub fn range<R>(range: R, bounds: RangeTo<usize>) -> Range<usize>
    where
        R: RangeBounds<usize>;
}

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

  • Should this method be defined on RangeBounds or Range instead? (Move slice::check_range to RangeBounds #76885)
  • Should a non-panicking version be added?
  • Should this wait for #[final] methods in traits?
    • The main reason this isn't on RangeBounds is that today unsafe code can't trust any method on a not-unsafe trait to validate anything. But if this was a final method, and thus not overridable, then unsafe code could trust it.

Implementation history

@dylni dylni added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Sep 5, 2020
@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 6, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 7, 2020
…check-range, r=jyn514

Adjust documentation for slice_check_range

Adjust documentation for rust-lang#76393.
@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Sep 10, 2020
@RalfJung
Copy link
Member

RalfJung commented Sep 13, 2020

With #75207 the type of this function changed, so the issue description should likely be adjusted -- unless you want to provide the old one as a convenience wrapper? I think that would be a bad idea though. The function's docs clearly position it as something used in unsafe code, and in unsafe code I'd rather not make people create slices when all they need is to compute their length. As #76662 showed, often creating those slices is not correct.

@RalfJung
Copy link
Member

Alternatively, maybe it would make more sense to make this a method on RangeBounds?

@dylni
Copy link
Contributor Author

dylni commented Sep 13, 2020

Well, the reason I chose to define it on a slice is that (I think) it's not possible to create a slice of length usize::MAX without a custom allocator. Thus, panicking for an inclusive usize::MAX index is reasonable. I didn't want this method to cause developers to always ignore ..=usize::MAX, even though I don't know why you'd need a range that large.

Outside the standard library, wouldn't &[MaybeUninit] be necessary to create an uninitialized slice anyway?

@RalfJung
Copy link
Member

RalfJung commented Sep 13, 2020

Well, the reason I chose to define it on a slice is that (I think) it's not possible to create a slice of length usize::MAX without a custom allocator.

It is possible though: &[(); usize::MAX].

Outside the standard library, wouldn't &[MaybeUninit] be necessary to create an uninitialized slice anyway?

This applies inside and outside the standard library. The alternative is a raw slice, *const [T].

@dylni
Copy link
Contributor Author

dylni commented Sep 13, 2020

It is possible though: &[(); usize::MAX].

Right, that's what I was missing.

This applies inside and outside the standard library. The alternative is a raw slice, *const [T].

I agree the API change makes sense now. Thanks for the explanations!

I also fixed the code sample in this issue.

@dylni
Copy link
Contributor Author

dylni commented Sep 13, 2020

Alternatively, maybe it would make more sense to make this a method on RangeBounds?

Now that it doesn't need a slice, I think this might be better. I'll think about it and likely create a PR to move it.

@dylni
Copy link
Contributor Author

dylni commented Sep 18, 2020

@RalfJung

Alternatively, maybe it would make more sense to make this a method on RangeBounds?

Is this possible? I don't think provided methods can restrict the trait's type parameter at all, but Add and PartialOrd would be required at least. It could be defined for (Bound<usize>, Bound<usize>), but that seems worse.

@RalfJung
Copy link
Member

RalfJung commented Sep 18, 2020

Is this possible? I don't think provided methods can restrict the trait's type parameter at all, but Add and PartialOrd would be required at least. It could be defined for (Bound, Bound), but that seems worse.

Hm, you could play with something like fn provided(self) where Self: Bound.

@dylni
Copy link
Contributor Author

dylni commented Sep 18, 2020

Hm, you could play with something like fn provided(self) where Self: Bound.

Thanks. I'm not sure how I missed bounds on the method itself.

@KodrAus KodrAus added the A-slice Area: `[T]` label Sep 22, 2020
@scottmcm
Copy link
Member

scottmcm commented Oct 3, 2020

I think having the #76885 version of this for precondition checking is great.

I'm dropping by from #77480 (comment) to ponder whether there's other, more user-focused versions of this that could make sense too. Spitballing: something on SliceIndex like .check_index(x) that could return Result<Range<usize>, _>, targeted more at users who want to check things before calling other methods than at people implementing APIs that take RangeBounds.

@dylni
Copy link
Contributor Author

dylni commented Oct 4, 2020

@scottmcm

Spitballing: something on SliceIndex like .check_index(x) that could return Result<Range<usize>, _>, targeted more at users who want to check things before calling other methods than at people implementing APIs that take RangeBounds.

The problem with adding it to SliceIndex is that there's no way to represent usize as Range<usize>, so users would end up going back to RangeBounds anyway. However, a non-panicking version of assert_len might be a good idea. I added it to the unresolved questions, but it can even be added after this method is stabilized.

@dylni dylni changed the title Tracking Issue for slice_check_range Tracking Issue for range_bounds_assert_len Oct 5, 2020
@clarfonthey
Copy link
Contributor

Note: I didn't even realise this method existed despite searching for it, and ended up adding an implementation of SliceIndex for (Bound<usize>, Bound<usize>), which could potentially be a better way of doing this? Both could coexist but I haven't thought enough about how that would happen.

@dylni
Copy link
Contributor Author

dylni commented Jan 17, 2021

I thought about that. Also, this is the PR for anyone reading this issue: #81108

I think it depends on how you expect your impl to be used. Would it be used in a way this method would already cover? The advantage of this method is that it can be used when you don't have a slice:

let Range { start, end } = range.assert_len(self.len());

However, the design has changed a few times, so it's worth discussing what would be best.

@RalfJung
Copy link
Member

IMO assert_len is not a great name, since it sounds like this is asserting the length of the range. It is impossible to tell what this function does without reading the docs. In this regardg, IMO it is worse than the previous slice::check_bounds(len, range).

The assert in the name makes sense because it panics on OOB, but on the other hand usually assert functions don't return anything. This function does both an assert and actually converts the indices to Range, so that will be tricky to express.

@scottmcm
Copy link
Member

Idea: it could take RangeTo<usize> to help clarify the meaning of the argument, without introducing overhead.

That would allow a call like range.assert_subset_of(..self.len()), pending bikeshedding.

@dylni
Copy link
Contributor Author

dylni commented Jan 17, 2021

IMO assert_len is not a great name, since it sounds like this is asserting the length of the range.

Naming this method has always been a problem. That was simply the best name I found at the time, and I'd be in favor of changing it.

That would allow a call like range.assert_subset_of(..self.len()), pending bikeshedding.

This is a great idea. To avoid confusion with other assert_* items, maybe ensure_subset_of would be better. I'll open a PR later with this new design to continue the discussion.

@dylni
Copy link
Contributor Author

dylni commented Jan 18, 2021

I created the PR: #81154

@bluss
Copy link
Member

bluss commented Jan 18, 2021

As @xfix detected in #81157, this method is mildly dangerous (NOTE: current iteration when this comment was posted). The method cannot be trusted for asserting safety-critical invariants, because the trait is neither sealed nor an unsafe trait.

@dylni
Copy link
Contributor Author

dylni commented Jan 18, 2021

Thanks @bluss.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Feb 11, 2021
… r=KodrAus

Improve design of `assert_len`

It was discussed in the [tracking issue](rust-lang#76393 (comment)) that `assert_len`'s name and usage are confusing. This PR improves them based on a suggestion by `@scottmcm` in that issue.

I also improved the documentation to make it clearer when you might want to use this method.

Old example:

```rust
let range = range.assert_len(slice.len());
```

New example:

```rust
let range = range.ensure_subset_of(..slice.len());
```

Fixes rust-lang#81157
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 23, 2021
… r=KodrAus

Improve design of `assert_len`

It was discussed in the [tracking issue](rust-lang#76393 (comment)) that `assert_len`'s name and usage are confusing. This PR improves them based on a suggestion by `@scottmcm` in that issue.

I also improved the documentation to make it clearer when you might want to use this method.

Old example:

```rust
let range = range.assert_len(slice.len());
```

New example:

```rust
let range = range.ensure_subset_of(..slice.len());
```

Fixes rust-lang#81157
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 23, 2021
… r=KodrAus

Improve design of `assert_len`

It was discussed in the [tracking issue](rust-lang#76393 (comment)) that `assert_len`'s name and usage are confusing. This PR improves them based on a suggestion by ``@scottmcm`` in that issue.

I also improved the documentation to make it clearer when you might want to use this method.

Old example:

```rust
let range = range.assert_len(slice.len());
```

New example:

```rust
let range = range.ensure_subset_of(..slice.len());
```

Fixes rust-lang#81157
@dylni dylni changed the title Tracking Issue for range_bounds_assert_len Tracking Issue for slice_range Mar 7, 2021
@jendrikw
Copy link
Contributor

Could slice::split_point_of benefit from this?

@dylni
Copy link
Contributor Author

dylni commented Jan 23, 2022

@jendrikw Unfortunately, I doubt it. This function is used primarily to apply a length to a range, but the length for split_point_ofis irrelevant. This function returns the same range for (Unbounded, Excluded(length)) and (Included(0), Unbounded), for example.

@reitermarkus
Copy link
Contributor

@dylni, given the implementation of slice:range hasn't changed in almost 4 years, is this ready to be stabilised?

@dylni
Copy link
Contributor Author

dylni commented Feb 3, 2024

@reitermarkus Personally, I no longer have a use for this method, so I cannot comment on if the current implementation is useful and makes sense. Do you have examples of this method providing benefit outside of libstd?

@reitermarkus
Copy link
Contributor

I used it in heapless to implement Vec::drain, which basically mirrors the std::Vec::drain implementation: rust-embedded/heapless#444

Stabilising would reduce the amount of code that needs to be duplicated, as is currently the case.

@jonhoo
Copy link
Contributor

jonhoo commented Feb 15, 2024

I've also had a use for it recently in an API that I would like to be able to take an argument of type impl RangeBounds<usize> (just like Vec::drain). Seems like this method is the (only?) way to turn impl RangeBounds<usize> into Range<usize> in std.

@clarfonthey
Copy link
Contributor

Since so many people mentioned it, I opened #121148 to add try_range, which returns an Option instead of panicking. It just feels weird not having the pair together, and hopefully having both will make this more comfortable to stabilise.

@dylni
Copy link
Contributor Author

dylni commented Feb 17, 2024

@reitermarkus Would you be able to create the stabilization report? Unfortunately, as I no longer make use of the feature and am not familiar with the stabilization process, I may not have the time to devote for a while. If not, I will look to create one once as soon as I have the opportunity.

@Dylan-DPC
Copy link
Member

@reitermarkus Would you be able to create the stabilization report? Unfortunately, as I no longer make use of the feature and am not familiar with the stabilization process, I may not have the time to devote for a while. If not, I will look to create one once as soon as I have the opportunity.

we just added a new function which isn't merged yet and is tied to the same tracking issue, so we need to spend some time for it to »bake« if we plan on stabilising this all together. We could move it to a different feature gate, but for a small feature like this, I'm not sure that's a good idea.

it's fine if you can't write the stabilisation report. The person who opens the tracking issue isn't obliged to write the stabilisation report, so once we give enough time for this, I will try and look towards writing the stabilisation report for it

@dtolnay
Copy link
Member

dtolnay commented Mar 11, 2024

#95274 added #[must_use] to this function, which seems potentially overbearing to me. Is #[must_use] a net benefit for slice::range? Or would it be common to call only for the bounds checking/panic side effect?

Of course for slice::try_range a #[must_use] is appropriate because that one has no side effect.

jhpratt added a commit to jhpratt/rust that referenced this issue Mar 11, 2024
Add slice::try_range

This adds a fallible version of the unstable `slice::range` (tracking: rust-lang#76393) which is highly requested in the tracking issue.

Hoping this can slide by without an ACP (since the feature is already being tracked), but let me know otherwise.
@scottmcm
Copy link
Member

@dtolnay To me, the point of slice::range is that you can't trust an impl RangeBounds, so it's incorrect to use slice::range for its bounds checking side effect only -- the next time you call .end_bound() it might give something different. So you must use the returned Range rather than the input.

Though of course it would be nice if the must_use actually said something like that. (I'm still kinda sad we're adding empty must_uses that don't have anything custom to say, rather than just making a better lint that warns on more things.)

@dtolnay
Copy link
Member

dtolnay commented Mar 11, 2024

Good call. Now must_use makes sense to me. Thank you!

I searched GitHub for "start <= end &&" and there are a few hits that might be able to correctly use slice::range without using the return value, in code that does not involve impl RangeBounds:

but arguably those are APIs that would be better redesigned to use impl RangeBounds instead of start: usize, end: usize, and if not, writing slice::range(start..end, ..self.len); is not a definite improvement over the assert they currently have.

If someone wants to extend the documentation of slice::range to point out why the return value can be necessary for soundness in the situation of an untrusted RangeBounds impl, #81138 has an example of a real-world bug like that.

jhpratt added a commit to jhpratt/rust that referenced this issue Mar 11, 2024
Add slice::try_range

This adds a fallible version of the unstable `slice::range` (tracking: rust-lang#76393) which is highly requested in the tracking issue.

Hoping this can slide by without an ACP (since the feature is already being tracked), but let me know otherwise.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 11, 2024
Rollup merge of rust-lang#121148 - clarfonthey:try-range, r=dtolnay

Add slice::try_range

This adds a fallible version of the unstable `slice::range` (tracking: rust-lang#76393) which is highly requested in the tracking issue.

Hoping this can slide by without an ACP (since the feature is already being tracked), but let me know otherwise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-slice Area: `[T]` C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests