From cbbd0ce236ca4bc086dbb271e548a55ab75dcfac Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 4 Nov 2018 15:39:53 +0100 Subject: [PATCH 01/15] initial RFC for unsafe blocks in unsafe fn --- text/0000-unsafe-block-in-unsafe-fn.md | 135 +++++++++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 text/0000-unsafe-block-in-unsafe-fn.md diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md new file mode 100644 index 00000000000..8edf1f3e7fb --- /dev/null +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -0,0 +1,135 @@ +- Feature Name: unsafe_block_in_unsafe_fn +- Start Date: 2018-11-04 +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary +[summary]: #summary + +No longer treat the body of an `unsafe fn` as being an `unsafe` block. To avoid +a breaking change, this is a warning now and may become an error in a future +edition. + +# Motivation +[motivation]: #motivation + +Marking a function as `unsafe` is one of Rust's key protections against +undefined behavior: Even if the programmer does not read the documentation, +calling an `unsafe` function (or performing another unsafe operation) outside an +`unsafe` block will lead to a compile error, hopefully followed by reading the +documentation. + +However, we currently entirely lose this protection when writing an `unsafe fn`: +If I, say, accidentally call `offset` instead of `wrapping_offset`, or if I +dereference a raw pointer thinking it is a reference, this happens without any +further notice when I am writing an `unsafe fn` because the body of an `unsafe +fn` is treated as an `unsafe` block. + +For example, notice how +[this PR](https://github.com/rust-lang/rust/pull/55043/files) significantly +increased the amount of code in the thread spawning function that is considered +to be inside an `unsafe` block. + +The original justification for this behavior (according to my understanding) was +that calling this function is anyway unsafe, so there is no harm done in +allowing *it* to perform unsafe operations. And indeed the current situation +*does* provide the guarantee that a program without `unsafe` cannot be UB. +However, this neglects the other aspect of `unsafe` that I described above: To +make the programmer aware that they are treading dangerous ground even when they +may not realize they are doing so. + +Using some more formal terminology, an `unsafe` block generally comes with a +proof *obligation*: The programmer has to ensure that this code is actually safe +to execute in the current context, because the compiler just trusts the +programmer to get this right. In contrast, `unsafe fn` represents an +*assumption*: As the author of this function, I make some assumptions that I +expect my callees to uphold. Making `unsafe fn` also implicitly play the role +of an `unsafe` block conflates these two dual aspects of unsafety (one party +making an assumption, another party having the obligation to prove that +assumption). There is no reason to believe that the assumption made by the +`unsafe fn` is the same as the obligation incurred by unsafe operations inside +this function, and hence the author of the `unsafe fn` should better carefully +check that their assumptions are sufficient to justify the unsafe operations +they are performing. This is what an `unsafe` block would indicate. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +When you perform an unsafe operation, like dereferencing a raw pointer or +calling an `unsafe` function, you must enclose that code in an `unsafe` block. +The purpose of this is to acknowledge that the operation you are performing here +has *not* been checked by the compiler, you are responsible yourself for +upholding Rust's safety guarantees. Generally, unsafe operations come with +detailed documentation for the conditions that must be met when this operation +is executed; it is up to you to check that all these conditions are indeed met. + +When you are writing a function that itself has additional conditions to ensure +safety (say, it accesses some data without making some necessary bounds checks, +or it takes some raw pointers as arguments and performs memory operations based +on them), then you should mark this as an `unsafe fn` and it is up to you to +document the conditions that must be met for the arguments. + +Your `unsafe fn` will likely perform unsafe operations; these have to be +enclosed by an `unsafe` block as usual. This is the place where you have to +check that the requirements you documented for your own function are sufficient +to satisfy the conditions required to perform this unsafe operation. + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +First of all, we no longer warn that an `unsafe` block is unnecessary when it is +nested immediately inside an `unsafe fn`. So, the following compiles without +any warning: + +```rust +unsafe fn get_unchecked(x: &[T], i: usize) -> &T { + unsafe { x.get_unchecked(i) } +} +``` + +However, nested `unsafe` blocks are still redundant, so this warns: + +```rust +unsafe fn get_unchecked(x: &[T], i: usize) -> &T { + unsafe { unsafe { x.get_unchecked(i) } } +} +``` + +In a next step, we have a lint that fires when an unsafe operation is performed +inside an `unsafe fn` but outside an `unsafe` block. So, this would trigger the +lint: + +```rust +unsafe fn get_unchecked(x: &[T], i: usize) -> &T { + x.get_unchecked(i) +} +``` + +This gets us into a state where programmers are much less likely to accidentally +perform undesired unsafe operations inside `unsafe fn`. + +Even later, it might be desirable to turn this warning into an error. + +# Drawbacks +[drawbacks]: #drawbacks + +This new warning will likely fire for the vast majority of `unsafe fn` out there. + +# Rationale and alternatives +[rationale-and-alternatives]: #rationale-and-alternatives + +I explained the rationale in the motivation section. + +The alternative is to not do anything, and live with the current situation. + +# Prior art +[prior-art]: #prior-art + +None that I am aware of: Other languages do not have `unsafe` blocks. + +# Unresolved questions +[unresolved-questions]: #unresolved-questions + +Should this lint be in clippy first before in becomes warn-by-default in rustc, +to avoid a huge flood of warnings showing up at once? Should the lint ever +become a hard error (on newer editions), or remain a warning indefinitely? From 51ca5b4133ecd0292de99a58866012d40c6a3224 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Nov 2018 11:30:31 +0100 Subject: [PATCH 02/15] C# has unsafe blocks --- text/0000-unsafe-block-in-unsafe-fn.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index 8edf1f3e7fb..d58e664458f 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -125,7 +125,13 @@ The alternative is to not do anything, and live with the current situation. # Prior art [prior-art]: #prior-art -None that I am aware of: Other languages do not have `unsafe` blocks. +The only other language that I am aware of that has a notion of `unsafe` blocks +and `unsafe` functions is C#. It +[looks like](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/unsafe) +there, unsafe operations can be freely used inside an `unsafe` function even +without a further `unsafe` block. However, based on @Ixrec's experience, +`unsafe` plays hardly any role in the C# ecosystem and they do not have a +culture of thinking about this in terms of proof obligations. # Unresolved questions [unresolved-questions]: #unresolved-questions From 77a99924b5c518d4e9d0e865d8f3dbff18acbfd2 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Nov 2018 12:52:47 +0100 Subject: [PATCH 03/15] link to logical introduction/elimination --- text/0000-unsafe-block-in-unsafe-fn.md | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index d58e664458f..5c464f18925 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -43,14 +43,19 @@ proof *obligation*: The programmer has to ensure that this code is actually safe to execute in the current context, because the compiler just trusts the programmer to get this right. In contrast, `unsafe fn` represents an *assumption*: As the author of this function, I make some assumptions that I -expect my callees to uphold. Making `unsafe fn` also implicitly play the role -of an `unsafe` block conflates these two dual aspects of unsafety (one party -making an assumption, another party having the obligation to prove that -assumption). There is no reason to believe that the assumption made by the -`unsafe fn` is the same as the obligation incurred by unsafe operations inside -this function, and hence the author of the `unsafe fn` should better carefully -check that their assumptions are sufficient to justify the unsafe operations -they are performing. This is what an `unsafe` block would indicate. +expect my callees to uphold. (In terms of +[introduction and elimination forms of natural deduction](https://en.wikipedia.org/wiki/Natural_deduction#Introduction_and_elimination), +`unsafe` blocks are where you introduce, and `unsafe fn` is where you +eliminate.) + +Making `unsafe fn` also implicitly play the role of an `unsafe` block conflates +these two dual aspects of unsafety (one party making an assumption, another +party having the obligation to prove that assumption). There is no reason to +believe that the assumption made by the `unsafe fn` is the same as the +obligation incurred by unsafe operations inside this function, and hence the +author of the `unsafe fn` should better carefully check that their assumptions +are sufficient to justify the unsafe operations they are performing. This is +what an `unsafe` block would indicate. # Guide-level explanation [guide-level-explanation]: #guide-level-explanation From aa85ef7acc3212066ec0360388d0a991849a5c64 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Nov 2018 12:54:14 +0100 Subject: [PATCH 04/15] this makes short unsafe fn less ergonomic --- text/0000-unsafe-block-in-unsafe-fn.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index 5c464f18925..7f505d33220 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -120,6 +120,10 @@ Even later, it might be desirable to turn this warning into an error. This new warning will likely fire for the vast majority of `unsafe fn` out there. +Many `unsafe fn` are actually rather short (no more than 3 lines) and will +likely end up just being one large `unsafe` block. This change would make such +functions less ergonomic to write. + # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives From 5d46a2d62ddfe9be1b3c9c1aec5bb514db3d313c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Nov 2018 12:59:57 +0100 Subject: [PATCH 05/15] numbered steps for the action plan --- text/0000-unsafe-block-in-unsafe-fn.md | 55 ++++++++++++++------------ 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index 7f505d33220..c3703b0930e 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -82,38 +82,39 @@ to satisfy the conditions required to perform this unsafe operation. # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -First of all, we no longer warn that an `unsafe` block is unnecessary when it is -nested immediately inside an `unsafe fn`. So, the following compiles without -any warning: +1. First of all, we no longer warn that an `unsafe` block is unnecessary when it is + nested immediately inside an `unsafe fn`. So, the following compiles without + any warning: -```rust -unsafe fn get_unchecked(x: &[T], i: usize) -> &T { - unsafe { x.get_unchecked(i) } -} -``` + ```rust + unsafe fn get_unchecked(x: &[T], i: usize) -> &T { + unsafe { x.get_unchecked(i) } + } + ``` -However, nested `unsafe` blocks are still redundant, so this warns: + However, nested `unsafe` blocks are still redundant, so this warns: -```rust -unsafe fn get_unchecked(x: &[T], i: usize) -> &T { - unsafe { unsafe { x.get_unchecked(i) } } -} -``` + ```rust + unsafe fn get_unchecked(x: &[T], i: usize) -> &T { + unsafe { unsafe { x.get_unchecked(i) } } + } + ``` -In a next step, we have a lint that fires when an unsafe operation is performed -inside an `unsafe fn` but outside an `unsafe` block. So, this would trigger the -lint: +2. In a next step, we have a lint that fires when an unsafe operation is performed + inside an `unsafe fn` but outside an `unsafe` block. So, this would trigger the + lint: -```rust -unsafe fn get_unchecked(x: &[T], i: usize) -> &T { - x.get_unchecked(i) -} -``` + ```rust + unsafe fn get_unchecked(x: &[T], i: usize) -> &T { + x.get_unchecked(i) + } + ``` -This gets us into a state where programmers are much less likely to accidentally -perform undesired unsafe operations inside `unsafe fn`. + This gets us into a state where programmers are much less likely to accidentally + perform undesired unsafe operations inside `unsafe fn`. -Even later, it might be desirable to turn this warning into an error. +3. Even later (in the 2021 edition), it might be desirable to turn this warning + into an error. # Drawbacks [drawbacks]: #drawbacks @@ -131,6 +132,10 @@ I explained the rationale in the motivation section. The alternative is to not do anything, and live with the current situation. +We could introduce named proof obligations (proposed by @Centril) such that the +compiler can be be told (to some extend) if the assumptions made by the `unsafe +fn` are sufficient to discharge the requirements of the unsafe operations. + # Prior art [prior-art]: #prior-art From 5571c39ece6599b765436bcb0332bbfeb165ae7f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Nov 2018 13:03:00 +0100 Subject: [PATCH 06/15] add clippy to action plan --- text/0000-unsafe-block-in-unsafe-fn.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index c3703b0930e..5bb227d6fe2 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -100,9 +100,9 @@ to satisfy the conditions required to perform this unsafe operation. } ``` -2. In a next step, we have a lint that fires when an unsafe operation is performed - inside an `unsafe fn` but outside an `unsafe` block. So, this would trigger the - lint: +2. Optionally, we could add a clippy "correctness" lint to warn about unsafe + operations inside an `unsafe fn`, but outside an `unsafe` block. So, this + would trigger the lint: ```rust unsafe fn get_unchecked(x: &[T], i: usize) -> &T { @@ -110,10 +110,11 @@ to satisfy the conditions required to perform this unsafe operation. } ``` - This gets us into a state where programmers are much less likely to accidentally - perform undesired unsafe operations inside `unsafe fn`. +3. In a next step, we move this lint to rustc proper, make it warn-by-default. + This gets us into a state where programmers are much less likely to + accidentally perform undesired unsafe operations inside `unsafe fn`. -3. Even later (in the 2021 edition), it might be desirable to turn this warning +4. Even later (in the 2021 edition), it might be desirable to turn this warning into an error. # Drawbacks From 2550a0b6ba7d875c9ebeeb2b8f7c622fef59f19a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Nov 2018 13:06:06 +0100 Subject: [PATCH 07/15] 2021 maybe --- text/0000-unsafe-block-in-unsafe-fn.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index 5bb227d6fe2..c00f9cb12fa 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -114,8 +114,8 @@ to satisfy the conditions required to perform this unsafe operation. This gets us into a state where programmers are much less likely to accidentally perform undesired unsafe operations inside `unsafe fn`. -4. Even later (in the 2021 edition), it might be desirable to turn this warning - into an error. +4. Even later (in the 2021 edition?), it might be desirable to turn this + warning into an error. # Drawbacks [drawbacks]: #drawbacks From 153d2866084f752fe4a97449523b4a011f77be4a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 5 Nov 2018 15:20:01 +0100 Subject: [PATCH 08/15] maybe we should employ cargo fix --- text/0000-unsafe-block-in-unsafe-fn.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index c00f9cb12fa..a5e46a281da 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -154,3 +154,11 @@ culture of thinking about this in terms of proof obligations. Should this lint be in clippy first before in becomes warn-by-default in rustc, to avoid a huge flood of warnings showing up at once? Should the lint ever become a hard error (on newer editions), or remain a warning indefinitely? + +Should we require `cargo fix` to be able to do *something* about this warning +before making it warn-by-default? `cargo fix` could, for example, wrap the body +of every `unsafe fn` in one big `unsafe` block. That would not improve the +amount of care that is taken for unsafety in the fixed code, but it would +provide a way to the incrementally improve the big functions, and new functions +written later would have the appropriate amount of care applied to them from the +start. From 8cb1b3f09dd8f567e9e5ae7759708280708061b8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 12 Nov 2018 14:26:20 +0100 Subject: [PATCH 09/15] more alternatives --- text/0000-unsafe-block-in-unsafe-fn.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index a5e46a281da..e4919320ed3 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -124,7 +124,13 @@ This new warning will likely fire for the vast majority of `unsafe fn` out there Many `unsafe fn` are actually rather short (no more than 3 lines) and will likely end up just being one large `unsafe` block. This change would make such -functions less ergonomic to write. +functions less ergonomic to write, they would likely become + +```rust +unsafe fn foo(...) -> ... { unsafe { + // Code goes here +} } +``` # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives @@ -137,6 +143,14 @@ We could introduce named proof obligations (proposed by @Centril) such that the compiler can be be told (to some extend) if the assumptions made by the `unsafe fn` are sufficient to discharge the requirements of the unsafe operations. +We could restrict this requirement to use `unsafe` blocks in `unsafe fn` to +those `unsafe fn` that contain at least one `unsafe` block, meaning short +`unsafe fn` would keep compiling like they do now. + +We could have separate marker for `unsafe fn` with and without an implicitly +unsafe body, e.g. `unsafe unsafe fn` has an unsafe body, or `unsafe fn foo(...) +-> ... unsafe { }` has an unsafe body, or `unsafe_to_call fn` has a safe body. + # Prior art [prior-art]: #prior-art From 949590b8afb8b703507f94cf3917cd13721edb46 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Mar 2020 15:55:34 +0200 Subject: [PATCH 10/15] improve motivation --- text/0000-unsafe-block-in-unsafe-fn.md | 36 +++++++++++++------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index e4919320ed3..79f35ddc2e3 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -38,24 +38,24 @@ However, this neglects the other aspect of `unsafe` that I described above: To make the programmer aware that they are treading dangerous ground even when they may not realize they are doing so. -Using some more formal terminology, an `unsafe` block generally comes with a -proof *obligation*: The programmer has to ensure that this code is actually safe -to execute in the current context, because the compiler just trusts the -programmer to get this right. In contrast, `unsafe fn` represents an -*assumption*: As the author of this function, I make some assumptions that I -expect my callees to uphold. (In terms of -[introduction and elimination forms of natural deduction](https://en.wikipedia.org/wiki/Natural_deduction#Introduction_and_elimination), -`unsafe` blocks are where you introduce, and `unsafe fn` is where you -eliminate.) - -Making `unsafe fn` also implicitly play the role of an `unsafe` block conflates -these two dual aspects of unsafety (one party making an assumption, another -party having the obligation to prove that assumption). There is no reason to -believe that the assumption made by the `unsafe fn` is the same as the -obligation incurred by unsafe operations inside this function, and hence the -author of the `unsafe fn` should better carefully check that their assumptions -are sufficient to justify the unsafe operations they are performing. This is -what an `unsafe` block would indicate. +In fact, this double role of `unsafe` in `unsafe fn` (making it both unsafe to +call and enabling it to call other unsafe operations) conflates the two *dual* +roles that `unsafe` plays in Rust. On the one hand, there are places that +*define* a proof obligation, these make things "unsafe to call/do" (e.g., the +language definition says that dereferencing a raw pointer requires it not to be +dangling). On the other hand, there are places that *discharge* the proof +obligation, these are "unsafe blocks of code" (e.g., unsafe code that +dereferences a raw pointer has to locally argue why it cannot be dangling). + +`unsafe {}` blocks are about *discharging* obligations, but `unsafe fn` are +about *defining* obligations. The fact that the body of an `unsafe fn` is also +implicitly treated like a block has made it hard to realize this duality +[even for experienced Rust developers][unsafe-dual]. (Completing the picture, +`unsafe Trait` also defines an obligation, that is discharged by `unsafe impl`. +Curiously, `unsafe impl` does *not* implicitly make all bodies of this `impl` +unsafe blocks, which is somewhat inconsistent with `unsafe fn`.) + +[unsafe-dual]: https://github.com/rust-lang/rfcs/pull/2585#issuecomment-577852430 # Guide-level explanation [guide-level-explanation]: #guide-level-explanation From ec454f290548a469f2fabe03a0214f57b468904b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Mar 2020 18:25:16 +0200 Subject: [PATCH 11/15] update plan to include the lint --- text/0000-unsafe-block-in-unsafe-fn.md | 123 +++++++++++++++---------- 1 file changed, 72 insertions(+), 51 deletions(-) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index 79f35ddc2e3..9eaf9b2a4e0 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -60,8 +60,10 @@ unsafe blocks, which is somewhat inconsistent with `unsafe fn`.) # Guide-level explanation [guide-level-explanation]: #guide-level-explanation -When you perform an unsafe operation, like dereferencing a raw pointer or -calling an `unsafe` function, you must enclose that code in an `unsafe` block. +The `unsafe` keyword in Rust serves two related purposes. + +When you perform an "unsafe to call" operation, like dereferencing a raw pointer +or calling an `unsafe fn`, you must enclose that code in an `unsafe {}` block. The purpose of this is to acknowledge that the operation you are performing here has *not* been checked by the compiler, you are responsible yourself for upholding Rust's safety guarantees. Generally, unsafe operations come with @@ -72,59 +74,72 @@ When you are writing a function that itself has additional conditions to ensure safety (say, it accesses some data without making some necessary bounds checks, or it takes some raw pointers as arguments and performs memory operations based on them), then you should mark this as an `unsafe fn` and it is up to you to -document the conditions that must be met for the arguments. - -Your `unsafe fn` will likely perform unsafe operations; these have to be -enclosed by an `unsafe` block as usual. This is the place where you have to -check that the requirements you documented for your own function are sufficient -to satisfy the conditions required to perform this unsafe operation. +document the conditions that must be met for the arguments. This use of the +`unsafe` keyword makes your function itself "unsafe to call". + +The same duality can be observed in traits: `unsafe trait` is like `unsafe fn`; +it makes implementing this trait an "unsafe to call" operation and it is up to +whoever defines the trait to precisely document what is unsafe about it. +`unsafe impl` is like `unsafe {}`, it acknowledges that there are extra +requirements here that are not checked by the compiler and that the programmer +is responsible to uphold. + +For this reason, "unsafe to call" operations inside an `unsafe fn` must be +contained inside an `unsafe {}` block like everywhere else. The author of these +functions has to ensure that the requirements of the operation are upheld. To +this end, the author may of course assume that the caller of the `unsafe fn` in +turn uphold their own requirements. + +For backwards compatibility reasons, this unsafety check inside `unsafe fn` is +controlled by a lint, `unsafe_op_in_unsafe_fn`. By setting +`#[deny(unsafe_op_in_unsafe_fn)]`, the compiler is as strict about unsafe +operations inside `unsafe fn` as it is everywhere else. + +This lint is allow-by-default initially, and will be warn-by-default across all +editions eventually. In future editions, it may become deny-by-default, or even +a hard error. # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -1. First of all, we no longer warn that an `unsafe` block is unnecessary when it is - nested immediately inside an `unsafe fn`. So, the following compiles without - any warning: - - ```rust - unsafe fn get_unchecked(x: &[T], i: usize) -> &T { - unsafe { x.get_unchecked(i) } - } - ``` +The new `unsafe_op_in_unsafe_fn` lint triggers when an unsafe operation is used +inside an `unsafe fn` but outside `unsafe {}` blocks. So, the following will +emit a warning: - However, nested `unsafe` blocks are still redundant, so this warns: - - ```rust - unsafe fn get_unchecked(x: &[T], i: usize) -> &T { - unsafe { unsafe { x.get_unchecked(i) } } - } - ``` +```rust +#[warn(unsafe_op_in_unsafe_fn)] +unsafe fn get_unchecked(x: &[T], i: usize) -> &T { + x.get_unchecked(i) +} +``` -2. Optionally, we could add a clippy "correctness" lint to warn about unsafe - operations inside an `unsafe fn`, but outside an `unsafe` block. So, this - would trigger the lint: +Moreover, if and only if the `unsafe_op_in_unsafe_fn` lint is not `allow`ed, we +no longer warn that an `unsafe` block is unnecessary when it is nested +immediately inside an `unsafe fn`. So, the following compiles without any +warning: - ```rust - unsafe fn get_unchecked(x: &[T], i: usize) -> &T { - x.get_unchecked(i) - } - ``` +```rust +#[warn(unsafe_op_in_unsafe_fn)] +unsafe fn get_unchecked(x: &[T], i: usize) -> &T { + unsafe { x.get_unchecked(i) } +} +``` -3. In a next step, we move this lint to rustc proper, make it warn-by-default. - This gets us into a state where programmers are much less likely to - accidentally perform undesired unsafe operations inside `unsafe fn`. +However, nested `unsafe` blocks are still redundant, so this warns: -4. Even later (in the 2021 edition?), it might be desirable to turn this - warning into an error. +```rust +#[warn(unsafe_op_in_unsafe_fn)] +unsafe fn get_unchecked(x: &[T], i: usize) -> &T { + unsafe { unsafe { x.get_unchecked(i) } } +} +``` # Drawbacks [drawbacks]: #drawbacks -This new warning will likely fire for the vast majority of `unsafe fn` out there. - -Many `unsafe fn` are actually rather short (no more than 3 lines) and will -likely end up just being one large `unsafe` block. This change would make such -functions less ergonomic to write, they would likely become +Many `unsafe fn` are actually rather short (no more than 3 lines) and will end +up just being one large `unsafe` block. This change would make such functions +less ergonomic to write, they would likely become ```rust unsafe fn foo(...) -> ... { unsafe { @@ -165,14 +180,20 @@ culture of thinking about this in terms of proof obligations. # Unresolved questions [unresolved-questions]: #unresolved-questions -Should this lint be in clippy first before in becomes warn-by-default in rustc, -to avoid a huge flood of warnings showing up at once? Should the lint ever -become a hard error (on newer editions), or remain a warning indefinitely? +What is the timeline for adding the lint, and cranking up its default level? +Should the default level depend on the edition? + +Should we ever make this deny-by-default or even a hard error, in a future +edition? Should we require `cargo fix` to be able to do *something* about this warning -before making it warn-by-default? `cargo fix` could, for example, wrap the body -of every `unsafe fn` in one big `unsafe` block. That would not improve the -amount of care that is taken for unsafety in the fixed code, but it would -provide a way to the incrementally improve the big functions, and new functions -written later would have the appropriate amount of care applied to them from the -start. +before making it even warn-by-default? (We certainly need to do something +before making it deny-by-default or a hard error in a future edition.) `cargo +fix` could add big `unsafe {}` blocks around the entire body of every `unsafe +fn`. That would not improve the amount of care that is taken for unsafety in +the fixed code, but it would provide a way to the incrementally improve the big +functions, and new functions written later would have the appropriate amount of +care applied to them from the start. Potentially, `rustfmt` could be taught to +format `unsafe` blocks that wrap the entire function body in a way that avoids +double-indent. "function bodies as expressions" would enable a format like +`unsafe fn foo() = unsafe { body }`. From 1b3e1dddbc9e3703fd019ff28110ac0bcd515adc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Mar 2020 18:30:24 +0200 Subject: [PATCH 12/15] mention that the lint name is not final yet --- text/0000-unsafe-block-in-unsafe-fn.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index 9eaf9b2a4e0..d72b383fcee 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -154,6 +154,8 @@ I explained the rationale in the motivation section. The alternative is to not do anything, and live with the current situation. +We could bikeshed the lint name. + We could introduce named proof obligations (proposed by @Centril) such that the compiler can be be told (to some extend) if the assumptions made by the `unsafe fn` are sufficient to discharge the requirements of the unsafe operations. From 2e47c418373789b10603aaaa98057882ec8d6ee8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Mar 2020 18:35:49 +0200 Subject: [PATCH 13/15] minor editing; unsafe_to_call alternative --- text/0000-unsafe-block-in-unsafe-fn.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index d72b383fcee..d3a9f748ed4 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -52,8 +52,9 @@ about *defining* obligations. The fact that the body of an `unsafe fn` is also implicitly treated like a block has made it hard to realize this duality [even for experienced Rust developers][unsafe-dual]. (Completing the picture, `unsafe Trait` also defines an obligation, that is discharged by `unsafe impl`. -Curiously, `unsafe impl` does *not* implicitly make all bodies of this `impl` -unsafe blocks, which is somewhat inconsistent with `unsafe fn`.) +Curiously, `unsafe trait` does *not* implicitly make all bodies of default +functions defined inside this trait unsafe blocks, which is somewhat +inconsistent with `unsafe fn` when viewed through this lens.) [unsafe-dual]: https://github.com/rust-lang/rfcs/pull/2585#issuecomment-577852430 @@ -156,6 +157,11 @@ The alternative is to not do anything, and live with the current situation. We could bikeshed the lint name. +We could avoid using `unsafe` for dual purpose, and instead have `unsafe_to_call +fn` for functions that are "unsafe to call" but do not implicitly have an +`unsafe {}` block in their body. For consistency, we might want `unsafe_to_impl +trait` for traits, though the behavior would be the same as `unsafe trait`. + We could introduce named proof obligations (proposed by @Centril) such that the compiler can be be told (to some extend) if the assumptions made by the `unsafe fn` are sufficient to discharge the requirements of the unsafe operations. @@ -164,10 +170,6 @@ We could restrict this requirement to use `unsafe` blocks in `unsafe fn` to those `unsafe fn` that contain at least one `unsafe` block, meaning short `unsafe fn` would keep compiling like they do now. -We could have separate marker for `unsafe fn` with and without an implicitly -unsafe body, e.g. `unsafe unsafe fn` has an unsafe body, or `unsafe fn foo(...) --> ... unsafe { }` has an unsafe body, or `unsafe_to_call fn` has a safe body. - # Prior art [prior-art]: #prior-art From 814301e45efdec4b3279e54dd756564529f2effd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 30 Mar 2020 14:38:02 +0200 Subject: [PATCH 14/15] mention an alternative that avoids lint-on-lint dependencies --- text/0000-unsafe-block-in-unsafe-fn.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index d3a9f748ed4..3faaf4e4fae 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -162,6 +162,13 @@ fn` for functions that are "unsafe to call" but do not implicitly have an `unsafe {}` block in their body. For consistency, we might want `unsafe_to_impl trait` for traits, though the behavior would be the same as `unsafe trait`. +We could avoid having the "unnecessary unsafe" lint depend on +`unsafe_op_in_unsafe_fn` and instead always behave like those blocks are +necessary (if they contain an "unsafe to call" operation). That would avoid a +dependency of one lint on another, but it could possibly be confusing when, +inside an `unsafe fn`, some operations are guarded by an unsafe block and others +are not. + We could introduce named proof obligations (proposed by @Centril) such that the compiler can be be told (to some extend) if the assumptions made by the `unsafe fn` are sufficient to discharge the requirements of the unsafe operations. From 9838e5d9243ce38b74fc064009fa4b592d9a08f8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 30 Mar 2020 17:44:48 +0200 Subject: [PATCH 15/15] mention 'other opt-in switch' as unresolved question --- text/0000-unsafe-block-in-unsafe-fn.md | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/text/0000-unsafe-block-in-unsafe-fn.md b/text/0000-unsafe-block-in-unsafe-fn.md index 3faaf4e4fae..66c72032ad3 100644 --- a/text/0000-unsafe-block-in-unsafe-fn.md +++ b/text/0000-unsafe-block-in-unsafe-fn.md @@ -151,24 +151,18 @@ unsafe fn foo(...) -> ... { unsafe { # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives -I explained the rationale in the motivation section. +To achieve the goals laid out in the motivation section, the proposed approach +is least invasive in the sense that it avoids introducing new keywords, and +instead relies on the existing lint mechanism to perform the transition. -The alternative is to not do anything, and live with the current situation. - -We could bikeshed the lint name. +One alternative always is to not do anything, and live with the current +situation. We could avoid using `unsafe` for dual purpose, and instead have `unsafe_to_call fn` for functions that are "unsafe to call" but do not implicitly have an `unsafe {}` block in their body. For consistency, we might want `unsafe_to_impl trait` for traits, though the behavior would be the same as `unsafe trait`. -We could avoid having the "unnecessary unsafe" lint depend on -`unsafe_op_in_unsafe_fn` and instead always behave like those blocks are -necessary (if they contain an "unsafe to call" operation). That would avoid a -dependency of one lint on another, but it could possibly be confusing when, -inside an `unsafe fn`, some operations are guarded by an unsafe block and others -are not. - We could introduce named proof obligations (proposed by @Centril) such that the compiler can be be told (to some extend) if the assumptions made by the `unsafe fn` are sufficient to discharge the requirements of the unsafe operations. @@ -177,6 +171,8 @@ We could restrict this requirement to use `unsafe` blocks in `unsafe fn` to those `unsafe fn` that contain at least one `unsafe` block, meaning short `unsafe fn` would keep compiling like they do now. +And of course, the lint name is subject to bikeshedding. + # Prior art [prior-art]: #prior-art @@ -208,3 +204,7 @@ care applied to them from the start. Potentially, `rustfmt` could be taught to format `unsafe` blocks that wrap the entire function body in a way that avoids double-indent. "function bodies as expressions" would enable a format like `unsafe fn foo() = unsafe { body }`. + +It is not entirely clear if having the behavior of one lint depend on another +will work (though most likely, it will). If it does not, we should try to find +some other mechanism to opt-in to the new treatment of `unsafe fn` bodies.