resolve clippy::unwrap_used in re_viewer #8745
Conversation
Wumpf
added
🚜 refactor
brody4hire
force-pushed
the
resolve-clippy-unwrap_used-in-re_viewer
branch
from
41931d1 to
e0d4ff3
Compare
brody4hire
force-pushed
the
resolve-clippy-unwrap_used-in-re_viewer
branch
from
a719ec0 to
fd9e54a
Compare
There was a problem hiding this comment.
@Wumpf I have shamelessly thrown away your cleanup work in favor of using .expect("... (internal error)") calls (I am assuming no offense will be taken). I think this should both reduce the number of lines in most cases and clarify the code a little better.
I think this should be ready now. Please do feel free to apply any other cleanup you think may be needed & be sure to add yourself as a co-author.
crates/viewer/re_viewer/src/ui/welcome_screen/welcome_section.rs
Outdated
Show resolved
Hide resolved
| @@ -89,7 +89,7 @@ impl Screenshotter { | |||
| let h = image.height() as _; | |||
| let image = | |||
| image::RgbaImage::from_raw(w, h, bytemuck::pod_collect_to_vec(&image.pixels)) | |||
| .expect("Failed to create image"); | |||
| .expect("Failed to create image (internal error)"); | |||
There was a problem hiding this comment.
I like the "internal error" message for our users, but I think we should add that to the panic handler of re_crash_handler instead, since any panic is an internal error.
There was a problem hiding this comment.
I would agree in most but not all cases, I think this points to a counter-example as I commented below: https://github.com/rerun-io/rerun/pull/8745/files#r1928954634
There was a problem hiding this comment.
How do you think we should proceed with this?
There was a problem hiding this comment.
A panic should only happen because of a bug in the rerun codebase, which is what I would call an internal error. If there are panics that could happen because of user error, we should fix that separately.
brody4hire
force-pushed
the
resolve-clippy-unwrap_used-in-re_viewer
branch
from
a1df8c2 to
82a262c
Compare
| Ok(any) => Some(*any.downcast::<T>().unwrap()), | ||
| Ok(any) => Some( | ||
| *any.downcast::<T>() | ||
| .unwrap_or_else(|err| panic!("downcast failure: {err:?}")), |
There was a problem hiding this comment.
I think this is an example where a panic is triggered not by an internal error but by an issue with the calling code.
There was a problem hiding this comment.
I am thinking it may be ideal to update the API to return a promise or result with a promise so that the calling code can poll it without the need for this downcast. We could add a separate API function to do this without breaking any API users. WDYT?
There was a problem hiding this comment.
The calling code is still Rerun code, so any panic here is still an error internal to the Rerun application (as opposed to a user error).
Related
Part of: #6330
What
crates/viewer/re_viewer/src/lib.rs. unwrap_or_elsewith panic in 1 case#[allow(clippy::unwrap_used)]directives in limited, focused cases.expectwith descriptive messages in all other cases(no changelog updates)
General rationale:
Most of the cases using
.unwrapseem to need these to work. I think the best solution is to use.expectwith a clear message in most of these cases.Sanity checks done:
These succeed for me with no warnings:
cargo clippy -p re_viewer --all-featurescargo build -p re_viewer --all-features