Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Random access APIs should be marked as unsafe #21

Closed
TomGillen opened this issue Nov 4, 2019 · 10 comments
Closed

Random access APIs should be marked as unsafe #21

TomGillen opened this issue Nov 4, 2019 · 10 comments
Labels
type: bug Something isn't working

Comments

@TomGillen
Copy link
Collaborator

Random access APIs, such as World.get_component cannot have their safety expressed statically in Rust's type system. Instead, they are runtime borrow checked.

However, this runtime borrow checking is only there as a diagnostic aid; it will provide more useful fail-fast behaviour and stack traces, rather than the state corruption that would otherwise have to be debugged.

The user must still carefully consider how these APIs are used and ensure that they are not breaking any borrowing rules. Therefore, these functions should still be marked as unsafe, to more clearly communicate this responsibility.

@TomGillen TomGillen added the type: bug Something isn't working label Nov 4, 2019
@Ralith
Copy link
Contributor

Ralith commented Nov 4, 2019

If the runtime borrow checks are sound, i.e. guaranteed to catch any incorrect access by safe code in any build configuration, these should not be marked unsafe, but could perhaps be carefully warned about in documentation. unsafe designates that misuse of an API can produce undefined behavior, not just unintended behavior. This is why, for example, RefCell::borrow_mut is considered safe.

@TomGillen
Copy link
Collaborator Author

An API that crashes the application rather than corupting state may not need to be marked as unsafe, but marking it as unsafe still communicates exactly the same precautions to the user that they should be taking in both cases. Just because the application can detect the bug and give a more useful stack trace does not mean that the rationale behind marking something like a pointer dereference as unsafe does not apply equally in this case.

Also, runtime borrow checking may not always be in effect.

@Ralith
Copy link
Contributor

Ralith commented Nov 4, 2019

While it's not unsound to mark safe functions as unsafe, it's confusing, unidiomatic and dilutes the strength of unsafe when it's actually necessary. It's not meant to be a general-purpose caution sign.

Also, runtime borrow checking may not always be in effect.

If so, the rest is irrelevant and the interface absolutely is and must be marked unsafe.

@TomGillen
Copy link
Collaborator Author

Put another way, the runtime borrow checking changes how the bug is expressed (stack trace vs state corruption), but it does nothing to change whether or not the bug exists, its cause, or what the programmer must do to ensure it does not happen.

The runtime checks should be viewed as a diagnostic aid, not as a real behavioural change. Their inclusion does not do enough to obviate the usage warning that unsafe provides. The way the programmer must view the API with borrow checking (even if borrow checking was always performed) is identical to how they must view it without - bugs will have exaftly the same cause and exactly the same solution in either case, because at its heart it is the same bug regardless of the existance of more useful diagnostics.

The semantics of unsafe to the programmer are exactly the same in both cases.

@Ralith
Copy link
Contributor

Ralith commented Nov 4, 2019

I appreciate your point here, and I agree that the programmer should be aware of the requirements of the API and taking care not to run afoul of them, no matter what is said about the precise consequences of misuse. That said, bugs in safe vs. unsafe versions of an API may have the same cause and solution, but they do not have the same effects. This is the critical distinction that unsafe makes: that your code will, literally, fail safe, for a specific formal definition of safety. The precise details are discussed in the nomicon, which is very good reading.

@TomGillen
Copy link
Collaborator Author

TomGillen commented Nov 4, 2019

That the application crashes the process immediately before the CPU was about to write into a memory location that it was erronously programmed to perform due to a memory-safety bug does nothing to impact the nature of the problem. Whether or not that runtime check is performed has no impact on how the programmer must consider the code. The effects are not what matter - unsafe is there purely to communicate certain responsibilities to the programmer (typically related to memory safety). If a keyword, that's sole purpose is to communicate responsibilities to the programmer, can be either present or absent from two situations that are indistinguishable from the perspective of what the programmer needs to consider and do with that code - then that keyword is meaningless.

From the nomicon:

The unsafe keyword has two uses: to declare the existence of contracts the compiler can't check, and to declare that a programmer has checked that these contracts have been upheld.

You can use unsafe to indicate the existence of unchecked contracts on functions and trait declarations. On functions, unsafe means that users of the function must check that function's documentation to ensure they are using it in a way that maintains the contracts the function requires. On trait declarations, unsafe means that implementors of the trait must check the trait documentation to ensure their implementation maintains the contracts the trait requires.

@ishitatsuyuki
Copy link

ishitatsuyuki commented Nov 4, 2019

What's the point of this multiple-borrow query system? I think even in specs you get the same semantics as indexing into std containers (that is one mutable borrow at once), and in most cases borrow issues can be overcomed by splitting components or deferring write.

@Ralith
Copy link
Contributor

Ralith commented Nov 4, 2019

That the application crashes the process immediately before the CPU was about to write into a memory location that it was erronously programmed to perform due to a memory-safety bug does nothing to impact the nature of the problem.

To reiterate, it's not about the nature of a downstream programmer's bug, but rather describing the scope of possible outcomes of misuse. This is important because the consequences of undefined behavior can be much more severe than suddenly killing the process.

If a keyword, that's sole purpose is to communicate responsibilities to the programmer, can be either present or absent from two situations that are indistinguishable from the perspective of what the programmer needs to consider and do with that code - then that keyword is meaningless.

This argument only makes sense the hypothetical programmer does not need to account for the possibility of bugs arising in their code despite their best efforts. Bugs do happen, and therefore unsafe code needs to be treated differently by a programmer (for example, by subjecting it to additional review or adding asserts for conditions that seem obvious), because bugs affecting unsafe code are much more serious.

From the nomicon:

The nomicon isn't referring to completely arbitrary logical contracts there. Keep reading:

The need for all of this separation boils down a single fundamental property of Safe Rust:

No matter what, Safe Rust can't cause Undefined Behavior.

@kabergstrom
Copy link
Contributor

I'd like to bring up an alternative approach here:
#20 (comment)

It would be ideal if the user can opt-in to the granularity (component type vs chunk vs entity) of the runtime borrow checking as well as the safety guarantees the API provides. This would allow for an entirely safe default, more granular safe options, and unsafe alternatives for access if performance is important for the caller.

@TomGillen
Copy link
Collaborator Author

Fixed in #27.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants