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 😄

[lukewilliamboswell]

Progress report

$ 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 )
1774

[rtfeldman]

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.

As a quick note, this should definitely be the preferred solution (even though it takes longer) whenever possible! A lot of the worst user experiences in Roc right now are due to compiler panics (e.g. because they block later steps in the compiler, such as error reporting, from running as normal), so anywhere we can replace a panic with telling the user what went wrong and then continuing, that's what we should do!

[lukewilliamboswell]

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 might be a crazy idea... but could we bulk add a #[ignore(clippy::panic)] etc on all the locations in the codebase, and then enable this in clip... so that we prevent any further regression in this area?

Then the process of fixing this is a matter of replacing the #[ignore(clippy::panic)] etc which is reasonably easy to search for.

[Anton-4]

Could this be simpler: we can execute the count command ($ cargo clean && cargo clippy --no-deps -- -W clippy::unwrap_used ... )) in CI and error if it has increased.

If we use #[ignore(clippy::panic)] in the code, people may be tempted to just copy that

[rtfeldman]

I like that idea, Anton!

We could also have a script which errors if the number decreases, so you can make sure to decrease the count.

So the error messages could be:

• If the number is higher than what we expect, say "hey this increased panics by N; here's what you should do instead of panicking"
• If the number is lower than what we expect, say "hey it looks like this decreased the number of panics, which is great! Can you update this number in this place to the new number, so we don't regress this progress?"

Also, it's a bit unfortunately, but things like unwrap are fine (and useful for conciseness) in tests - but grep doesn't know about the distinction between test code and production code. We could make something fancier to detect that distinction in the future perhaps.