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

entry API v3 #921

Merged
merged 2 commits into from
Mar 19, 2015
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 128 additions & 0 deletions text/0000-entry_v3.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
- Feature Name: entry_v3
- Start Date: 2015-03-01
- RFC PR: (leave this empty)
- Rust Issue: (leave this empty)

# Summary

Replace Entry::get with Entry::default and Entry::default_with for better ergonomics and clearer
code.

# Motivation

Entry::get was introduced to reduce a lot of the boiler-plate involved in simple Entry usage. Two
incredibly common patterns in particular stand out:

```
match map.entry(key) => {
Entry::Vacant(entry) => { entry.insert(1); },
Entry::Occupied(entry) => { *entry.get_mut() += 1; },
}
```

```
match map.entry(key) => {
Entry::Vacant(entry) => { entry.insert(vec![val]); },
Entry::Occupied(entry) => { entry.get_mut().push(val); },
}
```

This code is noisy, and is visibly fighting the Entry API a bit, such as having to supress
the return value of insert. It requires the `Entry` enum to be imported into scope. It requires
the user to learn a whole new API. It also introduces a "many ways to do it" stylistic ambiguity:

```
match map.entry(key) => {
Entry::Vacant(entry) => entry.insert(vec![]),
Entry::Occupied(entry) => entry.into_mut(),
}.push(val);
```

Entry::get tries to address some of this by doing something similar to `Result::ok`.
It maps the Entry into a more familiar Result, while automatically converting the
Occupied case into an `&mut V`. Usage looks like:


```
*map.entry(key).get().unwrap_or_else(|entry| entry.insert(0)) += 1;
```

```
map.entry(key).get().unwrap_or_else(|entry| entry.insert(vec![])).push(val);
```

This is certainly *nicer*. No imports are needed, the Occupied case is handled, and we're closer
to a "only one way". However this is still fairly tedious and arcane. `get` provides little
meaning for what is done; unwrap_or_else is long and scary-sounding; and VacantEntry litterally
*only* supports `insert`, so having to call it seems redundant.

# Detailed design

Replace `Entry::get` with the following two methods:

```
/// Ensures a value is in the entry by inserting the default if empty, and returns
/// a mutable reference to the value in the entry.
pub fn default(self. default: V) -> &'a mut V {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(&mut self, default: V) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self for sure. Want to consume the entry.

match self {
Occupied(entry) => entry.into_mut(),
Vacant(entry) => entry.insert(default),
}
}

/// Ensures a value is in the entry by inserting the result of the default function if empty,
/// and returns a mutable reference to the value in the entry.
pub fn default_with<F: FnOnce() -> V>(self. default: F) -> &'a mut V {
match self {
Occupied(entry) => entry.into_mut(),
Vacant(entry) => entry.insert(default()),
}
}
```

which allows the following:


```
*map.entry(key).default(0) += 1;
```

```
// vec![] doesn't even allocate, and is only 3 ptrs big.
map.entry(key).default(vec![]).push(val);
```

```
let val = map.entry(key).default_with(|| expensive(big, data));
```

Look at all that ergonomics. *Look at it*. This pushes us more into the "one right way"
territory, since this is unambiguously clearer and easier than a full `match` or abusing Result.
Novices don't really need to learn the entry API at all with this. They can just learn the
`.entry(key).default(value)` incantation to start, and work their way up to more complex
usage later.

Oh hey look this entire RFC is already implemented with all of `rust-lang/rust`'s `entry`
usage audited and updated: https://github.com/rust-lang/rust/pull/22930

# Drawbacks

Replaces the composability of just mapping to a Result with more adhoc specialty methods. This
is hardly a drawback for the reasons stated in the RFC. Maybe someone was really leveraging
the Result-ness in an exotic way, but it was likely an abuse of the API. Regardless, the `get`
method is trivial to write as a consumer of the API.

# Alternatives

Settle for Result chumpsville or abandon this sugar altogether. Truly, fates worse than death.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Pass more information into the default_with closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's literally no information to pass for a vacantentry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reference to the key?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gankro Was this addressed somewhere off-thread or there's a technical reason it can't be done?

It would be neat to save a clone if the 'expensive' case requires the key to generate the value data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API as a thin layer over Entry can't admit an &Key for the simple reason that Entry doesn't expose this. Otherwise it wasn't included in Entry simply because no one presented a compelling usecase (iirc every use of the old API I saw never used the key even though it was given). It's possible to add new methods that expose this functionality at a later date, though.

# Unresolved questions

`default` and `default_with` are universally reviled as *names*. Need a better name. Some candidates.

* set_default
* or_insert
* insert_default
* insert_if_vacant
* with_default