Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: update deno_core and use more concrete errors #27620

Merged
merged 6 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ repository = "https://github.com/denoland/deno"

[workspace.dependencies]
deno_ast = { version = "=0.44.0", features = ["transpiling"] }
deno_core = { version = "0.330.0" }
deno_core = { version = "0.331.0" }

deno_bench_util = { version = "0.179.0", path = "./bench_util" }
deno_config = { version = "=0.45.0", features = ["workspace", "sync"] }
Expand Down Expand Up @@ -122,7 +122,7 @@ dashmap = "5.5.3"
data-encoding = "2.3.3"
data-url = "=0.3.1"
deno_cache_dir = "=0.16.0"
deno_error = "=0.5.3"
deno_error = "=0.5.5"
deno_package_json = { version = "0.4.0", default-features = false }
deno_unsync = "0.4.2"
dlopen2 = "0.6.1"
Expand Down
24 changes: 14 additions & 10 deletions cli/args/lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,13 @@ impl<'a, T> std::ops::DerefMut for Guard<'a, T> {
}

#[derive(Debug, thiserror::Error, deno_error::JsError)]
#[error("Failed writing lockfile")]
#[class(inherit)]
struct AtomicWriteFileWithRetriesError {
#[source]
source: std::io::Error,
pub enum AtomicWriteFileWithRetriesError {
#[class(inherit)]
#[error(transparent)]
Changed(JsErrorBox),
#[class(inherit)]
#[error("Failed writing lockfile")]
Io(#[source] std::io::Error),
}

impl CliLockfile {
Expand All @@ -87,12 +89,16 @@ impl CliLockfile {
self.lockfile.lock().overwrite
}

pub fn write_if_changed(&self) -> Result<(), JsErrorBox> {
pub fn write_if_changed(
&self,
) -> Result<(), AtomicWriteFileWithRetriesError> {
if self.skip_write {
return Ok(());
}

self.error_if_changed()?;
self
.error_if_changed()
.map_err(AtomicWriteFileWithRetriesError::Changed)?;
let mut lockfile = self.lockfile.lock();
let Some(bytes) = lockfile.resolve_write_bytes() else {
return Ok(()); // nothing to do
Expand All @@ -105,9 +111,7 @@ impl CliLockfile {
&bytes,
cache::CACHE_PERM,
)
.map_err(|source| {
JsErrorBox::from_err(AtomicWriteFileWithRetriesError { source })
})?;
.map_err(AtomicWriteFileWithRetriesError::Io)?;
lockfile.has_content_changed = false;
Ok(())
}
Expand Down
1 change: 1 addition & 0 deletions cli/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ use deno_terminal::colors;
use dotenvy::from_filename;
pub use flags::*;
use import_map::resolve_import_map_value_from_specifier;
pub use lockfile::AtomicWriteFileWithRetriesError;
pub use lockfile::CliLockfile;
pub use lockfile::CliLockfileReadFromPathOptions;
use once_cell::sync::Lazy;
Expand Down
4 changes: 2 additions & 2 deletions cli/emit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,9 @@ impl Emitter {
&self,
specifier: &ModuleSpecifier,
media_type: MediaType,
module_kind: deno_ast::ModuleKind,
module_kind: ModuleKind,
source: &Arc<str>,
) -> Result<String, AnyError> {
) -> Result<String, EmitParsedSourceHelperError> {
// Note: keep this in sync with the sync version below
let helper = EmitParsedSourceHelper(self);
match helper.pre_emit_parsed_source(specifier, module_kind, source) {
Expand Down
2 changes: 1 addition & 1 deletion cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ async fn run_subcommand(flags: Arc<Flags>) -> Result<i32, AnyError> {
match result {
Ok(v) => Ok(v),
Err(script_err) => {
if let Some(ResolvePkgFolderFromDenoReqError::Byonm(ByonmResolvePkgFolderFromDenoReqError::UnmatchedReq(_))) = util::result::any_and_jserrorbox_downcast_ref::<ResolvePkgFolderFromDenoReqError>(&script_err) {
if let Some(worker::CreateCustomWorkerError::ResolvePkgFolderFromDenoReq(ResolvePkgFolderFromDenoReqError::Byonm(ByonmResolvePkgFolderFromDenoReqError::UnmatchedReq(_)))) = util::result::any_and_jserrorbox_downcast_ref::<worker::CreateCustomWorkerError>(&script_err) {
if flags.node_modules_dir.is_none() {
let mut flags = flags.deref().clone();
let watch = match &flags.subcommand {
Expand Down
93 changes: 77 additions & 16 deletions cli/module_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ use std::sync::Arc;

use deno_ast::MediaType;
use deno_ast::ModuleKind;
use deno_core::anyhow::anyhow;
use deno_core::anyhow::Context;
use deno_core::error::AnyError;
use deno_core::error::ModuleLoaderError;
use deno_core::futures::future::FutureExt;
Expand Down Expand Up @@ -45,6 +43,7 @@ use deno_lib::worker::ModuleLoaderFactory;
use deno_resolver::npm::DenoInNpmPackageChecker;
use deno_runtime::code_cache;
use deno_runtime::deno_node::create_host_defined_options;
use deno_runtime::deno_node::ops::require::UnableToGetCwdError;
use deno_runtime::deno_node::NodeRequireLoader;
use deno_runtime::deno_permissions::PermissionsContainer;
use deno_semver::npm::NpmPackageReqReference;
Expand Down Expand Up @@ -99,6 +98,11 @@ pub enum PrepareModuleLoadError {
Check(#[from] CheckError),
#[class(inherit)]
#[error(transparent)]
AtomicWriteFileWithRetries(
#[from] crate::args::AtomicWriteFileWithRetriesError,
),
#[class(inherit)]
#[error(transparent)]
Other(#[from] JsErrorBox),
}

Expand Down Expand Up @@ -419,6 +423,55 @@ impl ModuleLoaderFactory for CliModuleLoaderFactory {
}
}

#[derive(Debug, thiserror::Error, deno_error::JsError)]
pub enum LoadCodeSourceError {
#[class(inherit)]
#[error(transparent)]
NpmModuleLoad(crate::resolver::NpmModuleLoadError),
#[class(inherit)]
#[error(transparent)]
LoadPreparedModule(#[from] LoadPreparedModuleError),
#[class(generic)]
#[error("Loading unprepared module: {}{}", .specifier, .maybe_referrer.as_ref().map(|r| format!(", imported from: {}", r)).unwrap_or_default())]
LoadUnpreparedModule {
specifier: ModuleSpecifier,
maybe_referrer: Option<ModuleSpecifier>,
},
}

#[derive(Debug, thiserror::Error, deno_error::JsError)]
pub enum LoadPreparedModuleError {
#[class(inherit)]
#[error(transparent)]
NpmModuleLoad(#[from] crate::emit::EmitParsedSourceHelperError),
#[class(inherit)]
#[error(transparent)]
LoadMaybeCjs(#[from] LoadMaybeCjsError),
#[class(inherit)]
#[error(transparent)]
Other(#[from] JsErrorBox),
}

#[derive(Debug, thiserror::Error, deno_error::JsError)]
pub enum LoadMaybeCjsError {
#[class(inherit)]
#[error(transparent)]
NpmModuleLoad(#[from] crate::emit::EmitParsedSourceHelperError),
#[class(inherit)]
#[error(transparent)]
TranslateCjsToEsm(#[from] node_resolver::analyze::TranslateCjsToEsmError),
}

#[derive(Debug, thiserror::Error, deno_error::JsError)]
#[class(inherit)]
#[error("Could not resolve '{reference}'")]
pub struct CouldNotResolveError {
reference: deno_semver::npm::NpmPackageNvReference,
#[source]
#[inherit]
source: node_resolver::errors::PackageSubpathResolveError,
}

struct CliModuleLoaderInner<TGraphContainer: ModuleGraphContainer> {
lib: TsTypeLib,
is_worker: bool,
Expand All @@ -443,7 +496,10 @@ impl<TGraphContainer: ModuleGraphContainer>
maybe_referrer: Option<&ModuleSpecifier>,
requested_module_type: RequestedModuleType,
) -> Result<ModuleSource, ModuleLoaderError> {
let code_source = self.load_code_source(specifier, maybe_referrer).await?;
let code_source = self
.load_code_source(specifier, maybe_referrer)
.await
.map_err(JsErrorBox::from_err)?;
let code = if self.shared.is_inspecting
|| code_source.media_type == MediaType::Wasm
{
Expand Down Expand Up @@ -504,7 +560,7 @@ impl<TGraphContainer: ModuleGraphContainer>
&self,
specifier: &ModuleSpecifier,
maybe_referrer: Option<&ModuleSpecifier>,
) -> Result<ModuleCodeStringSource, AnyError> {
) -> Result<ModuleCodeStringSource, LoadCodeSourceError> {
if let Some(code_source) = self.load_prepared_module(specifier).await? {
return Ok(code_source);
}
Expand All @@ -513,14 +569,14 @@ impl<TGraphContainer: ModuleGraphContainer>
.shared
.npm_module_loader
.load(specifier, maybe_referrer)
.await;
.await
.map_err(LoadCodeSourceError::NpmModuleLoad);
}

let mut msg = format!("Loading unprepared module: {specifier}");
if let Some(referrer) = maybe_referrer {
msg = format!("{}, imported from: {}", msg, referrer.as_str());
}
Err(anyhow!(msg))
Err(LoadCodeSourceError::LoadUnpreparedModule {
specifier: specifier.clone(),
maybe_referrer: maybe_referrer.cloned(),
})
}

fn resolve_referrer(
Expand All @@ -543,7 +599,8 @@ impl<TGraphContainer: ModuleGraphContainer>
.map_err(|e| e.into())
} else {
// this cwd check is slow, so try to avoid it
let cwd = std::env::current_dir().context("Unable to get CWD")?;
let cwd = std::env::current_dir()
.map_err(|e| JsErrorBox::from_err(UnableToGetCwdError(e)))?;
deno_core::resolve_path(referrer, &cwd).map_err(|e| e.into())
}
}
Expand Down Expand Up @@ -622,8 +679,11 @@ impl<TGraphContainer: ModuleGraphContainer>
ResolutionMode::Import,
NodeResolutionKind::Execution,
)
.with_context(|| {
format!("Could not resolve '{}'.", module.nv_reference)
.map_err(|source| {
JsErrorBox::from_err(CouldNotResolveError {
reference: module.nv_reference.clone(),
source,
})
})?
}
Some(Module::Node(module)) => module.specifier.clone(),
Expand All @@ -644,7 +704,7 @@ impl<TGraphContainer: ModuleGraphContainer>
async fn load_prepared_module(
&self,
specifier: &ModuleSpecifier,
) -> Result<Option<ModuleCodeStringSource>, AnyError> {
) -> Result<Option<ModuleCodeStringSource>, LoadPreparedModuleError> {
// Note: keep this in sync with the sync version below
let graph = self.graph_container.graph();
match self.load_prepared_module_or_defer_emit(&graph, specifier)? {
Expand Down Expand Up @@ -676,7 +736,8 @@ impl<TGraphContainer: ModuleGraphContainer>
}) => self
.load_maybe_cjs(specifier, media_type, source)
.await
.map(Some),
.map(Some)
.map_err(LoadPreparedModuleError::LoadMaybeCjs),
None => Ok(None),
}
}
Expand Down Expand Up @@ -837,7 +898,7 @@ impl<TGraphContainer: ModuleGraphContainer>
specifier: &ModuleSpecifier,
media_type: MediaType,
original_source: &Arc<str>,
) -> Result<ModuleCodeStringSource, AnyError> {
) -> Result<ModuleCodeStringSource, LoadMaybeCjsError> {
let js_source = if media_type.is_emittable() {
Cow::Owned(
self
Expand Down
17 changes: 10 additions & 7 deletions cli/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@ use std::sync::Arc;

use deno_ast::MediaType;
use deno_ast::ModuleSpecifier;
use deno_core::error::AnyError;
use deno_error::JsErrorBox;
use deno_graph::ParsedSourceStore;
use deno_resolver::npm::DenoInNpmPackageChecker;
use deno_runtime::deno_fs;
use deno_runtime::deno_node::RealIsBuiltInNodeModuleChecker;
use node_resolver::analyze::CjsAnalysis as ExtNodeCjsAnalysis;
use node_resolver::analyze::CjsAnalysisExports;
use node_resolver::analyze::CjsCodeAnalysisError;
use node_resolver::analyze::CjsCodeAnalyzer;
use node_resolver::analyze::NodeCodeTranslator;
use serde::Deserialize;
Expand Down Expand Up @@ -75,7 +76,7 @@ impl CliCjsCodeAnalyzer {
&self,
specifier: &ModuleSpecifier,
source: &str,
) -> Result<CliCjsAnalysis, AnyError> {
) -> Result<CliCjsAnalysis, CjsCodeAnalysisError> {
let source_hash = CacheDBHash::from_hashable(source);
if let Some(analysis) =
self.cache.get_cjs_analysis(specifier.as_str(), source_hash)
Expand All @@ -102,9 +103,10 @@ impl CliCjsCodeAnalyzer {
deno_core::unsync::spawn_blocking({
let specifier = specifier.clone();
let source: Arc<str> = source.into();
move || -> Result<_, AnyError> {
let parsed_source =
maybe_parsed_source.map(Ok).unwrap_or_else(|| {
move || -> Result<_, CjsCodeAnalysisError> {
let parsed_source = maybe_parsed_source
.map(Ok)
.unwrap_or_else(|| {
deno_ast::parse_program(deno_ast::ParseParams {
specifier,
text: source,
Expand All @@ -113,7 +115,8 @@ impl CliCjsCodeAnalyzer {
scope_analysis: false,
maybe_syntax: None,
})
})?;
})
.map_err(JsErrorBox::from_err)?;
let is_script = parsed_source.compute_is_script();
let is_cjs = cjs_tracker.is_cjs_with_known_is_script(
parsed_source.specifier(),
Expand Down Expand Up @@ -151,7 +154,7 @@ impl CjsCodeAnalyzer for CliCjsCodeAnalyzer {
&self,
specifier: &ModuleSpecifier,
source: Option<Cow<'a, str>>,
) -> Result<ExtNodeCjsAnalysis<'a>, AnyError> {
) -> Result<ExtNodeCjsAnalysis<'a>, CjsCodeAnalysisError> {
let source = match source {
Some(source) => source,
None => {
Expand Down
Loading
Loading