From c90036ab88bb1ae6b9c87d5e368f56d8c8afab69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Mon, 20 Jan 2020 16:50:16 +0100 Subject: [PATCH] refactor: reduce number of ErrorKind variants (#3662) --- Cargo.lock | 2 - cli/Cargo.toml | 2 - cli/deno_error.rs | 98 +++--------------------------- cli/file_fetcher.rs | 10 +-- cli/fs.rs | 4 +- cli/js/errors.ts | 49 ++++----------- cli/js/fetch_test.ts | 8 +-- cli/js/lib.deno_runtime.d.ts | 45 ++++---------- cli/js/permissions_test.ts | 5 +- cli/js/streams/shared-internals.ts | 6 +- cli/msg.rs | 45 +++----------- cli/ops/permissions.rs | 4 +- cli/permissions.rs | 7 +-- 13 files changed, 61 insertions(+), 224 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 997b3d6ca9..9d288be2d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -265,8 +265,6 @@ dependencies = [ "futures 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "fwdansi 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "http 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", - "hyper 0.13.1 (registry+https://github.com/rust-lang/crates.io-index)", - "hyper-rustls 0.19.0 (registry+https://github.com/rust-lang/crates.io-index)", "indexmap 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "libc 0.2.66 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 3ca0857e02..a8f6e4c61d 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -36,8 +36,6 @@ dirs = "2.0.2" dlopen = "0.1.8" futures = { version = "0.3.1", features = [ "compat", "io-compat" ] } http = "0.2.0" -hyper = "0.13.1" -hyper-rustls = "0.19.0" indexmap = "1.3.0" lazy_static = "1.4.0" libc = "0.2.66" diff --git a/cli/deno_error.rs b/cli/deno_error.rs index 80ee218eee..0e1a8d9cf3 100644 --- a/cli/deno_error.rs +++ b/cli/deno_error.rs @@ -7,8 +7,6 @@ use deno_core::AnyError; use deno_core::ErrBox; use deno_core::ModuleResolutionError; use dlopen::Error as DlopenError; -use http::uri; -use hyper; use reqwest; use rustyline::error::ReadlineError; use std; @@ -77,33 +75,16 @@ pub fn permission_denied_msg(msg: String) -> ErrBox { DenoError::new(ErrorKind::PermissionDenied, msg).into() } -pub fn op_not_implemented() -> ErrBox { - StaticError(ErrorKind::OpNotAvailable, "op not implemented").into() -} - pub fn no_buffer_specified() -> ErrBox { StaticError(ErrorKind::InvalidInput, "no buffer specified").into() } -pub fn no_async_support() -> ErrBox { - StaticError(ErrorKind::NoAsyncSupport, "op doesn't support async calls") - .into() -} - -pub fn no_sync_support() -> ErrBox { - StaticError(ErrorKind::NoSyncSupport, "op doesn't support sync calls").into() -} - pub fn invalid_address_syntax() -> ErrBox { StaticError(ErrorKind::InvalidInput, "invalid address syntax").into() } -pub fn too_many_redirects() -> ErrBox { - StaticError(ErrorKind::TooManyRedirects, "too many redirects").into() -} - -pub fn type_error(msg: String) -> ErrBox { - DenoError::new(ErrorKind::TypeError, msg).into() +pub fn other_error(msg: String) -> ErrBox { + DenoError::new(ErrorKind::Other, msg).into() } pub trait GetErrorKind { @@ -136,7 +117,7 @@ impl GetErrorKind for Diagnostic { impl GetErrorKind for ImportMapError { fn kind(&self) -> ErrorKind { - ErrorKind::ImportMapError + ErrorKind::Other } } @@ -187,44 +168,9 @@ impl GetErrorKind for io::Error { } } -impl GetErrorKind for uri::InvalidUri { - fn kind(&self) -> ErrorKind { - // The http::uri::ErrorKind exists and is similar to url::ParseError. - // However it is also private, so we can't get any details out. - ErrorKind::InvalidUri - } -} - impl GetErrorKind for url::ParseError { fn kind(&self) -> ErrorKind { - use url::ParseError::*; - match self { - EmptyHost => ErrorKind::EmptyHost, - IdnaError => ErrorKind::IdnaError, - InvalidDomainCharacter => ErrorKind::InvalidDomainCharacter, - InvalidIpv4Address => ErrorKind::InvalidIpv4Address, - InvalidIpv6Address => ErrorKind::InvalidIpv6Address, - InvalidPort => ErrorKind::InvalidPort, - Overflow => ErrorKind::Overflow, - RelativeUrlWithCannotBeABaseBase => { - ErrorKind::RelativeUrlWithCannotBeABaseBase - } - RelativeUrlWithoutBase => ErrorKind::RelativeUrlWithoutBase, - SetHostOnCannotBeABaseUrl => ErrorKind::SetHostOnCannotBeABaseUrl, - _ => ErrorKind::Other, - } - } -} - -impl GetErrorKind for hyper::Error { - fn kind(&self) -> ErrorKind { - match self { - e if e.is_canceled() => ErrorKind::HttpCanceled, - e if e.is_closed() => ErrorKind::HttpClosed, - e if e.is_parse() => ErrorKind::HttpParse, - e if e.is_user() => ErrorKind::HttpUser, - _ => ErrorKind::HttpOther, - } + ErrorKind::UrlParse } } @@ -234,7 +180,6 @@ impl GetErrorKind for reqwest::Error { match self.source() { Some(err_ref) => None - .or_else(|| err_ref.downcast_ref::().map(Get::kind)) .or_else(|| err_ref.downcast_ref::().map(Get::kind)) .or_else(|| err_ref.downcast_ref::().map(Get::kind)) .or_else(|| { @@ -242,8 +187,8 @@ impl GetErrorKind for reqwest::Error { .downcast_ref::() .map(Get::kind) }) - .unwrap_or_else(|| ErrorKind::HttpOther), - None => ErrorKind::HttpOther, + .unwrap_or_else(|| ErrorKind::Http), + None => ErrorKind::Http, } } } @@ -324,14 +269,12 @@ impl GetErrorKind for dyn AnyError { None .or_else(|| self.downcast_ref::().map(Get::kind)) .or_else(|| self.downcast_ref::().map(Get::kind)) - .or_else(|| self.downcast_ref::().map(Get::kind)) .or_else(|| self.downcast_ref::().map(Get::kind)) .or_else(|| self.downcast_ref::().map(Get::kind)) .or_else(|| self.downcast_ref::().map(Get::kind)) .or_else(|| self.downcast_ref::().map(Get::kind)) .or_else(|| self.downcast_ref::().map(Get::kind)) .or_else(|| self.downcast_ref::().map(Get::kind)) - .or_else(|| self.downcast_ref::().map(Get::kind)) .or_else(|| self.downcast_ref::().map(Get::kind)) .or_else(|| self.downcast_ref::().map(Get::kind)) .or_else(|| self.downcast_ref::().map(Get::kind)) @@ -456,8 +399,8 @@ mod tests { #[test] fn test_simple_error() { let err = - ErrBox::from(DenoError::new(ErrorKind::NoError, "foo".to_string())); - assert_eq!(err.kind(), ErrorKind::NoError); + ErrBox::from(DenoError::new(ErrorKind::NotFound, "foo".to_string())); + assert_eq!(err.kind(), ErrorKind::NotFound); assert_eq!(err.to_string(), "foo"); } @@ -471,7 +414,7 @@ mod tests { #[test] fn test_url_error() { let err = ErrBox::from(url_error()); - assert_eq!(err.kind(), ErrorKind::EmptyHost); + assert_eq!(err.kind(), ErrorKind::UrlParse); assert_eq!(err.to_string(), "empty host"); } @@ -494,7 +437,7 @@ mod tests { #[test] fn test_import_map_error() { let err = ErrBox::from(import_map_error()); - assert_eq!(err.kind(), ErrorKind::ImportMapError); + assert_eq!(err.kind(), ErrorKind::Other); assert_eq!(err.to_string(), "an import map error"); } @@ -520,31 +463,10 @@ mod tests { assert_eq!(err.to_string(), "run again with the --allow-net flag"); } - #[test] - fn test_op_not_implemented() { - let err = op_not_implemented(); - assert_eq!(err.kind(), ErrorKind::OpNotAvailable); - assert_eq!(err.to_string(), "op not implemented"); - } - #[test] fn test_no_buffer_specified() { let err = no_buffer_specified(); assert_eq!(err.kind(), ErrorKind::InvalidInput); assert_eq!(err.to_string(), "no buffer specified"); } - - #[test] - fn test_no_async_support() { - let err = no_async_support(); - assert_eq!(err.kind(), ErrorKind::NoAsyncSupport); - assert_eq!(err.to_string(), "op doesn't support async calls"); - } - - #[test] - fn test_no_sync_support() { - let err = no_sync_support(); - assert_eq!(err.kind(), ErrorKind::NoSyncSupport); - assert_eq!(err.to_string(), "op doesn't support sync calls"); - } } diff --git a/cli/file_fetcher.rs b/cli/file_fetcher.rs index 03b4a9d025..18c9cbf8a9 100644 --- a/cli/file_fetcher.rs +++ b/cli/file_fetcher.rs @@ -1,5 +1,4 @@ // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license. -use crate::deno_error::too_many_redirects; use crate::deno_error::DenoError; use crate::deno_error::ErrorKind; use crate::deno_error::GetErrorKind; @@ -107,7 +106,7 @@ impl SourceFileFetcher { if !SUPPORTED_URL_SCHEMES.contains(&url.scheme()) { return Err( DenoError::new( - ErrorKind::UnsupportedFetchScheme, + ErrorKind::Other, format!("Unsupported scheme \"{}\" for module \"{}\". Supported schemes: {:#?}", url.scheme(), url, SUPPORTED_URL_SCHEMES), ).into() ); @@ -358,7 +357,8 @@ impl SourceFileFetcher { redirect_limit: i64, ) -> Pin> { if redirect_limit < 0 { - return futures::future::err(too_many_redirects()).boxed(); + let e = DenoError::new(ErrorKind::Http, "too many redirects".to_string()); + return futures::future::err(e.into()).boxed(); } let is_blacklisted = @@ -1295,7 +1295,7 @@ mod tests { .map(move |result| { assert!(result.is_err()); let err = result.err().unwrap(); - assert_eq!(err.kind(), ErrorKind::TooManyRedirects); + assert_eq!(err.kind(), ErrorKind::Http); }); tokio_util::run(fut); @@ -1571,7 +1571,7 @@ mod tests { SourceFileFetcher::check_if_supported_scheme(&url) .unwrap_err() .kind(), - ErrorKind::UnsupportedFetchScheme + ErrorKind::Other ); } } diff --git a/cli/fs.rs b/cli/fs.rs index 272eaf84c5..8d3a31bb5b 100644 --- a/cli/fs.rs +++ b/cli/fs.rs @@ -126,7 +126,9 @@ pub fn chown(path: &str, uid: u32, gid: u32) -> Result<(), ErrBox> { pub fn chown(_path: &str, _uid: u32, _gid: u32) -> Result<(), ErrBox> { // Noop // TODO: implement chown for Windows - Err(crate::deno_error::op_not_implemented()) + Err(crate::deno_error::other_error( + "Op not implemented".to_string(), + )) } pub fn resolve_from_cwd(path: &Path) -> Result { diff --git a/cli/js/errors.ts b/cli/js/errors.ts index f8210fb60c..0f75453cea 100644 --- a/cli/js/errors.ts +++ b/cli/js/errors.ts @@ -8,9 +8,9 @@ * } catch (e) { * if ( * e instanceof Deno.DenoError && - * e.kind === Deno.ErrorKind.Overflow + * e.kind === Deno.ErrorKind.NotFound * ) { - * console.error("Overflow error!"); + * console.error("NotFound error!"); * } * } * @@ -25,7 +25,6 @@ export class DenoError extends Error { // Warning! The values in this enum are duplicated in cli/msg.rs // Update carefully! export enum ErrorKind { - NoError = 0, NotFound = 1, PermissionDenied = 2, ConnectionRefused = 3, @@ -45,39 +44,13 @@ export enum ErrorKind { Other = 17, UnexpectedEof = 18, BadResource = 19, - CommandFailed = 20, - EmptyHost = 21, - IdnaError = 22, - InvalidPort = 23, - InvalidIpv4Address = 24, - InvalidIpv6Address = 25, - InvalidDomainCharacter = 26, - RelativeUrlWithoutBase = 27, - RelativeUrlWithCannotBeABaseBase = 28, - SetHostOnCannotBeABaseUrl = 29, - Overflow = 30, - HttpUser = 31, - HttpClosed = 32, - HttpCanceled = 33, - HttpParse = 34, - HttpOther = 35, - TooLarge = 36, - InvalidUri = 37, - InvalidSeekMode = 38, - OpNotAvailable = 39, - WorkerInitFailed = 40, - UnixError = 41, - NoAsyncSupport = 42, - NoSyncSupport = 43, - ImportMapError = 44, - InvalidPath = 45, - ImportPrefixMissing = 46, - UnsupportedFetchScheme = 47, - TooManyRedirects = 48, - Diagnostic = 49, - JSError = 50, - TypeError = 51, - - /** TODO this is a DomException type, and should be moved out of here when possible */ - DataCloneError = 52 + UrlParse = 20, + Http = 21, + TooLarge = 22, + InvalidSeekMode = 23, + UnixError = 24, + InvalidPath = 25, + ImportPrefixMissing = 26, + Diagnostic = 27, + JSError = 28 } diff --git a/cli/js/fetch_test.ts b/cli/js/fetch_test.ts index 97d6fd717e..92d1176ccc 100644 --- a/cli/js/fetch_test.ts +++ b/cli/js/fetch_test.ts @@ -15,8 +15,8 @@ testPerm({ net: true }, async function fetchConnectionError(): Promise { } catch (err_) { err = err_; } - assertEquals(err.kind, Deno.ErrorKind.HttpOther); - assertEquals(err.name, "HttpOther"); + assertEquals(err.kind, Deno.ErrorKind.Http); + assertEquals(err.name, "Http"); assertStrContains(err.message, "error trying to connect"); }); @@ -106,8 +106,8 @@ testPerm({ net: true }, async function fetchEmptyInvalid(): Promise { } catch (err_) { err = err_; } - assertEquals(err.kind, Deno.ErrorKind.RelativeUrlWithoutBase); - assertEquals(err.name, "RelativeUrlWithoutBase"); + assertEquals(err.kind, Deno.ErrorKind.UrlParse); + assertEquals(err.name, "UrlParse"); }); testPerm({ net: true }, async function fetchMultipartFormDataSuccess(): Promise< diff --git a/cli/js/lib.deno_runtime.d.ts b/cli/js/lib.deno_runtime.d.ts index 95c22305bf..0fa1833481 100644 --- a/cli/js/lib.deno_runtime.d.ts +++ b/cli/js/lib.deno_runtime.d.ts @@ -1114,9 +1114,9 @@ declare namespace Deno { * } catch (e) { * if ( * e instanceof Deno.DenoError && - * e.kind === Deno.ErrorKind.Overflow + * e.kind === Deno.ErrorKind.NotFound * ) { - * console.error("Overflow error!"); + * console.error("NotFound error!"); * } * } * @@ -1126,7 +1126,6 @@ declare namespace Deno { constructor(kind: T, msg: string); } export enum ErrorKind { - NoError = 0, NotFound = 1, PermissionDenied = 2, ConnectionRefused = 3, @@ -1146,37 +1145,15 @@ declare namespace Deno { Other = 17, UnexpectedEof = 18, BadResource = 19, - CommandFailed = 20, - EmptyHost = 21, - IdnaError = 22, - InvalidPort = 23, - InvalidIpv4Address = 24, - InvalidIpv6Address = 25, - InvalidDomainCharacter = 26, - RelativeUrlWithoutBase = 27, - RelativeUrlWithCannotBeABaseBase = 28, - SetHostOnCannotBeABaseUrl = 29, - Overflow = 30, - HttpUser = 31, - HttpClosed = 32, - HttpCanceled = 33, - HttpParse = 34, - HttpOther = 35, - TooLarge = 36, - InvalidUri = 37, - InvalidSeekMode = 38, - OpNotAvailable = 39, - WorkerInitFailed = 40, - UnixError = 41, - NoAsyncSupport = 42, - NoSyncSupport = 43, - ImportMapError = 44, - InvalidPath = 45, - ImportPrefixMissing = 46, - UnsupportedFetchScheme = 47, - TooManyRedirects = 48, - Diagnostic = 49, - JSError = 50 + UrlParse = 20, + Http = 21, + TooLarge = 22, + InvalidSeekMode = 23, + UnixError = 24, + InvalidPath = 25, + ImportPrefixMissing = 26, + Diagnostic = 27, + JSError = 28 } /** UNSTABLE: potentially want names to overlap more with browser. diff --git a/cli/js/permissions_test.ts b/cli/js/permissions_test.ts index 22e8494e9f..d63dddb15c 100644 --- a/cli/js/permissions_test.ts +++ b/cli/js/permissions_test.ts @@ -35,15 +35,14 @@ test(async function permissionInvalidName(): Promise { // eslint-disable-next-line @typescript-eslint/no-explicit-any await Deno.permissions.query({ name: "foo" as any }); } catch (e) { - assert(e.name === "TypeError"); + assert(e.name === "Other"); } }); test(async function permissionNetInvalidUrl(): Promise { try { - // Invalid url causes TypeError. await Deno.permissions.query({ name: "net", url: ":" }); } catch (e) { - assert(e.name === "TypeError"); + assert(e.name === "UrlParse"); } }); diff --git a/cli/js/streams/shared-internals.ts b/cli/js/streams/shared-internals.ts index 90e66e5917..93155fecc0 100644 --- a/cli/js/streams/shared-internals.ts +++ b/cli/js/streams/shared-internals.ts @@ -12,7 +12,6 @@ // TODO don't disable this warning import { AbortSignal, QueuingStrategySizeCallback } from "../dom_types.ts"; -import { DenoError, ErrorKind } from "../errors.ts"; // common stream fields @@ -208,10 +207,7 @@ export function cloneValue(value: any): any { default: // TODO this should be a DOMException, // https://github.com/stardazed/sd-streams/blob/master/packages/streams/src/shared-internals.ts#L171 - throw new DenoError( - ErrorKind.DataCloneError, - "Uncloneable value in stream" - ); + throw new Error("Uncloneable value in stream"); } } diff --git a/cli/msg.rs b/cli/msg.rs index e1d0f07f26..643c63869a 100644 --- a/cli/msg.rs +++ b/cli/msg.rs @@ -6,7 +6,6 @@ #[repr(i8)] #[derive(Clone, Copy, PartialEq, Debug)] pub enum ErrorKind { - NoError = 0, NotFound = 1, PermissionDenied = 2, ConnectionRefused = 3, @@ -26,41 +25,15 @@ pub enum ErrorKind { Other = 17, UnexpectedEof = 18, BadResource = 19, - CommandFailed = 20, - EmptyHost = 21, - IdnaError = 22, - InvalidPort = 23, - InvalidIpv4Address = 24, - InvalidIpv6Address = 25, - InvalidDomainCharacter = 26, - RelativeUrlWithoutBase = 27, - RelativeUrlWithCannotBeABaseBase = 28, - SetHostOnCannotBeABaseUrl = 29, - Overflow = 30, - HttpUser = 31, - HttpClosed = 32, - HttpCanceled = 33, - HttpParse = 34, - HttpOther = 35, - TooLarge = 36, - InvalidUri = 37, - InvalidSeekMode = 38, - OpNotAvailable = 39, - WorkerInitFailed = 40, - UnixError = 41, - NoAsyncSupport = 42, - NoSyncSupport = 43, - ImportMapError = 44, - InvalidPath = 45, - ImportPrefixMissing = 46, - UnsupportedFetchScheme = 47, - TooManyRedirects = 48, - Diagnostic = 49, - JSError = 50, - TypeError = 51, - - /** TODO this is a DomException type, and should be moved out of here when possible */ - DataCloneError = 52, + UrlParse = 20, + Http = 21, + TooLarge = 22, + InvalidSeekMode = 23, + UnixError = 24, + InvalidPath = 25, + ImportPrefixMissing = 26, + Diagnostic = 27, + JSError = 28, } // Warning! The values in this enum are duplicated in js/compiler.ts diff --git a/cli/ops/permissions.rs b/cli/ops/permissions.rs index 172ce97b15..4d1fb33377 100644 --- a/cli/ops/permissions.rs +++ b/cli/ops/permissions.rs @@ -1,6 +1,6 @@ // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license. use super::dispatch_json::{Deserialize, JsonOp, Value}; -use crate::deno_error::type_error; +use crate::deno_error::other_error; use crate::fs as deno_fs; use crate::ops::json_op; use crate::state::ThreadSafeState; @@ -99,7 +99,7 @@ pub fn op_request_permission( "env" => Ok(permissions.request_env()), "plugin" => Ok(permissions.request_plugin()), "hrtime" => Ok(permissions.request_hrtime()), - n => Err(type_error(format!("No such permission name: {}", n))), + n => Err(other_error(format!("No such permission name: {}", n))), }?; Ok(JsonOp::Sync(json!({ "state": perm.to_string() }))) } diff --git a/cli/permissions.rs b/cli/permissions.rs index 4f94fafc60..3964b07a3c 100644 --- a/cli/permissions.rs +++ b/cli/permissions.rs @@ -1,5 +1,5 @@ // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license. -use crate::deno_error::{permission_denied_msg, type_error}; +use crate::deno_error::{other_error, permission_denied_msg}; use crate::flags::DenoFlags; use ansi_term::Style; #[cfg(not(test))] @@ -179,8 +179,7 @@ impl DenoPermissions { } let url: &str = url.unwrap(); // If url is invalid, then throw a TypeError. - let parsed = Url::parse(url) - .map_err(|_| type_error(format!("Invalid url: {}", url)))?; + let parsed = Url::parse(url).map_err(ErrBox::from)?; Ok( self.get_state_net(&format!("{}", parsed.host().unwrap()), parsed.port()), ) @@ -289,7 +288,7 @@ impl DenoPermissions { "env" => Ok(self.allow_env), "plugin" => Ok(self.allow_plugin), "hrtime" => Ok(self.allow_hrtime), - n => Err(type_error(format!("No such permission name: {}", n))), + n => Err(other_error(format!("No such permission name: {}", n))), } } }