Remove all uses of panic!, unwrap(), expect(...), and unreachable!()

[bhansconnect]

We want to use our new error macros instead of panic!, unwrap(), expect(...), and unreachable!().

The new error macros both live in roc_reporting and can be imported with use roc_reporting:{internal_error, user_error}.
Essentially, all cases were the panic!/unwrap()/expect(...) would be a sign of a compiler bug, we want to use internal_error! instead. user_error! should be used in cases where we eventually want to use roc_reporting to print a pretty error message. It is essentially documentation that eventually better errors should be return there. todo! should be used for cases were the feature is planned to be implemented but is not yet implemented. unimplemented! should be used if the feature is not implemented, and there is not a current plan to implement the feature.

Uses of unreachable!() should always be replaced with internal_error!. If possible, a message around why the statement is unreachable/what invariant was broken, should be added.

This issue will be fixed when we can enable clippy::panic, clippy::unwrap_used, clippy::expect_used, and clippy::unreachable on CI by default.
This command can be used to track how many use cases are left:

cargo clean && cargo clippy --no-deps -- -W clippy::unwrap_used -W clippy::expect_used -W clippy::panic -W clippy::unreachable 2> >(grep -e "warning: \`panic\`" -e "warning: used" -e "warning: usage of" | wc -l )

At the date of creating this issue, we have 1283 cases to go.

[bhansconnect]

As an extra notes, in many cases, it may also be fine to replace panic!, unwrap(), and expect(...) with a ? or returning None/Err(...). These are ways of propagating the error up to the calling function. This should be done whenever it is reasonable for the calling function to expect and handle an option or result. If the calling function couldn't cleanly handle and resolve the issues, please stick to other solutions.

[mdevlamynck]

Should we also add unreachable!() to that list?

[bhansconnect]

Yes, thanks for the catch. unreachable!() should also be removed. It should be replaced by internal_error!(...) with a reason explaining why it should be unreachable or other helpful information. Updated the issue to reflect this.

[satotake]

I suspect that this is a long way.
How about adding some checks to CI, like coverage reporting? (though this might be too much)

[brian-carroll]

I've applied the new panic macros in the main part of the compiler that I work on. But in other parts it's currently blocked by circular dependencies.

The macros are defined in roc_reporting, which depends on roc_mono, which means roc_mono can't use them without creating a circular dependency.

The same thing applies to all internal dependencies of roc_reporting:

• roc_collections
• roc_region
• roc_module
• roc_parse
• roc_problem
• roc_types
• roc_can
• roc_solve
• roc_mono
• roc_constrain
• roc_builtins

If we want to use the macros in these places then we will need to move them somewhere else. (Or maybe we're OK with that? I don't know.)

I ran the command below to find Cargo.toml files that don't have lines beginning with roc_ . Most of them look unsuitable. Maybe utils is as good a place as any?

$ for f in $(find . -name Cargo.toml)
do
  if ! grep -q '^roc_' $f
  then echo $f
  fi
done
./roc_std/Cargo.toml
./Cargo.toml
./utils/Cargo.toml
./test_utils/Cargo.toml
./nightly_benches/Cargo.toml
./ci/bench-runner/Cargo.toml
./compiler/arena_pool/Cargo.toml
./compiler/parse/fuzz/Cargo.toml
./compiler/region/Cargo.toml
./compiler/test_mono_macros/Cargo.toml
./compiler/ident/Cargo.toml
./compiler/collections/Cargo.toml
./vendor/inkwell/Cargo.toml
./vendor/pretty/Cargo.toml
./vendor/ena/Cargo.toml
./vendor/morphic_lib/Cargo.toml

[bhansconnect]

We want to move them into their own crate to fix this. There is already an issue for it, just hasn't been done yet: #2130

[matssigge]

#2130 is now fixed, and I thought I would try to help out with this one, but there are lots of places where panic/unwrap/expect/unreachable are still used, and I don't want to create unnecessary merge ambushes. Are there specific places in the code that are best avoided/left to those who actively work on them?

[rtfeldman]

Are there specific places in the code that are best avoided/left to those who actively work on them?

I think these would be the least likely to have conflicts right now:

• compiler/ident
• compiler/region
• compiler/collections
• compiler/module
• compiler/parse
• compiler/can
• compiler/problem
• compiler/builtins
• compiler/fmt
• compiler/load
• compiler/gen_dev
• compiler/build
• compiler/arena_pool
• compiler/test_gen
• editor
• ast
• cli
• code_markup
• reporting
• roc_std
• test_utils
• utils
• docs
• linker

...and these would be more likely to have conflicts:

• compiler/constrain
• compiler/unify
• compiler/solve
• compiler/types
• compiler/mono
• compiler/test_mono
• compiler/gen_wasm
• compiler/gen_llvm

I'd also avoid anything in the vendor/ directory since those are all forks of libraries that we tweaked for our own needs 😄