Re: [PATCH v2 2/5] rust: maple_tree: add MapleTree

["Danilo Krummrich" <dakr@kernel.org>]

On Fri Aug 22, 2025 at 11:22 PM CEST, Miguel Ojeda wrote:
> As for being clearer, what we want to assert is that the cause is a
> given one, so `assert_eq!` seems like a natural choice (and it isn't a
> case like `is_none()` where there is an advantage). Plus it means it
> is able to show both sides if it fails. So it is not a clear win-win
> in my eyes.

As for

	assert_eq!(foo().unwrap_err().kind(), ErrorKind::NotFound);

vs.

	assert!(foo().is_err_and(|e| x.kind() == ErrorKind::NotFound));

the only thing I can think of is that the former fails differently for when
foo() is Ok() vs. the error kind does not match. I assume that's what you mean?

If so, I agree it's indeed a downside.

>> But especially people new to the kernel starting to write Rust drivers may not
>> even be aware of this fact. If they see some unwrap_err() calls in examples they
>> might not even thing about it a lot before starting to use them, e.g. because
>> they're used to it from userspace projects anyways.
>
> We still have the issue that they will see the `assert!` anyway and
> thus can easily think panics are OK. I understand what you are saying:
> you want to minimize those cases anyway.

Yeah, exactly. Another reason I'm less concernt about the assert!() is that I
think it's generally more more associated with test cases than unwrap(). But
this one might also be just my perception. :)

>> I think we should do this; I really think otherwise we gonna see a lot of them
>> once we get more drivers. It's just too convinient. :)
>
> A potential middle ground is to apply the lint in normal code, but not
> in examples.
>
> Or, even better, we can actually only allow it within `assert!`s,
> since it is a custom macro I wrote for KUnit and not the real one,
> i.e. enforcing what I suggested above [1].

I think this would be a great solution; thanks for pointing this out.

> Another one is a lint that just affects `unwrap()` and not
> `unwrap_err()` -- I think the Clippy one doesn't allow it now, but we
> could request it. It could be combined with the above so that only
> `unwrap_err()` is allowed within `assert!`s.
>
> We could also go the C way, warning in `checkpatch.pl` about it like
> it does `BUG_ON`.

I think your suggestion above is perfect, whereas I think this one is likely to
still slip through.

> What I like about the Clippy approach is that it can be locally `expect`ed.

Agreed!