*_unchecked always unsafe?

[farnbams]

Hi all,

a basic question: Should functions that omit checks always be declared unsafe?

Background

I have a type T that holds a certain contract which is tested when a new T is constructed. However, these tests are expensive, so I want a public API that omits these checks when the programmer can ensure that the contract is valid. Therefore, there is now a fn new_unchecked(...). However, having an *_unchecked() method public, it can no longer be guaranteed that every T holds its contract.

The question is should such methods be declared unsafe even they do technically nothing unsafe? As far as I'm concerned unsafe fn refers in Rust to "Could result in memory violation if not used correctly" but my case is more "Could result in unexpected behavior if not used correctly".

[ExpHP]

Don't do it. Not unless you plan to write actual memory-unsafe code that relies on these types upholding these invariants.

Questions like this are frequently asked, but the consensus is overwhelmingly that unsafe should be reserved for Undefined Behavior, period. Using it for other purposes cheapens its role in helping track down UB when it actually occurs.

The _unchecked suffix ought to be quite sufficient for communicating the potential for logic errors.

[Michael-F-Bryan]

In addition to the comments from @ExpHP's answer, you should make sure the logical invariants are mentioned in new_unchecked()'s doc-comment. Possibly mentioning that while breaking the invariants won't lead to memory unsafety, it will probably break your application, which may in turn trigger panics, bugs, etc.

If possible, it's also good practice to add debug_assert!() statements at the top of new_unchecked(). That way in debug builds you can easily detect when the invariants are being broken but it won't have any performance impact on a release build. Of course, that only really works when the invariant is expressible in code e.g. x != 0 or some_float.is_finite()).

[Yandros]

As an example, the following assertion can fail:

• assert!({
      set.insert(key);
      set.contains(&key)
  });
  

  • Playground

and no unsafe nor _unchecked methods were used. But I have still managed to break a contract (that of Hash + Eq: if a == b, then hash(a) == hash(b) ought to hold), thus getting these "logic bugs" for APIs that require a narrow contract / some properties to be upheld by the caller, as HashSet does.

Since it is not possible to compromise memory safety with this "bug", it not having required unsafe is fine; as long, of course, as the documentation is clear about it:

@https://doc.rust-lang.org/std/collections/struct.HashSet.html

As with the HashMap type, a HashSet requires that the elements implement the Eq and Hash traits. This can frequently be achieved by using #[derive(PartialEq, Eq, Hash)]. If you implement these yourself, it is important that the following property holds:

k1 == k2 -> hash(k1) == hash(k2)

In other words, if two keys are equal, their hashes must be equal.

It is a logic error for an item to be modified in such a way that the item's hash, as determined by the Hash trait, or its equality, as determined by the Eq trait, changes while it is in the set.

[cuviper]

As an alternate example, in num-rational we have new_raw to construct it without reducing the ratio. There's nothing memory-unsafe about this.