Settling some key naming conventions

[cmr]

unwrap_or_fail. Long, a bit unwieldy perhaps, but obeys the fallible vs infallible distinction, and is perfectly explicit about what it is doing, down to reusing the same unwrap word that the rest do.

I think the length will cause people to hate this suggestion.

(Also, I think we should be using total/partial instead of infallible/fallible…)

[bachm]

@aturon what do you think of renaming assert_err to assert_ok? Assuming it’s even meant to fail on an Err.

[chris]

You’re still misunderstanding assert_err. Look at the signature:

fn assert_err(self) -> E

E, not V. This is for getting the error out:

match self {
    Ok(_) => fail!(),
    Err(e) => e,
}

[phaylon]

Was assume_* considered yet? E.g.

foo.assume_ok()
foo.assume_some()
foo.assume_err()

[aturon]

I think the similarity to assert! is a good thing. We have many functions – like get, in this proposal – where we use the same name with different arguments/returns to get at a similar concept.

[aturon]

I thought about assert_ok/assert_some. It would certainly be a reasonable choice, but it’s more verbose and I’m not sure it ultimately clarifies much. My feeling is that seeing foo.assert() the first time will raise an eyebrow (a good thing!) and a quick look at the docs will make clear what’s going on.

assert is very similar to Option::expect, which I think people are generally happy with.

[cmr]

Yeah, it seems to be as good of a name as we’re going to get…

[eridius]

expect() doesn’t have a long history of precedence of having some other meaning. At least, I’ve never run across any usage of an expect() function before. This lack of semantic baggage is why it works well.

I still strongly disagree that assert() is right. I would rather stick with unwrap() if we can’t come up with a better name. I know that means that unwrap() on Option is fallable but is (presumably) not fallible elsewhere, but that hasn’t actually been a problem in practice with this particular function. Probably because the vast majority of unwrap() uses is for unwrapping an Option (or a Result), so the few times you unwrap something else you already know you’re working with a different type where it’s infallible.

I’d certainly like to have a different name that allows us to make the fallible nature of this API consistent, but assert() is not that name.

---

New suggestion: since unwrap() is usually used with Option/Result (AFAIK; I’d love to see some data on this), leave that alone. It’s a good name, it’s short, it doesn’t have the baggage of assert(), and variants work well for e.g. Result::unwrap_err().

Instead, rename unwrap() on other wrappers/cells to something like .into_inner() or .into_value(). Or maybe even just .inner(), which is nice and short and suggests that there is a single “inner” value that will be returned. Even better, it’s a name that’s not already used in the standard libraries (according to rustdoc search).

[aturon]

@eridius renaming other uses of unwrap to into_inner() or inner() is something I had considered as well. At least in libstd, there aren’t many types providing unwrap, and several of those that do could use conversion conventions instead. I’m certainly open to this route.

But I still prefer assert, precisely because of the semantic “baggage” associated with it: it signals that you are making an assertion that will fail! if it doesn’t hold. By contrast, unwrap doesn’t immediately suggest the possibility of task failure.

So, to be clear, I’m trying to achieve two things here:

1. To separate current uses of unwrap into those that can fail and those that can’t.
2. To make using the unwrap operation on Option/Result more clearly dangerous.

and I think that assert serves both goals well.

Another possibility would be to use expect, which has similar connotations, and rename our current expect to something else (something like expect_msg, but hopefully better).

[eridius]

unwrap() is not the only method we have in our libraries that can fail. I don’t see why it should be special and use assert() to indicate that, when every other potentially-failing method wouldn’t get that.

[aturon]

@eridius unwrap plays a somewhat special role, since it’s the place where error handling via Result (or Option) is converted into failure.

In general, we’d like to move closer to a world where fail! is the result of either indexing operations or assert (and perhaps a few others), or catastrophic situations like OOM.

But even if that doesn’t happen, I still think assert is a better choice than unwrap for the reasons I gave above.

[aturon]

Another option, rather than into_iter, would be iter_owned.

(More generally, using suffix _owned rather than prefix or suffix move.)

Thoughts?

[alexcrichton]

Hm I suppose iter_owned does stay in line with iter_mut and iter, so it’s definitely nice in that respect!

I suppose though that we could spin it such that the first part of a method name is the “consumption mode” where in this case it’s into becuase it’s consuming the value, and the second part of the name is iter as it’s producing an iterator. As in, we may be able to explain into_iter in terms of conventions even if it doesn’t line up exactly with iter_mut and iter (perhaps).

Are we avoiding the term “owned” like we are with “move” because not all values are owned though? (thinks like integers).

[aturon]

I agree that into_iter is fine; I’m not worried about the prefix/suffix here per se.

But I was thinking more generally about “move” variants, and whether we should have a consistent way to signal them here and elsewhere. This tends to come up when the “default” variant of some operation gives you a reference.

The idea of “owned” here is in opposition to “borrowed”, but I agree it could be confusing when applied to Copy types.

[aturon]

After discussion during the API stabilization meeting, we’ve settled on the conventions now described in http://aturon.github.io/style/naming/README.html and subsections.

[pnkfelix]

(For the record, I like @cmr’s unwrap_or_fail more than assert. In particular, I think it satisfies all of the criteria @aturon laid out in his comment above, while it also directly conveys that it returns a value, which assert does not, in my mind.)

[jfager]

The biggest problem w/ either unwrap or assert is that they’re the least-effort path to get the value out of an Option. Why would you want the only method on Option whose documentation comes with a warning to avoid using it to be the easiest thing to reach for? In ergonomic order:

// short, natural looking, but no hint of failure
foo(opt.unwrap());  

// short, hints at failure, but unexpectedly returns
foo(opt.assert());   

// short, infallible, but longer than assert and weighted down with a closure.
opt.map(|o| foo(o)); 

// explicit about its failure and costs more to write than map
foo(opt.unwrap_or_fail());

// get rid of the method altogether (could also axe expect)
foo(opt.unwrap_or_else(|| fail!());

// the proposed if-let
if let Some(o) = opt { foo(o); }

// the doc-blessed alternative
match opt {
    Some(o) => foo(o),
    None => ()
}

I’m +1 for either unwrap_or_fail or getting rid of the method altogether.

[nrc]

+1 for unwrap_or_fail, -1 for assert, for reasons given above…

[pnkfelix]

A comment on rust-lang/rust from llogiq had this slight shortening mixed in with a collection of not-so-serious suggestions: myOption::or_fail()

That would be another reasonable option along the same lines as unwrap_or_fail, if for some reason we want to ensure that unwrap is only used with non-fallible variants.

[jfager]

or_fail would be weird b/c it would return t in the Some(t) case, where or and or_else both return Some(t).