Comment by mlindner | Hacker News Reader

Comment by mlindner | Hacker News Reader

Parent Full thread

> But it is possible to have a private safe function that can only possibly be accessed from a small part of your code base.

This would also be unsound. Multiple people develop software in a library. Any future developer on your library will automatically assume that any safe Rust function can be called with any parameters and not invoke undefined behavior. That is the default assumption of any Rust developer. If you have such code in your libraries, you should fix them, or mark them as clearly unsafe such that any caller of that function ensures they keep the invariants that are required of that unsafe code that is propagated upward. You're falling back to old ideas from C.

> If you are careful in preserving this invariant

This is C/C++ mentality. The entire point about Rust is that you do not need to be careful. It's guaranteed and checked by the compiler. Again, if you can invoke undefined behavior by calling ANY safe function with ANY form of input, then the function is unsound. This is simply how Rust assumptions work.

> But this obviously gets more risky, the more code that has access to the function. I'll grant that it's not an uncommon pitfall to assume a crate-wide invariant somewhere that you later forget about.

I suggest you re-learn how to program Rust, personally. You've picked up some really bad habits that don't line up with general consensus on how to code in Rust.

Since unsoundness is a property of an interface, that private safe function would be unsound but could be used as part of the implementation details of a sound public API.

It's not something I would do personally, and would be something I'd put down as a black mark against using a library as a dependency, but it's nowhere near as bad as doing the same thing on a public API.

Every single function call is using an interface. Your public API is not the only interface.

It's an extremely bad habit to pick up as this ruins one of the main purposes of Rust, namely it's ability to allow collaboration and while not causing undefined behavior. Any sort of internal "don't do this or it'll cause undefined behavior" is just falling back to C/C++ land.

Some developer later will modify that code later, maybe even the same developer and will forget about the invariants that need to be held if they're not documented in unsafety comments around unsafe code. Anything that relies on code review to catch unsoundness outside of unsafe code is automatically wrong.

> This would also be unsound. Multiple people develop software in a library. Any future developer on your library will automatically assume that any safe Rust function can be called with any parameters and not invoke undefined behavior. That is the default assumption of any Rust developer.

That's quite the broad generalization. Obviously, if you're at all sane, you're internally documenting your internal invariants, and keeping the section of code upholding them as small and tightly-packed as possible. Rust's unsafe keyword is simply a tool that lets us use interfaces without being fully familiar with their implementations.

It's generally a great idea to encapsulate unsafe code into a sound internal interface whenever possible, but this doesn't necessarily require that every single function be its own standalone interface. What matters is that it's clear to the maintainers that the callers (which should best be nearby in the code) are bound by its requirements.

> Again, if you can invoke undefined behavior by calling ANY safe function with ANY form of input, then the function is unsound. This is simply how Rust assumptions work.

Suppose I have a function that internally creates an Option<T> in such a way that a certain invariant is upheld. I call Option::map() on it, passing it an inner function with an unsafe block assuming that the invariant is upheld. The inner function is called nowhere else. By necessity, this inner function is not marked unsafe, since Option::map() requires a safe FnOnce.

Is there unsoundness in my code? I say not, since the inner function is not a standalone interface; it is an implementation detail inseparable from its outer function. There is absolutely no way to invoke my code as it stands so as to cause UB.

> I suggest you re-learn how to program Rust, personally. You've picked up some really bad habits that don't line up with general consensus on how to code in Rust.

I would prefer that you not make baseless allegations about my code. I mention that pitfall only from experience reviewing others' unsafe code. Again, if you're sane, you keep the parts of the code manually upholding your invariants as small as possible. Marking your internal functions unsafe and clearly documenting their preconditions is a perfectly fine way to go, and that's how I write my own code, being a sane person.

But having an invariant held across multiple functions does not automatically make your code unsound. In certain situations (internal implementations of external traits, callbacks from external code), it is even necessary to have an unsafe precondition pass through the call of a function or closure marked safe. All it takes is the recognition that not every single function is its own standalone interface.

> It's generally a great idea to encapsulate unsafe code into a sound internal interface whenever possible, but this doesn't necessarily require that every single function be its own standalone interface. What matters is that it's clear to the maintainers that the callers (which should best be nearby in the code) are bound by its requirements.

If a safe function has requirements to maintain soundness then it's either an unsafe function or an unsound improperly-labeled safe function. It's a contradiction in terms to label a function safe while also having unsoundness that can be created by misuse of the function.

> Suppose I have a function that internally creates an Option<T> in such a way that a certain invariant is upheld. I call Option::map() on it, passing it an inner function with an unsafe block assuming that the invariant is upheld. The inner function is called nowhere else. By necessity, this inner function is not marked unsafe, since Option::map() requires a safe FnOnce.

The inner function (assuming it's within the scope of the outer function) I would say is still "part of" the outer function. Inner functions are in-effect just more complex scoping within the outer function. They're not first-class citizens. As long as it is impossible for the inner function to ever escape it's scope then I would still consider this safe as long as the outer function can safely accept any input. Again, making sure that all unsafe blocks are properly commented. I would make it extremely clear by putting the unsafe {} as starting at the beginning of the function and ending at the end.

However there's a slight problem here after thinking about this a bit more. It must be solely the responsibility of the outer function that the inner function is used correctly. It cannot rely on the implementation of any piece of code outside the inner function.

The acid test here is whether it is possible that if the implementation of Option::map() were to ever change (while itself still being sound) whether it is possible that it could expose unsoundness without violating the FnOnce preconditions, i.e. without causing a compiler error. If a language (or library) version upgrades can expose unsoundness in your code, then your function is unsound. It's unclear from your example if that would be the case.

In general though I would say this is a poorly written function however and this type of thing should be avoided.

> But having an invariant held across multiple functions does not automatically make your code unsound.

Again, to that I would disagree.

> In certain situations (internal implementations of external traits, callbacks from external code), it is even necessary to have an unsafe precondition pass through the call of a function or closure marked safe.

I would need a more concrete example to look at. However the callbacks from external code example sounds especially fraught.

> All it takes is the recognition that not every single function is its own standalone interface.

I would go even further and say it is an invariant of the Rust language itself that any safe function will never expose unsoundness.

Sorry for taking a while to respond; I've compiled a list of examples of this pattern in common crates [0], including the standard library. For each crate, safe functions and closures above the horizontal rule depend on the behavior of other functions, and those below the horizontal rule depend only on the behavior of their containing function. Closures* marked with an asterisk capture data from their environment, so it would take some effort to refactor those to use unsafe fn() pointers.

One example of the safe trait implementation scenario is in tracing-log [1]. The <LogVisitor<'a> as Visit>::record_str() method would leak dangling &str references if the value argument didn't live as long as 'a. LogVisitor<'a> is a private type in the tracing-log crate, and Visit is a public trait in the tracing-core crate. The Visit trait had existed before tracing-log was first released, so it could not be modified without breaking backward compatibility.

As written, there is no actual way for someone to cause UB with this library. tracing-log is written so that LogVisitor<'a> values only ever go into Event::<'a>::record(), and tracing-core is written so that it passes the visitor &str references directly from the Event<'a>, that live at least as long as it. If someone added a function to tracing-log that otherwise called <LogVisitor<'a> as Visit>::record_str(), or if someone modified tracing-core to pass a copy of the string to Visit::record_str(), then someone could cause UB with this library; but no one has done so, so no one can cause UB.

> The acid test here is whether it is possible that if the implementation of Option::map() were to ever change (while itself still being sound) whether it is possible that it could expose unsoundness without violating the FnOnce preconditions, i.e. without causing a compiler error. If a language (or library) version upgrades can expose unsoundness in your code, then your function is unsound.

Are you saying that unsafe invariants must never be broken, even if external functions have behavior completely different from how they are documented? In that case, it would seem like nearly all nontrivial unsafe code would disappoint you, since it depends at least on the safe standard library functions like Option::map() doing what they say on the tin. Public documentation of behavior is a binding promise under the SemVer regime that Cargo follows.

More generally, I think we both agree that a sound library is one that "cannot be used to cause UB from safe code". But I say that it's fine as long as the public interface as it stands (or well-defined internal interface) cannot be used to cause UB, whereas you say that it must not cause UB even if someone were to hypothetically add another function that calls any safe private function.

To me, that standard seems somewhat arbitrary. Suppose I were to add a new function inside the alloc::vec module:

  pub fn safe_set_len<T, A: Allocator>(vec: &mut Vec<T, A>, new_len: usize) {
      vec.len = new_len;
  }

This function is perfectly safe to write, and it would be perfectly safe for external users to call. However, it would allow half of the safe Vec methods to cause UB. So since I could hypothetically add this safe function, does that mean that the Vec API is already unsound? I'd say not, since there is an invariant attached to Vec that the length is no greater than the capacity, and adding this function would allow this invariant to be violated.

Would you disagree, and say that the Vec API as a whole is unsound, since the soundness of some of its unsafe code depends on the nonexistence of certain functions that would be safe to write? Or would you instead say that such an invariant can be attached to a type, but only begrudgingly to an outer function, and never to a module or a crate? I can't see the latter distinction as being anything but arbitrary.

You claim that your position "is the default assumption of any Rust developer", but I think my list of examples provides evidence against that claim: plenty of large crates' maintainers have little issue defining "safe" private functions or closures that reach outward for their invariants. What's important is that the maintainers don't actually merge any PRs that break the invariants. (Ideally, this would be by way of thorough documentation, but that is far from strictly necessary.)

[0] https://gist.github.com/LegionMammal978/7654f484b495d67c63d0...

[1] https://github.com/tokio-rs/tracing/blob/tracing-log-0.1.3/t...

Reply on news.ycombinator.com