[[PATCH]] rust/list: replace unwrap() with ? in doctest examples

[[PATCH]] rust/list: replace unwrap() with ? in doctest examples

[Albin Babu Varghese]

Replace panicking `unwrap()` calls in the `kernel::list` doctests with `ok_or(EINVAL)?` so they return a proper `Error` instead of panicking.

Suggested-by: Miguel Ojeda <ojeda@kernel.org>

Link: https://github.com/Rust-for-Linux/linux/issues/1164
Signed-off-by: Albin Babu Varghese <albinbabuvarghese20@gmail.com>
---
 rust/kernel/list.rs | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
index c391c30b80f8..fe58a3920e70 100644
--- a/rust/kernel/list.rs
+++ b/rust/kernel/list.rs
@@ -82,9 +82,9 @@
 /// // [15, 10, 30]
 /// {
 ///     let mut iter = list.iter();
-///     assert_eq!(iter.next().unwrap().value, 15);
-///     assert_eq!(iter.next().unwrap().value, 10);
-///     assert_eq!(iter.next().unwrap().value, 30);
+///     assert_eq!(iter.next().ok_or(EINVAL)?.value, 15);
+///     assert_eq!(iter.next().ok_or(EINVAL)?.value, 10);
+///     assert_eq!(iter.next().ok_or(EINVAL)?.value, 30);
 ///     assert!(iter.next().is_none());
 ///
 ///     // Verify the length of the list.
@@ -93,9 +93,9 @@
 ///
 /// // Pop the items from the list using `pop_back()` and verify the content.
 /// {
-///     assert_eq!(list.pop_back().unwrap().value, 30);
-///     assert_eq!(list.pop_back().unwrap().value, 10);
-///     assert_eq!(list.pop_back().unwrap().value, 15);
+///     assert_eq!(list.pop_back().ok_or(EINVAL)?.value, 30);
+///     assert_eq!(list.pop_back().ok_or(EINVAL)?.value, 10);
+///     assert_eq!(list.pop_back().ok_or(EINVAL)?.value, 15);
 /// }
 ///
 /// // Insert 3 elements using `push_front()`.
@@ -107,9 +107,9 @@
 /// // [30, 10, 15]
 /// {
 ///     let mut iter = list.iter();
-///     assert_eq!(iter.next().unwrap().value, 30);
-///     assert_eq!(iter.next().unwrap().value, 10);
-///     assert_eq!(iter.next().unwrap().value, 15);
+///     assert_eq!(iter.next().ok_or(EINVAL)?.value, 30);
+///     assert_eq!(iter.next().ok_or(EINVAL)?.value, 10);
+///     assert_eq!(iter.next().ok_or(EINVAL)?.value, 15);
 ///     assert!(iter.next().is_none());
 ///
 ///     // Verify the length of the list.
@@ -118,8 +118,8 @@
 ///
 /// // Pop the items from the list using `pop_front()` and verify the content.
 /// {
-///     assert_eq!(list.pop_front().unwrap().value, 30);
-///     assert_eq!(list.pop_front().unwrap().value, 10);
+///     assert_eq!(list.pop_front().ok_or(EINVAL)?.value, 30);
+///     assert_eq!(list.pop_front().ok_or(EINVAL)?.value, 10);
 /// }
 ///
 /// // Push `list2` to `list` through `push_all_back()`.
@@ -135,9 +135,9 @@
 ///     // list: [15, 25, 35]
 ///     // list2: []
 ///     let mut iter = list.iter();
-///     assert_eq!(iter.next().unwrap().value, 15);
-///     assert_eq!(iter.next().unwrap().value, 25);
-///     assert_eq!(iter.next().unwrap().value, 35);
+///     assert_eq!(iter.next().ok_or(EINVAL)?.value, 15);
+///     assert_eq!(iter.next().ok_or(EINVAL)?.value, 25);
+///     assert_eq!(iter.next().ok_or(EINVAL)?.value, 35);
 ///     assert!(iter.next().is_none());
 ///     assert!(list2.is_empty());
 /// }
@@ -809,11 +809,11 @@ fn next(&mut self) -> Option<ArcBorrow<'a, T>> {
 /// merge_sorted(&mut list, list2);
 ///
 /// let mut items = list.into_iter();
-/// assert_eq!(items.next().unwrap().value, 10);
-/// assert_eq!(items.next().unwrap().value, 11);
-/// assert_eq!(items.next().unwrap().value, 12);
-/// assert_eq!(items.next().unwrap().value, 13);
-/// assert_eq!(items.next().unwrap().value, 14);
+/// assert_eq!(items.next().ok_or(EINVAL)?.value, 10);
+/// assert_eq!(items.next().ok_or(EINVAL)?.value, 11);
+/// assert_eq!(items.next().ok_or(EINVAL)?.value, 12);
+/// assert_eq!(items.next().ok_or(EINVAL)?.value, 13);
+/// assert_eq!(items.next().ok_or(EINVAL)?.value, 14);
 /// assert!(items.next().is_none());
 /// # Result::<(), Error>::Ok(())
 /// ```
-- 
2.49.0

Re: [[PATCH]] rust/list: replace unwrap() with ? in doctest examples

[Benno Lossin]

Hi,

Thanks for the patch, a couple comments below.

The subject of your email starts with `[[PATCH]]`, normally we use
`[PATCH]` instead. Did you use `git send-email` and `git format-patch`?

On Mon May 26, 2025 at 12:59 AM CEST, Albin Babu Varghese wrote:
> Replace panicking `unwrap()` calls in the `kernel::list` doctests with `ok_or(EINVAL)?` so they return a proper `Error` instead of panicking.

Commit messages should be wrapped to a sensible column length such as 72
characters.

Also this is missing the motivation that newcomers might get the
impression that calling `unwrap` is OK.

> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
>

No need for blank lines in the taglist.

> Link: https://github.com/Rust-for-Linux/linux/issues/1164
> Signed-off-by: Albin Babu Varghese <albinbabuvarghese20@gmail.com>
> ---
>  rust/kernel/list.rs | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)

The changes themselves look good, if we want to use the `ok_or` + error
combination. I haven't thought about this, so there might be something
better here, but it already is an improvement over the previous one.

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

Re: [[PATCH]] rust/list: replace unwrap() with ? in doctest examples

[Miguel Ojeda]

On Mon, May 26, 2025 at 10:18 AM Benno Lossin <lossin@kernel.org> wrote:
>
> The changes themselves look good, if we want to use the `ok_or` + error
> combination. I haven't thought about this, so there might be something
> better here, but it already is an improvement over the previous one.

Yeah, I thought if perhaps we could have a better method (extension
trait) for these cases, at least for unit tests. On the other hand, if
it is just for unit tests, we are back at using something that is not
"real code", which is what triggered this patch (but at least it would
not panic).

Cheers,
Miguel

Re: [[PATCH]] rust/list: replace unwrap() with ? in doctest examples

[Benno Lossin]

On Mon May 26, 2025 at 12:33 PM CEST, Miguel Ojeda wrote:
> On Mon, May 26, 2025 at 10:18 AM Benno Lossin <lossin@kernel.org> wrote:
>>
>> The changes themselves look good, if we want to use the `ok_or` + error
>> combination. I haven't thought about this, so there might be something
>> better here, but it already is an improvement over the previous one.
>
> Yeah, I thought if perhaps we could have a better method (extension
> trait) for these cases, at least for unit tests. On the other hand, if
> it is just for unit tests, we are back at using something that is not
> "real code", which is what triggered this patch (but at least it would
> not panic).

Having thought about this more, I think this is fine. I don't know which
extension function we would add to make this nicer.

---
Cheers,
Benno

Re: [[PATCH]] rust/list: replace unwrap() with ? in doctest examples

[Albin Babu Varghese]

Thank you, Benno and Miguel, for your feedback

> Having thought about this more, I think this is fine. I don't know which
> extension function we would add to make this nicer.

If the code looks good, I’ll submit a v2 with updates to the subject line, commit
message, description and tag list.

---
Cheers,
Albin

Re: [[PATCH]] rust/list: replace unwrap() with ? in doctest examples

[Albin Babu Varghese]

Thank you, Benno and Miguel, for taking the time to review this patch.

> The subject of your email starts with `[[PATCH]]`, normally we use
> `[PATCH]` instead. Did you use `git send-email` and `git format-patch`?

I apologize for the inconsistency in the subject line. I did use both git
send-email and git format-patch when submitting. I believe the double brackets
were caused by a subject prefix I’d added in my email setup and only noticed
afterward. It’s working as intended now.
 
> Commit messages should be wrapped to a sensible column length such as 72
> characters.

> Also this is missing the motivation that newcomers might get the
> impression that calling `unwrap` is OK.
> 
> > Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> >
> 
> No need for blank lines in the taglist.

I’ll rewrite the commit message to wrap at 72 characters and remove the extra
blank tag lines.

-------
Thanks again,
Albin