Skip to content

Commit

Permalink
Reduce number of unwrap calls and make clippy warning for it opt-out …
Browse files Browse the repository at this point in the history
…per crate (#6311)

<!--
Open the PR up as a draft until you feel it is ready for a proper
review.

Do not make PR:s from your own `main` branch, as that makes it difficult
for reviewers to add their own fixes.

Add any improvements to the branch as new commits to make it easier for
reviewers to follow the progress. All commits will be squashed to a
single commit once the PR is merged into `main`.

Make sure you mention any issues that this PR closes in the description,
as well as any other related issues.

To get an auto-generated PR description you can put "copilot:summary" or
"copilot:walkthrough" anywhere.
-->

### 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
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/6311)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
  • Loading branch information
Artxiom authored May 15, 2024
1 parent 3805781 commit cd31f9b
Show file tree
Hide file tree
Showing 139 changed files with 532 additions and 316 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ missing_errors_doc = "allow"
# This would be nice to enable, but we have way too many unwraps right now 😭
# Enabling this lint in 2023-04-27 yielded 556 hits.
# Enabling this lint in 2023-09-23 yielded 352 hits
unwrap_used = "allow"
unwrap_used = "warn"

[patch.crates-io]
# Try to avoid patching crates! It prevents us from publishing the crates on crates.io.
Expand Down
6 changes: 3 additions & 3 deletions crates/re_analytics/examples/end_to_end.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@ use re_analytics::Event;
use re_analytics::Properties;
use re_analytics::{Analytics, AnalyticsEvent};

fn main() -> ! {
fn main() -> Result<(), Box<dyn std::error::Error>> {
re_log::setup_logging();

let analytics = Analytics::new(Duration::from_secs(3)).unwrap();
let analytics = Analytics::new(Duration::from_secs(3))?;
let application_id = "end_to_end_example".to_owned();
let recording_id = uuid::Uuid::new_v4().to_string();

println!("any non-empty line written here will be sent as an analytics datapoint");
loop {
let mut input = String::new();
std::io::stdin().read_line(&mut input).unwrap();
std::io::stdin().read_line(&mut input)?;

let input = input.trim();
if !input.is_empty() {
Expand Down
4 changes: 3 additions & 1 deletion crates/re_analytics/src/native/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,9 @@ impl Config {
}

pub fn config_dir(&self) -> &Path {
self.config_file_path.parent().unwrap()
self.config_file_path
.parent()
.expect("config file has no parent")
}

pub fn config_file(&self) -> &Path {
Expand Down
4 changes: 2 additions & 2 deletions crates/re_build_info/src/build_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,8 @@ fn crate_version_from_build_info_string() {

{
let expected_crate_version = build_info.version;
let crate_version = CrateVersion::try_parse_from_build_info_string(build_info_str).unwrap();
let crate_version = CrateVersion::try_parse_from_build_info_string(build_info_str);

assert_eq!(expected_crate_version, crate_version);
assert_eq!(Ok(expected_crate_version), crate_version);
}
}
2 changes: 1 addition & 1 deletion crates/re_build_tools/src/hashing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::{rerun_if_changed, rerun_if_changed_or_doesnt_exist};
fn encode_hex(bytes: &[u8]) -> String {
let mut s = String::with_capacity(bytes.len() * 2);
for &b in bytes {
write!(&mut s, "{b:02x}").unwrap();
write!(&mut s, "{b:02x}").expect("writing to string should never fail");
}
s
}
Expand Down
3 changes: 3 additions & 0 deletions crates/re_data_store/benches/arrow2.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Keeping track of performance issues/regressions in `arrow2` that directly affect us.
// Allow unwrap() in benchmarks
#![allow(clippy::unwrap_used)]

#[global_allocator]
static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc;

Expand Down
3 changes: 3 additions & 0 deletions crates/re_data_store/benches/data_store.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Allow unwrap() in benchmarks
#![allow(clippy::unwrap_used)]

#[global_allocator]
static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc;

Expand Down
3 changes: 3 additions & 0 deletions crates/re_data_store/benches/gc.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Allow unwrap() in benchmarks
#![allow(clippy::unwrap_used)]

#[global_allocator]
static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc;

Expand Down
3 changes: 3 additions & 0 deletions crates/re_data_store/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
#![doc = document_features::document_features!()]
//!
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

mod arrow_util;
mod store;
mod store_arrow;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_data_store/tests/correctness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
//!
//! Bending and twisting the datastore APIs in all kinds of weird ways to try and break them.
// https://github.com/rust-lang/rust-clippy/issues/10011
#![cfg(test)]

use rand::Rng;

use re_data_store::{
Expand Down
3 changes: 3 additions & 0 deletions crates/re_data_store/tests/data_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
//!
//! Testing & demonstrating expected usage of the datastore APIs, no funny stuff.
// https://github.com/rust-lang/rust-clippy/issues/10011
#![cfg(test)]

use itertools::Itertools;
use rand::Rng;
use re_data_store::{
Expand Down
3 changes: 3 additions & 0 deletions crates/re_data_store/tests/dump.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Dumping a datastore to log messages and back.
// https://github.com/rust-lang/rust-clippy/issues/10011
#![cfg(test)]

use itertools::Itertools;
use re_data_store::{
test_row,
Expand Down
3 changes: 3 additions & 0 deletions crates/re_data_store/tests/internals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
//!
//! They're awful, but sometimes you just have to…
// https://github.com/rust-lang/rust-clippy/issues/10011
#![cfg(test)]

use re_data_store::{DataStore, DataStoreConfig};
use re_log_types::{
build_frame_nr, example_components::MyIndex, DataRow, EntityPath, RowId, TimePoint,
Expand Down
3 changes: 3 additions & 0 deletions crates/re_data_store/tests/memory_test.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Measures the memory overhead of the data store.
// https://github.com/rust-lang/rust-clippy/issues/10011
#![cfg(test)]

use std::sync::atomic::{AtomicUsize, Ordering::Relaxed};

thread_local! {
Expand Down
3 changes: 3 additions & 0 deletions crates/re_dev_tools/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
//!
//! To get an overview over all tools run `pixi run dev-tools --help`.
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

use argh::FromArgs;

mod build_examples;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_entity_db/examples/memory_usage.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

use std::sync::atomic::{AtomicUsize, Ordering::Relaxed};

thread_local! {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions crates/re_entity_db/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#![doc = document_features::document_features!()]
//!
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

pub mod entity_db;
pub mod entity_properties;
pub mod entity_tree;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_entity_db/tests/clear.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// https://github.com/rust-lang/rust-clippy/issues/10011
#![cfg(test)]

use re_data_store::{DataStore, LatestAtQuery};
use re_entity_db::EntityDb;
use re_log_types::{
Expand Down
3 changes: 3 additions & 0 deletions crates/re_entity_db/tests/time_histograms.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// https://github.com/rust-lang/rust-clippy/issues/10011
#![cfg(test)]

use std::collections::BTreeSet;

use re_data_store::GarbageCollectionOptions;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_format_arrow/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Formatting for tables of Arrow arrays
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

use std::fmt::Formatter;

use arrow2::{
Expand Down
3 changes: 3 additions & 0 deletions crates/re_log_encoding/benches/msg_encode_benchmark.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// Allow unwrap() in benchmarks
#![allow(clippy::unwrap_used)]

#[cfg(not(all(feature = "decoder", feature = "encoder")))]
compile_error!("msg_encode_benchmark requires 'decoder' and 'encoder' features.");

Expand Down
8 changes: 5 additions & 3 deletions crates/re_log_encoding/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,14 @@ impl FileHeader {

#[cfg(feature = "decoder")]
pub fn decode(read: &mut impl std::io::Read) -> Result<Self, decoder::DecodeError> {
let to_array_4b = |slice: &[u8]| slice.try_into().expect("always returns an Ok() variant");

let mut buffer = [0_u8; Self::SIZE];
read.read_exact(&mut buffer)
.map_err(decoder::DecodeError::Read)?;
let magic = buffer[0..4].try_into().unwrap();
let version = buffer[4..8].try_into().unwrap();
let options = EncodingOptions::from_bytes(buffer[8..].try_into().unwrap())?;
let magic = to_array_4b(&buffer[0..4]);
let version = to_array_4b(&buffer[4..8]);
let options = EncodingOptions::from_bytes(to_array_4b(&buffer[8..]))?;
Ok(Self {
magic,
version,
Expand Down
4 changes: 4 additions & 0 deletions crates/re_log_encoding/src/stream_rrd_from_http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ pub fn stream_rrd_from_http(url: String, on_msg: Arc<HttpMessageCallback>) {
}

#[cfg(target_arch = "wasm32")]
// TODO(#3408): remove unwrap()
#[allow(clippy::unwrap_used)]
mod web_event_listener {
use super::HttpMessageCallback;
use js_sys::Uint8Array;
Expand Down Expand Up @@ -157,6 +159,8 @@ mod web_event_listener {
pub use web_event_listener::stream_rrd_from_event_listener;

#[cfg(target_arch = "wasm32")]
// TODO(#3408): remove unwrap()
#[allow(clippy::unwrap_used)]
pub mod web_decode {
use super::{HttpMessage, HttpMessageCallback};
use std::sync::Arc;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_log_types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
//! e.g. the entity `foo/bar/baz` is has the transform that is the product of
//! `foo.transform * foo/bar.transform * foo/bar/baz.transform`.
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

pub mod arrow_msg;
pub mod example_components;
pub mod hash;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_query/benches/latest_at.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
//! Contains:
//! - A 1:1 port of the benchmarks in `crates/re_query/benches/query_benchmarks.rs`, with caching enabled.
// Allow unwrap() in benchmarks
#![allow(clippy::unwrap_used)]

use criterion::{criterion_group, criterion_main, Criterion};

use itertools::Itertools;
Expand Down
6 changes: 4 additions & 2 deletions crates/re_query/src/latest_at/results.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,8 @@ fn downcast<C: Component>(cached: &(dyn ErasedFlatVecDeque + Send + Sync)) -> cr
if cached.num_entries() != 1 {
return Err(anyhow::anyhow!("latest_at deque must be single entry").into());
}
// unwrap checked just above ^^^
Ok(cached.iter().next().unwrap())
Ok(cached
.iter()
.next()
.expect("checked existence of cached value already"))
}
3 changes: 3 additions & 0 deletions crates/re_query/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Caching datastructures for `re_query`.
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

mod cache;
mod cache_stats;
mod flat_vec_deque;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_query/tests/latest_at.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// https://github.com/rust-lang/rust-clippy/issues/10011
#![cfg(test)]

use re_data_store::{DataStore, LatestAtQuery, StoreSubscriber};
use re_log_types::{
build_frame_nr,
Expand Down
3 changes: 3 additions & 0 deletions crates/re_query/tests/range.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// https://github.com/rust-lang/rust-clippy/issues/10011
#![cfg(test)]

use itertools::Itertools as _;

use re_data_store::{DataStore, RangeQuery, ResolvedTimeRange, StoreSubscriber as _, TimeInt};
Expand Down
3 changes: 3 additions & 0 deletions crates/re_renderer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
#![doc = document_features::document_features!()]
//!
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

pub mod config;
pub mod importer;
pub mod mesh;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_renderer_examples/2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
//!
//! On the left is a 2D view, on the right a 3D view of the same scene.
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

use itertools::Itertools as _;
use re_renderer::Hsva;

Expand Down
3 changes: 3 additions & 0 deletions crates/re_renderer_examples/depth_cloud.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
//! cargo run-wasm --example depth_cloud
//! ```
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

use std::f32::consts::TAU;

use glam::Vec3;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_renderer_examples/depth_offset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
//! Press arrow up/down to increase/decrease the distance of the camera to the z==0 plane in tandem with the scale of the rectangles.
//! Press arrow left/right to increase/decrease the near plane distance.
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

use re_renderer::Hsva;
use re_renderer::{
renderer::{ColormappedTexture, RectangleDrawData, RectangleOptions, TexturedRect},
Expand Down
3 changes: 3 additions & 0 deletions crates/re_renderer_examples/framework.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Example framework
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

use std::sync::Arc;

use anyhow::Context as _;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_renderer_examples/multiview.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Example with several independent views, using various primitives.
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

use std::f32::consts::TAU;

use framework::Example;
Expand Down
3 changes: 3 additions & 0 deletions crates/re_renderer_examples/outlines.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Demonstrates outline rendering.
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

use itertools::Itertools;
use re_renderer::{
renderer::MeshInstance,
Expand Down
3 changes: 3 additions & 0 deletions crates/re_renderer_examples/picking.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
//! Demonstrates the dedicated picking layer support.
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]

use itertools::Itertools as _;
use rand::Rng;
use re_renderer::{
Expand Down
2 changes: 2 additions & 0 deletions crates/re_sdk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
#![doc = document_features::document_features!()]
//!
// TODO(#3408): remove unwrap()
#![allow(clippy::unwrap_used)]
#![warn(missing_docs)] // Let's keep the this crate well-documented!

// ----------------
Expand Down
Loading

0 comments on commit cd31f9b

Please sign in to comment.