From cbf7134c06ffb1f6c7819d1c49c02dff77f717cd Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Fri, 20 May 2022 14:48:41 +0200 Subject: [PATCH 1/3] fix: error on conflicting assertions Previously, if a specifier was imported more than once, with conflicting assertions, we'd only really consider the final import. This is wrong: conflicting assertions are an error in-themselves already. I think this behaviour is still wrong, because if a module is imported with incorrect assertions later in the graph after the module has already been loaded by an import with a correct assertion, no error will be raised. --- src/graph.rs | 70 +++++++++++++++++++++++++++++++--------------------- src/info.rs | 3 +++ 2 files changed, 45 insertions(+), 28 deletions(-) diff --git a/src/graph.rs b/src/graph.rs index ca5aa3f01..6905b38cc 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -133,6 +133,7 @@ pub enum ModuleGraphError { actual_media_type: MediaType, expected_media_type: MediaType, }, + ConflictingAssertions(ModuleSpecifier), LoadingErr(ModuleSpecifier, Arc), Missing(ModuleSpecifier), ParseErr(ModuleSpecifier, deno_ast::Diagnostic), @@ -163,6 +164,9 @@ impl Clone for ModuleGraphError { actual_media_type: *actual_media_type, expected_media_type: *expected_media_type, }, + Self::ConflictingAssertions(specifier) => { + Self::ConflictingAssertions(specifier.clone()) + } Self::UnsupportedImportAssertionType(specifier, kind) => { Self::UnsupportedImportAssertionType(specifier.clone(), kind.clone()) } @@ -184,7 +188,8 @@ impl ModuleGraphError { | Self::InvalidSource(s, _) | Self::UnsupportedMediaType(s, _) | Self::UnsupportedImportAssertionType(s, _) - | Self::Missing(s) => s, + | Self::Missing(s) + | Self::ConflictingAssertions(s) => s, Self::InvalidTypeAssertion { specifier, .. } => specifier, } } @@ -201,6 +206,7 @@ impl fmt::Display for ModuleGraphError { Self::InvalidSource(specifier, Some(filename)) => write!(f, "The source code is invalid, as it does not match the expected hash in the lock file.\n Specifier: {}\n Lock file: {}", specifier, filename), Self::InvalidSource(specifier, None) => write!(f, "The source code is invalid, as it does not match the expected hash in the lock file.\n Specifier: {}", specifier), Self::InvalidTypeAssertion { specifier, actual_media_type, expected_media_type } => write!(f, "Expected a {} module, but identified a {} module.\n Specifier: {}", expected_media_type, actual_media_type, specifier), + Self::ConflictingAssertions(specifier) => write!(f, "Module \"{specifier}\" was imported with conflicting assertions."), Self::UnsupportedMediaType(specifier, MediaType::Json) => write!(f, "Expected a JavaScript or TypeScript module, but identified a Json module. Consider importing Json modules with an import assertion with the type of \"json\".\n Specifier: {}", specifier), Self::UnsupportedMediaType(specifier, media_type) => write!(f, "Expected a JavaScript or TypeScript module, but identified a {} module. Importing these types of modules is currently not supported.\n Specifier: {}", media_type, specifier), Self::UnsupportedImportAssertionType(_, kind) => write!(f, "The import assertion type of \"{}\" is unsupported.", kind), @@ -1559,15 +1565,7 @@ impl<'a> Builder<'a> { Some((specifier, kind, Ok(Some(response)))) => { let assert_types = self.pending_assert_types.remove(&specifier).unwrap(); - for maybe_assert_type in assert_types { - self.visit( - &specifier, - &kind, - &response, - &build_kind, - maybe_assert_type, - ) - } + self.visit(&specifier, &kind, &response, &build_kind, assert_types); Some(specifier) } Some((specifier, _, Ok(None))) => { @@ -1700,23 +1698,35 @@ impl<'a> Builder<'a> { kind: &ModuleKind, response: &LoadResponse, build_kind: &BuildKind, - maybe_assert_type: Option, + assert_types: HashSet>, ) { let (specifier, module_slot) = match response { LoadResponse::BuiltIn { specifier } => { - self.check_specifier(requested_specifier, specifier); - let module_slot = ModuleSlot::Module(Module::new_without_source( - specifier.clone(), - ModuleKind::BuiltIn, - )); + self.check_specifier(requested_specifier, &specifier); + let module_slot = if assert_types.len() != 1 { + ModuleSlot::Err(ModuleGraphError::ConflictingAssertions( + specifier.clone(), + )) + } else { + ModuleSlot::Module(Module::new_without_source( + specifier.clone(), + ModuleKind::BuiltIn, + )) + }; (specifier, module_slot) } LoadResponse::External { specifier } => { - self.check_specifier(requested_specifier, specifier); - let module_slot = ModuleSlot::Module(Module::new_without_source( - specifier.clone(), - ModuleKind::External, - )); + self.check_specifier(requested_specifier, &specifier); + let module_slot = if assert_types.len() != 1 { + ModuleSlot::Err(ModuleGraphError::ConflictingAssertions( + specifier.clone(), + )) + } else { + ModuleSlot::Module(Module::new_without_source( + specifier.clone(), + ModuleKind::External, + )) + }; (specifier, module_slot) } LoadResponse::Module { @@ -1724,18 +1734,22 @@ impl<'a> Builder<'a> { content, maybe_headers, } => { - self.check_specifier(requested_specifier, specifier); - ( - specifier, + self.check_specifier(requested_specifier, &specifier); + let module_slot = if assert_types.len() != 1 { + ModuleSlot::Err(ModuleGraphError::ConflictingAssertions( + specifier.clone(), + )) + } else { self.visit_module( - specifier, + &specifier, kind, maybe_headers.as_ref(), content.clone(), build_kind, - maybe_assert_type, - ), - ) + assert_types.into_iter().next().unwrap(), + ) + }; + (specifier, module_slot) } }; self diff --git a/src/info.rs b/src/info.rs index 5da2e3efc..68096e3f5 100644 --- a/src/info.rs +++ b/src/info.rs @@ -249,6 +249,9 @@ impl ModuleGraphError { Self::InvalidTypeAssertion { .. } => { fmt_error_msg(f, prefix, last, specifier, "(invalid import assertion)") } + Self::ConflictingAssertions { .. } => { + fmt_error_msg(f, prefix, last, specifier, "(conflicting assertions)") + } Self::LoadingErr(_, _) => { fmt_error_msg(f, prefix, last, specifier, "(loading error)") } From 024c08e5af4a2e838690269c3837674a1d3b950a Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Fri, 20 May 2022 14:21:18 +0200 Subject: [PATCH 2/3] refactor: load module content an owned value This is a pre-requisite for WASM imports that additionally allows us to minimize clones when doing things like BOM stripping. --- src/graph.rs | 31 +++++++++++++++---------------- src/lib.rs | 41 ++++++++++++++--------------------------- src/source.rs | 7 +++---- 3 files changed, 32 insertions(+), 47 deletions(-) diff --git a/src/graph.rs b/src/graph.rs index 6905b38cc..dc90ceb98 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -1177,7 +1177,7 @@ fn resolve( pub(crate) fn parse_module( specifier: &ModuleSpecifier, maybe_headers: Option<&HashMap>, - content: Arc, + content: String, maybe_assert_type: Option<&str>, maybe_kind: Option<&ModuleKind>, maybe_resolver: Option<&dyn Resolver>, @@ -1187,6 +1187,8 @@ pub(crate) fn parse_module( ) -> ModuleSlot { let media_type = get_media_type(specifier, maybe_headers); + let content = content.into(); + // here we check any media types that should have assertions made against them // if they aren't the root and add them to the graph, otherwise we continue if media_type == MediaType::Json @@ -1565,7 +1567,7 @@ impl<'a> Builder<'a> { Some((specifier, kind, Ok(Some(response)))) => { let assert_types = self.pending_assert_types.remove(&specifier).unwrap(); - self.visit(&specifier, &kind, &response, &build_kind, assert_types); + self.visit(&specifier, &kind, response, &build_kind, assert_types); Some(specifier) } Some((specifier, _, Ok(None))) => { @@ -1696,7 +1698,7 @@ impl<'a> Builder<'a> { &mut self, requested_specifier: &ModuleSpecifier, kind: &ModuleKind, - response: &LoadResponse, + response: LoadResponse, build_kind: &BuildKind, assert_types: HashSet>, ) { @@ -1744,7 +1746,7 @@ impl<'a> Builder<'a> { &specifier, kind, maybe_headers.as_ref(), - content.clone(), + content, build_kind, assert_types.into_iter().next().unwrap(), ) @@ -1752,10 +1754,7 @@ impl<'a> Builder<'a> { (specifier, module_slot) } }; - self - .graph - .module_slots - .insert(specifier.clone(), module_slot); + self.graph.module_slots.insert(specifier, module_slot); } /// Visit a module, parsing it and resolving any dependencies. @@ -1764,7 +1763,7 @@ impl<'a> Builder<'a> { specifier: &ModuleSpecifier, kind: &ModuleKind, maybe_headers: Option<&HashMap>, - content: Arc, + content: String, build_kind: &BuildKind, maybe_assert_type: Option, ) -> ModuleSlot { @@ -2055,7 +2054,7 @@ mod tests { fn test_module_dependency_includes() { let specifier = ModuleSpecifier::parse("file:///a.ts").unwrap(); let source_parser = ast::DefaultSourceParser::default(); - let content = Arc::new(r#"import * as b from "./b.ts";"#.to_string()); + let content = r#"import * as b from "./b.ts";"#.to_string(); let slot = parse_module( &specifier, None, @@ -2144,7 +2143,7 @@ mod tests { Ok(Some(LoadResponse::Module { specifier: specifier.clone(), maybe_headers: None, - content: Arc::new("await import('file:///bar.js')".to_string()), + content: "await import('file:///bar.js')".to_string(), })) }) } @@ -2155,7 +2154,7 @@ mod tests { Ok(Some(LoadResponse::Module { specifier: specifier.clone(), maybe_headers: None, - content: Arc::new("import 'file:///baz.js'".to_string()), + content: "import 'file:///baz.js'".to_string(), })) }) } @@ -2166,7 +2165,7 @@ mod tests { Ok(Some(LoadResponse::Module { specifier: specifier.clone(), maybe_headers: None, - content: Arc::new("console.log('Hello, world!')".to_string()), + content: "console.log('Hello, world!')".to_string(), })) }) } @@ -2210,7 +2209,7 @@ mod tests { Ok(Some(LoadResponse::Module { specifier: specifier.clone(), maybe_headers: None, - content: Arc::new("await import('file:///bar.js')".to_string()), + content: "await import('file:///bar.js')".to_string(), })) }), "file:///bar.js" => Box::pin(async move { Ok(None) }), @@ -2270,14 +2269,14 @@ mod tests { Ok(Some(LoadResponse::Module { specifier: Url::parse("file:///foo_actual.js").unwrap(), maybe_headers: None, - content: Arc::new("import 'file:///bar.js'".to_string()), + content: "import 'file:///bar.js'".to_string(), })) }), "file:///bar.js" => Box::pin(async move { Ok(Some(LoadResponse::Module { specifier: Url::parse("file:///bar_actual.js").unwrap(), maybe_headers: None, - content: Arc::new("(".to_string()), + content: "(".to_string(), })) }), _ => unreachable!(), diff --git a/src/lib.rs b/src/lib.rs index f91e23430..5152d0892 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -25,7 +25,6 @@ use source::Resolver; use std::cell::RefCell; use std::collections::HashMap; use std::rc::Rc; -use std::sync::Arc; cfg_if! { if #[cfg(feature = "rust")] { @@ -149,7 +148,7 @@ cfg_if! { pub fn parse_module( specifier: &ModuleSpecifier, maybe_headers: Option<&HashMap>, - content: Arc, + content: String, maybe_kind: Option<&ModuleKind>, maybe_resolver: Option<&dyn Resolver>, maybe_parser: Option<&dyn SourceParser>, @@ -294,7 +293,7 @@ cfg_if! { match graph::parse_module( &specifier, maybe_headers.as_ref(), - Arc::new(content), + content, None, maybe_kind.as_ref(), maybe_resolver.as_ref().map(|r| r as &dyn Resolver), @@ -2817,15 +2816,13 @@ export function a(a) { let result = parse_module( &specifier, None, - Arc::new( - r#" + r#" import { a } from "./a.ts"; import * as b from "./b.ts"; export { c } from "./c.ts"; const d = await import("./d.ts"); "# - .to_string(), - ), + .to_string(), None, None, None, @@ -2843,13 +2840,11 @@ export function a(a) { let result = parse_module( &specifier, None, - Arc::new( - r#" + r#" import a from "./a.json" assert { type: "json" }; await import("./b.json", { assert: { type: "json" } }); "# - .to_string(), - ), + .to_string(), Some(&ModuleKind::Esm), None, None, @@ -2910,16 +2905,14 @@ export function a(a) { let result = parse_module( &specifier, None, - Arc::new( - r#" + r#" /** @jsxImportSource https://example.com/preact */ export function A() { return
Hello Deno
; } "# - .to_string(), - ), + .to_string(), Some(&ModuleKind::Esm), None, None, @@ -2956,12 +2949,10 @@ export function a(a) { let result = parse_module( &specifier, maybe_headers, - Arc::new( - r#"declare interface A { + r#"declare interface A { a: string; }"# - .to_string(), - ), + .to_string(), Some(&ModuleKind::Esm), None, None, @@ -2975,8 +2966,7 @@ export function a(a) { let result = parse_module( &specifier, None, - Arc::new( - r#" + r#" /** * Some js doc * @@ -2987,8 +2977,7 @@ export function a(a) { return; } "# - .to_string(), - ), + .to_string(), Some(&ModuleKind::Esm), None, None, @@ -3046,8 +3035,7 @@ export function a(a) { let result = parse_module( &specifier, None, - Arc::new( - r#" + r#" /** * Some js doc * @@ -3058,8 +3046,7 @@ export function a(a: A): B { return; } "# - .to_string(), - ), + .to_string(), Some(&ModuleKind::Esm), None, None, diff --git a/src/source.rs b/src/source.rs index e1a9dc005..f9dc82c61 100644 --- a/src/source.rs +++ b/src/source.rs @@ -19,7 +19,6 @@ use std::collections::HashMap; use std::fmt; use std::path::PathBuf; use std::pin::Pin; -use std::sync::Arc; pub static DEFAULT_JSX_IMPORT_SOURCE_MODULE: &str = "jsx-runtime"; @@ -56,7 +55,7 @@ pub enum LoadResponse { /// A loaded module. Module { /// The content of the remote module. - content: Arc, + content: String, /// The final specifier of the module. specifier: ModuleSpecifier, /// If the module is a remote module, the headers should be returned as a @@ -213,7 +212,7 @@ pub fn load_data_url( Ok(Some(LoadResponse::Module { specifier: specifier.clone(), maybe_headers: Some(headers), - content: Arc::new(content), + content, })) } @@ -265,7 +264,7 @@ impl MemoryLoader { }) .collect() }), - content: Arc::new(content.as_ref().to_string()), + content: content.as_ref().to_string(), }), Source::BuiltIn(specifier) => Ok(LoadResponse::BuiltIn { specifier: ModuleSpecifier::parse(specifier.as_ref()).unwrap(), From 6b6c5d76d872c69a53a9b8772372f93391b33fed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Mon, 30 May 2022 19:00:08 +0200 Subject: [PATCH 3/3] lint --- src/graph.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/graph.rs b/src/graph.rs index 0c908a303..76c9c6d3e 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -2058,7 +2058,7 @@ mod tests { let slot = parse_module( &specifier, None, - content.into(), + content, None, Some(&ModuleKind::Esm), None,