From 645c8f697952800ee2492fd2db7d4ea5a95edb72 Mon Sep 17 00:00:00 2001 From: ldm0 Date: Thu, 3 Sep 2026 12:14:33 +0800 Subject: [PATCH] fix(modules): preserve graph error constructors --- moli-renderer-v8/src/module_runtime/graph.rs | 44 ++++++++++ .../src/script_vm/native_module.rs | 87 +++++++++++++++---- .../native_module/child_dynamic_import.rs | 37 ++++---- 3 files changed, 130 insertions(+), 38 deletions(-) diff --git a/moli-renderer-v8/src/module_runtime/graph.rs b/moli-renderer-v8/src/module_runtime/graph.rs index 94ca19de8b..01fcad83c6 100644 --- a/moli-renderer-v8/src/module_runtime/graph.rs +++ b/moli-renderer-v8/src/module_runtime/graph.rs @@ -1320,6 +1320,7 @@ impl module_tree::ModuleScriptTreeHost "failed to resolve module specifier `{specifier}` from `{base_url}`: {error}" ), ) + .with_error_constructor(module_tree::ModuleErrorConstructorKind::TypeError) })?; let attributes = local_attributes(attributes); let local_key = ModuleMapKey::from_url_and_attributes(&resolved_url, &attributes).map_err( @@ -2589,6 +2590,7 @@ where request.base_url() ), ) + .with_error_constructor(ScriptErrorConstructorKind::TypeError) })?, }; let integrity = owner.resolve_module_integrity(&source_url); @@ -3446,6 +3448,44 @@ import "./c.mjs"; ); } + #[test] + fn static_bare_specifier_resolve_failure_is_a_type_error() { + let mut vm = new_test_vm("https://app.example.test/page"); + let root_url = url("https://app.example.test/root.mjs"); + let job = parser_owned_external_module_script_graph_job( + &mut vm, + &root_url, + &url("https://app.example.test/page"), + &ScriptFetchMetadata::default(), + ); + + let root_fetch = expect_single_fetch( + advance_module_script_graph(&mut vm, job) + .expect("external parser graph should request root"), + "bare-specifier root", + ); + let error = match root_fetch.finish_source_for_test( + &mut vm, + Ok(ModuleSource::text( + r#"import "unmapped-bare-specifier";"#.to_owned(), + )), + ) { + Ok(_) => panic!("an unmapped bare specifier should fail during graph resolution"), + Err(error) => error, + }; + + assert_eq!(error.stage(), ModuleLoadStage::Resolve); + assert_eq!( + error.error_constructor(), + Some(ScriptErrorConstructorKind::TypeError) + ); + assert!( + error.message().contains("unmapped-bare-specifier"), + "{}", + error.message() + ); + } + #[test] fn static_import_with_invalid_attribute_key_fails_before_dependency_fetch() { let mut vm = new_test_vm("https://app.example.test/page"); @@ -3551,6 +3591,10 @@ import "./c.mjs"; assert!(!job.has_chromium_tree_for_test()); assert_eq!(error.stage(), ModuleLoadStage::Resolve); + assert_eq!( + error.error_constructor(), + Some(ScriptErrorConstructorKind::TypeError) + ); assert!( error.message().contains( "module type `text` is not a valid module type for module `https://example.test/app/dep.txt`" diff --git a/moli-renderer-v8/src/script_vm/native_module.rs b/moli-renderer-v8/src/script_vm/native_module.rs index f20dd3a2ae..4f382e98c4 100644 --- a/moli-renderer-v8/src/script_vm/native_module.rs +++ b/moli-renderer-v8/src/script_vm/native_module.rs @@ -3564,6 +3564,7 @@ impl ScriptVm { format!("native root module entry {root_entry:?} is not compiled"), ) })?; + let mut caught_error_constructor = None; self.renderer_document_isolate .with_entered_renderer_document_isolate(|isolate| { let scope = pin!(v8::HandleScope::new(isolate)); @@ -3571,7 +3572,7 @@ impl ScriptVm { let context = unsafe { v8::Local::new(scope, &*context_ptr) }; let scope = &mut v8::ContextScope::new(scope, context); let try_catch = pin!(v8::TryCatch::new(scope)); - let scope = try_catch.init(); + let mut scope = try_catch.init(); let root_module = v8::Local::new(&scope, &root_module); let _resolver_scope = ResolverScopeGuard::new(document_modulator); @@ -3583,11 +3584,17 @@ impl ScriptVm { Some(true) => Ok(()), Some(false) => Err(anyhow::anyhow!("v8 reported module instantiate failure")), None => { - let exception = scope - .exception() - .and_then(|exception| exception.to_string(&scope)) - .map(|message| message.to_rust_string_lossy(&scope)) - .unwrap_or_else(|| "unknown instantiate exception".to_owned()); + let exception = match scope.exception() { + Some(exception) => { + caught_error_constructor = + script_error_constructor_kind_from_value(&mut scope, exception); + exception + .to_string(&scope) + .map(|message| message.to_rust_string_lossy(&scope)) + .unwrap_or_else(|| "unknown instantiate exception".to_owned()) + } + None => "unknown instantiate exception".to_owned(), + }; Err(anyhow::anyhow!( "{}", canonical_native_module_instantiate_error(&exception, &graph_urls) @@ -3596,19 +3603,11 @@ impl ScriptVm { } }) .map_err(|error| { - let message = error.to_string(); - let load_error = - ModuleLoadError::new(ModuleLoadStage::Instantiate, message.clone()); - if message.contains("does not provide an export named") - || message.contains("does not export") - { - load_error.with_error_constructor(ScriptErrorConstructorKind::SyntaxError) - } else if has_wasm_entry { - load_error - .with_error_constructor(ScriptErrorConstructorKind::WebAssemblyLinkError) - } else { - load_error - } + native_module_instantiate_load_error( + error.to_string(), + caught_error_constructor, + has_wasm_entry, + ) })?; self.document_runtime .mark_native_module_instantiated(root_entry); @@ -4811,6 +4810,20 @@ fn canonical_native_module_instantiate_error(exception: &str, graph_urls: &[Url] format!("v8 failed to instantiate native module graph: {exception}") } +fn native_module_instantiate_load_error( + message: String, + caught_error_constructor: Option, + has_wasm_entry: bool, +) -> ModuleLoadError { + let fallback_constructor = if has_wasm_entry { + ScriptErrorConstructorKind::WebAssemblyLinkError + } else { + ScriptErrorConstructorKind::SyntaxError + }; + ModuleLoadError::new(ModuleLoadStage::Instantiate, message) + .with_error_constructor(caught_error_constructor.unwrap_or(fallback_constructor)) +} + fn canonical_missing_export_link_error(exception: &str, graph_urls: &[Url]) -> Option { let module = quoted_value_after(exception, "The requested module ")?; let export = quoted_value_after(exception, "does not provide an export named ")?; @@ -5084,6 +5097,7 @@ mod tests { use crate::script_vm::{ScriptVm, ScriptVmDefaultWorldBootstrap, StandaloneScriptVmHarness}; use crate::types::{ ModuleGraphFetchCompletion, ModuleGraphFetchOrdering, ModuleGraphFetchRequester, + ScriptErrorConstructorKind, }; use crate::util::v8str; use moli_fetch::FetchConfig; @@ -5395,6 +5409,41 @@ mod tests { ); } + #[test] + fn native_module_instantiate_errors_use_stage_appropriate_constructors() { + let javascript_error = super::native_module_instantiate_load_error( + "javascript link failed".to_owned(), + None, + false, + ); + assert_eq!(javascript_error.stage(), ModuleLoadStage::Instantiate); + assert_eq!( + javascript_error.error_constructor(), + Some(ScriptErrorConstructorKind::SyntaxError) + ); + + let wasm_error = super::native_module_instantiate_load_error( + "WebAssembly link failed".to_owned(), + None, + true, + ); + assert_eq!( + wasm_error.error_constructor(), + Some(ScriptErrorConstructorKind::WebAssemblyLinkError) + ); + + let caught_syntax_error = super::native_module_instantiate_load_error( + "mixed graph failed".to_owned(), + Some(ScriptErrorConstructorKind::SyntaxError), + true, + ); + assert_eq!( + caught_syntax_error.error_constructor(), + Some(ScriptErrorConstructorKind::SyntaxError), + "an exact V8 exception constructor must win over the module-kind fallback" + ); + } + #[test] fn dynamic_import_fetch_completion_requires_owner_facade() { let mut vm = new_test_vm("https://app.example.test/page.html"); diff --git a/moli-renderer-v8/src/script_vm/native_module/child_dynamic_import.rs b/moli-renderer-v8/src/script_vm/native_module/child_dynamic_import.rs index 84faec2303..6b9b0038c0 100644 --- a/moli-renderer-v8/src/script_vm/native_module/child_dynamic_import.rs +++ b/moli-renderer-v8/src/script_vm/native_module/child_dynamic_import.rs @@ -863,6 +863,7 @@ impl ScriptVm { format!("native root module entry {root_entry:?} is not compiled"), ) })?; + let mut caught_error_constructor = None; self.renderer_document_isolate .with_entered_renderer_document_isolate(|isolate| { let scope = pin!(v8::HandleScope::new(isolate)); @@ -870,7 +871,7 @@ impl ScriptVm { let context = unsafe { v8::Local::new(scope, &*context_ptr) }; let scope = &mut v8::ContextScope::new(scope, context); let try_catch = pin!(v8::TryCatch::new(scope)); - let scope = try_catch.init(); + let mut scope = try_catch.init(); let root_module = v8::Local::new(&scope, &root_module); let _resolver_scope = ResolverScopeGuard::new(document_modulator_ptr); @@ -882,11 +883,17 @@ impl ScriptVm { Some(true) => Ok(()), Some(false) => Err(anyhow::anyhow!("v8 reported module instantiate failure")), None => { - let exception = scope - .exception() - .and_then(|exception| exception.to_string(&scope)) - .map(|message| message.to_rust_string_lossy(&scope)) - .unwrap_or_else(|| "unknown instantiate exception".to_owned()); + let exception = match scope.exception() { + Some(exception) => { + caught_error_constructor = + script_error_constructor_kind_from_value(&mut scope, exception); + exception + .to_string(&scope) + .map(|message| message.to_rust_string_lossy(&scope)) + .unwrap_or_else(|| "unknown instantiate exception".to_owned()) + } + None => "unknown instantiate exception".to_owned(), + }; Err(anyhow::anyhow!( "{}", canonical_native_module_instantiate_error(&exception, &graph_urls) @@ -895,19 +902,11 @@ impl ScriptVm { } }) .map_err(|error| { - let message = error.to_string(); - let load_error = - ModuleLoadError::new(ModuleLoadStage::Instantiate, message.clone()); - if message.contains("does not provide an export named") - || message.contains("does not export") - { - load_error.with_error_constructor(ScriptErrorConstructorKind::SyntaxError) - } else if has_wasm_entry { - load_error - .with_error_constructor(ScriptErrorConstructorKind::WebAssemblyLinkError) - } else { - load_error - } + native_module_instantiate_load_error( + error.to_string(), + caught_error_constructor, + has_wasm_entry, + ) })?; document_modulator.mark_instantiated(root_entry); Ok(())