Github Add `unwrap_unchecked()` methods for `Option` and `Result` by ojeda · Pul... - JOYK Joy of Geek, Geek News, Link all geek

Conversation

Contributor

ojeda commented on Jan 10

In particular:

unwrap_unchecked() for Option.
unwrap_unchecked() and unwrap_err_unchecked() for Result.

These complement other *_unchecked() methods in core etc.

Currently there are a couple of places it may be used inside rustc (LinkedList, BTree). It is also easy to find other repositories with similar functionality.

Fixes #48278.

Collaborator

rust-highfive commented on Jan 10

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Contributor

Author

ojeda commented on Jan 10 •

edited

A few extra bits:

I was not sure if this was considered a hole in the API (given we have other *_unchecked() methods, including one equivalent function for Option in btree) or perhaps an RFC is required.
I will add the tracking issue number if people want to see this in.
The Option one could be const, but it would require const_refs_to_cell for the assert. Since anyway it is not a high-priority use case, I left it non-const.
I assumed changing the couple of potential users in core etc. would be done later, so I didn't touch them here. Please let me know if it is the other way around.

Collaborator

rust-log-analyzer commented on Jan 10

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure:

Member

scottmcm commented on Jan 15

perhaps an RFC is required

An RFC is generally not required for individual inherent method additions, unless they'd be setting a new pattern. Usually a method going in unstable like this just needs a code review and a "seems plausible" from a libs member. (Only stabilization requires the whole-team & FCP.)

I can't predict whether this is desired. It seems plausible, but one could also argue that .unwrap_or_else(hint::unreachable_unchecked) is fine.

Contributor

Author

ojeda commented on Jan 15

An RFC is generally not required for individual inherent method additions, unless they'd be setting a new pattern. Usually a method going in unstable like this just needs a code review and a "seems plausible" from a libs member. (Only stabilization requires the whole-team & FCP.)

Yeah, I wasn't sure -- thanks for confirming & the explanation!

I can't predict whether this is desired. It seems plausible, but one could also argue that .unwrap_or_else(hint::unreachable_unchecked) is fine.

Indeed, I can see both angles. I think the major argument in favor is that even ourselves are writing things like:

pub unsafe fn unwrap_unchecked<T>(val: Option<T>) -> T { val.unwrap_or_else(|| { if cfg!(debug_assertions) { panic!("'unchecked' unwrap on None in BTreeMap"); } else { unsafe { core::intrinsics::unreachable(); } } }) }

Member

scottmcm commented on Jan 15 •

edited

Yeah, I don't doubt that its semantics are handy.

Unfortunately a big downside is that debug_assert!s in core only actually run in the bors test suite -- a user calling this method would never see the debug assert fire, since core is always shipped in release. So if this is an extension method in a crate instead, it could be more helpful. (Maybe https://docs.rs/new_debug_unreachable/ could be interested?)

Hopefully one day that'll no longer be the case and these will be more useful again...

Member

Mark-Simulacrum commented on Jan 15

I feel that we should not add these, as they would encourage what is almost always an antipattern. The cases in which the UB actually improves performance in a manner that meaningfully affect the program overall are few, and it is not difficult to write this manually if necessary.

r? @m-ou-se for libs team member review

Contributor

Author

ojeda commented on Jan 15 •

edited

Unfortunately a big downside is that debug_assert!s in core only actually run in the bors test suite -- a user calling this method would never see the debug assert fire, since core is always shipped in release.

There are a few counter-points to that I can think of:

The method (as documented, at least) should not do the assert for users even if it could -- i.e. if a project wants to assert a particular condition on debug for their own testing, they can do so explicitly. That way, they can control the behavior in debug. Otherwise, it would be forced on them. I guess one could argue to also provide a macro that does the assert, though; that seems useful too.
Projects building their own core, e.g. kernels/embedded/freestanding, can still take advantage of them. This can be useful not just to avoid writing an assert on their side (they should still write it if they rely on it being there for some reason), but rather to validate that core is working as expected when compiling for their particular target etc. When working with such projects, every bit counts towards ensuring things are sane.
It is the same for other *_unchecked() methods we have.

I feel that we should not add these, as they would encourage what is almost always an antipattern. The cases in which the UB actually improves performance in a manner that meaningfully affect the program overall are few, and it is not difficult to write this manually if necessary.

I understand it is not a very common use case and the desire to steer newcomers to the most commonly used methods, but not providing useful methods just for that reason is not tackling the right problem. If one wants to avoid newcomers using certain methods, one should add a feature to put them in an "expert" section in the documentation, or something like that (e.g. in the same spirit of spotlight/notable_trait; we could have an expert label too); rather than making life difficult for other developers.

Furthermore, this is an unsafe method, which means it is already being flagged as "you should know what you are doing". That is to say: it is not like adding a particularly unsuited (e.g. slow), but ultimately safe operation to a data structure that users should avoid unless they know what they are doing.

In any case, like everything else (specially unsafe bits), it should be used judiciously. Having the method or not will not stop anyone determined enough to "improve" performance. In the end, Rust is a systems programming language, which means people is more likely to need this kind of functionality compared to other languages.

Member

m-ou-se commented 28 days ago

I think it makes sense to have these. Of course we'd want to steer people away from using the unchecked functions, but that's what unsafe is for. We also have get_unchecked on slices, even though a.get(i).unwrap_or_else(|| unsafe { unreachable_unchecked() }) would result in the exact same code.

Let's try out the unstable version to get some experience with this function. Then we can have a more informed discussion about whether we want to stabilize this later.

@ojeda Can you open a tracking issue for this?

Member