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

Improve note on unsafe functions and unsafe blocks #4153

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matthias-t
Copy link

See discussion in #4147, #4148.

One thing I am not quite satisfied about is that the reference to the appendix is somewhat far away from the first mention of editions in this note. Another option would be to move it between the first two sentences and parenthesize it.

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, and I had missed that we have mentioned editions. I think this works well. I made one suggested tweak, and I think we can land it with that in place.

Comment on lines +189 to +195
> Note: In earlier editions of Rust, the body of an unsafe function was treated
> as an `unsafe` block, so you could perform any unsafe operation within it.
> Starting with the 2024 edition, the compiler will warn you that you need to
> use an `unsafe` block to perform unsafe operations even in the body of an
> unsafe function. This helps to keep `unsafe` blocks as small as possible, as
> unsafe operations may not be needed across the whole function body. For more
> information about Rust editions, refer to [Appendix E][editions].
Copy link
Contributor

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.

Suggested change
> Note: In earlier editions of Rust, the body of an unsafe function was treated
> as an `unsafe` block, so you could perform any unsafe operation within it.
> Starting with the 2024 edition, the compiler will warn you that you need to
> use an `unsafe` block to perform unsafe operations even in the body of an
> unsafe function. This helps to keep `unsafe` blocks as small as possible, as
> unsafe operations may not be needed across the whole function body. For more
> information about Rust editions, refer to [Appendix E][editions].
> Note: Starting with the 2024 Edition of Rust, the compiler will warn you that
> you need to use an `unsafe` block to perform unsafe operations even in the
> body of an unsafe function. This helps to keep `unsafe` blocks as small as
> possible, as unsafe operations may not be needed across the whole function
> body. In earlier editions, the body of an unsafe function was treated as an
> `unsafe` block, so you could perform any unsafe operation within it. For more
> information about Rust editions, refer to [Appendix E][editions].

Copy link
Author

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 unsafe blocks 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 effectively unsafe blocks, 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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> Note: In earlier editions of Rust, the body of an unsafe function was treated
> as an `unsafe` block, so you could perform any unsafe operation within it.
> Starting with the 2024 edition, the compiler will warn you that you need to
> use an `unsafe` block to perform unsafe operations even in the body of an
> unsafe function. This helps to keep `unsafe` blocks as small as possible, as
> unsafe operations may not be needed across the whole function body. For more
> information about Rust editions, refer to [Appendix E][editions].
> Note: Since the Rust 2024 Edition, Rust distinguishes between the body of an
> `unsafe fn` and an `unsafe` block. In earlier editions, the body of an unsafe
> function was treated as an `unsafe` block, so you could perform any unsafe
> operation within it. Starting with the 2024 edition, the compiler will warn
> you that you need to use an `unsafe` block to perform unsafe operations even
> in the body of an unsafe function. This helps to keep `unsafe` blocks as small
> as possible, as unsafe operations may not be needed across the whole function
> body. For more information about Rust editions, refer to [Appendix
> E][editions].

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!

Copy link
Author

@matthias-t matthias-t Dec 13, 2024

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 unsafe blocks, 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:

Suggested change
> Note: In earlier editions of Rust, the body of an unsafe function was treated
> as an `unsafe` block, so you could perform any unsafe operation within it.
> Starting with the 2024 edition, the compiler will warn you that you need to
> use an `unsafe` block to perform unsafe operations even in the body of an
> unsafe function. This helps to keep `unsafe` blocks as small as possible, as
> unsafe operations may not be needed across the whole function body. For more
> information about Rust editions, refer to [Appendix E][editions].
> Note: To perform unsafe operations in the body of an unsafe function, you need
> to use an `unsafe` block just as within a regular function.
>
> This helps to keep `unsafe` blocks as small as possible, as unsafe operations
> may not be needed across the whole function body. It was different in editions
> of Rust prior to 2024, where the body of an unsafe function was treated as an
> `unsafe` block. For more information about Rust editions, refer to [Appendix
> E][editions].

Copy link
Contributor

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:

To perform unsafe operations in the body of an unsafe function, you still need
to use an `unsafe` block just as within a regular function, and the compiler
will warn you if you forget. This helps to keep `unsafe` blocks as small as
possible, as unsafe operations may not be needed across the whole function body.

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!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants