[PATCH v2 0/4] Replace unwraps in doctests with graceful handling

[PATCH v2 0/4] Replace unwraps in doctests with graceful handling

[Daniel Sedlak]

In the following patches, I touch on error handling in the documentation
tests/examples. There are multiple occurrences of unwraps which can be
replaced by a question mark operator which should simplify the code.
Also, people typically tend to copy & paste code from the examples
(citation needed). If we leave the unwraps there, I think we can convey
the wrong impression, that it is OK to use unwraps unless the programmer
has a really good reason to use them.

Changes since RFC/v1:
- Remove opinionated changes from `rust/kernel/rbtree.rs`.
- Run clippy
- Add description to each commit message body with rationale.
- Link to RFC/v1: https://lore.kernel.org/rust-for-linux/20241116195221.373332-1-daniel@sedlak.dev/

Daniel Sedlak (4):
  rust: kernel: init: replace unwraps with the question mark operators
  rust: kernel: rbtree: remove unwrap in asserts
  rust: kernel: page: remove unnecessary helper function from doctest
  rust: kernel: str: replace unwraps with the question mark operators

 rust/kernel/init.rs   |  6 ++++--
 rust/kernel/page.rs   |  6 ++----
 rust/kernel/rbtree.rs | 48 +++++++++++++++++++++----------------------
 rust/kernel/str.rs    | 28 +++++++++++++++----------
 4 files changed, 47 insertions(+), 41 deletions(-)

-- 
2.47.0

[PATCH v2 1/4] rust: kernel: init: replace unwraps with the question mark operators

[Daniel Sedlak]

Use `?` operator in the doctest. Since it is in the examples,
using unwraps can convey a wrong impression that unwrapping is
safe behavior, thus this patch removes this unwrapping.

Link: https://lore.kernel.org/rust-for-linux/CANiq72nsK1D4NuQ1U7NqMWoYjXkqQSj4QuUEL98OmFbq022Z9A@mail.gmail.com/
Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
---
 rust/kernel/init.rs | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 347049df556b..81d69d22090c 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -1076,8 +1076,9 @@ pub fn uninit<T, E>() -> impl Init<MaybeUninit<T>, E> {
 /// ```rust
 /// use kernel::{alloc::KBox, error::Error, init::init_array_from_fn};
 /// let array: KBox<[usize; 1_000]> =
-///     KBox::init::<Error>(init_array_from_fn(|i| i), GFP_KERNEL).unwrap();
+///     KBox::init::<Error>(init_array_from_fn(|i| i), GFP_KERNEL)?;
 /// assert_eq!(array.len(), 1_000);
+/// # Ok::<(), Error>(())
 /// ```
 pub fn init_array_from_fn<I, const N: usize, T, E>(
     mut make_init: impl FnMut(usize) -> I,
@@ -1120,8 +1121,9 @@ pub fn init_array_from_fn<I, const N: usize, T, E>(
 /// ```rust
 /// use kernel::{sync::{Arc, Mutex}, init::pin_init_array_from_fn, new_mutex};
 /// let array: Arc<[Mutex<usize>; 1_000]> =
-///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i)), GFP_KERNEL).unwrap();
+///     Arc::pin_init(pin_init_array_from_fn(|i| new_mutex!(i)), GFP_KERNEL)?;
 /// assert_eq!(array.len(), 1_000);
+/// # Ok::<(), Error>(())
 /// ```
 pub fn pin_init_array_from_fn<I, const N: usize, T, E>(
     mut make_init: impl FnMut(usize) -> I,
-- 
2.47.0

[PATCH v2 2/4] rust: kernel: rbtree: remove unwrap in asserts

[Daniel Sedlak]

Remove `uwnrap` in asserts and replace it with `Option::Some`
matching. By doing it this way, the examples is more
descriptive, so it disambiguates the return type of
the `get(...)` and `next(...)`, because the `unwrap(...)`
can also be called on `Result`.

Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
---
 rust/kernel/rbtree.rs | 48 +++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
index cb4415a12258..1ada2d274c7c 100644
--- a/rust/kernel/rbtree.rs
+++ b/rust/kernel/rbtree.rs
@@ -36,17 +36,17 @@
 ///
 /// // Check the nodes we just inserted.
 /// {
-///     assert_eq!(tree.get(&10).unwrap(), &100);
-///     assert_eq!(tree.get(&20).unwrap(), &200);
-///     assert_eq!(tree.get(&30).unwrap(), &300);
+///     assert_eq!(tree.get(&10), Some(&100));
+///     assert_eq!(tree.get(&20), Some(&200));
+///     assert_eq!(tree.get(&30), Some(&300));
 /// }
 ///
 /// // Iterate over the nodes we just inserted.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &100));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
-///     assert_eq!(iter.next().unwrap(), (&30, &300));
+///     assert_eq!(iter.next(), Some((&10, &100)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
+///     assert_eq!(iter.next(), Some((&30, &300)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -61,9 +61,9 @@
 /// // Check that the tree reflects the replacement.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &1000));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
-///     assert_eq!(iter.next().unwrap(), (&30, &300));
+///     assert_eq!(iter.next(), Some((&10, &1000)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
+///     assert_eq!(iter.next(), Some((&30, &300)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -73,9 +73,9 @@
 /// // Check that the tree reflects the update.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &1000));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
-///     assert_eq!(iter.next().unwrap(), (&30, &3000));
+///     assert_eq!(iter.next(), Some((&10, &1000)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
+///     assert_eq!(iter.next(), Some((&30, &3000)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -85,8 +85,8 @@
 /// // Check that the tree reflects the removal.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
-///     assert_eq!(iter.next().unwrap(), (&30, &3000));
+///     assert_eq!(iter.next(), Some((&20, &200)));
+///     assert_eq!(iter.next(), Some((&30, &3000)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -128,20 +128,20 @@
 /// // Check the nodes we just inserted.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &100));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
-///     assert_eq!(iter.next().unwrap(), (&30, &300));
+///     assert_eq!(iter.next(), Some((&10, &100)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
+///     assert_eq!(iter.next(), Some((&30, &300)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
 /// // Remove a node, getting back ownership of it.
-/// let existing = tree.remove(&30).unwrap();
+/// let existing = tree.remove(&30);
 ///
 /// // Check that the tree reflects the removal.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &100));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
+///     assert_eq!(iter.next(), Some((&10, &100)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -155,9 +155,9 @@
 /// // Check that the tree reflect the new insertion.
 /// {
 ///     let mut iter = tree.iter();
-///     assert_eq!(iter.next().unwrap(), (&10, &100));
-///     assert_eq!(iter.next().unwrap(), (&15, &150));
-///     assert_eq!(iter.next().unwrap(), (&20, &200));
+///     assert_eq!(iter.next(), Some((&10, &100)));
+///     assert_eq!(iter.next(), Some((&15, &150)));
+///     assert_eq!(iter.next(), Some((&20, &200)));
 ///     assert!(iter.next().is_none());
 /// }
 ///
@@ -677,7 +677,7 @@ fn drop(&mut self) {
 /// Nodes adjacent to the current node can also be removed.
 ///
 /// ```
-/// use kernel::{alloc::flags, rbtree::RBTree};
+/// use kernel::{alloc::flags, rbtree::{RBTree, RBTreeNode}};
 ///
 /// // Create a new tree.
 /// let mut tree = RBTree::new();
-- 
2.47.0

[PATCH v2 3/4] rust: kernel: page: remove unnecessary helper function from doctest

[Daniel Sedlak]

Doctest in the `page.rs` contained helper function `dox` which acted
as a wrapper for using the `?` operator. However, this is not needed
because doctests are implicitly wrapper in function see [1].

[1]: https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#using--in-doc-tests

Link: https://lore.kernel.org/rust-for-linux/459782fe-afca-4fe6-8ffb-ba7c7886de0a@de.bosch.com/
Suggested-by: Dirk Behme <dirk.behme@de.bosch.com>
Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off: Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
---
 rust/kernel/page.rs | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index fdac6c375fe4..f6126aca33a6 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -57,9 +57,8 @@ impl Page {
     /// ```
     /// use kernel::page::Page;
     ///
-    /// # fn dox() -> Result<(), kernel::alloc::AllocError> {
     /// let page = Page::alloc_page(GFP_KERNEL)?;
-    /// # Ok(()) }
+    /// # Ok::<(), kernel::alloc::AllocError>(())
     /// ```
     ///
     /// Allocate memory for a page and zero its contents.
@@ -67,9 +66,8 @@ impl Page {
     /// ```
     /// use kernel::page::Page;
     ///
-    /// # fn dox() -> Result<(), kernel::alloc::AllocError> {
     /// let page = Page::alloc_page(GFP_KERNEL | __GFP_ZERO)?;
-    /// # Ok(()) }
+    /// # Ok::<(), kernel::alloc::AllocError>(())
     /// ```
     pub fn alloc_page(flags: Flags) -> Result<Self, AllocError> {
         // SAFETY: Depending on the value of `gfp_flags`, this call may sleep. Other than that, it
-- 
2.47.0

[PATCH v2 4/4] rust: kernel: str: replace unwraps with the question mark operators

[Daniel Sedlak]

Simplify the error handling by replacing unwraps with the
question mark operator. Furthermore, unwraps can convey
a wrong impression that unwrapping is safe behavior,
thus this patch removes this unwrapping.

Link: https://lore.kernel.org/rust-for-linux/CANiq72nsK1D4NuQ1U7NqMWoYjXkqQSj4QuUEL98OmFbq022Z9A@mail.gmail.com/
Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
---
 rust/kernel/str.rs | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index d04c12a1426d..55755f008675 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -39,12 +39,13 @@ impl fmt::Display for BStr {
     /// ```
     /// # use kernel::{fmt, b_str, str::{BStr, CString}};
     /// let ascii = b_str!("Hello, BStr!");
-    /// let s = CString::try_from_fmt(fmt!("{}", ascii)).unwrap();
+    /// let s = CString::try_from_fmt(fmt!("{}", ascii))?;
     /// assert_eq!(s.as_bytes(), "Hello, BStr!".as_bytes());
     ///
     /// let non_ascii = b_str!("🦀");
-    /// let s = CString::try_from_fmt(fmt!("{}", non_ascii)).unwrap();
+    /// let s = CString::try_from_fmt(fmt!("{}", non_ascii))?;
     /// assert_eq!(s.as_bytes(), "\\xf0\\x9f\\xa6\\x80".as_bytes());
+    /// # Ok::<(), kernel::error::Error>(())
     /// ```
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         for &b in &self.0 {
@@ -70,12 +71,13 @@ impl fmt::Debug for BStr {
     /// # use kernel::{fmt, b_str, str::{BStr, CString}};
     /// // Embedded double quotes are escaped.
     /// let ascii = b_str!("Hello, \"BStr\"!");
-    /// let s = CString::try_from_fmt(fmt!("{:?}", ascii)).unwrap();
+    /// let s = CString::try_from_fmt(fmt!("{:?}", ascii))?;
     /// assert_eq!(s.as_bytes(), "\"Hello, \\\"BStr\\\"!\"".as_bytes());
     ///
     /// let non_ascii = b_str!("😺");
-    /// let s = CString::try_from_fmt(fmt!("{:?}", non_ascii)).unwrap();
+    /// let s = CString::try_from_fmt(fmt!("{:?}", non_ascii))?;
     /// assert_eq!(s.as_bytes(), "\"\\xf0\\x9f\\x98\\xba\"".as_bytes());
+    /// # Ok::<(), kernel::error::Error>(())
     /// ```
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.write_char('"')?;
@@ -273,8 +275,9 @@ pub const fn as_bytes_with_nul(&self) -> &[u8] {
     ///
     /// ```
     /// # use kernel::str::CStr;
-    /// let cstr = CStr::from_bytes_with_nul(b"foo\0").unwrap();
+    /// let cstr = CStr::from_bytes_with_nul(b"foo\0")?;
     /// assert_eq!(cstr.to_str(), Ok("foo"));
+    /// # Ok::<(), kernel::error::Error>(())
     /// ```
     #[inline]
     pub fn to_str(&self) -> Result<&str, core::str::Utf8Error> {
@@ -384,12 +387,13 @@ impl fmt::Display for CStr {
     /// # use kernel::str::CStr;
     /// # use kernel::str::CString;
     /// let penguin = c_str!("🐧");
-    /// let s = CString::try_from_fmt(fmt!("{}", penguin)).unwrap();
+    /// let s = CString::try_from_fmt(fmt!("{}", penguin))?;
     /// assert_eq!(s.as_bytes_with_nul(), "\\xf0\\x9f\\x90\\xa7\0".as_bytes());
     ///
     /// let ascii = c_str!("so \"cool\"");
-    /// let s = CString::try_from_fmt(fmt!("{}", ascii)).unwrap();
+    /// let s = CString::try_from_fmt(fmt!("{}", ascii))?;
     /// assert_eq!(s.as_bytes_with_nul(), "so \"cool\"\0".as_bytes());
+    /// # Ok::<(), kernel::error::Error>(())
     /// ```
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         for &c in self.as_bytes() {
@@ -413,13 +417,14 @@ impl fmt::Debug for CStr {
     /// # use kernel::str::CStr;
     /// # use kernel::str::CString;
     /// let penguin = c_str!("🐧");
-    /// let s = CString::try_from_fmt(fmt!("{:?}", penguin)).unwrap();
+    /// let s = CString::try_from_fmt(fmt!("{:?}", penguin))?;
     /// assert_eq!(s.as_bytes_with_nul(), "\"\\xf0\\x9f\\x90\\xa7\"\0".as_bytes());
     ///
     /// // Embedded double quotes are escaped.
     /// let ascii = c_str!("so \"cool\"");
-    /// let s = CString::try_from_fmt(fmt!("{:?}", ascii)).unwrap();
+    /// let s = CString::try_from_fmt(fmt!("{:?}", ascii))?;
     /// assert_eq!(s.as_bytes_with_nul(), "\"so \\\"cool\\\"\"\0".as_bytes());
+    /// # Ok::<(), kernel::error::Error>(())
     /// ```
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.write_str("\"")?;
@@ -799,16 +804,17 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
 /// ```
 /// use kernel::{str::CString, fmt};
 ///
-/// let s = CString::try_from_fmt(fmt!("{}{}{}", "abc", 10, 20)).unwrap();
+/// let s = CString::try_from_fmt(fmt!("{}{}{}", "abc", 10, 20))?;
 /// assert_eq!(s.as_bytes_with_nul(), "abc1020\0".as_bytes());
 ///
 /// let tmp = "testing";
-/// let s = CString::try_from_fmt(fmt!("{tmp}{}", 123)).unwrap();
+/// let s = CString::try_from_fmt(fmt!("{tmp}{}", 123))?;
 /// assert_eq!(s.as_bytes_with_nul(), "testing123\0".as_bytes());
 ///
 /// // This fails because it has an embedded `NUL` byte.
 /// let s = CString::try_from_fmt(fmt!("a\0b{}", 123));
 /// assert_eq!(s.is_ok(), false);
+/// # Ok::<(), kernel::error::Error>(())
 /// ```
 pub struct CString {
     buf: KVec<u8>,
-- 
2.47.0

Re: [PATCH v2 4/4] rust: kernel: str: replace unwraps with the question mark operators

[Alice Ryhl]

On Tue, Nov 19, 2024 at 8:36 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Simplify the error handling by replacing unwraps with the
> question mark operator. Furthermore, unwraps can convey
> a wrong impression that unwrapping is safe behavior,
> thus this patch removes this unwrapping.
>
> Link: https://lore.kernel.org/rust-for-linux/CANiq72nsK1D4NuQ1U7NqMWoYjXkqQSj4QuUEL98OmFbq022Z9A@mail.gmail.com/
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2 3/4] rust: kernel: page: remove unnecessary helper function from doctest

[Alice Ryhl]

On Tue, Nov 19, 2024 at 8:36 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Doctest in the `page.rs` contained helper function `dox` which acted
> as a wrapper for using the `?` operator. However, this is not needed
> because doctests are implicitly wrapper in function see [1].
>
> [1]: https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#using--in-doc-tests

Normally you'd format this using a `Link: url [1]` tag instead.

> Link: https://lore.kernel.org/rust-for-linux/459782fe-afca-4fe6-8ffb-ba7c7886de0a@de.bosch.com/
> Suggested-by: Dirk Behme <dirk.behme@de.bosch.com>
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off: Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2 2/4] rust: kernel: rbtree: remove unwrap in asserts

[Alice Ryhl]

On Tue, Nov 19, 2024 at 8:36 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Remove `uwnrap` in asserts and replace it with `Option::Some`

typo: unwrap

> matching. By doing it this way, the examples is more

typo: is -> are

> -/// use kernel::{alloc::flags, rbtree::RBTree};
> +/// use kernel::{alloc::flags, rbtree::{RBTree, RBTreeNode}};

What's up with this change? Is the example broken?

Alice

Re: [PATCH v2 3/4] rust: kernel: page: remove unnecessary helper function from doctest

[Tamir Duberstein]

On Tue, Nov 19, 2024 at 3:01 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Doctest in the `page.rs` contained helper function `dox` which acted
> as a wrapper for using the `?` operator. However, this is not needed
> because doctests are implicitly wrapper in function see [1].

Typo: s/wrapper in/wrapped in a/

> [1]: https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#using--in-doc-tests
>
> Link: https://lore.kernel.org/rust-for-linux/459782fe-afca-4fe6-8ffb-ba7c7886de0a@de.bosch.com/
> Suggested-by: Dirk Behme <dirk.behme@de.bosch.com>
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off: Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>

Reviewed-by: Tamir Duberstein <tamird@gmail.com>

Re: [PATCH v2 3/4] rust: kernel: page: remove unnecessary helper function from doctest

[Daniel Sedlak]

On 11/20/24 10:01 AM, Alice Ryhl wrote:
> On Tue, Nov 19, 2024 at 8:36 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>>
>> Doctest in the `page.rs` contained helper function `dox` which acted
>> as a wrapper for using the `?` operator. However, this is not needed
>> because doctests are implicitly wrapper in function see [1].
>>
>> [1]: https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#using--in-doc-tests
> 
> Normally you'd format this using a `Link: url [1]` tag instead.

Thank you, will incorporate it in the next patch.
> 
>> Link: https://lore.kernel.org/rust-for-linux/459782fe-afca-4fe6-8ffb-ba7c7886de0a@de.bosch.com/
>> Suggested-by: Dirk Behme <dirk.behme@de.bosch.com>
>> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
>> Signed-off: Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2 2/4] rust: kernel: rbtree: remove unwrap in asserts

[Daniel Sedlak]

On 11/20/24 10:02 AM, Alice Ryhl wrote:
> On Tue, Nov 19, 2024 at 8:36 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>>
>> Remove `uwnrap` in asserts and replace it with `Option::Some`
> 
> typo: unwrap
> 
>> matching. By doing it this way, the examples is more
> 
> typo: is -> are
> 
>> -/// use kernel::{alloc::flags, rbtree::RBTree};
>> +/// use kernel::{alloc::flags, rbtree::{RBTree, RBTreeNode}};
> 
> What's up with this change? Is the example broken?

Ops, messed up rebase, it shouldn't be there. However, I am surprised 
that clippy/rustc wasn't complaining about the unused import.
> 
> Alice