Reduce number of unwrap calls and make clippy warning for it opt-out per crate

[Artxiom]

What

Resolves #3408: Remove all usages of unwrap() and forbid it's use (if applicable).

If applicable because even after the first batch of changes there are over 800(!) usages of unwrap() in the code - for various reasons, and often explicitly allowed. So I think it's probably unrealistic to get rid of all of them, and probably also not necessary.

The strategy:

1. Warn on all usages of unwrap().
2. Remove unwrap() module by module.
3. Where unwrap() would be theoretically okay use expect("error msg").
4. Leave build tools as they are for now - panics there are annoying but not critical (can happen when e.g. some command line tool is missing).
5. For tests and benchmarks unwrap is totally okay imho (allow-unwrap-in-tests was anyway already set in clippy.toml)
6. Where errors are returned combine error types and "?-ify" the code, or use some other idiomatic Rust expression.
7. Goal: eventually deny clippy::unwrap_used everywhere, unless explicitly allowed.

Note: as this is my first PR, early feedback is greatly appreciated🙏
Also - rookie question - what (or where) are the guidelines for the last three items in the checklist? Are they needed for this PR, as this is just a cleanup?

Checklist

• I have read and agree to Contributor Guide and the Code of Conduct
• I've included a screenshot or gif (if applicable)
• The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

• PR Build Summary
• Recent benchmark results
• Wasm size tracking

To run all checks from main, comment on the PR with @rerun-bot full-check.

[Wumpf]

Wow, thank you so much for taking this on, love it! And even made the codegen shorter <3

So I think it's probably unrealistic to get rid of all of them, and probably also not necessary.

Yeah I also think that's more realistic than the aspiration on that ticket. However, in the limit we manage to handle it similar to unsafe code: opt-in where absolutely necessary but generally avoid it.
I.e. 100% agree with the strategy you described there.

Apart from the small questions/nits I had I think we should actually get this out of draft and land it almost as is - doing this in smaller steps reduces merge conflicts, makes it easier to review and gives it more churn sooner.

[Artxiom]

@Wumpf

Thanks a lot!

Apart from the small questions/nits I had I think we should actually get this out of draft and land it almost as is - doing this in smaller steps reduces merge conflicts, makes it easier to review and gives it more churn sooner.

Okay, great - I just have some changes from today that I want to commit first (mostly for tests and some low hanging fruits).

And what about the last checkbox - what do I have to do to meet the conditions?

[Artxiom]

Just one more thing: as this is work in progress - should we set again globally unwrap_used to allow before merging? Else everyone will get masses of lint warnings.

[Wumpf]

Just one more thing: as this is work in progress - should we set again globally unwrap_used to allow before merging? Else everyone will get masses of lint warnings.

The lint warnings will make ci fail (so that if you check out it's always warning free), so leaving them is is not an option. Meaning we either have to opt-out crates that aren't ready or opt-in those that are. I'd prefer having crates to opt-out because new crates then have the new rules from the start, but it ofc means that we have to annotate a lot of them

[Wumpf]

getting there 🙃

[Artxiom]

The lint warnings will make ci fail (so that if you check out it's always warning free), so leaving them is is not an option. Meaning we either have to opt-out crates that aren't ready or opt-in those that are. I'd prefer having crates to opt-out because new crates then have the new rules from the start, but it ofc means that we have to annotate a lot of them

Hmmm, makes sense but also has one disadvantage: we won't know then which ones will opt out because they are not refactored yet and which ones opt out for legit reasons.
Maybe - as a compromise - I can add:

// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

What do you think?...there aren't that many crates that require it.

[Wumpf]

ah yes that's what I meant with opting out 😄 . Using #![allow(clippy::unwrap_used)] they opt out of getting the warning :)

[Wumpf]

"opting out for legit reasons" should almost never happen on crate level anyways and if so we can comment on why :)

[Artxiom]

"opting out for legit reasons" should almost never happen on crate level anyways and if so we can comment on why :)

There are quite a lot of tests/benches that are technically their own crates that need it. But I documented it anyway wherever I encountered it.

[Artxiom]

getting there 🙃

Oooookay...hope now it arrived 😄
Thanks for all the input! I think it's a really good base now to refactor the code step-by-step while new stuff has to be un-unwrap-ered.

[Wumpf]

nice, thanks again!

Still need to get ci green though:

• unwraps where they are no longer allowed: https://github.com/rerun-io/rerun/actions/runs/9080837063/job/24958946682?pr=6311
• pixi run lint-rerun is unhappy:
  https://github.com/rerun-io/rerun/actions/runs/9080837063/job/24958945687?pr=6311

[Wumpf]

something went sideways here or is cmd! internally doing an unwrap?

[Artxiom]

I have fixed hopefully all the linting errors.

something went sideways here or is cmd! internally doing an unwrap?

I'm not sure I understand, does it fail somewhere? I haven't seen anything in the logs.

[Artxiom]

Btw, locally I get another error when running cargo test --quiet --all-targets:
(I added some logs to bench_downcast_first in rerun/crates/re_data_store/benches/arrow2.rs):

bench_downcast_first: re_arrow2::array::fixed_size_list::FixedSizeListArray for kind: Struct
bench_downcast_first: cell:DataCell { inner: DataCellInner { name: "example.MyPoint", size_bytes: 0, values: StructArray[{x: 0, y: 0}] } }
thread 'main' panicked at crates/re_data_store/benches/arrow2.rs:298:30:
called `Option::unwrap()` on a `None` value

I don't know this code well but it seems some dynamic casting goes haywire.
The culprit seems to be this line:
ArrayKind::Struct => bench_downcast_first::<FixedSizeListArray>(&mut group, kind)
shouldn't it be?
ArrayKind::Struct => bench_downcast_first::<StructArray>(&mut group, kind)

...because judging from the logs above generate_cells generates a StructArray for MyPoint.
impl Loggable for MyPointalso uses Struct/StructArray and not FixedSizeListArray.

I don't think this is caused by my changes though - and ideas?

[Wumpf]

does it fail somewhere? I haven't seen anything in the logs.

I was just blind, didn't see the unwrap below. nvm.

Btw, locally I get another error

Hmm strange, also ci doesn't catch this either, it apparently doesn't run this benchmark. Repros for me cleanly on main, so definitely not this changeset here. Cc: @teh-cmc