4 ms·

> 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 sing

> 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.

LegionMammal978 4y ago
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/7654f484b495d67c63d035266a03fc33 https://gist.github.com/LegionMammal978/7654f484b495d67c63d0... [1] https://github.com/tokio-rs/tracing/blob/tracing-log-0.1.3/tracing-log/src/lib.rs#L555-L570 https://github.com/tokio-rs/tracing/blob/tracing-log-0.1.3/t...

LegionMammal978 4y ago

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/7654f484b495d67c63d035266a03fc33 https://gist.github.com/LegionMammal978/7654f484b495d67c63d0... [1] https://github.com/tokio-rs/tracing/blob/tracing-log-0.1.3/tracing-log/src/lib.rs#L555-L570 https://github.com/tokio-rs/tracing/blob/tracing-log-0.1.3/t...