roc-lang / roc

A fast, friendly, functional language.
https://roc-lang.org
Universal Permissive License v1.0
4.41k stars 310 forks source link

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

Open bhansconnect opened 2 years ago

bhansconnect commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

bhansconnect commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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: https://github.com/rtfeldman/roc/issues/2130

matssigge commented 2 years ago

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 commented 2 years ago

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:

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

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

lukewilliamboswell commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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:

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.