Tracking Issue for option_result_unwrap_unchecked

[ojeda]

Feature gate: #![feature(option_result_unwrap_unchecked)]

This is a tracking issue for the unchecked variants of unwrap() and unwrap_err() of Option and Result.

These are unsafe functions intended to be used in hot paths of performance sensitive applications where the variant an Option/Result holds is known, allowing to skip the branch that the safe version introduces. They complement other *_unchecked() methods already available, such as get_unchecked() in slices.

Public API

impl<T> Option<T> {
    pub unsafe fn unwrap_unchecked(self) -> T;
}

impl<T, E> Result<T, E> {
    pub unsafe fn unwrap_unchecked(self) -> T;
    pub unsafe fn unwrap_err_unchecked(self) -> E;
}

Steps / History

• Implementation: Add unwrap_unchecked() methods for Option and Result #80876
• Stabilization report: Tracking Issue for option_result_unwrap_unchecked #81383 (comment)
• Final commenting period (FCP): Tracking Issue for option_result_unwrap_unchecked #81383 (comment)
• Stabilization PR: Stabilize option_result_unwrap_unchecked #89951

Unresolved Questions

• Should we add expect_unchecked, and if yes, do it within another feature flag, to unblock unwrap_unchecked? Tracking Issue for option_result_unwrap_unchecked #81383 (comment)

  • It seems best to have it under another feature flag to avoid blocking this one.

• Should we panic on failure in debug (i.e. for those projects that build core themselves)? If yes, that would mean there is no way to actually remove the check for those projects, which could be potentially needed by someone. If we do, should it be documented? Tracking Issue for option_result_unwrap_unchecked #81383 (comment)

  • The current implementation uses debug_assert!, which may be useful for projects that build core with debug assertions enabled (e.g. the bors test suite or some embedded projects), but this is not documented, and indeed most users will not see such behavior, since core is shipped in release mode.
  • Furthermore, since it is UB anyway, we have room for changing the implementation, e.g. if we end up distributing the standard library with debug assertions enabled for some use cases.

• Should we name them unchecked_unwrap instead? Tracking Issue for option_result_unwrap_unchecked #81383 (comment).

  • The current name (unwrap_unchecked) follows the convention of other stable methods in the standard library such as get_unchecked, map_unchecked, new_unchecked, unreachable_unchecked... (see as well https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter).
  • On the other hand, the unchecked_{add,...} methods seem to follow the opposite convention, but those are unstable and they likely do it because the stable {checked,...}_{add,...} exist.

[camelid]

Also: start dogfooding this in std.

[ojeda]

@camelid Indeed! I just submitted the first two PRs for that.

[ojeda]

There are a couple more that can be replaced in library/core/src/iter/adapters/fuse.rs (not counting the macro ones, which are quite a few, but those would require several equivalent functions), but it may make codegen worse due to missed optimizations in LLVM.

I have filled https://bugs.llvm.org/show_bug.cgi?id=48975 with the IR reduction and an analysis of why manually integrating the function (e.g. via a macro) helps sometimes. This may also help other *_unchecked() functions too, e.g. #81354.

[bhgomes]

Is there a way to express that this is a "safe" operation when we know that the Result::Err type is Infallible?

[ojeda]

That's an interesting idea, although I am not sure what the policy is on having differing function qualifiers for the same method name, @m-ou-se?

I guess that could ideally be achieved with the future specialization feature (if it supports inherent impls). We can also (ab)use the priority of inherent methods over traits, but it feels a bit dirty:

use std::convert::Infallible;

pub enum MyResult<T, E> {
    Ok(T),
    Err(E),
}

trait UnsafeUnwrapUnchecked<T> {
    unsafe fn unwrap_unchecked(self) -> T;
}

impl<T, E> UnsafeUnwrapUnchecked<T> for MyResult<T, E> {
    unsafe fn unwrap_unchecked(self) -> T {
        match self {
            MyResult::Ok(t) => t,
            MyResult::Err(_) => core::hint::unreachable_unchecked(),
        }
    }
}

impl<T> MyResult<T, Infallible> {
    fn unwrap_unchecked(self) -> T {
        match self {
            MyResult::Ok(t) => t,
            MyResult::Err(_) => unsafe { core::hint::unreachable_unchecked() },
        }
    }
}
    
pub fn f() {
    let _ = MyResult::<i32, Infallible>::Ok(42).unwrap_unchecked();
    let _ = unsafe { MyResult::<i32, i32>::Ok(42).unwrap_unchecked() };
}

[CryZe]

There already is into_ok() and into_err() that do this, there is no need to overload unwrap_unchecked.

[m-ou-se]

@ojeda Even if we could make unsafe depend on the generic type, I wouldn't want to call the safe version unchecked, since there's no check that's being skipped in that case. And as @CryZe mentioned, we already have into_ok() for that.

[ojeda]

Yeah, that is what I imagined, thanks!

[alecmocatta]

Personally I try to "label" unwraps by instead using expect. I for one would appreciate the presence of unsafe fn expect_unchecked("...") -> T if it were to be added as well?

[ojeda]

That could be a bit misleading since the message will not be used. On the other hand, we could use it in debug builds for asserting.

[alecmocatta]

Yes, in almost all cases the message would never be printed. Note that I believe it requires -Zbuild-std or similar to compile core with debug assertions enabled.

Personally I would still find this useful. My flow is to start with unwrap/expects, and cautiously relax them to unwrap_unchecked/expect_unchecked when necessitated by a perf bottleneck, using a trait like unchecked_unwrap::UncheckedUnwrap.

[ojeda]

Yeah. Some projects do build core on their own (in particular the ones where debugging can be harder, e.g. embedded, kernels, etc.), plus rustc development can benefit from it too, so it can still be helpful (which is why we have debug_assert! in the unwrap_unchecked).

[camelid]

If expect_unchecked is added, I think it might be better to have a separate feature flag for it so it can be considered separately. Some people may feel that unwrap_unchecked makes sense but expect_unchecked does not, so one should not block the other.

[TrolledWoods]

I feel that expect_unchecked makes sense if it still panics with the message in debug mode. Since unwrap_unchecked also panics in debug mode, should this be noted in the documentation?

[dbeckwith]

I agree that the panic in debug mode should be documented, that seems very useful to know.

[PureWhiteWu]

Hi, I feel these methods are pretty helpful and am looking forward to using them in my lib. Is there a plan to stabilize now?

[camelid]

One thing I've been wondering about is the slight inconsistency between the name unwrap_unchecked and pre-existing "unchecked" functions like unchecked_add. So in some ways it seems like the function should be renamed unchecked_unwrap (and maybe in the future, unchecked_expect). I think both names would be fine, but I wanted to point out the naming inconsistency while the function is still unstable.

[ojeda]

Note that there are stable methods called get_unchecked, map_unchecked, new_unchecked, unreachable_unchecked...

So I would say the unchecked_add etc. are the ones that should be changed to follow the normal convention before stabilizing them.

[PureWhiteWu]

Should we panic in debug mode? If yes, that would mean there is no way to actually remove the check for debug builds, which could be potentially needed. If we do panic, should it be documented?

For this question, I think maybe just leave it an "undefined behavior" makes sense, which is what has been documented.
We don't guarantee to panic in debug mode and verse vice, so we are not blocked for potential behavior change.
And this is exactly what "unsafe" means.

Should we split this into two, to unblock unwrap_unchecked?

This sounds good to me.

[ojeda]

Yeah, we could always decide which behavior is best, if any, later. Possibly after the discussion around expect_unchecked has lead to a result. In any case, note that the current debug_assert! is only seen by projects that build core themselves.

I will write a stabilization report.

[ojeda]

Stabilization report

Summary

Unsafe versions of unwrap and unwrap_err of Option and Result which allow to skip the branch:

let x = Some("air");
assert_eq!(unsafe { x.unwrap_unchecked() }, "air");

Tests

• Option::unwrap_unchecked
  

  rust/library/core/tests/option.rs

  Lines 163 to 168 in af9b508

  	#[test]
  	fn test_unwrap_unchecked() {
  	assert_eq!(unsafe { Some(1).unwrap_unchecked() }, 1);
  	let s = unsafe { Some("hello".to_string()).unwrap_unchecked() };
  	assert_eq!(s, "hello");
  	}

• Result::unwrap_unchecked
  

  rust/library/core/tests/result.rs

  Lines 131 to 135 in af9b508

  	#[test]
  	fn test_unwrap_unchecked() {
  	let ok: Result<isize, &'static str> = Ok(100);
  	assert_eq!(unsafe { ok.unwrap_unchecked() }, 100);
  	}

• Result::unwrap_err_unchecked
  

  rust/library/core/tests/result.rs

  Lines 137 to 141 in af9b508

  	#[test]
  	fn test_unwrap_err_unchecked() {
  	let ok_err: Result<isize, &'static str> = Err("Err");
  	assert_eq!(unsafe { ok_err.unwrap_err_unchecked() }, "Err");
  	}

Other relevant information

• There seem to be a fair amount of users in the wild, plus we also have a few users in the standard library, e.g. in Option itself:
  

  rust/library/core/src/option.rs

  Lines 1209 to 1214 in af9b508

  	pub fn insert(&mut self, value: T) -> &mut T {
  	*self = Some(value);
  	// SAFETY: the code above just filled the option
  	unsafe { self.as_mut().unwrap_unchecked() }
  	}

• A missed optimization bug in LLVM was discovered involving these methods, which was fixed in LLVM 13, and now the reported Rust code is optimized as expected in rustc nightly.

Resolutions of unresolved questions

• Should we add expect_unchecked, and if yes, do it within another feature flag, to unblock unwrap_unchecked?

  • It seems best to have it under another feature flag to avoid blocking this one.

• Should we panic on failure in debug (i.e. for those projects that build core themselves)? If yes, that would mean there is no way to actually remove the check for those projects, which could be potentially needed by someone. If we do, should it be documented?

  • The current implementation uses debug_assert!, which may be useful for projects that build core with debug assertions enabled (e.g. the bors test suite or some embedded projects), but this is not documented, and indeed most users will not see such behavior, since core is shipped in release mode.
  • Furthermore, since it is UB anyway, we have room for changing the implementation, e.g. if we end up distributing the standard library with debug assertions enabled for some use cases.

• Should we name them unchecked_unwrap instead?

  • The current name (unwrap_unchecked) follows the convention of other stable methods in the standard library such as {get,map,new,unreachable,...}_unchecked. Moreover, the API guidelines mention:

    For getters that do runtime validation such as bounds checking, consider adding unsafe _unchecked variants.

  • On the other hand, the unchecked_{add,...} methods seem to follow the opposite convention, but those are unstable and they likely do it because the stable {checked,...}_{add,...} exist.

[camelid]

• If yes, that would mean there is no way to actually remove the check for those projects, which could be potentially needed by someone.

Also, if it were needed for some reason, users could just write their own match with unreachable_unchecked(). But, IIRC, many "unchecked" stdlib functions will panic in debug mode if used incorrectly, without providing any guarantee that they always will; so that seems to me like the right path forward.

[dtolnay]

@rfcbot fcp merge
See Miguel's stabilization report: #81383 (comment).

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.