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 .remove() to .take() in most collection types #3034

Open
mqudsi opened this issue Dec 11, 2020 · 4 comments
Open

Rename .remove() to .take() in most collection types #3034

mqudsi opened this issue Dec 11, 2020 · 4 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@mqudsi
Copy link

mqudsi commented Dec 11, 2020

This is something that threw me off years ago when I spent more time than I'd care to admit looking for a Collection::take() when it was actually named Collection::remove(), more specifically because I wanted to move an object out of a container (rather than simply remove it from the container) and I was going by what syntax I knew rust used in other places to do just that. Although it's been some years, it occurred to me to file an issue today, although I may have forgotten the exact details of what types I was dealing with at the time (given that this just gets harder the more one waits).

I would like to propose that methods returning T or Option<T> rather than bool should be named Foo::take() rather than Foo::remove(). This would apply to, among others, VecDeque::remove(), LinkedList::remove(), HashMap::remove(), HashMap::remove_entry(), btree_map::OccupiedEntry, hash_map::RawOccupiedEntryMut::remove(), hash_map::RawOccupiedEntryMut::remove_entry(), Vec::remove(), and (somewhat contentiously, but only if you don't primarily think of it as a collection) String::remove().

Reasons in favor of this change:

  1. It is semantically clearer: .take() implies both removing something from its current holder/owner ("taking" it away), and gaining ownership of said thing yourself, whereas .remove() only implies the former and doesn't actually differ (semantically/linguistically) from what we refer to as drop() in the rust world.
  2. It is more consistent. BTreeSet and HashSet already use .take() and most rust users are introduced to Option<T> before they ever use collections, where the nomenclature is .take() and not .remove(). Option<T>, std::mem, Cell, RefCell, OnceCell, SyncOnceCell all use .take() to relinquish ownership of an/the item/entry.
  3. It is not yet too late, as none of the mentioned types currently use .take() for anything, so .remove() could be deprecated without any breaking changes (unless any implement Iterator directly - which I don't think is the case - as std::iter::Iterator::take() is a thing)

Reasons against:

  1. It's a lot of churn.

Possible alternatives:

  1. Reserve .take() for all these types (i.e. make sure no future method is created with that name) both a) to prevent confusion, and b) so that rustc can automatically suggest using remove() any time someone mistypes .take() instead on one of these types.
@Lokathor
Copy link
Contributor

Renaming existing methods in the standard library is, simply, not an attainable goal at this time.

There's not going to be breaking changes in the standard library any time in the foreseeable future.

@mqudsi
Copy link
Author

mqudsi commented Dec 11, 2020

Is labeling a method deprecated considered a breaking change? I could see that argument going both ways.

@sfackler
Copy link
Member

It is not, but we are not deprecating remove. You could add a rustdoc alias to take for them though.

@camsteffen
Copy link

I think HashSet has take because it is the more unusual case and remove is reserved for the more typical case (if set.remove() { .. }). But the general convention, I believe, is to use take for singletons like Option and use remove for collections of multiple values.

@workingjubilee workingjubilee added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

5 participants