Should the traits be unsafe?

[Rufflewind]

It would be nice if Unsigned/Integer could be marked unsafe so that unsafe code downstream can “trust” that any Unsigned type will yield sensible answers. I do see that

This trait should not be implemented for anything outside this crate.

so unsafe would simply be a formal way to document this (either that, or through a private supertrait).

[paholg]

Marking the traits unsafe wouldn't stop people from impling them, it would just make them type unsafe when doing so.

Ideally, they would be marked sealed, but that hasn't been added yet, and I'm not sure what its status is.

[Rufflewind]

it would just make them type unsafe when doing so.

It makes a big difference in responsibility. If a trait is unsafe, then unsafe code can assume that its laws* hold for all implementations. If an implementation is unlawful and unsafety occurs as a result, the blame is on the trait implementor.

In contrast, if a trait is not marked unsafe, then unsafe code cannot assume that all implementations are lawful and must take extra care in making sure unsafety cannot occur even in the presence of unlawful implementations.

* Here, the “law” of Unsigned could be as simple as “other crates aren’t allowed to implement it.”

[causal-agent]

It is not possible to violate safety just by implementing Unsigned (as with Sync and Send), so it is not appropriate.

The documentation already tells others not to implement the traits, and there's currently no way to enforce that.

[Rufflewind]

It is not possible to violate safety just by implementing Unsigned (as with Sync and Send), so it is not appropriate.

It is appropriate for Sync and Send so why not Unsigned and Integer? Send and Sync are unsafe traits for the same analogous reasons.

Note that in and of itself it is impossible to incorrectly derive Send and Sync. Only types that are ascribed special meaning by other unsafe code can possible cause trouble by being incorrectly Send or Sync.

Likewise, unsafe code that ascribes special meaning to Unsigned and Integer can run into problems with an incorrectly implemented Unsigned or Integer.

[causal-agent]

Likewise, unsafe code that ascribes special meaning to Unsigned and Integer can run into problems with an incorrectly implemented Unsigned or Integer.

This could apply to literally any public trait, no? Typenum doesn't do anything unsafe with these traits, and frankly I find it hard to imagine how one could implement them in such a way as to cause unsafety when their methods do nothing more than return integers.

[paholg]

In my view, a crate that incorrectly implements Unsigned or Integer has broken its API contract with typenum, which explicitly states not to do that. I feel no need to cater to such a crate, and I don't think marking those traits unsafe would be the right way to do so anyway.

Marking the traits unsafe would add confusion to those using typenum properly, and I don't think that's a good idea.

Someday, sealed will likely exist, and that will take care of it. Until then, I think documentation is the best way to express how the traits should be used.

[Rufflewind]

This could apply to literally any public trait, no?

Not every trait. Consider an unsafe B tree implementation: if the Ord implementation is horribly broken, then the unsafe code needs to be prepared for that situation; it can't just segfault because Ord is not an unsafe trait. rust-lang/rust#33197

Likewise one could consider uses of Integer / Unsigned for indexing into arrays in unsafe code, which would blow up if those traits are improperly implemented.

An unsafe trait doesn't impose any restrictions on its users. It only affects implementors. There is precedent in the use of unsafe as a workaround for sealed traits: https://docs.rs/ndarray/0.9.1/ndarray/trait.Data.html

[golddranks]

Marking the traits unsafe would add confusion to those using Typenum properly

Note that using unsafe traits isn't unsafe or require unsafe. It's implementing them that is unsafe. The users are just using the traits; if there would be confusion, it would be because some user would have a wrong mental model what an unsafe trait means.

That being said, unsafe code cannot trust unknown implementations of a trait, but if it can be sure that only known implementations are used, and the implementation semantics are backed up by invariants that are considered a part of the public API, there shouldn't be any problem. So if you feed your unsafe code only stuff implemented in this crate, there is no problem, and you don't need the traits to be unsafe.

[ExpHP]

That being said, unsafe code cannot trust unknown implementations of a trait

I feel this is an over-generalization. By this reasoning, every single pub unsafe Trait must be sealed, which is unreasonable.

I believe there is nothing wrong with providing an unsafe abstraction that is open. A pub unsafe Trait that is not sealed should come with:

• A contract on implementors. Basically, you say implementations must satisfy such and such property, else behavior is undefined. For instance, "it must hold that align_of::<Self>() == align_of::<B>(), else behavior is undefined." Or maybe even "Implementing this on your own types is UB" (a.k.a I was too lazy to seal this).
• Explicit guarantees to users, that even unsafe code can trust. Not all pub unsafe traits need to have these; this is for unsafe abstractions that are meant to be reusable by other crates, which I believe is the sort of thing the OP in this issue is looking for.
  The crucial thing to understand however is that if typenum wanted to provide open abstractions that unsafe code can rely on, then they would have to be unsafe traits, period. This is to ensure that external impls are marked unsafe, which they must be because an incorrect impl could cause memory unsafety in other crates.

The one tricky bit here is that changing the contract on implementors requires a major semver bump, but is likely to be missed by implementors during an update since it's often "just a documentation change." While I've never been in this position myself, I suspect you can make this situation less dangerous by making a trivial change to the unsafe trait with the specific intent to prevent old implementations from continuing to compile.

---

I have no particularly strong feeling with regards to whether or not typenum should feel obligated to provide such abstractions for unsafe code, though. (huh, why did I write this comment, then...?)

[paholg]

I am unconvinced that adding unsafe to all traits will provide any real benefit. If and when sealed traits are added to the language, the traits will be marked sealed.