Improve design of assert_len

[dylni]

It was discussed in the tracking issue 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:

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

New example:

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

Fixes #81157

[rust-highfive]

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

[RalfJung]

Name bikeshedding: another option might be ensure_subrange_of. It's slightly longer though.

[dylni]

This PR has been updated to fix #81157 by moving the method to Range. That might make it more difficult to find, but it should be safe to use for bounds checking.

[dylni]

another option might be ensure_subrange_of.

Now that the method's defined on Range, it might be repetitive to write Range::ensure_subrange_of, but I'm not against either name.

[camelid]

I believe this should link to nightly since this is a nightly-only feature:

Suggested change

	[`Range::ensure_subset_of`]: https://doc.rust-lang.org/std/ops/struct.Range.html#method.ensure_subset_of
	[`Range::ensure_subset_of`]: https://doc.rust-lang.org/nightly/std/ops/struct.Range.html#method.ensure_subset_of

[scottmcm]

Could also just delete the file, since library features don't usually add files here -- they just use auto-generated ones.

[RalfJung]

Can't this be a relative link?

[dylni]

@scottmcm I only added it based on the link in tracking issue templates. However, that page is a little confusing, so I might be misunderstanding its recommendation.

Since this page is mostly a link to the tracking issue, removing it makes sense.

[dylni]

@RalfJung Yes, that's a good idea. However, I removed the page instead since it's not very useful and seems to not be required.

[camsteffen]

I would move for assert_subrange_of. To me a "set" is an unordered group of items. And "assert" is well understood to mean "check a condition or panic" where "ensure" is more likely to be misunderstood as "adjust the range so that the condition is met and move on".

[KodrAus]

Since this is a constructor on Range I’m not sure that either ensure or assert are really ideal prefixes here. Maybe even just Range::subset_of(bounds, ..len)? Which naturally lends itself to a Range::checked_subset_of alternative.

[ExpHP]

I hate to continue the bikeshedding even further, but I had to stare a long time at 6da4baeb094c to actually understand how it fixed the soundness. I would have expected the name to become something more like Range::from_range, as it is now a constructor.

(though in that case the second Range argument makes less sense than the original usize)

[camsteffen]

Since this is a constructor on Range I’m not sure that either ensure or assert are really ideal prefixes here.

Ah, sorry I missed that it was a constructor. I agree. Though I still don't like "subset". I like subrange_of() -> Option. It's more flexible and feels more Rusty.

[scottmcm]

I think it'd be good for there to be the panicking_subrange_of version (whatever it's called), since only having the -> Option one would encourage just .unwrap()ing it, and thus getting a worse panic message than current assert_len.

[camsteffen]

Can we get the best of both worlds with a custom Error?

[dylni]

Maybe even just Range::subset_of(bounds, ..len)?

This is a good option, but it might distract from what this method's meant to do. It might be better to remove subset completely, since this method's only purpose is to convert RangeBounds to Range.

I would have expected the name to become something more like Range::from_range, as it is now a constructor.

I agree with @ExpHP about naming this method as a constructor. What does everyone think about Range::from_bounds(bounds, len)? Whenever I use this method, the length argument is obviously a length, so making it a range shouldn't be necessary. Now that there's another range argument, it can be confusing.

That method can return Result, or that can be done with checked_from_bounds or try_from_bounds. It might be better to not panic by default.

[scottmcm]

Hmm, if it has to move off RangeBounds for soundness, maybe it would make sense for it to go back to core::slice::something(...) -- as a constructor it feels a bit odd to me since it's usize-only and doesn't have implementations for the other range types. Whereas semantically it seems more reasonable over with other slice things. (I might see it when looking at the module for from_raw_parts, or something.)

[camsteffen]

Has it been considered to simply return an empty range instead of panicking or None? I think this would make the method nice to use. You can check range.is_empty() if you want, or just use the range.

let vec = vec![1, 2, 3];
let range = (4..5).assert_len(3); // returns 3..3
for n in &vec[range] {
    unreachable!("the slice is empty");
}

A panicking version would still be useful for validating input. But I'm starting to think that an Option/Result version might be the least useful in practice.

[scottmcm]

Has it been considered to simply return an empty range instead of panicking or None?

Note that an empty range is not a usable sentinel here, since (..0).assert_len(3) => 0..0 is expected and successful. (And just in general, "special value" returns are less usable than ones where the type system helps you to remember to look at the special case. One can always .unwrap_or(0..0) if one wants to just do nothing should the index be out of range.)

[camsteffen]

I'm not sure what you mean. My suggestion should not impact (..0).assert_len(3).

[ExpHP]

Has it been considered to simply return an empty range instead of panicking or None? I think this would make the method nice to use. You can check range.is_empty() if you want, or just use the range.

Quite honestly this makes some pretty strong assumptions about how the user intends to use the range. In rust, the general expectation is that if a user supplies a range r as input to something, it will refer to r.len() items, and no fewer.

(I'm also just not generally sure what you're suggesting here. If somebody did (2..7).assert_len(5) would it "saturate" and produce 2..5 like python's semantics, or would it produce 5..5 as some bizarrely defined sentinel? If the former, why are we talking about empty ranges?)

[camsteffen]

Well maybe it was a bad idea 😬. But it would be 2..7 ->2..5.

[dylni]

Whereas semantically it seems more reasonable over with other slice things.

That makes sense. Even though it's not a slice method anymore, you almost always want to use it for slices. Should we go back to slice::check_range? It can return Result<Range<usize>, RangeError>.

But it would be 2..7 ->2..5.

This method is designed to be very strict, so that wouldn't be compatible, but it might be useful as another method. One reason the method panics is to create helpful error messages with little effort. range end index 7 out of range for slice of length 5 is much more helpful than the slice is empty.

[KodrAus]

Moving to an inherent method in slice seems ok to me too, given the assumptions we make on usize. Maybe:

// panics if r isn't a subset of ..self.len()
let Range { start, end } = slice::range_from_subset(r, ..self.len());

I think there are really two things this method does which makes it tricky to name:

• Converts some bounds (a impl RangeBounds<usize>) into a range (a Range<usize>), which is much easier to work with and less risky than external generic code.
• Asserts that slice[bounds] is valid and equivalent to slice[range], except that it doesn't really, because it accepts an arbitrary range as an input and not the slice itself. That's what makes me lean towards a constructor-focused name over an assertion-focused name.

I think I'm more inclined towards a checked_* variant returning Option<Range<usize>> than a try_* variant returning Result<Range<usize>, RangeError> to keep the amount of machinery small.

[dylni]

I think there are really two things this method does which makes it tricky to name:

Yes, this is the problem. All the names given so far either show that the method panics or that it creates a range but not both. That's why I thought returning Result instead of panicking might be a good idea.

I think I'm more inclined towards a checked_* variant returning Option<Range<usize>>

If we add a checked_* method, checked_range_from_subset is very long. Would it be better to name them slice::range and slice::checked_range? This method is similar to BTreeSet::range in many ways.

[bors]

⌛ Testing commit dd1ab4c with merge f4dd191f29b4ad50b4602b82d7b4a5ca450951ad...

[bors]

💔 Test failed - checks-actions

[dylni]

@bors retry

[bors]

@dylni: 🔑 Insufficient privileges: not in try users

[dylni]

@KodrAus This PR should be ready to merge again.

[scottmcm]

@bors r=KodrAus

[bors]

📌 Commit fe4fe19 has been approved by KodrAus