From 9ae1c0ec92a11bb76337966215828ed5454ae7dc Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 1 Apr 2024 20:12:25 -0400 Subject: [PATCH] refactor(BREAKING): remove unused module parser build option and use less Options (#399) --- lib/lib.rs | 9 +++--- src/analyzer.rs | 7 +++++ src/ast.rs | 54 +++++++++++++++++++++------------ src/fast_check/transform.rs | 4 +-- src/fast_check/transform_dts.rs | 8 ++--- src/graph.rs | 35 ++++++++------------- src/lib.rs | 30 ++++++++---------- src/source/file_system.rs | 13 ++++++++ src/source/mod.rs | 6 ++++ tests/helpers/test_builder.rs | 3 +- tests/integration_test.rs | 2 +- 11 files changed, 98 insertions(+), 73 deletions(-) diff --git a/lib/lib.rs b/lib/lib.rs index e797b7816..b04a1488b 100644 --- a/lib/lib.rs +++ b/lib/lib.rs @@ -263,11 +263,10 @@ pub async fn js_create_graph( BuildOptions { is_dynamic: false, resolver: maybe_resolver.as_ref().map(|r| r as &dyn Resolver), - file_system: Some(&NullFileSystem), - jsr_url_provider: None, + file_system: &NullFileSystem, + jsr_url_provider: Default::default(), npm_resolver: None, - module_analyzer: None, - module_parser: None, + module_analyzer: Default::default(), imports, reporter: None, workspace_members: &[], @@ -318,7 +317,7 @@ pub fn js_parse_module( content: content.into(), file_system: &NullFileSystem, maybe_resolver: maybe_resolver.as_ref().map(|r| r as &dyn Resolver), - maybe_module_analyzer: None, + module_analyzer: Default::default(), maybe_npm_resolver: None, }) { Ok(module) => { diff --git a/src/analyzer.rs b/src/analyzer.rs index 493df9c79..022104dcd 100644 --- a/src/analyzer.rs +++ b/src/analyzer.rs @@ -18,6 +18,7 @@ use serde::Serialize; use serde::Serializer; use crate::graph::Position; +use crate::DefaultModuleAnalyzer; /// Matches the `@deno-types` pragma. static DENO_TYPES_RE: Lazy = Lazy::new(|| { @@ -314,6 +315,12 @@ pub trait ModuleAnalyzer { ) -> Result; } +impl<'a> Default for &'a dyn ModuleAnalyzer { + fn default() -> &'a dyn ModuleAnalyzer { + &DefaultModuleAnalyzer + } +} + #[cfg(test)] mod test { use std::collections::HashMap; diff --git a/src/ast.rs b/src/ast.rs index 5fe782f77..34ea1f436 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -209,18 +209,29 @@ impl<'a> ModuleParser for CapturingModuleParser<'a> { } } -/// Default module analyzer that analyzes based on a deno_ast::ParsedSource. #[derive(Default)] -pub struct DefaultModuleAnalyzer<'a> { - parser: Option<&'a dyn ModuleParser>, +pub struct DefaultModuleAnalyzer; + +impl ModuleAnalyzer for DefaultModuleAnalyzer { + fn analyze( + &self, + specifier: &deno_ast::ModuleSpecifier, + source: Arc, + media_type: MediaType, + ) -> Result { + ParserModuleAnalyzer::default().analyze(specifier, source, media_type) + } } -impl<'a> DefaultModuleAnalyzer<'a> { +/// Default module analyzer that analyzes based on a deno_ast::ParsedSource. +pub struct ParserModuleAnalyzer<'a> { + parser: &'a dyn ModuleParser, +} + +impl<'a> ParserModuleAnalyzer<'a> { /// Creates a new module analyzer. pub fn new(parser: &'a dyn ModuleParser) -> Self { - Self { - parser: Some(parser), - } + Self { parser } } /// Gets the module info from a parsed source. @@ -263,23 +274,29 @@ impl<'a> DefaultModuleAnalyzer<'a> { } } -impl<'a> ModuleAnalyzer for DefaultModuleAnalyzer<'a> { +impl<'a> Default for ParserModuleAnalyzer<'a> { + fn default() -> Self { + Self { + parser: &DefaultModuleParser, + } + } +} + +impl<'a> ModuleAnalyzer for ParserModuleAnalyzer<'a> { fn analyze( &self, specifier: &deno_ast::ModuleSpecifier, source: Arc, media_type: MediaType, ) -> Result { - let default_parser = DefaultModuleParser; - let parser = self.parser.unwrap_or(&default_parser); - let parsed_source = parser.parse_module(ParseOptions { + let parsed_source = self.parser.parse_module(ParseOptions { specifier, source, media_type, // scope analysis is not necessary for module parsing scope_analysis: false, })?; - Ok(DefaultModuleAnalyzer::module_info(&parsed_source)) + Ok(ParserModuleAnalyzer::module_info(&parsed_source)) } } @@ -323,7 +340,7 @@ impl ModuleAnalyzer for CapturingModuleAnalyzer { media_type: MediaType, ) -> Result { let capturing_parser = self.as_capturing_parser(); - let module_analyzer = DefaultModuleAnalyzer::new(&capturing_parser); + let module_analyzer = ParserModuleAnalyzer::new(&capturing_parser); module_analyzer.analyze(specifier, source, media_type) } } @@ -606,7 +623,7 @@ mod tests { }) .unwrap(); let text_info = parsed_source.text_info(); - let module_info = DefaultModuleAnalyzer::module_info(&parsed_source); + let module_info = ParserModuleAnalyzer::module_info(&parsed_source); let dependencies = module_info.dependencies; assert_eq!(dependencies.len(), 6); @@ -683,7 +700,7 @@ mod tests { scope_analysis: false, }) .unwrap(); - let module_info = DefaultModuleAnalyzer::module_info(&parsed_source); + let module_info = ParserModuleAnalyzer::module_info(&parsed_source); let text_info = parsed_source.text_info(); let dependencies = module_info.dependencies; assert_eq!(dependencies.len(), 10); @@ -720,7 +737,7 @@ mod tests { scope_analysis: false, }) .unwrap(); - let module_info = DefaultModuleAnalyzer::module_info(&parsed_source); + let module_info = ParserModuleAnalyzer::module_info(&parsed_source); let dependencies = module_info.dependencies; assert_eq!(dependencies.len(), 2); let dep = dependencies[0].as_static().unwrap(); @@ -771,7 +788,7 @@ const f = new Set(); scope_analysis: false, }) .unwrap(); - let module_info = DefaultModuleAnalyzer::module_info(&parsed_source); + let module_info = ParserModuleAnalyzer::module_info(&parsed_source); let dependencies = module_info.jsdoc_imports; assert_eq!( dependencies, @@ -840,8 +857,7 @@ const f = new Set(); /* @jsxImportSource preact */ export {}; "#; - let analyzer = DefaultModuleAnalyzer::new(&DefaultModuleParser); - let module_info = analyzer + let module_info = DefaultModuleAnalyzer .analyze(&specifier, source.into(), MediaType::Tsx) .unwrap(); assert_eq!( diff --git a/src/fast_check/transform.rs b/src/fast_check/transform.rs index 97d84b80f..d2db747b8 100644 --- a/src/fast_check/transform.rs +++ b/src/fast_check/transform.rs @@ -19,9 +19,9 @@ use deno_ast::ParsedSource; use deno_ast::SourceRange; use deno_ast::SourceRangedForSpanned; -use crate::DefaultModuleAnalyzer; use crate::ModuleGraph; use crate::ModuleInfo; +use crate::ParserModuleAnalyzer; use crate::WorkspaceMember; use super::range_finder::ModulePublicRanges; @@ -115,7 +115,7 @@ pub fn transform( if !transformer.diagnostics.is_empty() { return Err(transformer.diagnostics); } - let module_info = DefaultModuleAnalyzer::module_info_from_swc( + let module_info = ParserModuleAnalyzer::module_info_from_swc( parsed_source.media_type(), &module, parsed_source.text_info(), diff --git a/src/fast_check/transform_dts.rs b/src/fast_check/transform_dts.rs index 96b09cf6e..276f9d3ff 100644 --- a/src/fast_check/transform_dts.rs +++ b/src/fast_check/transform_dts.rs @@ -936,7 +936,7 @@ mod tests { use crate::source::Source; use crate::symbols::RootSymbol; use crate::BuildOptions; - use crate::DefaultModuleParser; + use crate::CapturingModuleAnalyzer; use crate::GraphKind; use crate::ModuleGraph; use url::Url; @@ -955,20 +955,20 @@ mod tests { )], vec![], ); - let parser = DefaultModuleParser {}; + let analyzer = CapturingModuleAnalyzer::default(); let mut graph = ModuleGraph::new(GraphKind::All); graph .build( vec![specifier.clone()], &mut loader, BuildOptions { - module_parser: Some(&parser), + module_analyzer: &analyzer, ..Default::default() }, ) .await; - let root_sym = RootSymbol::new(&graph, &parser); + let root_sym = RootSymbol::new(&graph, &analyzer); let module_info = root_sym .module_from_specifier(&specifier) diff --git a/src/graph.rs b/src/graph.rs index 6e1833685..3b7dcfb2a 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -11,8 +11,6 @@ use crate::analyzer::SpecifierWithRange; use crate::analyzer::TypeScriptReference; #[cfg(feature = "fast_check")] use crate::fast_check::FastCheckDtsModule; -use crate::CapturingModuleAnalyzer; -use crate::ModuleParser; use crate::ReferrerImports; use crate::fast_check::FastCheckDiagnostic; @@ -1001,7 +999,7 @@ pub struct BuildFastCheckTypeGraphOptions<'a> { pub fast_check_cache: Option<&'a dyn crate::fast_check::FastCheckCache>, pub fast_check_dts: bool, pub jsr_url_provider: Option<&'a dyn JsrUrlProvider>, - pub module_parser: Option<&'a dyn ModuleParser>, + pub module_parser: Option<&'a dyn crate::ModuleParser>, pub resolver: Option<&'a dyn Resolver>, pub npm_resolver: Option<&'a dyn NpmResolver>, /// Whether to fill workspace members with fast check TypeScript data. @@ -1016,15 +1014,14 @@ pub struct BuildOptions<'a> { /// be extra modules such as TypeScript's "types" option or JSX /// runtime types. pub imports: Vec, - pub file_system: Option<&'a dyn FileSystem>, - pub jsr_url_provider: Option<&'a dyn JsrUrlProvider>, - pub resolver: Option<&'a dyn Resolver>, + pub executor: &'a dyn Executor, + pub file_system: &'a dyn FileSystem, + pub jsr_url_provider: &'a dyn JsrUrlProvider, + pub module_analyzer: &'a dyn ModuleAnalyzer, pub npm_resolver: Option<&'a dyn NpmResolver>, - pub module_analyzer: Option<&'a dyn ModuleAnalyzer>, - pub module_parser: Option<&'a dyn ModuleParser>, pub reporter: Option<&'a dyn Reporter>, + pub resolver: Option<&'a dyn Resolver>, pub workspace_members: &'a [WorkspaceMember], - pub executor: &'a dyn Executor, } #[derive(Debug, Copy, Clone)] @@ -1444,25 +1441,17 @@ impl ModuleGraph { loader: &mut dyn Loader, options: BuildOptions<'a>, ) -> Vec { - let default_jsr_url_provider = DefaultJsrUrlProvider; - let default_module_parser = CapturingModuleAnalyzer::default(); - #[cfg(not(target_arch = "wasm32"))] - let file_system = RealFileSystem; - #[cfg(target_arch = "wasm32")] - let file_system = NullFileSystem; Builder::build( self, roots, options.imports, options.is_dynamic, - options.file_system.unwrap_or(&file_system), - options - .jsr_url_provider - .unwrap_or(&default_jsr_url_provider), + options.file_system, + options.jsr_url_provider, options.resolver, options.npm_resolver, loader, - options.module_analyzer.unwrap_or(&default_module_parser), + options.module_analyzer, options.reporter, options.workspace_members, options.executor, @@ -1494,7 +1483,7 @@ impl ModuleGraph { return; } - let default_module_parser = CapturingModuleAnalyzer::default(); + let default_module_parser = crate::CapturingModuleAnalyzer::default(); let root_symbol = crate::symbols::RootSymbol::new( self, options.module_parser.unwrap_or(&default_module_parser), @@ -4588,7 +4577,7 @@ where #[cfg(test)] mod tests { use crate::packages::JsrPackageInfoVersion; - use crate::DefaultModuleAnalyzer; + use crate::ParserModuleAnalyzer; use deno_ast::dep::ImportAttribute; use pretty_assertions::assert_eq; use serde_json::json; @@ -4656,7 +4645,7 @@ mod tests { #[test] fn test_module_dependency_includes() { let specifier = ModuleSpecifier::parse("file:///a.ts").unwrap(); - let module_analyzer = DefaultModuleAnalyzer::default(); + let module_analyzer = ParserModuleAnalyzer::default(); let content: Arc<[u8]> = Arc::from( b"import * as b from \"./b.ts\";\nimport * as c from \"./b.ts\"".to_vec(), ); diff --git a/src/lib.rs b/src/lib.rs index 16edff4d8..0ef4b18fe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -38,13 +38,13 @@ pub use analyzer::SpecifierWithRange; pub use analyzer::StaticDependencyDescriptor; pub use analyzer::TypeScriptReference; pub use ast::CapturingModuleAnalyzer; -pub use ast::CapturingModuleParser; pub use ast::DefaultModuleAnalyzer; pub use ast::DefaultModuleParser; pub use ast::DefaultParsedSourceStore; pub use ast::ModuleParser; pub use ast::ParseOptions; pub use ast::ParsedSourceStore; +pub use ast::ParserModuleAnalyzer; pub use deno_ast::MediaType; pub use fast_check::FastCheckCache; pub use fast_check::FastCheckCacheItem; @@ -109,8 +109,8 @@ pub struct ParseModuleOptions<'a> { pub content: Arc<[u8]>, pub file_system: &'a dyn FileSystem, pub maybe_resolver: Option<&'a dyn Resolver>, - pub maybe_module_analyzer: Option<&'a dyn ModuleAnalyzer>, pub maybe_npm_resolver: Option<&'a dyn NpmResolver>, + pub module_analyzer: &'a dyn ModuleAnalyzer, } /// Parse an individual module, returning the module as a result, otherwise @@ -119,10 +119,6 @@ pub struct ParseModuleOptions<'a> { pub fn parse_module( options: ParseModuleOptions, ) -> Result { - let default_module_analyzer = ast::DefaultModuleAnalyzer::default(); - let module_analyzer = options - .maybe_module_analyzer - .unwrap_or(&default_module_analyzer); graph::parse_module( options.graph_kind, options.specifier, @@ -132,7 +128,7 @@ pub fn parse_module( None, options.file_system, options.maybe_resolver, - module_analyzer, + options.module_analyzer, true, false, options.maybe_npm_resolver, @@ -156,7 +152,7 @@ pub fn parse_module_from_ast(options: ParseModuleFromAstOptions) -> JsModule { options.specifier, options.parsed_source.media_type(), options.maybe_headers, - DefaultModuleAnalyzer::module_info(options.parsed_source), + ParserModuleAnalyzer::module_info(options.parsed_source), options.parsed_source.text_info().text(), options.file_system, options.maybe_resolver, @@ -3039,8 +3035,8 @@ export function a(a) { content: code.to_vec().into(), file_system: &NullFileSystem, maybe_resolver: None, - maybe_module_analyzer: None, maybe_npm_resolver: None, + module_analyzer: Default::default(), }) .unwrap(); let actual = actual.js().unwrap(); @@ -3056,8 +3052,8 @@ export function a(a) { content: code.to_vec().into(), file_system: &NullFileSystem, maybe_resolver: None, - maybe_module_analyzer: None, maybe_npm_resolver: None, + module_analyzer: Default::default(), }) .unwrap(); let actual = actual.js().unwrap(); @@ -3079,8 +3075,8 @@ export function a(a) { .into(), file_system: &NullFileSystem, maybe_resolver: None, - maybe_module_analyzer: None, maybe_npm_resolver: None, + module_analyzer: Default::default(), }) .unwrap(); assert_eq!( @@ -3149,8 +3145,8 @@ export function a(a) { .into(), file_system: &NullFileSystem, maybe_resolver: None, - maybe_module_analyzer: None, maybe_npm_resolver: None, + module_analyzer: Default::default(), }) .unwrap(); let actual = actual.js().unwrap(); @@ -3192,8 +3188,8 @@ export function a(a) { .into(), file_system: &NullFileSystem, maybe_resolver: Some(&R), - maybe_module_analyzer: None, maybe_npm_resolver: None, + module_analyzer: Default::default(), }) .unwrap(); let actual = actual.js().unwrap(); @@ -3231,8 +3227,8 @@ export function a(a) { .into(), file_system: &NullFileSystem, maybe_resolver: None, - maybe_module_analyzer: None, maybe_npm_resolver: None, + module_analyzer: Default::default(), }); assert!(result.is_ok()); } @@ -3258,8 +3254,8 @@ export function a(a) { content: code.to_vec().into(), file_system: &NullFileSystem, maybe_resolver: None, - maybe_module_analyzer: None, maybe_npm_resolver: None, + module_analyzer: Default::default(), }) .unwrap(); assert_eq!( @@ -3314,8 +3310,8 @@ export function a(a) { content: code.to_vec().into(), file_system: &NullFileSystem, maybe_resolver: None, - maybe_module_analyzer: None, maybe_npm_resolver: None, + module_analyzer: Default::default(), }) .unwrap(); assert_eq!( @@ -3351,8 +3347,8 @@ export function a(a: A): B { .into(), file_system: &NullFileSystem, maybe_resolver: None, - maybe_module_analyzer: None, maybe_npm_resolver: None, + module_analyzer: Default::default(), }) .unwrap(); assert_eq!( diff --git a/src/source/file_system.rs b/src/source/file_system.rs index cd6b7f725..99c20a44d 100644 --- a/src/source/file_system.rs +++ b/src/source/file_system.rs @@ -24,6 +24,19 @@ pub trait FileSystem { fn read_dir(&self, dir_path: &ModuleSpecifier) -> Vec; } +impl<'a> Default for &'a dyn FileSystem { + fn default() -> &'a dyn FileSystem { + #[cfg(not(target_arch = "wasm32"))] + { + &RealFileSystem + } + #[cfg(target_arch = "wasm32")] + { + &NullFileSystem + } + } +} + #[derive(Debug, Clone, Default)] pub struct NullFileSystem; diff --git a/src/source/mod.rs b/src/source/mod.rs index d5bd5cb51..c79771803 100644 --- a/src/source/mod.rs +++ b/src/source/mod.rs @@ -213,6 +213,12 @@ pub trait JsrUrlProvider { } } +impl<'a> Default for &'a dyn JsrUrlProvider { + fn default() -> &'a dyn JsrUrlProvider { + &DefaultJsrUrlProvider + } +} + #[derive(Debug, Default, Copy, Clone)] pub struct DefaultJsrUrlProvider; diff --git a/tests/helpers/test_builder.rs b/tests/helpers/test_builder.rs index 3a4f1619f..1bf835588 100644 --- a/tests/helpers/test_builder.rs +++ b/tests/helpers/test_builder.rs @@ -237,8 +237,7 @@ impl TestBuilder { &mut self.loader, deno_graph::BuildOptions { workspace_members: &self.workspace_members, - module_analyzer: Some(&capturing_analyzer), - module_parser: Some(&capturing_analyzer), + module_analyzer: &capturing_analyzer, npm_resolver: Some(&TestNpmResolver), ..Default::default() }, diff --git a/tests/integration_test.rs b/tests/integration_test.rs index 7d83dd460..3e41bf359 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -617,7 +617,7 @@ async fn test_dynamic_imports_with_template_arg() { vec![Url::parse("file:///dev/main.ts").unwrap()], &mut loader, BuildOptions { - file_system: Some(&file_system), + file_system: &file_system, ..Default::default() }, )