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

Rename end field of inclusive Range APIs to last #511

Open
sendittothenewts opened this issue Dec 23, 2024 · 3 comments
Open

Rename end field of inclusive Range APIs to last #511

sendittothenewts opened this issue Dec 23, 2024 · 3 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@sendittothenewts
Copy link

sendittothenewts commented Dec 23, 2024

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 sendittothenewts added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Dec 23, 2024
@sendittothenewts
Copy link
Author

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
Copy link

pitaj commented Dec 23, 2024

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

2 participants