Forbid unwrap() in our code
#3408
Labels
😤 annoying
Something in the UI / SDK is annoying to use
good first issue
Good for newcomers
help wanted
Extra attention is needed
Comments
3 tasks
4 tasks
emilk
added a commit
that referenced
this issue
May 7, 2024
Found with `scripts/fetch_crashes.py` * See #3408
Wumpf
pushed a commit
that referenced
this issue
May 8, 2024
Remove an `unwrap()` that sometimes caused crashes. Found with `scripts/fetch_crashes.py` (analytics) * See #3408 ### 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] I have tested the web demo (if applicable): * Using examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6251?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6251?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! - [PR Build Summary](https://build.rerun.io/pr/6251) - [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`.
5 tasks
jleibs
pushed a commit
that referenced
this issue
May 10, 2024
* Part of #3408 ### 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] I have tested the web demo (if applicable): * Using examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6285?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/6285?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! - [PR Build Summary](https://build.rerun.io/pr/6285) - [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`.
3 tasks
Wumpf
pushed a commit
that referenced
this issue
May 15, 2024
…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`.
This was referenced May 15, 2024
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
All
unwrap()s will eventually cause a panic. For example:If you can prove the
unwrap()cannot happen, then you can often rewrite it in a way to avoid theunwrap(). If you can't: add a comment explaining why it is impossible and/or replace it withunreachable!().I suggest we turn on
clippy::unwrap_usedto enforce this.Right now there are too many
unwrap()s (~350) to make this feasible, and that scares me. Those are crashes waiting to happen.So the first step is to start removing
unwrapsin our code.#[allow(clippy::unwrap_used)]or use?instead.?instead.?or.unwrap_or_warnor similar.We should also:
try_prefixes of functionsWe can start working on this on a per-crate basis, e.g. adding
#![warn(clippy::unwrap_used)]tocrates/re_types/src/lib.rsfirst, and then take it from there.The text was updated successfully, but these errors were encountered: