Refactor the Exception and Error handling part

attila-lin commented 1 year ago

Hi all,

Currently, I'm working on clean up of deno's codebase. As we all know, Deno focus on security and have already being used on production. Rustlang is good at building a reliable application with a nice error handling.

However, I found that there are some problems with deno's panic, which may cause some people to have less confidence with it. So I wonder if it is possible to improve the stability of deno.

crate `anyhow` may be misused

AFAIK, anyhow is used on application level, while the thiserror is used on library level. They have the same author and work well together.

I think we can move anyhow to thiserror for the library parts like core, runtime, ext.

P.S. We may change cli to lib, too. And add bin for it.

For example:

core

// deno/core/error.rs
pub fn generic_error(message: impl Into<Cow<'static, str>>) -> Error {
  custom_error("Error", message)
}

pub fn type_error(message: impl Into<Cow<'static, str>>) -> Error {
  custom_error("TypeError", message)
}

pub fn range_error(message: impl Into<Cow<'static, str>>) -> Error {
  custom_error("RangeError", message)
}

change to


use thiserror::Error;

#[derive(Debug, Error)]
pub enum CoreError {
  #[error(transparent)]
  Generic(#[from] GenericError),
  #[error(transparent)]
  Type(#[from] TypeError),
  #[error(transparent)]
  Range(#[from] RangeError)
  // ...
}

ops

// deno/runtime/ops/https.rs

  let request = match &mut *rd {
    HttpRequestReader::Headers(request) => request,
    _ => {
      return Err(custom_error(
        "Http",
        "cannot upgrade because request body was used",
      ))
    }
  };

change to

use thiserror::Error;
#[derive(Debug, Error)
pub enum HttpError {
  #[error(cannot upgrade because request body was used")]
  CannotUpgradeBecauseRequestBodyWasUsed,
  // ...
}

// need some refactor for `op` macro
#[op]
async fn op_http_upgrade(
  ..
) -> Result<HttpUpgradeResult, HttpError> {
  ..
}

Due to these changes, we can make the codebase:

more clear: more clear of the detail of all error kinds of the modules and the transform of different errors
better performance: avoid runtime downcast ops like e.downcast_ref::<ImportMapError>()
less code: no need impl Error for
...

Too much `unwrap` and `panic` used

We should treat the use of unwrap and panic more seriously.

The misused unwraps leads to panics. A lot of panic methods could be handled and users receive more clear error message.

Conclusion

The following points are suggested:

[ ] get rid of anyhow with thiserror in the lib crates
[ ] treat unwrap more seriously.
- [ ] deny unwrap with Result or except
[ ] reduce panic with Result or just removed
- [ ] create an Error and return a Result
- [ ] delete unnecessary panic in test case
[ ] more clippy deny:
- [ ] clippy::unwrap_used
- [ ] clippy::unwrap_in_result
- [ ] maybe more?
- [ ] clippy::indexing_slicing
- [ ] ...

It may cause a several PR and many diffs, so I create the issue to see if the deno team is ok with these changes.

Please feel free to contact me if there are some questions or suggestions.

Thank you!

crowlKats commented 1 year ago

regarding less usages of unwrap on Results, what do you suggest to do i scenarios where errors cannot be propagated? Do you plan to replace all those cases with expect?

attila-lin commented 1 year ago

regarding less usages of unwrap on Results, what do you suggest to do i scenarios where errors cannot be propagated? Do you plan to replace all those cases with expect?

It depends.

I will try to move to Result it may happen at first.

If we're quite sure the condition will never happen, we can replace it with expect, which will provide a better error message.

Please look at this case. We are sure pending_promise_exceptions is not empty, so we can leave the unwrap to expect("pending_promise_exceptions is not empty").(I will prefer to change it to if let Some() type in this case to avoid the double check)

dsherret commented 1 year ago

We want to use thiserror everywhere except the CLI crate. We discussed that internally a while ago and a PR for this would be welcomed. Many upstream crates would need to be updated though. For example: https://github.com/denoland/deno_ast/issues/118

On unwraps, I don’t think we should do this because unwrapping can be useful in many scenarios (for example, for surfacing scenarios that shouldn’t happen and if they do then that indicates something wrong, or when the state dictates a panic won’t happen—ex. a file path will always have a parent, so unwrapping to get the parent is ok because panicking will never occur). Surfacing all unwraps as errors would often be wrong and if not, then we should remove those specific unwraps/panics. Also, we recently migrated away from expect to unwrap because expect is a maintenance burden that doesn’t help that much—an unwrap panicking indicates a bug that should be reported and having the location of the panic that’s automatically provided has been sufficient.

denoland / deno