Rename `end` field of inclusive `Range` APIs to `last`

[sendittothenewts]

Proposal

Problem statement

RangeToInclusive contains an end field that has the same name, but slightly different semantics to the end field of RangeTo. This means that innocuous-looking changes like replacing ..=n-1 with ..n can cause silent behaviour changes and off-by-one bugs. Such changes are suggested by the clippy::range_minus_one and clippy::range_plus_one lints. This issue continues to exist in the new core::range::RangeToInclusive API, and will now also affect core::range::RangeInclusive.

Motivating examples or use cases

fn main() {
	let names = ["fred", "barney", "wilma"];
	let bound = ..=names.len() - 1; // clippy suggests `..names.len()` instead

	println!("{}", bound.end); // Would print "3" instead of "2"
	println!("{}", names[bound.end - 1]); // Would print "wilma" instead of "barney"
	println!("{}", names[bound.end]); // Would panic instead of printing "wilma"
}

Solution sketch

Rename the end field to last in the unstable core::range::RangeInclusive and core::range::RangeToInclusive types. The legacy core::ops::RangeInclusive and core::ops::RangeToInclusive types would not be changed, to preserve backwards compatibility.

Disadvantages

• A new RangeToInclusive type would have to be introduced, rather than re-exporting the original type, causing additional compatibility issues that do not presently exist.
• RangeInclusive would be less affected, as it is already a new type, and existing users already have to change .end() to .end.

Alternatives

• Don't do this, and just accept the footgun.
• Make this change, and also provide a deprecated end() method on RangeInclusive that steers users towards the new last field. This unfortunately would not work for RangeToInclusive, where end is already a field.
• It would be possible to introduce this change only for RangeInclusive to avoid some incompatibility, but that would create a peculiar inconsistency.

Links and related work

rust-lang/rust-clippy#3307 (comment)
rust-lang/rust#125687 (comment)

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

• We think this problem seems worth solving, and the standard library might be the right place to solve it.
• We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

• We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
• We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

[sendittothenewts]

I don't think the compatibility issue mentioned under 'Disadvantages' above is really a big problem for 2 reasons:

• The new Range APIs are opt-in, so this won't be a breaking change for existing core::ops::RangeInclusive users, until such time as they elect to migrate to core::Range::*
• Existing users of RangeInclusive already have to make some changes, like replacing .start() and .end() with .start and .end, and requiring a change from .end or .end() to .last seems no more onerous than that.

[pitaj]

There is no new RangeToInclusive type (core::range::RangeToInclusive is just a re-export of core::ops::RangeToInclusive), so this would involve adding a new type just for this field rename.

[sendittothenewts]

Ah, yes, that does make quite a difference. Maybe in the future there could be a smoother solution for the compatibility problems inherent in RangeInclusive that could then easily be applied to RangeToInclusive as well, but as things stand this change probably is rather disruptive for such a minor issue.

[traviscross]

In the libs-api call today, this was discussed, and people were generally inclined toward accepting it, on the basis that if we're going to do a migration here, we might as well fix everything that we can as part of that.

We wanted to check first, though, @pitaj, for your thoughts and whether this sounds right to you overall.

[pitaj]

I'm not opposed.

My only reservation is that adding a new RangeToInclusive type seems a little overkill just for the field rename. Then again, it's a relatively rarely used range variant anyways, so it wouldn't be very significant compared to the other changes.

[Amanieu]

Since @pitaj is OK with this then we consider this ACP accepted.

[pitaj]

@rustbot label +unimplemented