-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Improve note on unsafe functions and unsafe blocks
#4153
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rereading this gave me the idea to invert the ordering a bit, and I think that helps a little with the flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I somewhat prefer the current order. However, feel free to reuse and amend this as you like! If it helps, I can commit the reordering as well.
The way I see it is that the section is about (a) the fact that
unsafeblocks allow you to call unsafe functions, which is otherwise disallowed and (b) introducing what unsafe functions are. (a) is mentioned above. As for (b), one aspect of unsafe functions is that they define an obligation to the caller, this is also explained above. The other aspect, that their bodies are effectivelyunsafeblocks, is in the process of being removed, and the note is about that. In its reordered version, it seems to assume that the reader already knows, or at least expects this second feature. In my opinion this breaks the link to the reader.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s reasonable, and got me thinking further on how to phrase it. Let me suggest another iteration – I cannot commit directly to the PR, but if you check the "let maintainers update this" button I'll be able to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one sentence longer, but I think it does what I was wanting to do, i.e. emphasize the current state, which is what we generally aim for in the book, while keeping your original flow overall. I think it is fine to leave the reference to the editions appendix at the end, given the text now says "Edition" or "edition" three separate times before that!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like that manages well to avoid the expectation of the feature. However, the back-and-forth gives the note an agitation that lacks the simple, calm style of the book.
Is it very important to you to begin with the newer editions' behavior? I sympathize with the goal to emphasize the current state, and I'd tend to agree that even if Rust never had this feature, it would still be helpful to clarify that unsafe functions' bodies are not
unsafeblocks, which some (but likely not all) readers might expect. But I think this comes across with sufficient immediacy in the first sentence in this PR, which then quickly gives way to discussion of the current behavior.If so, here's a suggestion that does so:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is indeed quite important that we lead with the behavior new users will see, since that’s the default experience for someone reading through the book. I like your overall direction, but I think we can go a step further: we can just say that part inline:
We don’t actually need to say anything about the edition-level change, just document the new default behavior. If someone is curious why they are not yet seeing that behavior, they can reference the edition guide etc. (This is true of all such changes to the book across editions!)