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

unwrap/get/get_ref/take etc. conventions #13159

Closed
brson opened this issue Mar 26, 2014 · 9 comments
Closed

unwrap/get/get_ref/take etc. conventions #13159

brson opened this issue Mar 26, 2014 · 9 comments
Assignees
Milestone

Comments

@brson
Copy link
Contributor

brson commented Mar 26, 2014

I'm sure there are other issues about this, which naturally I can't find. We need to make sure the conventions for these accessors are consistent for 1.0. Nominating.

@nikomatsakis
Copy link
Contributor

Off-line with @brson and @alexcrichton suggested that there is some sort of conventions, though they are not universally followed. These conventions can be roughly broken down based on what sort of self they take:

  • unwrap(self) is for converting from Foo<T> to T. It takes self by value.
  • take(&mut self) is for swapping the current value with something else (generally something like None) and returning it.
  • get_ref(&self) and get_mut_ref(&mut self) are for getting a pointer to the contents. These may be shorthands for a longer version that converts from Foo<T> to Foo<&T> and Foo<&mut T>, combined with unwrap.

One name that I did not list is get(). It appears to be something of a mixed bag:

  • For Cell, it converts from Cell<T> to T, but by copying (that being the point of Cell).
  • For collections, it generally plays a lookup role, and generally has the form of get_ref as listed above.

In all cases, these operations can be fallible or infallible depending on the type, which is perhaps unfortunate. I'd like to remedy this if possible but I'm not sure if it is.

@liigo
Copy link
Contributor

liigo commented Mar 28, 2014

how about .v() returns a (cloned) value, and .r() returns a ref? (If ref is not a keyword, .ref() is better.)

@sfackler
Copy link
Member

It'd be nice if we could separate the fallable and infallable methods naming-wise somehow. I always double-take a bit when seeing an unwrap that turns out to be called on an e.g. MemWriter instead of an Option or Result.

@nikomatsakis
Copy link
Contributor

I was hoping to incorporate the word assert or check into the fallible variants, but I never quite found something I liked. I think the name must be short for the following philosophical reason. I feel like there is this distinction between "failures that can be recovered from and those that cannot". In general, libraries should push to make failure recoverable: e.g., vec.last() returns Option<T> rather than failing for an empty vector. But at the layer above, there may be an additional invariant that this vector should never be empty. unwrap is then the cheap way to convert between those two failure modes. In that sense, it's good that it be visible, but bad if it's too long and painful.

Some ideas:

  • get (infallible) vs unwrap (fallible)
    • downside: currently, there is some association of get with "getting a reference", though I think we also call that get_ref
  • unwrap (infallible) vs assert (fallible)
    • downside: foo.assert() seems like an incomplete sentence, foo.assert_some() seems clear but long. But maybe assert_some() the best, despite it's length.
    • downside: assert!() will no doubt wind up being disabled on debug builds. So perhaps expect_some() or enforce_some().

One a related note, I think we should at some point remove get() from collections (or just have it return Option<T> and remove find). We can use an overloaded [] operator to be the fallible variant.

In general, the best state is one where the only fallible APIs are Option/Result's unwrap (whatever it winds up being called) and the [] operator (the generalized equivalent of bounds checks). I imagine there might be a few others, but otherwise I'd think that (in the stdlib at least) most things are recoverable.

@thestinger
Copy link
Contributor

An infallible one should probably just be a Deref overload in many cases now.

@sfackler
Copy link
Member

I don't think Deref is a good fit here. It can't handle the by-value cases, and poses some significant issues even in the by-reference case. For example, new methods can never be added to a type that implements Deref without breaking backwards compatibility.

@nikomatsakis
Copy link
Contributor

On Sat, Mar 29, 2014 at 12:18:36AM -0700, Steven Fackler wrote:

I don't think Deref is a good fit here. It can't handle the
by-value cases, and poses some significant issues even in the
by-reference case. For example, new methods can never be added to
a type that implements Deref without breaking backwards
compatibility.

I tend to agree. It's likely we can overcome the by-value restriction
at some point, but fundamentally I am worried about abuse of Deref.
It's really intended to be used for transparent wrappers like
pointers, not types that encapsulate something else and have
significant interface of their own.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 3, 2014

marking P-backcompat-libs, 1.0

@pnkfelix pnkfelix added this to the 1.0 milestone Apr 3, 2014
@aturon
Copy link
Member

aturon commented Aug 4, 2014

The guidelines now have a convention for these names. I will be rolling them out in a patch soon.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Nov 22, 2014
This change applies the conventions to unwrap listed in [RFC 430][rfc] to rename
non-failing `unwrap` methods to `into_inner`. This is a breaking change, but all
`unwrap` methods are retained as `#[deprecated]` for the near future. To update
code rename `unwrap` method calls to `into_inner`.

[rfc]: rust-lang/rfcs#430
[breaking-change]

Closes rust-lang#13159
cc rust-lang#19091
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants