From d52ccd2834a76596c5ec0be4dd656a9f7832fc4d Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Fri, 22 Nov 2024 18:33:25 +0000 Subject: [PATCH 01/33] feat(check): compiler options from workspace members --- cli/args/mod.rs | 14 ++ cli/lsp/config.rs | 14 +- cli/lsp/language_server.rs | 1 + cli/tools/check.rs | 210 +++++++++++++++++- .../check/check_workspace/__test__.jsonc | 14 ++ .../check_workspace/check_config_flag.out | 11 + .../check/check_workspace/check_discover.out | 9 + tests/specs/check/check_workspace/deno.json | 3 + tests/specs/check/check_workspace/main.ts | 8 + .../check/check_workspace/member/deno.json | 5 + .../specs/check/check_workspace/member/mod.ts | 5 + 11 files changed, 286 insertions(+), 8 deletions(-) create mode 100644 tests/specs/check/check_workspace/__test__.jsonc create mode 100644 tests/specs/check/check_workspace/check_config_flag.out create mode 100644 tests/specs/check/check_workspace/check_discover.out create mode 100644 tests/specs/check/check_workspace/deno.json create mode 100644 tests/specs/check/check_workspace/main.ts create mode 100644 tests/specs/check/check_workspace/member/deno.json create mode 100644 tests/specs/check/check_workspace/member/mod.ts diff --git a/cli/args/mod.rs b/cli/args/mod.rs index a1a9c49cbe..901c2f7c08 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -68,6 +68,7 @@ use once_cell::sync::Lazy; use serde::Deserialize; use serde::Serialize; use std::borrow::Cow; +use std::collections::BTreeSet; use std::collections::HashMap; use std::env; use std::io::BufReader; @@ -790,6 +791,15 @@ struct CliOptionOverrides { import_map_specifier: Option>, } +/// Overrides for the options below that when set will +/// use these values over the values derived from the +/// CLI flags or config file. +#[derive(Debug, Clone)] +pub struct ScopeOptions { + pub scope: Option>, + pub all_scopes: Arc>>, +} + /// Holds the resolved options of many sources used by subcommands /// and provides some helper function for creating common objects. pub struct CliOptions { @@ -804,6 +814,7 @@ pub struct CliOptions { overrides: CliOptionOverrides, pub start_dir: Arc, pub deno_dir_provider: Arc, + pub scope_options: Option>, } impl CliOptions { @@ -814,6 +825,7 @@ impl CliOptions { npmrc: Arc, start_dir: Arc, force_global_cache: bool, + scope_options: Option>, ) -> Result { if let Some(insecure_allowlist) = flags.unsafely_ignore_certificate_errors.as_ref() @@ -853,6 +865,7 @@ impl CliOptions { main_module_cell: std::sync::OnceLock::new(), start_dir, deno_dir_provider, + scope_options, }) } @@ -937,6 +950,7 @@ impl CliOptions { npmrc, Arc::new(start_dir), false, + None, ) } diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index ea77e36bcf..e363e75bdb 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -1410,9 +1410,17 @@ impl ConfigData { .unwrap_or_default(), ); - let ts_config = LspTsConfig::new( - member_dir.workspace.root_deno_json().map(|c| c.as_ref()), - ); + // TODO(nayeemrmn): This is a hack to get member-specific compiler options. + let ts_config = if let Some(config_file) = member_dir + .maybe_deno_json() + .filter(|c| c.json.compiler_options.is_some()) + { + LspTsConfig::new(Some(config_file)) + } else { + LspTsConfig::new( + member_dir.workspace.root_deno_json().map(|c| c.as_ref()), + ) + }; let deno_lint_config = if ts_config.inner.0.get("jsx").and_then(|v| v.as_str()) == Some("react") diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index cbe194e14e..a9ffcd9682 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -3681,6 +3681,7 @@ impl Inner { .unwrap_or_else(create_default_npmrc), workspace, force_global_cache, + None, )?; let open_docs = self.documents.documents(DocumentsFilter::OpenDiagnosable); diff --git a/cli/tools/check.rs b/cli/tools/check.rs index ad5c7c3ab1..ce7e66ccf4 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -1,11 +1,16 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::collections::HashSet; use std::collections::VecDeque; use std::sync::Arc; use deno_ast::MediaType; use deno_ast::ModuleSpecifier; +use deno_config::deno_json::get_ts_config_for_emit; +use deno_config::glob::PathOrPattern; +use deno_core::anyhow::anyhow; use deno_core::error::AnyError; use deno_graph::Module; use deno_graph::ModuleGraph; @@ -15,9 +20,15 @@ use once_cell::sync::Lazy; use regex::Regex; use crate::args::check_warn_tsconfig; +use crate::args::discover_npmrc_from_workspace; use crate::args::CheckFlags; +use crate::args::CliLockfile; use crate::args::CliOptions; +use crate::args::ConfigFlag; +use crate::args::FileFlags; use crate::args::Flags; +use crate::args::LintFlags; +use crate::args::ScopeOptions; use crate::args::TsConfig; use crate::args::TsConfigType; use crate::args::TsTypeLib; @@ -40,6 +51,159 @@ pub async fn check( flags: Arc, check_flags: CheckFlags, ) -> Result<(), AnyError> { + let is_discovered_config = match flags.config_flag { + ConfigFlag::Discover => true, + ConfigFlag::Path(_) => false, + ConfigFlag::Disabled => false, + }; + if is_discovered_config { + let factory = CliFactory::from_flags(flags.clone()); + let cli_options = factory.cli_options()?; + let (remote_files, files) = check_flags + .files + .iter() + .cloned() + .partition::, _>(|f| { + f.starts_with("http://") + || f.starts_with("https://") + || f.starts_with("npm:") + || f.starts_with("jsr:") + }); + let cwd_prefix = format!( + "{}{}", + cli_options.initial_cwd().to_string_lossy(), + std::path::MAIN_SEPARATOR + ); + // TODO(nayeemrmn): Using lint options for now. Add proper API to deno_config. + let mut by_workspace_directory = cli_options + .resolve_lint_options_for_members(&LintFlags { + files: FileFlags { + ignore: Default::default(), + include: files, + }, + ..Default::default() + })? + .into_iter() + .map(|(d, o)| { + let files = o + .files + .include + .iter() + .flat_map(|p| { + p.inner().iter().flat_map(|p| match p { + PathOrPattern::NegatedPath(_) => None, + PathOrPattern::Path(p) => Some(p.to_string_lossy().to_string()), + PathOrPattern::Pattern(p) => { + // TODO(nayeemrmn): Absolute globs don't work for specifier + // collection, we make them relative here for now. + let s = p.as_str(); + Some(s.strip_prefix(&cwd_prefix).unwrap_or(&s).to_string()) + } + PathOrPattern::RemoteUrl(_) => None, + }) + }) + .collect::>(); + (d.dir_url().clone(), (Arc::new(d), files)) + }) + .collect::>(); + if !remote_files.is_empty() { + by_workspace_directory + .entry(cli_options.start_dir.dir_url().clone()) + .or_insert((cli_options.start_dir.clone(), vec![])) + .1 + .extend(remote_files); + } + let all_scopes = Arc::new( + by_workspace_directory + .iter() + .filter(|(_, (d, _))| d.has_deno_or_pkg_json()) + .map(|(s, (_, _))| s.clone()) + .collect::>(), + ); + let dir_count = by_workspace_directory.len(); + let mut check_errors = vec![]; + let mut found_specifiers = false; + for (dir_url, (workspace_directory, files)) in by_workspace_directory { + let check_flags = CheckFlags { + files, + doc: check_flags.doc, + doc_only: check_flags.doc_only, + }; + let (npmrc, _) = + discover_npmrc_from_workspace(&workspace_directory.workspace)?; + let lockfile = + CliLockfile::discover(&flags, &workspace_directory.workspace)?; + let scope_options = (dir_count > 1).then(|| ScopeOptions { + scope: workspace_directory + .has_deno_or_pkg_json() + .then(|| dir_url.clone()), + all_scopes: all_scopes.clone(), + }); + let cli_options = CliOptions::new( + flags.clone(), + cli_options.initial_cwd().to_path_buf(), + lockfile.map(Arc::new), + npmrc, + workspace_directory, + false, + scope_options.map(Arc::new), + )?; + let factory = CliFactory::from_cli_options(Arc::new(cli_options)); + let main_graph_container = factory.main_module_graph_container().await?; + let specifiers = + main_graph_container.collect_specifiers(&check_flags.files)?; + if specifiers.is_empty() { + continue; + } else { + found_specifiers = true; + } + let specifiers_for_typecheck = if check_flags.doc || check_flags.doc_only + { + let file_fetcher = factory.file_fetcher()?; + let root_permissions = factory.root_permissions_container()?; + let mut specifiers_for_typecheck = if check_flags.doc { + specifiers.clone() + } else { + vec![] + }; + for s in specifiers { + let file = file_fetcher.fetch(&s, root_permissions).await?; + let snippet_files = extract::extract_snippet_files(file)?; + for snippet_file in snippet_files { + specifiers_for_typecheck.push(snippet_file.specifier.clone()); + file_fetcher.insert_memory_files(snippet_file); + } + } + + specifiers_for_typecheck + } else { + specifiers + }; + if let Err(err) = main_graph_container + .check_specifiers(&specifiers_for_typecheck, None) + .await + { + check_errors.push(err); + } + } + if !found_specifiers { + log::warn!("{} No matching files found.", colors::yellow("Warning")); + } + if !check_errors.is_empty() { + // TODO(nayeemrmn): More integrated way of concatenating diagnostics from + // different checks. + return Err(anyhow!( + "{}", + check_errors + .into_iter() + .map(|e| e.to_string()) + .collect::>() + .join("\n\n"), + )); + } + return Ok(()); + } + let factory = CliFactory::from_flags(flags); let main_graph_container = factory.main_module_graph_container().await?; @@ -168,9 +332,22 @@ impl TypeChecker { } log::debug!("Type checking."); - let ts_config_result = self + // TODO(nayeemrmn): This is a hack to get member-specific compiler options. + let ts_config_result = if let Some(config_file) = self .cli_options - .resolve_ts_config_for_emit(TsConfigType::Check { lib: options.lib })?; + .start_dir + .maybe_deno_json() + .filter(|c| c.json.compiler_options.is_some()) + { + get_ts_config_for_emit( + TsConfigType::Check { lib: options.lib }, + Some(config_file), + )? + } else { + self + .cli_options + .resolve_ts_config_for_emit(TsConfigType::Check { lib: options.lib })? + }; if options.log_ignored_options { check_warn_tsconfig(&ts_config_result); } @@ -259,10 +436,33 @@ impl TypeChecker { let mut diagnostics = response.diagnostics.filter(|d| { if self.is_remote_diagnostic(d) { - type_check_mode == TypeCheckMode::All && d.include_when_remote() - } else { - true + return type_check_mode == TypeCheckMode::All + && d.include_when_remote() + && self + .cli_options + .scope_options + .as_ref() + .map(|o| o.scope.is_none()) + .unwrap_or(true); } + let Some(scope_options) = &self.cli_options.scope_options else { + return true; + }; + let Some(specifier) = d + .file_name + .as_ref() + .and_then(|s| ModuleSpecifier::parse(s).ok()) + else { + return true; + }; + if specifier.scheme() != "file" { + return true; + } + let scope = scope_options + .all_scopes + .iter() + .rfind(|s| specifier.as_str().starts_with(s.as_str())); + scope == scope_options.scope.as_ref() }); diagnostics.apply_fast_check_source_maps(&graph); diff --git a/tests/specs/check/check_workspace/__test__.jsonc b/tests/specs/check/check_workspace/__test__.jsonc new file mode 100644 index 0000000000..08f50df7db --- /dev/null +++ b/tests/specs/check/check_workspace/__test__.jsonc @@ -0,0 +1,14 @@ +{ + "tests": { + "discover": { + "args": "check --quiet main.ts member/mod.ts", + "output": "check_discover.out", + "exitCode": 1 + }, + "config_flag": { + "args": "check --quiet --config deno.json main.ts member/mod.ts", + "output": "check_config_flag.out", + "exitCode": 1 + } + } +} diff --git a/tests/specs/check/check_workspace/check_config_flag.out b/tests/specs/check/check_workspace/check_config_flag.out new file mode 100644 index 0000000000..519f60b88b --- /dev/null +++ b/tests/specs/check/check_workspace/check_config_flag.out @@ -0,0 +1,11 @@ +error: TS2304 [ERROR]: Cannot find name 'onmessage'. +onmessage; +~~~~~~~~~ + at file:///home/nayeem/projects/deno/tests/specs/check/check_workspace/main.ts:8:1 + +TS2304 [ERROR]: Cannot find name 'onmessage'. +onmessage; +~~~~~~~~~ + at file:///home/nayeem/projects/deno/tests/specs/check/check_workspace/member/mod.ts:5:1 + +Found 2 errors. diff --git a/tests/specs/check/check_workspace/check_discover.out b/tests/specs/check/check_workspace/check_discover.out new file mode 100644 index 0000000000..14c733c9c0 --- /dev/null +++ b/tests/specs/check/check_workspace/check_discover.out @@ -0,0 +1,9 @@ +error: TS2304 [ERROR]: Cannot find name 'onmessage'. +onmessage; +~~~~~~~~~ + at file:///[WILDCARD]/main.ts:8:1 + +TS2304 [ERROR]: Cannot find name 'localStorage'. +localStorage; +~~~~~~~~~~~~ + at file:///[WILDCARD]/member/mod.ts:2:1 diff --git a/tests/specs/check/check_workspace/deno.json b/tests/specs/check/check_workspace/deno.json new file mode 100644 index 0000000000..c914638468 --- /dev/null +++ b/tests/specs/check/check_workspace/deno.json @@ -0,0 +1,3 @@ +{ + "workspace": ["member"] +} diff --git a/tests/specs/check/check_workspace/main.ts b/tests/specs/check/check_workspace/main.ts new file mode 100644 index 0000000000..713e64febe --- /dev/null +++ b/tests/specs/check/check_workspace/main.ts @@ -0,0 +1,8 @@ +// We shouldn't get diagnostics from this import under this check scope. +import "./member/mod.ts"; + +// Only defined for window. +localStorage; + +// Only defined for worker. +onmessage; diff --git a/tests/specs/check/check_workspace/member/deno.json b/tests/specs/check/check_workspace/member/deno.json new file mode 100644 index 0000000000..00feda1e44 --- /dev/null +++ b/tests/specs/check/check_workspace/member/deno.json @@ -0,0 +1,5 @@ +{ + "compilerOptions": { + "lib": ["deno.worker"] + } +} diff --git a/tests/specs/check/check_workspace/member/mod.ts b/tests/specs/check/check_workspace/member/mod.ts new file mode 100644 index 0000000000..846c13a74a --- /dev/null +++ b/tests/specs/check/check_workspace/member/mod.ts @@ -0,0 +1,5 @@ +// Only defined for window. +localStorage; + +// Only defined for worker. +onmessage; From 88f7510b95e941ff1206ecb8f9b1a3b1dedfff86 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Fri, 22 Nov 2024 19:23:53 +0000 Subject: [PATCH 02/33] wildcard --- tests/specs/check/check_workspace/check_config_flag.out | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/specs/check/check_workspace/check_config_flag.out b/tests/specs/check/check_workspace/check_config_flag.out index 519f60b88b..a586465d80 100644 --- a/tests/specs/check/check_workspace/check_config_flag.out +++ b/tests/specs/check/check_workspace/check_config_flag.out @@ -1,11 +1,11 @@ error: TS2304 [ERROR]: Cannot find name 'onmessage'. onmessage; ~~~~~~~~~ - at file:///home/nayeem/projects/deno/tests/specs/check/check_workspace/main.ts:8:1 + at file:///[WILDCARD]/main.ts:8:1 TS2304 [ERROR]: Cannot find name 'onmessage'. onmessage; ~~~~~~~~~ - at file:///home/nayeem/projects/deno/tests/specs/check/check_workspace/member/mod.ts:5:1 + at file:///[WILDCARD]/member/mod.ts:5:1 Found 2 errors. From 7a3a73c782ad9e519f2b22d2362dad9f9c5b00a9 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 26 Nov 2024 06:40:26 +0000 Subject: [PATCH 03/33] fix file patterns --- cli/tools/check.rs | 65 +++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 39 deletions(-) diff --git a/cli/tools/check.rs b/cli/tools/check.rs index ce7e66ccf4..cad3cc4239 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -9,6 +9,7 @@ use std::sync::Arc; use deno_ast::MediaType; use deno_ast::ModuleSpecifier; use deno_config::deno_json::get_ts_config_for_emit; +use deno_config::glob::FilePatterns; use deno_config::glob::PathOrPattern; use deno_core::anyhow::anyhow; use deno_core::error::AnyError; @@ -45,6 +46,8 @@ use crate::tsc; use crate::tsc::Diagnostics; use crate::tsc::TypeCheckingCjsTracker; use crate::util::extract; +use crate::util::fs::collect_specifiers; +use crate::util::path::is_script_ext; use crate::util::path::to_percent_decoded_str; pub async fn check( @@ -69,11 +72,6 @@ pub async fn check( || f.starts_with("npm:") || f.starts_with("jsr:") }); - let cwd_prefix = format!( - "{}{}", - cli_options.initial_cwd().to_string_lossy(), - std::path::MAIN_SEPARATOR - ); // TODO(nayeemrmn): Using lint options for now. Add proper API to deno_config. let mut by_workspace_directory = cli_options .resolve_lint_options_for_members(&LintFlags { @@ -84,34 +82,21 @@ pub async fn check( ..Default::default() })? .into_iter() - .map(|(d, o)| { - let files = o - .files - .include - .iter() - .flat_map(|p| { - p.inner().iter().flat_map(|p| match p { - PathOrPattern::NegatedPath(_) => None, - PathOrPattern::Path(p) => Some(p.to_string_lossy().to_string()), - PathOrPattern::Pattern(p) => { - // TODO(nayeemrmn): Absolute globs don't work for specifier - // collection, we make them relative here for now. - let s = p.as_str(); - Some(s.strip_prefix(&cwd_prefix).unwrap_or(&s).to_string()) - } - PathOrPattern::RemoteUrl(_) => None, - }) - }) - .collect::>(); - (d.dir_url().clone(), (Arc::new(d), files)) + .flat_map(|(d, o)| { + Some((d.dir_url().clone(), (Arc::new(d), o.files.include?))) }) .collect::>(); if !remote_files.is_empty() { by_workspace_directory .entry(cli_options.start_dir.dir_url().clone()) - .or_insert((cli_options.start_dir.clone(), vec![])) + .or_insert((cli_options.start_dir.clone(), Default::default())) .1 - .extend(remote_files); + .append( + remote_files + .iter() + .flat_map(|s| ModuleSpecifier::parse(s).ok()) + .map(PathOrPattern::RemoteUrl), + ); } let all_scopes = Arc::new( by_workspace_directory @@ -121,14 +106,10 @@ pub async fn check( .collect::>(), ); let dir_count = by_workspace_directory.len(); + let initial_cwd = cli_options.initial_cwd().to_path_buf(); let mut check_errors = vec![]; let mut found_specifiers = false; - for (dir_url, (workspace_directory, files)) in by_workspace_directory { - let check_flags = CheckFlags { - files, - doc: check_flags.doc, - doc_only: check_flags.doc_only, - }; + for (dir_url, (workspace_directory, patterns)) in by_workspace_directory { let (npmrc, _) = discover_npmrc_from_workspace(&workspace_directory.workspace)?; let lockfile = @@ -141,22 +122,29 @@ pub async fn check( }); let cli_options = CliOptions::new( flags.clone(), - cli_options.initial_cwd().to_path_buf(), + initial_cwd.clone(), lockfile.map(Arc::new), npmrc, workspace_directory, false, scope_options.map(Arc::new), )?; - let factory = CliFactory::from_cli_options(Arc::new(cli_options)); - let main_graph_container = factory.main_module_graph_container().await?; - let specifiers = - main_graph_container.collect_specifiers(&check_flags.files)?; + let specifiers = collect_specifiers( + FilePatterns { + include: Some(patterns), + exclude: cli_options.workspace().resolve_config_excludes()?, + base: initial_cwd.clone(), + }, + cli_options.vendor_dir_path().map(ToOwned::to_owned), + |e| is_script_ext(e.path), + )?; if specifiers.is_empty() { continue; } else { found_specifiers = true; } + let factory = CliFactory::from_cli_options(Arc::new(cli_options)); + let main_graph_container = factory.main_module_graph_container().await?; let specifiers_for_typecheck = if check_flags.doc || check_flags.doc_only { let file_fetcher = factory.file_fetcher()?; @@ -174,7 +162,6 @@ pub async fn check( file_fetcher.insert_memory_files(snippet_file); } } - specifiers_for_typecheck } else { specifiers From 27561dd48542be97e1641ebbe0ae03e4acc0e93e Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 26 Nov 2024 07:44:54 +0000 Subject: [PATCH 04/33] smart diagnostics concatenation --- cli/graph_container.rs | 5 +- cli/graph_util.rs | 1 + cli/module_loader.rs | 3 +- cli/tools/check.rs | 56 +++++++++++++++---- cli/tsc/diagnostics.rs | 2 +- .../check/check_workspace/check_discover.out | 2 + 6 files changed, 54 insertions(+), 15 deletions(-) diff --git a/cli/graph_container.rs b/cli/graph_container.rs index c463d71a6a..ba8b649698 100644 --- a/cli/graph_container.rs +++ b/cli/graph_container.rs @@ -13,6 +13,7 @@ use deno_runtime::deno_permissions::PermissionsContainer; use crate::args::CliOptions; use crate::module_loader::ModuleLoadPreparer; +use crate::tools::check::MaybeDiagnostics; use crate::util::fs::collect_specifiers; use crate::util::path::is_script_ext; @@ -69,7 +70,7 @@ impl MainModuleGraphContainer { &self, specifiers: &[ModuleSpecifier], ext_overwrite: Option<&String>, - ) -> Result<(), AnyError> { + ) -> Result<(), MaybeDiagnostics> { let mut graph_permit = self.acquire_update_permit().await; let graph = graph_permit.graph_mut(); self @@ -99,7 +100,7 @@ impl MainModuleGraphContainer { log::warn!("{} No matching files found.", colors::yellow("Warning")); } - self.check_specifiers(&specifiers, None).await + Ok(self.check_specifiers(&specifiers, None).await?) } pub fn collect_specifiers( diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 6ed0506dd7..059c069c53 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -371,6 +371,7 @@ impl ModuleGraphCreator { }, ) .await + .map_err(AnyError::from) } } diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 035ae4264b..eb7c4cfc30 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -33,6 +33,7 @@ use crate::resolver::ModuleCodeStringSource; use crate::resolver::NotSupportedKindInNpmError; use crate::resolver::NpmModuleLoader; use crate::tools::check; +use crate::tools::check::MaybeDiagnostics; use crate::tools::check::TypeChecker; use crate::util::progress_bar::ProgressBar; use crate::util::text_encoding::code_without_source_map; @@ -117,7 +118,7 @@ impl ModuleLoadPreparer { lib: TsTypeLib, permissions: PermissionsContainer, ext_overwrite: Option<&String>, - ) -> Result<(), AnyError> { + ) -> Result<(), MaybeDiagnostics> { log::debug!("Preparing module load."); let _pb_clear_guard = self.progress_bar.clear_guard(); diff --git a/cli/tools/check.rs b/cli/tools/check.rs index cad3cc4239..125d58b30f 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -4,6 +4,8 @@ use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashSet; use std::collections::VecDeque; +use std::error::Error; +use std::fmt; use std::sync::Arc; use deno_ast::MediaType; @@ -107,7 +109,8 @@ pub async fn check( ); let dir_count = by_workspace_directory.len(); let initial_cwd = cli_options.initial_cwd().to_path_buf(); - let mut check_errors = vec![]; + let mut diagnostics = vec![]; + let mut all_errors = vec![]; let mut found_specifiers = false; for (dir_url, (workspace_directory, patterns)) in by_workspace_directory { let (npmrc, _) = @@ -170,18 +173,24 @@ pub async fn check( .check_specifiers(&specifiers_for_typecheck, None) .await { - check_errors.push(err); + match err { + MaybeDiagnostics::Diagnostics(Diagnostics(d)) => { + diagnostics.extend(d) + } + MaybeDiagnostics::Other(err) => all_errors.push(err), + } } } if !found_specifiers { log::warn!("{} No matching files found.", colors::yellow("Warning")); } - if !check_errors.is_empty() { - // TODO(nayeemrmn): More integrated way of concatenating diagnostics from - // different checks. + if !diagnostics.is_empty() { + all_errors.push(AnyError::from(Diagnostics(diagnostics))); + } + if !all_errors.is_empty() { return Err(anyhow!( "{}", - check_errors + all_errors .into_iter() .map(|e| e.to_string()) .collect::>() @@ -225,9 +234,11 @@ pub async fn check( specifiers }; - main_graph_container - .check_specifiers(&specifiers_for_typecheck, None) - .await + Ok( + main_graph_container + .check_specifiers(&specifiers_for_typecheck, None) + .await?, + ) } /// Options for performing a check of a module graph. Note that the decision to @@ -249,6 +260,29 @@ pub struct CheckOptions { pub type_check_mode: TypeCheckMode, } +#[derive(Debug)] +pub enum MaybeDiagnostics { + Diagnostics(Diagnostics), + Other(AnyError), +} + +impl fmt::Display for MaybeDiagnostics { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + MaybeDiagnostics::Diagnostics(d) => d.fmt(f), + MaybeDiagnostics::Other(err) => err.fmt(f), + } + } +} + +impl Error for MaybeDiagnostics {} + +impl From for MaybeDiagnostics { + fn from(err: AnyError) -> Self { + MaybeDiagnostics::Other(err) + } +} + pub struct TypeChecker { caches: Arc, cjs_tracker: Arc, @@ -285,14 +319,14 @@ impl TypeChecker { &self, graph: ModuleGraph, options: CheckOptions, - ) -> Result, AnyError> { + ) -> Result, MaybeDiagnostics> { let (graph, mut diagnostics) = self.check_diagnostics(graph, options).await?; diagnostics.emit_warnings(); if diagnostics.is_empty() { Ok(graph) } else { - Err(diagnostics.into()) + Err(MaybeDiagnostics::Diagnostics(diagnostics)) } } diff --git a/cli/tsc/diagnostics.rs b/cli/tsc/diagnostics.rs index d3795706eb..1a987f0e0f 100644 --- a/cli/tsc/diagnostics.rs +++ b/cli/tsc/diagnostics.rs @@ -274,7 +274,7 @@ impl fmt::Display for Diagnostic { } #[derive(Clone, Debug, Default, Eq, PartialEq)] -pub struct Diagnostics(Vec); +pub struct Diagnostics(pub Vec); impl Diagnostics { #[cfg(test)] diff --git a/tests/specs/check/check_workspace/check_discover.out b/tests/specs/check/check_workspace/check_discover.out index 14c733c9c0..9a8ce47280 100644 --- a/tests/specs/check/check_workspace/check_discover.out +++ b/tests/specs/check/check_workspace/check_discover.out @@ -7,3 +7,5 @@ TS2304 [ERROR]: Cannot find name 'localStorage'. localStorage; ~~~~~~~~~~~~ at file:///[WILDCARD]/member/mod.ts:2:1 + +Found 2 errors. From 6e3ceddf17b3a541f5c0588d966204f8853485fa Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 27 Nov 2024 19:11:45 +0000 Subject: [PATCH 05/33] use deno_config temp branch --- Cargo.lock | 3 +-- Cargo.toml | 3 ++- cli/args/mod.rs | 17 ++++++++++++--- cli/graph_util.rs | 2 +- cli/lsp/config.rs | 17 +++++---------- cli/lsp/resolver.rs | 2 +- cli/tools/check.rs | 53 +++++++++++++++------------------------------ 7 files changed, 42 insertions(+), 55 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fc567b4a38..85a7aa8777 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1430,8 +1430,7 @@ dependencies = [ [[package]] name = "deno_config" version = "0.39.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "38fb809500238be2b10eee42944a47b3ac38974e1edbb47f73afcfca7df143bf" +source = "git+https://github.com/denoland/deno_config.git?rev=1a5c07c05085f0cb922bdf0a899589b1c720ea60#1a5c07c05085f0cb922bdf0a899589b1c720ea60" dependencies = [ "anyhow", "deno_package_json", diff --git a/Cargo.toml b/Cargo.toml index 652d55e071..6017cf8a29 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -50,7 +50,8 @@ deno_ast = { version = "=0.43.3", features = ["transpiling"] } deno_core = { version = "0.323.0" } deno_bench_util = { version = "0.173.0", path = "./bench_util" } -deno_config = { version = "=0.39.2", features = ["workspace", "sync"] } +# TODO(nayeemrmn): Use proper version when https://github.com/denoland/deno_config/pull/143 lands! +deno_config = { git = "https://github.com/denoland/deno_config.git", rev = "1a5c07c05085f0cb922bdf0a899589b1c720ea60", features = ["workspace", "sync"] } deno_lockfile = "=0.23.1" deno_media_type = { version = "0.2.0", features = ["module_specifier"] } deno_npm = "=0.25.4" diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 9bba007e31..095669543e 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1266,7 +1266,7 @@ impl CliOptions { &self, config_type: TsConfigType, ) -> Result { - self.workspace().resolve_ts_config_for_emit(config_type) + self.start_dir.to_ts_config_for_emit(config_type) } pub fn resolve_inspector_server( @@ -1296,7 +1296,7 @@ impl CliOptions { &self, ) -> Result, AnyError> { self - .workspace() + .start_dir .to_compiler_option_types() .map(|maybe_imports| { maybe_imports @@ -1347,6 +1347,17 @@ impl CliOptions { WorkspaceLintOptions::resolve(&lint_config, lint_flags) } + pub fn resolve_file_flags_for_members( + &self, + file_flags: &FileFlags, + ) -> Result, AnyError> { + let cli_arg_patterns = file_flags.as_file_patterns(self.initial_cwd())?; + let member_patterns = self + .workspace() + .resolve_file_patterns_for_members(&cli_arg_patterns)?; + Ok(member_patterns) + } + pub fn resolve_lint_options_for_members( &self, lint_flags: &LintFlags, @@ -1445,7 +1456,7 @@ impl CliOptions { } pub fn check_js(&self) -> bool { - self.workspace().check_js() + self.start_dir.check_js() } pub fn coverage_dir(&self) -> Option { diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 1bbb7d469b..690331b50f 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -750,7 +750,7 @@ impl ModuleGraphBuilder { fn create_graph_resolver(&self) -> Result { let jsx_import_source_config = self .cli_options - .workspace() + .start_dir .to_maybe_jsx_import_source_config()?; Ok(CliGraphResolver { cjs_tracker: &self.cjs_tracker, diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index e363e75bdb..0bda1f80ae 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -1410,17 +1410,11 @@ impl ConfigData { .unwrap_or_default(), ); - // TODO(nayeemrmn): This is a hack to get member-specific compiler options. - let ts_config = if let Some(config_file) = member_dir - .maybe_deno_json() - .filter(|c| c.json.compiler_options.is_some()) - { - LspTsConfig::new(Some(config_file)) - } else { - LspTsConfig::new( - member_dir.workspace.root_deno_json().map(|c| c.as_ref()), - ) - }; + let ts_config = LspTsConfig::new( + member_dir + .deno_json_for_compiler_options() + .map(|c| c.as_ref()), + ); let deno_lint_config = if ts_config.inner.0.get("jsx").and_then(|v| v.as_str()) == Some("react") @@ -1668,7 +1662,6 @@ impl ConfigData { ) -> Option { self .member_dir - .workspace .to_maybe_jsx_import_source_config() .ok() .flatten() diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index c705511f30..52e03afe52 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -139,7 +139,7 @@ impl LspScopeResolver { let maybe_jsx_import_source_config = config_data.and_then(|d| d.maybe_jsx_import_source_config()); let graph_imports = config_data - .and_then(|d| d.member_dir.workspace.to_compiler_option_types().ok()) + .and_then(|d| d.member_dir.to_compiler_option_types().ok()) .map(|imports| { Arc::new( imports diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 125d58b30f..04ccc80b09 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -10,7 +10,6 @@ use std::sync::Arc; use deno_ast::MediaType; use deno_ast::ModuleSpecifier; -use deno_config::deno_json::get_ts_config_for_emit; use deno_config::glob::FilePatterns; use deno_config::glob::PathOrPattern; use deno_core::anyhow::anyhow; @@ -30,7 +29,6 @@ use crate::args::CliOptions; use crate::args::ConfigFlag; use crate::args::FileFlags; use crate::args::Flags; -use crate::args::LintFlags; use crate::args::ScopeOptions; use crate::args::TsConfig; use crate::args::TsConfigType; @@ -74,25 +72,28 @@ pub async fn check( || f.starts_with("npm:") || f.starts_with("jsr:") }); - // TODO(nayeemrmn): Using lint options for now. Add proper API to deno_config. let mut by_workspace_directory = cli_options - .resolve_lint_options_for_members(&LintFlags { - files: FileFlags { - ignore: Default::default(), - include: files, - }, - ..Default::default() + .resolve_file_flags_for_members(&FileFlags { + ignore: Default::default(), + include: files, })? .into_iter() - .flat_map(|(d, o)| { - Some((d.dir_url().clone(), (Arc::new(d), o.files.include?))) - }) + .map(|(d, p)| (d.dir_url().clone(), (Arc::new(d), p))) .collect::>(); if !remote_files.is_empty() { by_workspace_directory .entry(cli_options.start_dir.dir_url().clone()) - .or_insert((cli_options.start_dir.clone(), Default::default())) + .or_insert(( + cli_options.start_dir.clone(), + FilePatterns { + base: cli_options.initial_cwd().to_path_buf(), + include: None, + exclude: Default::default(), + }, + )) .1 + .include + .get_or_insert_with(Default::default) .append( remote_files .iter() @@ -108,7 +109,6 @@ pub async fn check( .collect::>(), ); let dir_count = by_workspace_directory.len(); - let initial_cwd = cli_options.initial_cwd().to_path_buf(); let mut diagnostics = vec![]; let mut all_errors = vec![]; let mut found_specifiers = false; @@ -125,7 +125,7 @@ pub async fn check( }); let cli_options = CliOptions::new( flags.clone(), - initial_cwd.clone(), + cli_options.initial_cwd().to_path_buf(), lockfile.map(Arc::new), npmrc, workspace_directory, @@ -133,11 +133,7 @@ pub async fn check( scope_options.map(Arc::new), )?; let specifiers = collect_specifiers( - FilePatterns { - include: Some(patterns), - exclude: cli_options.workspace().resolve_config_excludes()?, - base: initial_cwd.clone(), - }, + patterns, cli_options.vendor_dir_path().map(ToOwned::to_owned), |e| is_script_ext(e.path), )?; @@ -353,22 +349,9 @@ impl TypeChecker { } log::debug!("Type checking."); - // TODO(nayeemrmn): This is a hack to get member-specific compiler options. - let ts_config_result = if let Some(config_file) = self + let ts_config_result = self .cli_options - .start_dir - .maybe_deno_json() - .filter(|c| c.json.compiler_options.is_some()) - { - get_ts_config_for_emit( - TsConfigType::Check { lib: options.lib }, - Some(config_file), - )? - } else { - self - .cli_options - .resolve_ts_config_for_emit(TsConfigType::Check { lib: options.lib })? - }; + .resolve_ts_config_for_emit(TsConfigType::Check { lib: options.lib })?; if options.log_ignored_options { check_warn_tsconfig(&ts_config_result); } From 3e9a63f8ce5224b66af8d00684d287978b5ff0d8 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 27 Nov 2024 20:51:17 +0000 Subject: [PATCH 06/33] fix --- Cargo.lock | 2 +- Cargo.toml | 2 +- cli/tools/check.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 85a7aa8777..ee6efadaa2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1430,7 +1430,7 @@ dependencies = [ [[package]] name = "deno_config" version = "0.39.2" -source = "git+https://github.com/denoland/deno_config.git?rev=1a5c07c05085f0cb922bdf0a899589b1c720ea60#1a5c07c05085f0cb922bdf0a899589b1c720ea60" +source = "git+https://github.com/denoland/deno_config.git?rev=485e7a71f1057437d2a2c4adddbfbe348a812667#485e7a71f1057437d2a2c4adddbfbe348a812667" dependencies = [ "anyhow", "deno_package_json", diff --git a/Cargo.toml b/Cargo.toml index 6017cf8a29..1b22e1c23f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,7 +51,7 @@ deno_core = { version = "0.323.0" } deno_bench_util = { version = "0.173.0", path = "./bench_util" } # TODO(nayeemrmn): Use proper version when https://github.com/denoland/deno_config/pull/143 lands! -deno_config = { git = "https://github.com/denoland/deno_config.git", rev = "1a5c07c05085f0cb922bdf0a899589b1c720ea60", features = ["workspace", "sync"] } +deno_config = { git = "https://github.com/denoland/deno_config.git", rev = "485e7a71f1057437d2a2c4adddbfbe348a812667", features = ["workspace", "sync"] } deno_lockfile = "=0.23.1" deno_media_type = { version = "0.2.0", features = ["module_specifier"] } deno_npm = "=0.25.4" diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 04ccc80b09..657d8c1f2c 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -87,7 +87,7 @@ pub async fn check( cli_options.start_dir.clone(), FilePatterns { base: cli_options.initial_cwd().to_path_buf(), - include: None, + include: Some(Default::default()), exclude: Default::default(), }, )) From 6b73819d12ae87503f9accf43e99ac2468ce63a3 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 27 Nov 2024 21:11:04 +0000 Subject: [PATCH 07/33] fix fixture --- tests/specs/workspaces/non_fatal_diagnostics/lint.out | 2 +- tests/specs/workspaces/non_fatal_diagnostics/sub/deno.json | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/specs/workspaces/non_fatal_diagnostics/lint.out b/tests/specs/workspaces/non_fatal_diagnostics/lint.out index 864dc47ac4..8b4ae8edfc 100644 --- a/tests/specs/workspaces/non_fatal_diagnostics/lint.out +++ b/tests/specs/workspaces/non_fatal_diagnostics/lint.out @@ -1,4 +1,4 @@ -Warning "compilerOptions" field can only be specified in the workspace root deno.json file. +Warning "lock" field can only be specified in the workspace root deno.json file. at file:///[WILDLINE]/sub/deno.json Warning "lint.report" field can only be specified in the workspace root deno.json file. at file:///[WILDLINE]/sub/deno.json diff --git a/tests/specs/workspaces/non_fatal_diagnostics/sub/deno.json b/tests/specs/workspaces/non_fatal_diagnostics/sub/deno.json index 0a21df89f7..b0375f79af 100644 --- a/tests/specs/workspaces/non_fatal_diagnostics/sub/deno.json +++ b/tests/specs/workspaces/non_fatal_diagnostics/sub/deno.json @@ -2,7 +2,5 @@ "lint": { "report": "compact" }, - "compilerOptions": { - "strict": true - } + "lock": false } From de4e64136017c734f6d5e9ecdd258628da273648 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Thu, 5 Dec 2024 08:04:31 +0000 Subject: [PATCH 08/33] WorkspaceFileContainer --- Cargo.lock | 2 +- Cargo.toml | 2 +- cli/args/mod.rs | 2 +- cli/factory.rs | 142 +++++++++++++++++++++++++++++++++++++++++++++ cli/tools/check.rs | 115 +++++++----------------------------- 5 files changed, 165 insertions(+), 98 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d7bddb2a73..df9af4b355 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1432,7 +1432,7 @@ dependencies = [ [[package]] name = "deno_config" version = "0.39.3" -source = "git+https://github.com/denoland/deno_config.git?branch=compiler-options-from-workspace-member#81d8e844624a8f6b3961213d1480dcc923d08b11" +source = "git+https://github.com/denoland/deno_config.git?rev=81d8e844624a8f6b3961213d1480dcc923d08b11#81d8e844624a8f6b3961213d1480dcc923d08b11" dependencies = [ "anyhow", "deno_package_json", diff --git a/Cargo.toml b/Cargo.toml index a8f4e44160..a01f86aa94 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,7 +52,7 @@ deno_core = { version = "0.323.0" } deno_bench_util = { version = "0.174.0", path = "./bench_util" } # TODO(nayeemrmn): Use proper version when https://github.com/denoland/deno_config/pull/143 lands! -deno_config = { git = "https://github.com/denoland/deno_config.git", branch = "compiler-options-from-workspace-member", features = ["workspace", "sync"] } +deno_config = { git = "https://github.com/denoland/deno_config.git", rev = "81d8e844624a8f6b3961213d1480dcc923d08b11", features = ["workspace", "sync"] } deno_lockfile = "=0.23.2" deno_media_type = { version = "0.2.0", features = ["module_specifier"] } deno_npm = "=0.26.0" diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 2782ea3acd..f78cd4edc6 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -811,7 +811,7 @@ pub struct ScopeOptions { pub struct CliOptions { // the source of the options is a detail the rest of the // application need not concern itself with, so keep these private - flags: Arc, + pub flags: Arc, initial_cwd: PathBuf, main_module_cell: std::sync::OnceLock>, maybe_node_modules_folder: Option, diff --git a/cli/factory.rs b/cli/factory.rs index 6937b750f9..e1611df14f 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1,12 +1,15 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use crate::args::check_warn_tsconfig; +use crate::args::discover_npmrc_from_workspace; use crate::args::get_root_cert_store; use crate::args::CaData; +use crate::args::CliLockfile; use crate::args::CliOptions; use crate::args::DenoSubcommand; use crate::args::Flags; use crate::args::NpmInstallDepsProvider; +use crate::args::ScopeOptions; use crate::args::StorageKeyResolver; use crate::args::TsConfigType; use crate::cache::Caches; @@ -51,22 +54,30 @@ use crate::resolver::CliSloppyImportsResolver; use crate::resolver::NpmModuleLoader; use crate::resolver::SloppyImportsCachedFs; use crate::standalone::DenoCompileBinaryWriter; +use crate::tools::check::MaybeDiagnostics; use crate::tools::check::TypeChecker; use crate::tools::coverage::CoverageCollector; use crate::tools::lint::LintRuleProvider; use crate::tools::run::hmr::HmrRunner; +use crate::tsc::Diagnostics; use crate::tsc::TypeCheckingCjsTracker; +use crate::util::extract; use crate::util::file_watcher::WatcherCommunicator; use crate::util::fs::canonicalize_path_maybe_not_exists; use crate::util::progress_bar::ProgressBar; use crate::util::progress_bar::ProgressBarStyle; use crate::worker::CliMainWorkerFactory; use crate::worker::CliMainWorkerOptions; +use std::collections::BTreeSet; use std::path::PathBuf; +use deno_ast::ModuleSpecifier; use deno_cache_dir::npm::NpmCacheDir; +use deno_config::glob::FilePatterns; use deno_config::workspace::PackageJsonDepResolution; +use deno_config::workspace::WorkspaceDirectory; use deno_config::workspace::WorkspaceResolver; +use deno_core::anyhow::anyhow; use deno_core::error::AnyError; use deno_core::futures::FutureExt; use deno_core::FeatureChecker; @@ -1052,3 +1063,134 @@ impl CliFactory { }) } } + +pub struct WorkspaceFileContainerEntry { + main_graph_container: Arc, + specifiers: Vec<(ModuleSpecifier, T)>, + snippet_file_specifiers: Option>, +} + +pub struct WorkspaceFileContainer { + entries: Vec>, +} + +impl WorkspaceFileContainer { + #[allow(clippy::type_complexity)] + pub async fn from_workspace_dirs_with_files( + mut workspace_dirs_with_files: Vec<(Arc, FilePatterns)>, + collect_files: fn( + FilePatterns, + Arc, + Arc, + ) -> std::pin::Pin< + Box, AnyError>>>, + >, + cli_options: &CliOptions, + check_doc: bool, + ) -> Result { + workspace_dirs_with_files.sort_by_cached_key(|(d, _)| d.dir_url().clone()); + let all_scopes = Arc::new( + workspace_dirs_with_files + .iter() + .filter(|(d, _)| d.has_deno_or_pkg_json()) + .map(|(d, _)| d.dir_url().clone()) + .collect::>(), + ); + let dir_count = workspace_dirs_with_files.len(); + let mut entries = Vec::with_capacity(dir_count); + for (workspace_dir, files) in workspace_dirs_with_files { + let (npmrc, _) = discover_npmrc_from_workspace(&workspace_dir.workspace)?; + let lockfile = + CliLockfile::discover(&cli_options.flags, &workspace_dir.workspace)?; + let scope_options = (dir_count > 1).then(|| ScopeOptions { + scope: workspace_dir + .has_deno_or_pkg_json() + .then(|| workspace_dir.dir_url().clone()), + all_scopes: all_scopes.clone(), + }); + let cli_options = Arc::new(CliOptions::new( + cli_options.flags.clone(), + cli_options.initial_cwd().to_path_buf(), + lockfile.map(Arc::new), + npmrc, + workspace_dir, + false, + scope_options.map(Arc::new), + )?); + let factory = CliFactory::from_cli_options(cli_options.clone()); + let file_fetcher = factory.file_fetcher()?; + let main_graph_container = + factory.main_module_graph_container().await?.clone(); + let specifiers = + collect_files(files, cli_options, file_fetcher.clone()).await?; + let snippet_file_specifiers = if check_doc { + let root_permissions = factory.root_permissions_container()?; + let mut snippet_file_specifiers = Vec::new(); + for (s, _) in specifiers.iter() { + let file = file_fetcher.fetch(s, root_permissions).await?; + let snippet_files = extract::extract_snippet_files(file)?; + for snippet_file in snippet_files { + snippet_file_specifiers.push(snippet_file.specifier.clone()); + file_fetcher.insert_memory_files(snippet_file); + } + } + Some(snippet_file_specifiers) + } else { + None + }; + entries.push(WorkspaceFileContainerEntry { + main_graph_container, + specifiers, + snippet_file_specifiers, + }); + } + Ok(Self { entries }) + } + + pub async fn check(&self) -> Result<(), AnyError> { + let mut diagnostics = vec![]; + let mut all_errors = vec![]; + for entry in &self.entries { + let specifiers_for_typecheck = entry + .specifiers + .iter() + .map(|(s, _)| s) + .chain(entry.snippet_file_specifiers.iter().flatten()) + .cloned() + .collect::>(); + if specifiers_for_typecheck.is_empty() { + continue; + } + if let Err(err) = entry + .main_graph_container + .check_specifiers(&specifiers_for_typecheck, None) + .await + { + match err { + MaybeDiagnostics::Diagnostics(Diagnostics(d)) => { + diagnostics.extend(d) + } + MaybeDiagnostics::Other(err) => all_errors.push(err), + } + } + } + if !diagnostics.is_empty() { + all_errors.push(AnyError::from(Diagnostics(diagnostics))); + } + if !all_errors.is_empty() { + return Err(anyhow!( + "{}", + all_errors + .into_iter() + .map(|e| e.to_string()) + .collect::>() + .join("\n\n"), + )); + } + Ok(()) + } + + pub fn has_specifiers(&self) -> bool { + self.entries.iter().any(|e| !e.specifiers.is_empty()) + } +} diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 657d8c1f2c..53b89f21b7 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -1,7 +1,6 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use std::collections::BTreeMap; -use std::collections::BTreeSet; use std::collections::HashSet; use std::collections::VecDeque; use std::error::Error; @@ -12,8 +11,8 @@ use deno_ast::MediaType; use deno_ast::ModuleSpecifier; use deno_config::glob::FilePatterns; use deno_config::glob::PathOrPattern; -use deno_core::anyhow::anyhow; use deno_core::error::AnyError; +use deno_core::futures::FutureExt; use deno_graph::Module; use deno_graph::ModuleGraph; use deno_runtime::deno_node::NodeResolver; @@ -22,14 +21,11 @@ use once_cell::sync::Lazy; use regex::Regex; use crate::args::check_warn_tsconfig; -use crate::args::discover_npmrc_from_workspace; use crate::args::CheckFlags; -use crate::args::CliLockfile; use crate::args::CliOptions; use crate::args::ConfigFlag; use crate::args::FileFlags; use crate::args::Flags; -use crate::args::ScopeOptions; use crate::args::TsConfig; use crate::args::TsConfigType; use crate::args::TsTypeLib; @@ -39,6 +35,7 @@ use crate::cache::Caches; use crate::cache::FastInsecureHasher; use crate::cache::TypeCheckCache; use crate::factory::CliFactory; +use crate::factory::WorkspaceFileContainer; use crate::graph_util::BuildFastCheckGraphOptions; use crate::graph_util::ModuleGraphBuilder; use crate::npm::CliNpmResolver; @@ -101,99 +98,27 @@ pub async fn check( .map(PathOrPattern::RemoteUrl), ); } - let all_scopes = Arc::new( - by_workspace_directory - .iter() - .filter(|(_, (d, _))| d.has_deno_or_pkg_json()) - .map(|(s, (_, _))| s.clone()) - .collect::>(), - ); - let dir_count = by_workspace_directory.len(); - let mut diagnostics = vec![]; - let mut all_errors = vec![]; - let mut found_specifiers = false; - for (dir_url, (workspace_directory, patterns)) in by_workspace_directory { - let (npmrc, _) = - discover_npmrc_from_workspace(&workspace_directory.workspace)?; - let lockfile = - CliLockfile::discover(&flags, &workspace_directory.workspace)?; - let scope_options = (dir_count > 1).then(|| ScopeOptions { - scope: workspace_directory - .has_deno_or_pkg_json() - .then(|| dir_url.clone()), - all_scopes: all_scopes.clone(), - }); - let cli_options = CliOptions::new( - flags.clone(), - cli_options.initial_cwd().to_path_buf(), - lockfile.map(Arc::new), - npmrc, - workspace_directory, - false, - scope_options.map(Arc::new), - )?; - let specifiers = collect_specifiers( - patterns, - cli_options.vendor_dir_path().map(ToOwned::to_owned), - |e| is_script_ext(e.path), - )?; - if specifiers.is_empty() { - continue; - } else { - found_specifiers = true; - } - let factory = CliFactory::from_cli_options(Arc::new(cli_options)); - let main_graph_container = factory.main_module_graph_container().await?; - let specifiers_for_typecheck = if check_flags.doc || check_flags.doc_only - { - let file_fetcher = factory.file_fetcher()?; - let root_permissions = factory.root_permissions_container()?; - let mut specifiers_for_typecheck = if check_flags.doc { - specifiers.clone() - } else { - vec![] - }; - for s in specifiers { - let file = file_fetcher.fetch(&s, root_permissions).await?; - let snippet_files = extract::extract_snippet_files(file)?; - for snippet_file in snippet_files { - specifiers_for_typecheck.push(snippet_file.specifier.clone()); - file_fetcher.insert_memory_files(snippet_file); - } + let container = WorkspaceFileContainer::from_workspace_dirs_with_files( + by_workspace_directory.into_values().collect(), + |patterns, cli_options, _| { + async move { + collect_specifiers( + patterns, + cli_options.vendor_dir_path().map(ToOwned::to_owned), + |e| is_script_ext(e.path), + ) + .map(|s| s.into_iter().map(|s| (s, ())).collect()) } - specifiers_for_typecheck - } else { - specifiers - }; - if let Err(err) = main_graph_container - .check_specifiers(&specifiers_for_typecheck, None) - .await - { - match err { - MaybeDiagnostics::Diagnostics(Diagnostics(d)) => { - diagnostics.extend(d) - } - MaybeDiagnostics::Other(err) => all_errors.push(err), - } - } - } - if !found_specifiers { + .boxed_local() + }, + cli_options, + check_flags.doc || check_flags.doc_only, + ) + .await?; + if !container.has_specifiers() { log::warn!("{} No matching files found.", colors::yellow("Warning")); } - if !diagnostics.is_empty() { - all_errors.push(AnyError::from(Diagnostics(diagnostics))); - } - if !all_errors.is_empty() { - return Err(anyhow!( - "{}", - all_errors - .into_iter() - .map(|e| e.to_string()) - .collect::>() - .join("\n\n"), - )); - } - return Ok(()); + return container.check().await; } let factory = CliFactory::from_flags(flags); From f36d70d194fe9aea2404253603a4f8886b37b5d7 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Sat, 7 Dec 2024 01:19:38 +0000 Subject: [PATCH 09/33] move remote specifier handling to deno_config --- Cargo.lock | 2 +- Cargo.toml | 2 +- cli/tools/check.rs | 37 ++----------------------------------- 3 files changed, 4 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e7a1874e46..df6196d22f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1444,7 +1444,7 @@ dependencies = [ [[package]] name = "deno_config" version = "0.39.3" -source = "git+https://github.com/denoland/deno_config.git?rev=81d8e844624a8f6b3961213d1480dcc923d08b11#81d8e844624a8f6b3961213d1480dcc923d08b11" +source = "git+https://github.com/denoland/deno_config.git?rev=0d588fb1831bf4d33d3279a1a75db6138d81a75b#0d588fb1831bf4d33d3279a1a75db6138d81a75b" dependencies = [ "anyhow", "deno_package_json", diff --git a/Cargo.toml b/Cargo.toml index 80b1ab3de4..fc379cee73 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,7 +52,7 @@ deno_core = { version = "0.324.0" } deno_bench_util = { version = "0.175.0", path = "./bench_util" } # TODO(nayeemrmn): Use proper version when https://github.com/denoland/deno_config/pull/143 lands! -deno_config = { git = "https://github.com/denoland/deno_config.git", rev = "81d8e844624a8f6b3961213d1480dcc923d08b11", features = ["workspace", "sync"] } +deno_config = { git = "https://github.com/denoland/deno_config.git", rev = "0d588fb1831bf4d33d3279a1a75db6138d81a75b", features = ["workspace", "sync"] } deno_lockfile = "=0.23.2" deno_media_type = { version = "0.2.0", features = ["module_specifier"] } deno_npm = "=0.26.0" diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 53b89f21b7..bc6fe0d44b 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -9,8 +9,6 @@ use std::sync::Arc; use deno_ast::MediaType; use deno_ast::ModuleSpecifier; -use deno_config::glob::FilePatterns; -use deno_config::glob::PathOrPattern; use deno_core::error::AnyError; use deno_core::futures::FutureExt; use deno_graph::Module; @@ -59,45 +57,14 @@ pub async fn check( if is_discovered_config { let factory = CliFactory::from_flags(flags.clone()); let cli_options = factory.cli_options()?; - let (remote_files, files) = check_flags - .files - .iter() - .cloned() - .partition::, _>(|f| { - f.starts_with("http://") - || f.starts_with("https://") - || f.starts_with("npm:") - || f.starts_with("jsr:") - }); - let mut by_workspace_directory = cli_options + let by_workspace_directory = cli_options .resolve_file_flags_for_members(&FileFlags { ignore: Default::default(), - include: files, + include: check_flags.files, })? .into_iter() .map(|(d, p)| (d.dir_url().clone(), (Arc::new(d), p))) .collect::>(); - if !remote_files.is_empty() { - by_workspace_directory - .entry(cli_options.start_dir.dir_url().clone()) - .or_insert(( - cli_options.start_dir.clone(), - FilePatterns { - base: cli_options.initial_cwd().to_path_buf(), - include: Some(Default::default()), - exclude: Default::default(), - }, - )) - .1 - .include - .get_or_insert_with(Default::default) - .append( - remote_files - .iter() - .flat_map(|s| ModuleSpecifier::parse(s).ok()) - .map(PathOrPattern::RemoteUrl), - ); - } let container = WorkspaceFileContainer::from_workspace_dirs_with_files( by_workspace_directory.into_values().collect(), |patterns, cli_options, _| { From 59511cba6ae7d92aed295b689d69382b1d2a8102 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Sat, 7 Dec 2024 01:34:20 +0000 Subject: [PATCH 10/33] cleanup --- cli/tools/check.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cli/tools/check.rs b/cli/tools/check.rs index bc6fe0d44b..a0b624c8e7 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -1,6 +1,5 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. -use std::collections::BTreeMap; use std::collections::HashSet; use std::collections::VecDeque; use std::error::Error; @@ -57,16 +56,16 @@ pub async fn check( if is_discovered_config { let factory = CliFactory::from_flags(flags.clone()); let cli_options = factory.cli_options()?; - let by_workspace_directory = cli_options + let workspace_dirs_with_files = cli_options .resolve_file_flags_for_members(&FileFlags { ignore: Default::default(), include: check_flags.files, })? .into_iter() - .map(|(d, p)| (d.dir_url().clone(), (Arc::new(d), p))) - .collect::>(); + .map(|(d, p)| (Arc::new(d), p)) + .collect(); let container = WorkspaceFileContainer::from_workspace_dirs_with_files( - by_workspace_directory.into_values().collect(), + workspace_dirs_with_files, |patterns, cli_options, _| { async move { collect_specifiers( From 7ceeb0ba20d70f89571f358e924415c6a3fc496e Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Sat, 7 Dec 2024 02:14:20 +0000 Subject: [PATCH 11/33] fix --doc-only --- cli/factory.rs | 30 +++++++++++++++++------------- cli/tools/check.rs | 3 ++- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index e1611df14f..18ca531026 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1067,7 +1067,8 @@ impl CliFactory { pub struct WorkspaceFileContainerEntry { main_graph_container: Arc, specifiers: Vec<(ModuleSpecifier, T)>, - snippet_file_specifiers: Option>, + doc_snippet_specifiers: Option>, + check_doc_only: bool, } pub struct WorkspaceFileContainer { @@ -1087,6 +1088,7 @@ impl WorkspaceFileContainer { >, cli_options: &CliOptions, check_doc: bool, + check_doc_only: bool, ) -> Result { workspace_dirs_with_files.sort_by_cached_key(|(d, _)| d.dir_url().clone()); let all_scopes = Arc::new( @@ -1123,25 +1125,26 @@ impl WorkspaceFileContainer { factory.main_module_graph_container().await?.clone(); let specifiers = collect_files(files, cli_options, file_fetcher.clone()).await?; - let snippet_file_specifiers = if check_doc { + let doc_snippet_specifiers = if check_doc || check_doc_only { let root_permissions = factory.root_permissions_container()?; - let mut snippet_file_specifiers = Vec::new(); + let mut doc_snippet_specifiers = Vec::new(); for (s, _) in specifiers.iter() { let file = file_fetcher.fetch(s, root_permissions).await?; let snippet_files = extract::extract_snippet_files(file)?; for snippet_file in snippet_files { - snippet_file_specifiers.push(snippet_file.specifier.clone()); + doc_snippet_specifiers.push(snippet_file.specifier.clone()); file_fetcher.insert_memory_files(snippet_file); } } - Some(snippet_file_specifiers) + Some(doc_snippet_specifiers) } else { None }; entries.push(WorkspaceFileContainerEntry { main_graph_container, specifiers, - snippet_file_specifiers, + doc_snippet_specifiers, + check_doc_only, }); } Ok(Self { entries }) @@ -1151,13 +1154,14 @@ impl WorkspaceFileContainer { let mut diagnostics = vec![]; let mut all_errors = vec![]; for entry in &self.entries { - let specifiers_for_typecheck = entry - .specifiers - .iter() - .map(|(s, _)| s) - .chain(entry.snippet_file_specifiers.iter().flatten()) - .cloned() - .collect::>(); + let mut specifiers_for_typecheck = vec![]; + if !entry.check_doc_only { + specifiers_for_typecheck + .extend(entry.specifiers.iter().map(|(s, _)| s.clone())); + } + if let Some(doc_snippet_specifiers) = &entry.doc_snippet_specifiers { + specifiers_for_typecheck.extend(doc_snippet_specifiers.clone()); + } if specifiers_for_typecheck.is_empty() { continue; } diff --git a/cli/tools/check.rs b/cli/tools/check.rs index a0b624c8e7..e45697f078 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -78,7 +78,8 @@ pub async fn check( .boxed_local() }, cli_options, - check_flags.doc || check_flags.doc_only, + check_flags.doc, + check_flags.doc_only, ) .await?; if !container.has_specifiers() { From 4d081d2cfc0931e25bf09abc51cada58397e23ca Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Sat, 7 Dec 2024 02:31:48 +0000 Subject: [PATCH 12/33] dedup discovered/specified config --- cli/factory.rs | 4 +- cli/tools/check.rs | 101 +++++++++++++++------------------------------ 2 files changed, 36 insertions(+), 69 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index 18ca531026..7c72ae1ed5 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1079,7 +1079,7 @@ impl WorkspaceFileContainer { #[allow(clippy::type_complexity)] pub async fn from_workspace_dirs_with_files( mut workspace_dirs_with_files: Vec<(Arc, FilePatterns)>, - collect_files: fn( + collect_specifiers: fn( FilePatterns, Arc, Arc, @@ -1124,7 +1124,7 @@ impl WorkspaceFileContainer { let main_graph_container = factory.main_module_graph_container().await?.clone(); let specifiers = - collect_files(files, cli_options, file_fetcher.clone()).await?; + collect_specifiers(files, cli_options, file_fetcher.clone()).await?; let doc_snippet_specifiers = if check_doc || check_doc_only { let root_permissions = factory.root_permissions_container()?; let mut doc_snippet_specifiers = Vec::new(); diff --git a/cli/tools/check.rs b/cli/tools/check.rs index e45697f078..56e031bc92 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -39,7 +39,6 @@ use crate::npm::CliNpmResolver; use crate::tsc; use crate::tsc::Diagnostics; use crate::tsc::TypeCheckingCjsTracker; -use crate::util::extract; use crate::util::fs::collect_specifiers; use crate::util::path::is_script_ext; use crate::util::path::to_percent_decoded_str; @@ -53,80 +52,48 @@ pub async fn check( ConfigFlag::Path(_) => false, ConfigFlag::Disabled => false, }; - if is_discovered_config { - let factory = CliFactory::from_flags(flags.clone()); - let cli_options = factory.cli_options()?; - let workspace_dirs_with_files = cli_options + let factory = CliFactory::from_flags(flags); + let cli_options = factory.cli_options()?; + let workspace_dirs_with_files = if is_discovered_config { + cli_options .resolve_file_flags_for_members(&FileFlags { ignore: Default::default(), include: check_flags.files, })? .into_iter() .map(|(d, p)| (Arc::new(d), p)) - .collect(); - let container = WorkspaceFileContainer::from_workspace_dirs_with_files( - workspace_dirs_with_files, - |patterns, cli_options, _| { - async move { - collect_specifiers( - patterns, - cli_options.vendor_dir_path().map(ToOwned::to_owned), - |e| is_script_ext(e.path), - ) - .map(|s| s.into_iter().map(|s| (s, ())).collect()) - } - .boxed_local() - }, - cli_options, - check_flags.doc, - check_flags.doc_only, - ) - .await?; - if !container.has_specifiers() { - log::warn!("{} No matching files found.", colors::yellow("Warning")); - } - return container.check().await; - } - - let factory = CliFactory::from_flags(flags); - - let main_graph_container = factory.main_module_graph_container().await?; - - let specifiers = - main_graph_container.collect_specifiers(&check_flags.files)?; - if specifiers.is_empty() { + .collect() + } else { + let file_flags = FileFlags { + ignore: Default::default(), + include: check_flags.files, + }; + let patterns = file_flags.as_file_patterns(cli_options.initial_cwd())?; + let patterns = cli_options.start_dir.to_resolved_file_patterns(patterns)?; + vec![(cli_options.start_dir.clone(), patterns)] + }; + let container = WorkspaceFileContainer::from_workspace_dirs_with_files( + workspace_dirs_with_files, + |patterns, cli_options, _| { + async move { + collect_specifiers( + patterns, + cli_options.vendor_dir_path().map(ToOwned::to_owned), + |e| is_script_ext(e.path), + ) + .map(|s| s.into_iter().map(|s| (s, ())).collect()) + } + .boxed_local() + }, + cli_options, + check_flags.doc, + check_flags.doc_only, + ) + .await?; + if !container.has_specifiers() { log::warn!("{} No matching files found.", colors::yellow("Warning")); } - - let specifiers_for_typecheck = if check_flags.doc || check_flags.doc_only { - let file_fetcher = factory.file_fetcher()?; - let root_permissions = factory.root_permissions_container()?; - - let mut specifiers_for_typecheck = if check_flags.doc { - specifiers.clone() - } else { - vec![] - }; - - for s in specifiers { - let file = file_fetcher.fetch(&s, root_permissions).await?; - let snippet_files = extract::extract_snippet_files(file)?; - for snippet_file in snippet_files { - specifiers_for_typecheck.push(snippet_file.specifier.clone()); - file_fetcher.insert_memory_files(snippet_file); - } - } - - specifiers_for_typecheck - } else { - specifiers - }; - - Ok( - main_graph_container - .check_specifiers(&specifiers_for_typecheck, None) - .await?, - ) + container.check().await } /// Options for performing a check of a module graph. Note that the decision to From dab83524b8b82ebfca09a85cc289715c1a33e7eb Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 9 Dec 2024 11:56:15 +0000 Subject: [PATCH 13/33] use WorkspaceFileContainer in deno test --- cli/factory.rs | 117 +++++++++++++++++++---------- cli/tools/check.rs | 22 ++++-- cli/tools/test/mod.rs | 171 ++++++++++++++++++++++++++++++++++-------- 3 files changed, 229 insertions(+), 81 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index 7c72ae1ed5..0e4007ce24 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -25,6 +25,7 @@ use crate::cache::ModuleInfoCache; use crate::cache::NodeAnalysisCache; use crate::cache::ParsedSourceCache; use crate::emit::Emitter; +use crate::file_fetcher::File; use crate::file_fetcher::FileFetcher; use crate::graph_container::MainModuleGraphContainer; use crate::graph_util::FileWatcherReporter; @@ -61,7 +62,6 @@ use crate::tools::lint::LintRuleProvider; use crate::tools::run::hmr::HmrRunner; use crate::tsc::Diagnostics; use crate::tsc::TypeCheckingCjsTracker; -use crate::util::extract; use crate::util::file_watcher::WatcherCommunicator; use crate::util::fs::canonicalize_path_maybe_not_exists; use crate::util::progress_bar::ProgressBar; @@ -1064,31 +1064,55 @@ impl CliFactory { } } -pub struct WorkspaceFileContainerEntry { +#[derive(Debug, Copy, Clone)] +pub struct SpecifierInfo { + /// Type check as an ES module. + pub check: bool, + /// Type check virtual modules from doc snippets. If this is set but `check` + /// is not, this may be a markdown file for example. + pub check_doc: bool, +} + +struct WorkspaceFileContainerEntry { + specifiers: Vec<(ModuleSpecifier, SpecifierInfo)>, + doc_snippet_specifiers: Vec, main_graph_container: Arc, - specifiers: Vec<(ModuleSpecifier, T)>, - doc_snippet_specifiers: Option>, - check_doc_only: bool, + worker_factory: Arc, } -pub struct WorkspaceFileContainer { - entries: Vec>, +impl WorkspaceFileContainerEntry { + fn checked_specifiers(&self) -> impl Iterator { + self + .specifiers + .iter() + .filter_map(|(s, i)| i.check.then_some(s)) + .chain(self.doc_snippet_specifiers.iter()) + } } -impl WorkspaceFileContainer { +pub struct WorkspaceFileContainer { + entries: Vec, +} + +impl WorkspaceFileContainer { #[allow(clippy::type_complexity)] - pub async fn from_workspace_dirs_with_files( + pub async fn from_workspace_dirs_with_files( mut workspace_dirs_with_files: Vec<(Arc, FilePatterns)>, + cli_options: &CliOptions, + extract_doc_files: fn(File) -> Result, AnyError>, collect_specifiers: fn( FilePatterns, Arc, Arc, + T, ) -> std::pin::Pin< - Box, AnyError>>>, + Box< + dyn Future< + Output = Result, AnyError>, + >, + >, >, - cli_options: &CliOptions, - check_doc: bool, - check_doc_only: bool, + args: T, ) -> Result { workspace_dirs_with_files.sort_by_cached_key(|(d, _)| d.dir_url().clone()); let all_scopes = Arc::new( @@ -1121,30 +1145,32 @@ impl WorkspaceFileContainer { )?); let factory = CliFactory::from_cli_options(cli_options.clone()); let file_fetcher = factory.file_fetcher()?; + let specifiers = collect_specifiers( + files, + cli_options, + file_fetcher.clone(), + args.clone(), + ) + .await?; + let mut doc_snippet_specifiers = vec![]; + let root_permissions = factory.root_permissions_container()?; + for (s, _) in specifiers.iter().filter(|(_, i)| i.check_doc) { + let file = file_fetcher.fetch(s, root_permissions).await?; + let snippet_files = extract_doc_files(file)?; + for snippet_file in snippet_files { + doc_snippet_specifiers.push(snippet_file.specifier.clone()); + file_fetcher.insert_memory_files(snippet_file); + } + } let main_graph_container = factory.main_module_graph_container().await?.clone(); - let specifiers = - collect_specifiers(files, cli_options, file_fetcher.clone()).await?; - let doc_snippet_specifiers = if check_doc || check_doc_only { - let root_permissions = factory.root_permissions_container()?; - let mut doc_snippet_specifiers = Vec::new(); - for (s, _) in specifiers.iter() { - let file = file_fetcher.fetch(s, root_permissions).await?; - let snippet_files = extract::extract_snippet_files(file)?; - for snippet_file in snippet_files { - doc_snippet_specifiers.push(snippet_file.specifier.clone()); - file_fetcher.insert_memory_files(snippet_file); - } - } - Some(doc_snippet_specifiers) - } else { - None - }; + let worker_factory = + Arc::new(factory.create_cli_main_worker_factory().await?); entries.push(WorkspaceFileContainerEntry { - main_graph_container, specifiers, doc_snippet_specifiers, - check_doc_only, + main_graph_container, + worker_factory, }); } Ok(Self { entries }) @@ -1154,14 +1180,8 @@ impl WorkspaceFileContainer { let mut diagnostics = vec![]; let mut all_errors = vec![]; for entry in &self.entries { - let mut specifiers_for_typecheck = vec![]; - if !entry.check_doc_only { - specifiers_for_typecheck - .extend(entry.specifiers.iter().map(|(s, _)| s.clone())); - } - if let Some(doc_snippet_specifiers) = &entry.doc_snippet_specifiers { - specifiers_for_typecheck.extend(doc_snippet_specifiers.clone()); - } + let specifiers_for_typecheck = + entry.checked_specifiers().cloned().collect::>(); if specifiers_for_typecheck.is_empty() { continue; } @@ -1194,7 +1214,22 @@ impl WorkspaceFileContainer { Ok(()) } - pub fn has_specifiers(&self) -> bool { + pub fn found_specifiers(&self) -> bool { self.entries.iter().any(|e| !e.specifiers.is_empty()) } + + pub fn checked_specifiers_with_worker_factories( + &self, + ) -> Vec<(Vec, Arc)> { + self + .entries + .iter() + .map(|e| { + ( + e.checked_specifiers().cloned().collect(), + e.worker_factory.clone(), + ) + }) + .collect() + } } diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 56e031bc92..cbb7163497 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -32,6 +32,7 @@ use crate::cache::Caches; use crate::cache::FastInsecureHasher; use crate::cache::TypeCheckCache; use crate::factory::CliFactory; +use crate::factory::SpecifierInfo; use crate::factory::WorkspaceFileContainer; use crate::graph_util::BuildFastCheckGraphOptions; use crate::graph_util::ModuleGraphBuilder; @@ -39,6 +40,7 @@ use crate::npm::CliNpmResolver; use crate::tsc; use crate::tsc::Diagnostics; use crate::tsc::TypeCheckingCjsTracker; +use crate::util::extract::extract_snippet_files; use crate::util::fs::collect_specifiers; use crate::util::path::is_script_ext; use crate::util::path::to_percent_decoded_str; @@ -72,28 +74,32 @@ pub async fn check( let patterns = cli_options.start_dir.to_resolved_file_patterns(patterns)?; vec![(cli_options.start_dir.clone(), patterns)] }; - let container = WorkspaceFileContainer::from_workspace_dirs_with_files( + let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, - |patterns, cli_options, _| { + cli_options, + extract_snippet_files, + |patterns, cli_options, _, (doc, doc_only)| { async move { + let info = SpecifierInfo { + check: !doc_only, + check_doc: doc || doc_only, + }; collect_specifiers( patterns, cli_options.vendor_dir_path().map(ToOwned::to_owned), |e| is_script_ext(e.path), ) - .map(|s| s.into_iter().map(|s| (s, ())).collect()) + .map(|s| s.into_iter().map(|s| (s, info)).collect()) } .boxed_local() }, - cli_options, - check_flags.doc, - check_flags.doc_only, + (check_flags.doc, check_flags.doc_only), ) .await?; - if !container.has_specifiers() { + if !file_container.found_specifiers() { log::warn!("{} No matching files found.", colors::yellow("Warning")); } - container.check().await + file_container.check().await } /// Options for performing a check of a module graph. Note that the decision to diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 6357ebcae2..2131e563aa 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -7,6 +7,8 @@ use crate::args::TestReporterConfig; use crate::colors; use crate::display; use crate::factory::CliFactory; +use crate::factory::SpecifierInfo; +use crate::factory::WorkspaceFileContainer; use crate::file_fetcher::File; use crate::file_fetcher::FileFetcher; use crate::graph_util::has_graph_root_local_dependent_changed; @@ -1255,6 +1257,77 @@ async fn test_specifiers( Ok(()) } +/// Test a collection of specifiers with test modes concurrently. +async fn test_specifiers2( + file_container: WorkspaceFileContainer, + permissions: &Permissions, + permission_desc_parser: &Arc, + options: TestSpecifiersOptions, +) -> Result<(), AnyError> { + let mut specifiers_with_factory = file_container + .checked_specifiers_with_worker_factories() + .into_iter() + .flat_map(|(s, f)| { + s.into_iter().map(|s| (s, f.clone())).collect::>() + }) + .collect::>(); + if let Some(seed) = options.specifier.shuffle { + let mut rng = SmallRng::seed_from_u64(seed); + specifiers_with_factory.sort_by_cached_key(|(s, _)| s.to_string()); + specifiers_with_factory.shuffle(&mut rng); + } + + let (test_event_sender_factory, receiver) = create_test_event_channel(); + let concurrent_jobs = options.concurrent_jobs; + + let mut cancel_sender = test_event_sender_factory.weak_sender(); + let sigint_handler_handle = spawn(async move { + signal::ctrl_c().await.unwrap(); + cancel_sender.send(TestEvent::Sigint).ok(); + }); + HAS_TEST_RUN_SIGINT_HANDLER.store(true, Ordering::Relaxed); + let reporter = get_test_reporter(&options); + let fail_fast_tracker = FailFastTracker::new(options.fail_fast); + + let join_handles = specifiers_with_factory.into_iter().map( + move |(specifier, worker_factory)| { + let permissions_container = PermissionsContainer::new( + permission_desc_parser.clone(), + permissions.clone(), + ); + let worker_sender = test_event_sender_factory.worker(); + let fail_fast_tracker = fail_fast_tracker.clone(); + let specifier_options = options.specifier.clone(); + spawn_blocking(move || { + create_and_run_current_thread(test_specifier( + worker_factory, + permissions_container, + specifier, + worker_sender, + fail_fast_tracker, + specifier_options, + )) + }) + }, + ); + + let join_stream = stream::iter(join_handles) + .buffer_unordered(concurrent_jobs.get()) + .collect::, tokio::task::JoinError>>>(); + + let handler = spawn(async move { report_tests(receiver, reporter).await.0 }); + + let (join_results, result) = future::join(join_stream, handler).await; + sigint_handler_handle.abort(); + HAS_TEST_RUN_SIGINT_HANDLER.store(false, Ordering::Relaxed); + for join_result in join_results { + join_result??; + } + result??; + + Ok(()) +} + /// Gives receiver back in case it was ended with `TestEvent::ForceEndReport`. pub async fn report_tests( mut receiver: TestEventReceiver, @@ -1539,6 +1612,54 @@ async fn fetch_specifiers_with_test_mode( Ok(specifiers_with_mode) } +pub async fn collect_specifiers_for_tests( + patterns: FilePatterns, + cli_options: Arc, + file_fetcher: Arc, + doc: bool, +) -> Result, AnyError> { + let vendor_folder = cli_options.vendor_dir_path(); + let module_specifiers = collect_specifiers( + patterns.clone(), + vendor_folder.map(ToOwned::to_owned), + is_supported_test_path_predicate, + )?; + let mut specifiers = if doc { + collect_specifiers(patterns, vendor_folder.map(ToOwned::to_owned), |e| { + is_supported_test_ext(e.path) + }) + .map(|specifiers| { + specifiers + .into_iter() + .map(|specifier| { + let info = SpecifierInfo { + check: module_specifiers.contains(&specifier), + check_doc: true, + }; + (specifier, info) + }) + .collect::>() + })? + } else { + let info = SpecifierInfo { + check: true, + check_doc: false, + }; + module_specifiers + .into_iter() + .map(|specifier| (specifier, info)) + .collect::>() + }; + for (specifier, info) in &mut specifiers { + let file = file_fetcher.fetch_bypass_permissions(specifier).await?; + let (media_type, _) = file.resolve_media_type_and_charset(); + if matches!(media_type, MediaType::Unknown | MediaType::Dts) { + info.check = false; + } + } + Ok(specifiers) +} + pub async fn run_tests( flags: Arc, test_flags: TestFlags, @@ -1547,7 +1668,6 @@ pub async fn run_tests( let cli_options = factory.cli_options()?; let workspace_test_options = cli_options.resolve_workspace_test_options(&test_flags); - let file_fetcher = factory.file_fetcher()?; // Various test files should not share the same permissions in terms of // `PermissionsContainer` - otherwise granting/revoking permissions in one // file would have impact on other files, which is undesirable. @@ -1558,51 +1678,38 @@ pub async fn run_tests( )?; let log_level = cli_options.log_level(); - let members_with_test_options = - cli_options.resolve_test_options_for_members(&test_flags)?; - let specifiers_with_mode = fetch_specifiers_with_test_mode( + let workspace_dirs_with_files = cli_options + .resolve_test_options_for_members(&test_flags)? + .into_iter() + .map(|(d, o)| (Arc::new(d), o.files)) + .collect(); + let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( + workspace_dirs_with_files, cli_options, - file_fetcher, - members_with_test_options.into_iter().map(|(_, v)| v.files), - &workspace_test_options.doc, + extract_doc_tests, + |patterns, cli_options, file_fetcher, doc| { + collect_specifiers_for_tests(patterns, cli_options, file_fetcher, doc) + .boxed_local() + }, + workspace_test_options.doc, ) .await?; - - if !workspace_test_options.permit_no_files && specifiers_with_mode.is_empty() + if !workspace_test_options.permit_no_files + && !file_container.found_specifiers() { return Err(generic_error("No test modules found")); } - let doc_tests = get_doc_tests(&specifiers_with_mode, file_fetcher).await?; - let specifiers_for_typecheck_and_test = - get_target_specifiers(specifiers_with_mode, &doc_tests); - for doc_test in doc_tests { - file_fetcher.insert_memory_files(doc_test); - } - - let main_graph_container = factory.main_module_graph_container().await?; - - // Typecheck - main_graph_container - .check_specifiers( - &specifiers_for_typecheck_and_test, - cli_options.ext_flag().as_ref(), - ) - .await?; + file_container.check().await?; if workspace_test_options.no_run { return Ok(()); } - let worker_factory = - Arc::new(factory.create_cli_main_worker_factory().await?); - - // Run tests - test_specifiers( - worker_factory, + test_specifiers2( + file_container, &permissions, permission_desc_parser, - specifiers_for_typecheck_and_test, TestSpecifiersOptions { cwd: Url::from_directory_path(cli_options.initial_cwd()).map_err( |_| { From 655190e82b8da21b2a275b5e31c55fd6c9451127 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 9 Dec 2024 19:30:38 +0000 Subject: [PATCH 14/33] fix ext flag --- cli/factory.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cli/factory.rs b/cli/factory.rs index 0e4007ce24..6caae4f192 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1078,6 +1078,7 @@ struct WorkspaceFileContainerEntry { doc_snippet_specifiers: Vec, main_graph_container: Arc, worker_factory: Arc, + ext_flag: Option, } impl WorkspaceFileContainerEntry { @@ -1143,6 +1144,7 @@ impl WorkspaceFileContainer { false, scope_options.map(Arc::new), )?); + let ext_flag = cli_options.ext_flag().clone(); let factory = CliFactory::from_cli_options(cli_options.clone()); let file_fetcher = factory.file_fetcher()?; let specifiers = collect_specifiers( @@ -1171,6 +1173,7 @@ impl WorkspaceFileContainer { doc_snippet_specifiers, main_graph_container, worker_factory, + ext_flag, }); } Ok(Self { entries }) @@ -1187,7 +1190,7 @@ impl WorkspaceFileContainer { } if let Err(err) = entry .main_graph_container - .check_specifiers(&specifiers_for_typecheck, None) + .check_specifiers(&specifiers_for_typecheck, entry.ext_flag.as_ref()) .await { match err { From 42eb554edfb415178e184f7e25776c0da7ae18ff Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 9 Dec 2024 20:27:24 +0000 Subject: [PATCH 15/33] fix fixture --- tests/specs/workspaces/lockfile/test_root.out | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/specs/workspaces/lockfile/test_root.out b/tests/specs/workspaces/lockfile/test_root.out index 2c62b615b1..da04d1973a 100644 --- a/tests/specs/workspaces/lockfile/test_root.out +++ b/tests/specs/workspaces/lockfile/test_root.out @@ -1,5 +1,4 @@ Check file:///[WILDLINE]/integration.test.ts -Check file:///[WILDLINE]/pkg/mod.test.ts running 1 test from ./integration.test.ts should add ... ok ([WILDLINE]) running 1 test from ./pkg/mod.test.ts From 0c44814e2127de87f7bf9176718a0f220d6a6376 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 10 Dec 2024 08:42:33 +0000 Subject: [PATCH 16/33] use WorkspaceFileContainer for test --watch --- cli/factory.rs | 85 ++++++++-- cli/tools/check.rs | 2 +- cli/tools/test/mod.rs | 352 ++++++------------------------------------ 3 files changed, 115 insertions(+), 324 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index 6caae4f192..d650dbf878 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -63,12 +63,14 @@ use crate::tools::run::hmr::HmrRunner; use crate::tsc::Diagnostics; use crate::tsc::TypeCheckingCjsTracker; use crate::util::file_watcher::WatcherCommunicator; +use crate::util::fs::canonicalize_path; use crate::util::fs::canonicalize_path_maybe_not_exists; use crate::util::progress_bar::ProgressBar; use crate::util::progress_bar::ProgressBarStyle; use crate::worker::CliMainWorkerFactory; use crate::worker::CliMainWorkerOptions; use std::collections::BTreeSet; +use std::collections::HashSet; use std::path::PathBuf; use deno_ast::ModuleSpecifier; @@ -82,6 +84,7 @@ use deno_core::error::AnyError; use deno_core::futures::FutureExt; use deno_core::FeatureChecker; +use deno_path_util::url_to_file_path; use deno_resolver::cjs::IsCjsResolutionMode; use deno_resolver::npm::NpmReqResolverOptions; use deno_resolver::DenoResolverOptions; @@ -1076,9 +1079,8 @@ pub struct SpecifierInfo { struct WorkspaceFileContainerEntry { specifiers: Vec<(ModuleSpecifier, SpecifierInfo)>, doc_snippet_specifiers: Vec, - main_graph_container: Arc, + factory: CliFactory, worker_factory: Arc, - ext_flag: Option, } impl WorkspaceFileContainerEntry { @@ -1099,7 +1101,7 @@ impl WorkspaceFileContainer { #[allow(clippy::type_complexity)] pub async fn from_workspace_dirs_with_files( mut workspace_dirs_with_files: Vec<(Arc, FilePatterns)>, - cli_options: &CliOptions, + factory: &CliFactory, extract_doc_files: fn(File) -> Result, AnyError>, collect_specifiers: fn( FilePatterns, @@ -1116,6 +1118,8 @@ impl WorkspaceFileContainer { args: T, ) -> Result { workspace_dirs_with_files.sort_by_cached_key(|(d, _)| d.dir_url().clone()); + let cli_options = factory.cli_options()?; + let watcher_communicator = &factory.watcher_communicator; let all_scopes = Arc::new( workspace_dirs_with_files .iter() @@ -1144,8 +1148,8 @@ impl WorkspaceFileContainer { false, scope_options.map(Arc::new), )?); - let ext_flag = cli_options.ext_flag().clone(); - let factory = CliFactory::from_cli_options(cli_options.clone()); + let mut factory = CliFactory::from_cli_options(cli_options.clone()); + factory.watcher_communicator = watcher_communicator.clone(); let file_fetcher = factory.file_fetcher()?; let specifiers = collect_specifiers( files, @@ -1164,16 +1168,13 @@ impl WorkspaceFileContainer { file_fetcher.insert_memory_files(snippet_file); } } - let main_graph_container = - factory.main_module_graph_container().await?.clone(); let worker_factory = Arc::new(factory.create_cli_main_worker_factory().await?); entries.push(WorkspaceFileContainerEntry { specifiers, doc_snippet_specifiers, - main_graph_container, + factory, worker_factory, - ext_flag, }); } Ok(Self { entries }) @@ -1183,14 +1184,16 @@ impl WorkspaceFileContainer { let mut diagnostics = vec![]; let mut all_errors = vec![]; for entry in &self.entries { + let main_graph_container = + entry.factory.main_module_graph_container().await?.clone(); let specifiers_for_typecheck = entry.checked_specifiers().cloned().collect::>(); if specifiers_for_typecheck.is_empty() { continue; } - if let Err(err) = entry - .main_graph_container - .check_specifiers(&specifiers_for_typecheck, entry.ext_flag.as_ref()) + let ext_flag = entry.factory.cli_options()?.ext_flag().as_ref(); + if let Err(err) = main_graph_container + .check_specifiers(&specifiers_for_typecheck, ext_flag) .await { match err { @@ -1221,18 +1224,70 @@ impl WorkspaceFileContainer { self.entries.iter().any(|e| !e.specifiers.is_empty()) } - pub fn checked_specifiers_with_worker_factories( + pub fn worker_factories_with_checked_specifiers( &self, - ) -> Vec<(Vec, Arc)> { + ) -> Vec<(Arc, Vec)> { self .entries .iter() .map(|e| { ( - e.checked_specifiers().cloned().collect(), e.worker_factory.clone(), + e.checked_specifiers().cloned().collect(), ) }) .collect() } + + /// Per workspace directory, return a list of included checked specifiers + /// which depend on the modules at the passed paths. + pub async fn worker_factories_with_dependent_checked_specifiers( + &self, + canonicalized_dep_paths: &HashSet, + ) -> Result, Vec)>, AnyError> + { + let mut result = Vec::with_capacity(self.entries.len()); + for entry in &self.entries { + let graph_kind = entry + .factory + .cli_options()? + .type_check_mode() + .as_graph_kind(); + let module_graph_creator = entry.factory.module_graph_creator().await?; + let specifiers = entry.checked_specifiers().cloned().collect::>(); + let graph = module_graph_creator + .create_graph(graph_kind, specifiers.clone()) + .await?; + module_graph_creator.graph_valid(&graph)?; + let dependent_specifiers = specifiers + .into_iter() + .filter(|s| { + let mut dependency_specifiers = graph.walk( + std::iter::once(s), + deno_graph::WalkOptions { + follow_dynamic: true, + kind: deno_graph::GraphKind::All, + prefer_fast_check_graph: true, + check_js: true, + }, + ); + while let Some((s, _)) = dependency_specifiers.next() { + if let Ok(path) = url_to_file_path(s) { + if let Ok(path) = canonicalize_path(&path) { + if canonicalized_dep_paths.contains(&path) { + return true; + } + } + } else { + // skip walking this remote module's dependencies + dependency_specifiers.skip_previous_dependencies(); + } + } + false + }) + .collect::>(); + result.push((entry.worker_factory.clone(), dependent_specifiers)); + } + Ok(result) + } } diff --git a/cli/tools/check.rs b/cli/tools/check.rs index cbb7163497..73f0fcb145 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -76,7 +76,7 @@ pub async fn check( }; let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, - cli_options, + &factory, extract_snippet_files, |patterns, cli_options, _, (doc, doc_only)| { async move { diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 2131e563aa..1424a11c55 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -9,9 +9,7 @@ use crate::display; use crate::factory::CliFactory; use crate::factory::SpecifierInfo; use crate::factory::WorkspaceFileContainer; -use crate::file_fetcher::File; use crate::file_fetcher::FileFetcher; -use crate::graph_util::has_graph_root_local_dependent_changed; use crate::ops; use crate::util::extract::extract_doc_tests; use crate::util::file_watcher; @@ -138,32 +136,6 @@ fn get_sanitizer_item_ref( } } -/// The test mode is used to determine how a specifier is to be tested. -#[derive(Debug, Clone, Eq, PartialEq)] -pub enum TestMode { - /// Test as documentation, type-checking fenced code blocks. - Documentation, - /// Test as an executable module, loading the module into the isolate and running each test it - /// defines. - Executable, - /// Test as both documentation and an executable module. - Both, -} - -impl TestMode { - /// Returns `true` if the test mode indicates that code snippet extraction is - /// needed. - fn needs_test_extraction(&self) -> bool { - matches!(self, Self::Documentation | Self::Both) - } - - /// Returns `true` if the test mode indicates that the test should be - /// type-checked and run. - fn needs_test_run(&self) -> bool { - matches!(self, Self::Executable | Self::Both) - } -} - #[derive(Clone, Debug, Default)] pub struct TestFilter { pub substring: Option, @@ -1191,83 +1163,17 @@ static HAS_TEST_RUN_SIGINT_HANDLER: AtomicBool = AtomicBool::new(false); /// Test a collection of specifiers with test modes concurrently. async fn test_specifiers( - worker_factory: Arc, - permissions: &Permissions, - permission_desc_parser: &Arc, - specifiers: Vec, - options: TestSpecifiersOptions, -) -> Result<(), AnyError> { - let specifiers = if let Some(seed) = options.specifier.shuffle { - let mut rng = SmallRng::seed_from_u64(seed); - let mut specifiers = specifiers; - specifiers.sort(); - specifiers.shuffle(&mut rng); - specifiers - } else { - specifiers - }; - - let (test_event_sender_factory, receiver) = create_test_event_channel(); - let concurrent_jobs = options.concurrent_jobs; - - let mut cancel_sender = test_event_sender_factory.weak_sender(); - let sigint_handler_handle = spawn(async move { - signal::ctrl_c().await.unwrap(); - cancel_sender.send(TestEvent::Sigint).ok(); - }); - HAS_TEST_RUN_SIGINT_HANDLER.store(true, Ordering::Relaxed); - let reporter = get_test_reporter(&options); - let fail_fast_tracker = FailFastTracker::new(options.fail_fast); - - let join_handles = specifiers.into_iter().map(move |specifier| { - let worker_factory = worker_factory.clone(); - let permissions_container = PermissionsContainer::new( - permission_desc_parser.clone(), - permissions.clone(), - ); - let worker_sender = test_event_sender_factory.worker(); - let fail_fast_tracker = fail_fast_tracker.clone(); - let specifier_options = options.specifier.clone(); - spawn_blocking(move || { - create_and_run_current_thread(test_specifier( - worker_factory, - permissions_container, - specifier, - worker_sender, - fail_fast_tracker, - specifier_options, - )) - }) - }); - - let join_stream = stream::iter(join_handles) - .buffer_unordered(concurrent_jobs.get()) - .collect::, tokio::task::JoinError>>>(); - - let handler = spawn(async move { report_tests(receiver, reporter).await.0 }); - - let (join_results, result) = future::join(join_stream, handler).await; - sigint_handler_handle.abort(); - HAS_TEST_RUN_SIGINT_HANDLER.store(false, Ordering::Relaxed); - for join_result in join_results { - join_result??; - } - result??; - - Ok(()) -} - -/// Test a collection of specifiers with test modes concurrently. -async fn test_specifiers2( - file_container: WorkspaceFileContainer, + factories_with_specifiers: Vec<( + Arc, + Vec, + )>, permissions: &Permissions, permission_desc_parser: &Arc, options: TestSpecifiersOptions, ) -> Result<(), AnyError> { - let mut specifiers_with_factory = file_container - .checked_specifiers_with_worker_factories() + let mut specifiers_with_factory = factories_with_specifiers .into_iter() - .flat_map(|(s, f)| { + .flat_map(|(f, s)| { s.into_iter().map(|s| (s, f.clone())).collect::>() }) .collect::>(); @@ -1527,91 +1433,6 @@ fn is_supported_test_ext(path: &Path) -> bool { } } -/// Collects specifiers marking them with the appropriate test mode while maintaining the natural -/// input order. -/// -/// - Specifiers matching the `is_supported_test_ext` predicate are marked as -/// `TestMode::Documentation`. -/// - Specifiers matching the `is_supported_test_path` are marked as `TestMode::Executable`. -/// - Specifiers matching both predicates are marked as `TestMode::Both` -fn collect_specifiers_with_test_mode( - cli_options: &CliOptions, - files: FilePatterns, - include_inline: &bool, -) -> Result, AnyError> { - // todo(dsherret): there's no need to collect twice as it's slow - let vendor_folder = cli_options.vendor_dir_path(); - let module_specifiers = collect_specifiers( - files.clone(), - vendor_folder.map(ToOwned::to_owned), - is_supported_test_path_predicate, - )?; - - if *include_inline { - return collect_specifiers( - files, - vendor_folder.map(ToOwned::to_owned), - |e| is_supported_test_ext(e.path), - ) - .map(|specifiers| { - specifiers - .into_iter() - .map(|specifier| { - let mode = if module_specifiers.contains(&specifier) { - TestMode::Both - } else { - TestMode::Documentation - }; - - (specifier, mode) - }) - .collect() - }); - } - - let specifiers_with_mode = module_specifiers - .into_iter() - .map(|specifier| (specifier, TestMode::Executable)) - .collect(); - - Ok(specifiers_with_mode) -} - -/// Collects module and document specifiers with test modes via -/// `collect_specifiers_with_test_mode` which are then pre-fetched and adjusted -/// based on the media type. -/// -/// Specifiers that do not have a known media type that can be executed as a -/// module are marked as `TestMode::Documentation`. Type definition files -/// cannot be run, and therefore need to be marked as `TestMode::Documentation` -/// as well. -async fn fetch_specifiers_with_test_mode( - cli_options: &CliOptions, - file_fetcher: &FileFetcher, - member_patterns: impl Iterator, - doc: &bool, -) -> Result, AnyError> { - let mut specifiers_with_mode = member_patterns - .map(|files| { - collect_specifiers_with_test_mode(cli_options, files.clone(), doc) - }) - .collect::, _>>()? - .into_iter() - .flatten() - .collect::>(); - - for (specifier, mode) in &mut specifiers_with_mode { - let file = file_fetcher.fetch_bypass_permissions(specifier).await?; - - let (media_type, _) = file.resolve_media_type_and_charset(); - if matches!(media_type, MediaType::Unknown | MediaType::Dts) { - *mode = TestMode::Documentation - } - } - - Ok(specifiers_with_mode) -} - pub async fn collect_specifiers_for_tests( patterns: FilePatterns, cli_options: Arc, @@ -1685,7 +1506,7 @@ pub async fn run_tests( .collect(); let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, - cli_options, + &factory, extract_doc_tests, |patterns, cli_options, file_fetcher, doc| { collect_specifiers_for_tests(patterns, cli_options, file_fetcher, doc) @@ -1706,8 +1527,8 @@ pub async fn run_tests( return Ok(()); } - test_specifiers2( - file_container, + test_specifiers( + file_container.worker_factories_with_checked_specifiers(), &permissions, permission_desc_parser, TestSpecifiersOptions { @@ -1777,13 +1598,13 @@ pub async fn run_tests_with_watch( let cli_options = factory.cli_options()?; let workspace_test_options = cli_options.resolve_workspace_test_options(&test_flags); - - let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); - let graph_kind = cli_options.type_check_mode().as_graph_kind(); + let permission_desc_parser = factory.permission_desc_parser()?; + let permissions = Permissions::from_options( + permission_desc_parser.as_ref(), + &cli_options.permissions_options(), + )?; let log_level = cli_options.log_level(); - let cli_options = cli_options.clone(); - let module_graph_creator = factory.module_graph_creator().await?; - let file_fetcher = factory.file_fetcher()?; + let members_with_test_options = cli_options.resolve_test_options_for_members(&test_flags)?; let watch_paths = members_with_test_options @@ -1798,96 +1619,43 @@ pub async fn run_tests_with_watch( .flatten() .collect::>(); let _ = watcher_communicator.watch_paths(watch_paths); - let test_modules = members_with_test_options - .iter() - .map(|(_, test_options)| { - collect_specifiers( - test_options.files.clone(), - cli_options.vendor_dir_path().map(ToOwned::to_owned), - if workspace_test_options.doc { - Box::new(|e: WalkEntry| is_supported_test_ext(e.path)) - as Box bool> - } else { - Box::new(is_supported_test_path_predicate) - }, - ) - }) - .collect::, _>>()? + let workspace_dirs_with_files = members_with_test_options .into_iter() - .flatten() - .collect::>(); - - let permission_desc_parser = factory.permission_desc_parser()?; - let permissions = Permissions::from_options( - permission_desc_parser.as_ref(), - &cli_options.permissions_options(), - )?; - let graph = module_graph_creator - .create_graph(graph_kind, test_modules) - .await?; - module_graph_creator.graph_valid(&graph)?; - let test_modules = &graph.roots; - - let test_modules_to_reload = if let Some(changed_paths) = changed_paths - { - let mut result = IndexSet::with_capacity(test_modules.len()); - let changed_paths = changed_paths.into_iter().collect::>(); - for test_module_specifier in test_modules { - if has_graph_root_local_dependent_changed( - &graph, - test_module_specifier, - &changed_paths, - ) { - result.insert(test_module_specifier.clone()); - } - } - result - } else { - test_modules.clone() - }; - - let specifiers_with_mode = fetch_specifiers_with_test_mode( - &cli_options, - file_fetcher, - members_with_test_options.into_iter().map(|(_, v)| v.files), - &workspace_test_options.doc, - ) - .await? - .into_iter() - .filter(|(specifier, _)| test_modules_to_reload.contains(specifier)) - .collect::>(); - - let doc_tests = - get_doc_tests(&specifiers_with_mode, file_fetcher).await?; - let specifiers_for_typecheck_and_test = - get_target_specifiers(specifiers_with_mode, &doc_tests); - for doc_test in doc_tests { - file_fetcher.insert_memory_files(doc_test); - } - - let main_graph_container = - factory.main_module_graph_container().await?; - - // Typecheck - main_graph_container - .check_specifiers( - &specifiers_for_typecheck_and_test, - cli_options.ext_flag().as_ref(), + .map(|(d, o)| (Arc::new(d), o.files)) + .collect(); + let file_container = + WorkspaceFileContainer::from_workspace_dirs_with_files( + workspace_dirs_with_files, + &factory, + extract_doc_tests, + |patterns, cli_options, file_fetcher, doc| { + collect_specifiers_for_tests( + patterns, + cli_options, + file_fetcher, + doc, + ) + .boxed_local() + }, + workspace_test_options.doc, ) .await?; - if workspace_test_options.no_run { - return Ok(()); - } - - let worker_factory = - Arc::new(factory.create_cli_main_worker_factory().await?); + let factories_with_specifiers = + if let Some(changed_paths) = changed_paths { + file_container + .worker_factories_with_dependent_checked_specifiers( + &changed_paths.into_iter().collect(), + ) + .await? + } else { + file_container.worker_factories_with_checked_specifiers() + }; test_specifiers( - worker_factory, + factories_with_specifiers, &permissions, permission_desc_parser, - specifiers_for_typecheck_and_test, TestSpecifiersOptions { cwd: Url::from_directory_path(cli_options.initial_cwd()).map_err( |_| { @@ -1922,38 +1690,6 @@ pub async fn run_tests_with_watch( Ok(()) } -/// Extracts doc tests from files specified by the given specifiers. -async fn get_doc_tests( - specifiers_with_mode: &[(Url, TestMode)], - file_fetcher: &FileFetcher, -) -> Result, AnyError> { - let specifiers_needing_extraction = specifiers_with_mode - .iter() - .filter(|(_, mode)| mode.needs_test_extraction()) - .map(|(s, _)| s); - - let mut doc_tests = Vec::new(); - for s in specifiers_needing_extraction { - let file = file_fetcher.fetch_bypass_permissions(s).await?; - doc_tests.extend(extract_doc_tests(file)?); - } - - Ok(doc_tests) -} - -/// Get a list of specifiers that we need to perform typecheck and run tests on. -/// The result includes "pseudo specifiers" for doc tests. -fn get_target_specifiers( - specifiers_with_mode: Vec<(Url, TestMode)>, - doc_tests: &[File], -) -> Vec { - specifiers_with_mode - .into_iter() - .filter_map(|(s, mode)| mode.needs_test_run().then_some(s)) - .chain(doc_tests.iter().map(|d| d.specifier.clone())) - .collect() -} - /// Tracks failures for the `--fail-fast` argument in /// order to tell when to stop running tests. #[derive(Clone, Default)] From 78de1fb248114d27e6e59228c0e0515c7561a673 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 10 Dec 2024 09:06:42 +0000 Subject: [PATCH 17/33] use WorkspaceFileContainer for bench --- cli/factory.rs | 18 ++-- cli/tools/bench/mod.rs | 189 +++++++++++++++++++++++++++++++++++------ cli/tools/check.rs | 2 +- cli/tools/test/mod.rs | 4 +- 4 files changed, 175 insertions(+), 38 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index d650dbf878..b184035e13 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1102,7 +1102,7 @@ impl WorkspaceFileContainer { pub async fn from_workspace_dirs_with_files( mut workspace_dirs_with_files: Vec<(Arc, FilePatterns)>, factory: &CliFactory, - extract_doc_files: fn(File) -> Result, AnyError>, + extract_doc_files: Option Result, AnyError>>, collect_specifiers: fn( FilePatterns, Arc, @@ -1159,13 +1159,15 @@ impl WorkspaceFileContainer { ) .await?; let mut doc_snippet_specifiers = vec![]; - let root_permissions = factory.root_permissions_container()?; - for (s, _) in specifiers.iter().filter(|(_, i)| i.check_doc) { - let file = file_fetcher.fetch(s, root_permissions).await?; - let snippet_files = extract_doc_files(file)?; - for snippet_file in snippet_files { - doc_snippet_specifiers.push(snippet_file.specifier.clone()); - file_fetcher.insert_memory_files(snippet_file); + if let Some(extract_doc_files) = extract_doc_files { + let root_permissions = factory.root_permissions_container()?; + for (s, _) in specifiers.iter().filter(|(_, i)| i.check_doc) { + let file = file_fetcher.fetch(s, root_permissions).await?; + let snippet_files = extract_doc_files(file)?; + for snippet_file in snippet_files { + doc_snippet_specifiers.push(snippet_file.specifier.clone()); + file_fetcher.insert_memory_files(snippet_file); + } } } let worker_factory = diff --git a/cli/tools/bench/mod.rs b/cli/tools/bench/mod.rs index 1d49fa061d..b62a6330df 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -5,6 +5,8 @@ use crate::args::Flags; use crate::colors; use crate::display::write_json_to_stdout; use crate::factory::CliFactory; +use crate::factory::SpecifierInfo; +use crate::factory::WorkspaceFileContainer; use crate::graph_util::has_graph_root_local_dependent_changed; use crate::ops; use crate::tools::test::format_test_error; @@ -21,6 +23,7 @@ use deno_core::error::AnyError; use deno_core::error::JsError; use deno_core::futures::future; use deno_core::futures::stream; +use deno_core::futures::FutureExt; use deno_core::futures::StreamExt; use deno_core::serde_v8; use deno_core::unsync::spawn; @@ -379,6 +382,133 @@ async fn bench_specifiers( Ok(()) } +/// Test a collection of specifiers with test modes concurrently. +async fn bench_specifiers2( + factories_with_specifiers: Vec<( + Arc, + Vec, + )>, + permissions: &Permissions, + permissions_desc_parser: &Arc, + options: BenchSpecifierOptions, +) -> Result<(), AnyError> { + let specifiers_with_factory = factories_with_specifiers + .into_iter() + .flat_map(|(f, s)| { + s.into_iter().map(|s| (s, f.clone())).collect::>() + }) + .collect::>(); + let (sender, mut receiver) = unbounded_channel::(); + let log_level = options.log_level; + let option_for_handles = options.clone(); + + let join_handles = specifiers_with_factory.into_iter().map( + move |(specifier, worker_factory)| { + let permissions_container = PermissionsContainer::new( + permissions_desc_parser.clone(), + permissions.clone(), + ); + let sender = sender.clone(); + let options = option_for_handles.clone(); + spawn_blocking(move || { + let future = bench_specifier( + worker_factory, + permissions_container, + specifier, + sender, + options.filter, + ); + create_and_run_current_thread(future) + }) + }, + ); + + let join_stream = stream::iter(join_handles) + .buffer_unordered(1) + .collect::, tokio::task::JoinError>>>(); + + let handler = { + spawn(async move { + let mut used_only = false; + let mut report = BenchReport::new(); + let mut reporter = + create_reporter(log_level != Some(Level::Error), options.json); + let mut benches = IndexMap::new(); + + while let Some(event) = receiver.recv().await { + match event { + BenchEvent::Plan(plan) => { + report.total += plan.total; + if plan.used_only { + used_only = true; + } + + reporter.report_plan(&plan); + } + + BenchEvent::Register(desc) => { + reporter.report_register(&desc); + benches.insert(desc.id, desc); + } + + BenchEvent::Wait(id) => { + reporter.report_wait(benches.get(&id).unwrap()); + } + + BenchEvent::Output(output) => { + reporter.report_output(&output); + } + + BenchEvent::Result(id, result) => { + let desc = benches.get(&id).unwrap(); + reporter.report_result(desc, &result); + match result { + BenchResult::Ok(stats) => { + report.measurements.push((desc.clone(), stats)); + } + + BenchResult::Failed(failure) => { + report.failed += 1; + report.failures.push((desc.clone(), failure)); + } + }; + } + + BenchEvent::UncaughtError(origin, error) => { + report.failed += 1; + reporter.report_uncaught_error(&origin, error); + } + } + } + + reporter.report_end(&report); + + if used_only { + return Err(generic_error( + "Bench failed because the \"only\" option was used", + )); + } + + if report.failed > 0 { + return Err(generic_error("Bench failed")); + } + + Ok(()) + }) + }; + + let (join_results, result) = future::join(join_stream, handler).await; + + // propagate any errors + for join_result in join_results { + join_result??; + } + + result??; + + Ok(()) +} + /// Checks if the path has a basename and extension Deno supports for benches. fn is_supported_bench_path(entry: WalkEntry) -> bool { if !is_script_ext(entry.path) { @@ -420,44 +550,49 @@ pub async fn run_benchmarks( permission_desc_parser.as_ref(), &cli_options.permissions_options(), )?; + let log_level = cli_options.log_level(); - let members_with_bench_options = - cli_options.resolve_bench_options_for_members(&bench_flags)?; - let specifiers = members_with_bench_options - .iter() - .map(|(_, bench_options)| { - collect_specifiers( - bench_options.files.clone(), - cli_options.vendor_dir_path().map(ToOwned::to_owned), - is_supported_bench_path, - ) - }) - .collect::, _>>()? + let workspace_dirs_with_files = cli_options + .resolve_bench_options_for_members(&bench_flags)? .into_iter() - .flatten() - .collect::>(); - - if specifiers.is_empty() { - return Err(generic_error("No bench modules found")); + .map(|(d, o)| (Arc::new(d), o.files)) + .collect(); + let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( + workspace_dirs_with_files, + &factory, + None, + |patterns, cli_options, _, _| { + async move { + let info = SpecifierInfo { + check: true, + check_doc: false, + }; + collect_specifiers( + patterns, + cli_options.vendor_dir_path().map(ToOwned::to_owned), + is_supported_bench_path, + ) + .map(|s| s.into_iter().map(|s| (s, info)).collect()) + } + .boxed_local() + }, + (), + ) + .await?; + if !file_container.found_specifiers() { + return Err(generic_error("No test modules found")); } - let main_graph_container = factory.main_module_graph_container().await?; - main_graph_container - .check_specifiers(&specifiers, cli_options.ext_flag().as_ref()) - .await?; + file_container.check().await?; if workspace_bench_options.no_run { return Ok(()); } - let log_level = cli_options.log_level(); - let worker_factory = - Arc::new(factory.create_cli_main_worker_factory().await?); - bench_specifiers( - worker_factory, + bench_specifiers2( + file_container.worker_factories_with_checked_specifiers(), &permissions, &permission_desc_parser, - specifiers, BenchSpecifierOptions { filter: TestFilter::from_flag(&workspace_bench_options.filter), json: workspace_bench_options.json, diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 73f0fcb145..a52d717fb9 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -77,7 +77,7 @@ pub async fn check( let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, &factory, - extract_snippet_files, + Some(extract_snippet_files), |patterns, cli_options, _, (doc, doc_only)| { async move { let info = SpecifierInfo { diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 1424a11c55..a8b20f709d 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -1507,7 +1507,7 @@ pub async fn run_tests( let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, &factory, - extract_doc_tests, + Some(extract_doc_tests), |patterns, cli_options, file_fetcher, doc| { collect_specifiers_for_tests(patterns, cli_options, file_fetcher, doc) .boxed_local() @@ -1627,7 +1627,7 @@ pub async fn run_tests_with_watch( WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, &factory, - extract_doc_tests, + Some(extract_doc_tests), |patterns, cli_options, file_fetcher, doc| { collect_specifiers_for_tests( patterns, From 1af0cdee0ad0833c3dd1f35a790408cb0c02df2c Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 10 Dec 2024 09:31:52 +0000 Subject: [PATCH 18/33] use single workspace dir for test -c and bench -c --- cli/args/flags.rs | 8 +++++++ cli/args/mod.rs | 4 ++++ cli/tools/bench/mod.rs | 20 ++++++++++++----- cli/tools/check.rs | 8 +------ cli/tools/test/mod.rs | 50 +++++++++++++++++++++++++++--------------- 5 files changed, 60 insertions(+), 30 deletions(-) diff --git a/cli/args/flags.rs b/cli/args/flags.rs index cdeaa1b335..1f07392e37 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -1172,6 +1172,14 @@ impl Flags { Ok(PathOrPatternSet::default()) } } + + pub fn is_discovered_config(&self) -> bool { + match self.config_flag { + ConfigFlag::Discover => true, + ConfigFlag::Path(_) => false, + ConfigFlag::Disabled => false, + } + } } static ENV_VARIABLES_HELP: &str = cstr!( diff --git a/cli/args/mod.rs b/cli/args/mod.rs index f78cd4edc6..5e0dc0ba09 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1011,6 +1011,10 @@ impl CliOptions { } } + pub fn is_discovered_config(&self) -> bool { + self.flags.is_discovered_config() + } + pub fn npm_system_info(&self) -> NpmSystemInfo { match self.sub_command() { DenoSubcommand::Compile(CompileFlags { diff --git a/cli/tools/bench/mod.rs b/cli/tools/bench/mod.rs index b62a6330df..a47d0de55f 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -1,6 +1,7 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use crate::args::BenchFlags; +use crate::args::BenchOptions; use crate::args::Flags; use crate::colors; use crate::display::write_json_to_stdout; @@ -552,11 +553,20 @@ pub async fn run_benchmarks( )?; let log_level = cli_options.log_level(); - let workspace_dirs_with_files = cli_options - .resolve_bench_options_for_members(&bench_flags)? - .into_iter() - .map(|(d, o)| (Arc::new(d), o.files)) - .collect(); + let workspace_dirs_with_files = if cli_options.is_discovered_config() { + cli_options + .resolve_bench_options_for_members(&bench_flags)? + .into_iter() + .map(|(d, o)| (Arc::new(d), o.files)) + .collect() + } else { + let patterns = bench_flags + .files + .as_file_patterns(cli_options.initial_cwd())?; + let config = cli_options.start_dir.to_bench_config(patterns)?; + let options = BenchOptions::resolve(config, &bench_flags); + vec![(cli_options.start_dir.clone(), options.files)] + }; let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, &factory, diff --git a/cli/tools/check.rs b/cli/tools/check.rs index a52d717fb9..52bd844ccc 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -20,7 +20,6 @@ use regex::Regex; use crate::args::check_warn_tsconfig; use crate::args::CheckFlags; use crate::args::CliOptions; -use crate::args::ConfigFlag; use crate::args::FileFlags; use crate::args::Flags; use crate::args::TsConfig; @@ -49,14 +48,9 @@ pub async fn check( flags: Arc, check_flags: CheckFlags, ) -> Result<(), AnyError> { - let is_discovered_config = match flags.config_flag { - ConfigFlag::Discover => true, - ConfigFlag::Path(_) => false, - ConfigFlag::Disabled => false, - }; let factory = CliFactory::from_flags(flags); let cli_options = factory.cli_options()?; - let workspace_dirs_with_files = if is_discovered_config { + let workspace_dirs_with_files = if cli_options.is_discovered_config() { cli_options .resolve_file_flags_for_members(&FileFlags { ignore: Default::default(), diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index a8b20f709d..0b873476d8 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -3,6 +3,7 @@ use crate::args::CliOptions; use crate::args::Flags; use crate::args::TestFlags; +use crate::args::TestOptions; use crate::args::TestReporterConfig; use crate::colors; use crate::display; @@ -1499,11 +1500,20 @@ pub async fn run_tests( )?; let log_level = cli_options.log_level(); - let workspace_dirs_with_files = cli_options - .resolve_test_options_for_members(&test_flags)? - .into_iter() - .map(|(d, o)| (Arc::new(d), o.files)) - .collect(); + let workspace_dirs_with_files = if cli_options.is_discovered_config() { + cli_options + .resolve_test_options_for_members(&test_flags)? + .into_iter() + .map(|(d, o)| (Arc::new(d), o.files)) + .collect() + } else { + let patterns = test_flags + .files + .as_file_patterns(cli_options.initial_cwd())?; + let config = cli_options.start_dir.to_test_config(patterns)?; + let options = TestOptions::resolve(config, &test_flags); + vec![(cli_options.start_dir.clone(), options.files)] + }; let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, &factory, @@ -1605,24 +1615,28 @@ pub async fn run_tests_with_watch( )?; let log_level = cli_options.log_level(); - let members_with_test_options = - cli_options.resolve_test_options_for_members(&test_flags)?; - let watch_paths = members_with_test_options + let workspace_dirs_with_files = if cli_options.is_discovered_config() { + cli_options + .resolve_test_options_for_members(&test_flags)? + .into_iter() + .map(|(d, o)| (Arc::new(d), o.files)) + .collect() + } else { + let patterns = test_flags + .files + .as_file_patterns(cli_options.initial_cwd())?; + let config = cli_options.start_dir.to_test_config(patterns)?; + let options = TestOptions::resolve(config, &test_flags); + vec![(cli_options.start_dir.clone(), options.files)] + }; + let watch_paths = workspace_dirs_with_files .iter() - .filter_map(|(_, test_options)| { - test_options - .files - .include - .as_ref() - .map(|set| set.base_paths()) + .filter_map(|(_, files)| { + files.include.as_ref().map(|set| set.base_paths()) }) .flatten() .collect::>(); let _ = watcher_communicator.watch_paths(watch_paths); - let workspace_dirs_with_files = members_with_test_options - .into_iter() - .map(|(d, o)| (Arc::new(d), o.files)) - .collect(); let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, From 0c188ebab91ec8a3a898d2858947177b37af8bda Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 10 Dec 2024 09:34:55 +0000 Subject: [PATCH 19/33] restore test --watch=... support --- cli/tools/test/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 0b873476d8..caf1a8dd29 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -1615,6 +1615,7 @@ pub async fn run_tests_with_watch( )?; let log_level = cli_options.log_level(); + let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); let workspace_dirs_with_files = if cli_options.is_discovered_config() { cli_options .resolve_test_options_for_members(&test_flags)? From b8b9eea548ee117e9487b1718a101610fd8aea28 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 10 Dec 2024 09:42:41 +0000 Subject: [PATCH 20/33] restore checking to test --watch --- cli/tools/test/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index caf1a8dd29..4899a89063 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -1656,6 +1656,12 @@ pub async fn run_tests_with_watch( ) .await?; + file_container.check().await?; + + if workspace_test_options.no_run { + return Ok(()); + } + let factories_with_specifiers = if let Some(changed_paths) = changed_paths { file_container From 37d45d0e844b5e0d0fd6633deea188ca75167be3 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 10 Dec 2024 09:51:25 +0000 Subject: [PATCH 21/33] use WorkspaceFileContainer for bench --watch --- cli/graph_util.rs | 34 ------ cli/tools/bench/mod.rs | 256 ++++++++++------------------------------- 2 files changed, 60 insertions(+), 230 deletions(-) diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 83dfade1de..a5f444aef2 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -21,7 +21,6 @@ use crate::resolver::SloppyImportsCachedFs; use crate::tools::check; use crate::tools::check::TypeChecker; use crate::util::file_watcher::WatcherCommunicator; -use crate::util::fs::canonicalize_path; use deno_config::deno_json::JsxImportSourceConfig; use deno_config::workspace::JsrPackageConfig; use deno_core::anyhow::bail; @@ -44,7 +43,6 @@ use deno_graph::ModuleGraph; use deno_graph::ModuleGraphError; use deno_graph::ResolutionError; use deno_graph::SpecifierError; -use deno_path_util::url_to_file_path; use deno_resolver::sloppy_imports::SloppyImportsResolutionKind; use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node; @@ -53,7 +51,6 @@ use deno_semver::jsr::JsrDepPackageReq; use deno_semver::package::PackageNv; use import_map::ImportMapError; use node_resolver::InNpmPackageChecker; -use std::collections::HashSet; use std::error::Error; use std::ops::Deref; use std::path::PathBuf; @@ -989,37 +986,6 @@ fn get_import_prefix_missing_error(error: &ResolutionError) -> Option<&str> { maybe_specifier.map(|s| s.as_str()) } -/// Gets if any of the specified root's "file:" dependents are in the -/// provided changed set. -pub fn has_graph_root_local_dependent_changed( - graph: &ModuleGraph, - root: &ModuleSpecifier, - canonicalized_changed_paths: &HashSet, -) -> bool { - let mut dependent_specifiers = graph.walk( - std::iter::once(root), - deno_graph::WalkOptions { - follow_dynamic: true, - kind: GraphKind::All, - prefer_fast_check_graph: true, - check_js: true, - }, - ); - while let Some((s, _)) = dependent_specifiers.next() { - if let Ok(path) = url_to_file_path(s) { - if let Ok(path) = canonicalize_path(&path) { - if canonicalized_changed_paths.contains(&path) { - return true; - } - } - } else { - // skip walking this remote module's dependencies - dependent_specifiers.skip_previous_dependencies(); - } - } - false -} - #[derive(Clone, Debug)] pub struct FileWatcherReporter { watcher_communicator: Arc, diff --git a/cli/tools/bench/mod.rs b/cli/tools/bench/mod.rs index a47d0de55f..7070d077f5 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -8,7 +8,6 @@ use crate::display::write_json_to_stdout; use crate::factory::CliFactory; use crate::factory::SpecifierInfo; use crate::factory::WorkspaceFileContainer; -use crate::graph_util::has_graph_root_local_dependent_changed; use crate::ops; use crate::tools::test::format_test_error; use crate::tools::test::TestFilter; @@ -42,7 +41,6 @@ use indexmap::IndexSet; use log::Level; use serde::Deserialize; use serde::Serialize; -use std::collections::HashSet; use std::path::Path; use std::sync::Arc; use std::time::Duration; @@ -267,124 +265,6 @@ async fn bench_specifier_inner( /// Test a collection of specifiers with test modes concurrently. async fn bench_specifiers( - worker_factory: Arc, - permissions: &Permissions, - permissions_desc_parser: &Arc, - specifiers: Vec, - options: BenchSpecifierOptions, -) -> Result<(), AnyError> { - let (sender, mut receiver) = unbounded_channel::(); - let log_level = options.log_level; - let option_for_handles = options.clone(); - - let join_handles = specifiers.into_iter().map(move |specifier| { - let worker_factory = worker_factory.clone(); - let permissions_container = PermissionsContainer::new( - permissions_desc_parser.clone(), - permissions.clone(), - ); - let sender = sender.clone(); - let options = option_for_handles.clone(); - spawn_blocking(move || { - let future = bench_specifier( - worker_factory, - permissions_container, - specifier, - sender, - options.filter, - ); - create_and_run_current_thread(future) - }) - }); - - let join_stream = stream::iter(join_handles) - .buffer_unordered(1) - .collect::, tokio::task::JoinError>>>(); - - let handler = { - spawn(async move { - let mut used_only = false; - let mut report = BenchReport::new(); - let mut reporter = - create_reporter(log_level != Some(Level::Error), options.json); - let mut benches = IndexMap::new(); - - while let Some(event) = receiver.recv().await { - match event { - BenchEvent::Plan(plan) => { - report.total += plan.total; - if plan.used_only { - used_only = true; - } - - reporter.report_plan(&plan); - } - - BenchEvent::Register(desc) => { - reporter.report_register(&desc); - benches.insert(desc.id, desc); - } - - BenchEvent::Wait(id) => { - reporter.report_wait(benches.get(&id).unwrap()); - } - - BenchEvent::Output(output) => { - reporter.report_output(&output); - } - - BenchEvent::Result(id, result) => { - let desc = benches.get(&id).unwrap(); - reporter.report_result(desc, &result); - match result { - BenchResult::Ok(stats) => { - report.measurements.push((desc.clone(), stats)); - } - - BenchResult::Failed(failure) => { - report.failed += 1; - report.failures.push((desc.clone(), failure)); - } - }; - } - - BenchEvent::UncaughtError(origin, error) => { - report.failed += 1; - reporter.report_uncaught_error(&origin, error); - } - } - } - - reporter.report_end(&report); - - if used_only { - return Err(generic_error( - "Bench failed because the \"only\" option was used", - )); - } - - if report.failed > 0 { - return Err(generic_error("Bench failed")); - } - - Ok(()) - }) - }; - - let (join_results, result) = future::join(join_stream, handler).await; - - // propagate any errors - for join_result in join_results { - join_result??; - } - - result??; - - Ok(()) -} - -/// Test a collection of specifiers with test modes concurrently. -async fn bench_specifiers2( factories_with_specifiers: Vec<( Arc, Vec, @@ -599,7 +479,7 @@ pub async fn run_benchmarks( return Ok(()); } - bench_specifiers2( + bench_specifiers( file_container.worker_factories_with_checked_specifiers(), &permissions, &permission_desc_parser, @@ -614,7 +494,6 @@ pub async fn run_benchmarks( Ok(()) } -// TODO(bartlomieju): heavy duplication of code with `cli/tools/test.rs` pub async fn run_benchmarks_with_watch( flags: Arc, bench_flags: BenchFlags, @@ -640,96 +519,81 @@ pub async fn run_benchmarks_with_watch( let cli_options = factory.cli_options()?; let workspace_bench_options = cli_options.resolve_workspace_bench_options(&bench_flags); - - let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); - - let graph_kind = cli_options.type_check_mode().as_graph_kind(); - let module_graph_creator = factory.module_graph_creator().await?; - let members_with_bench_options = - cli_options.resolve_bench_options_for_members(&bench_flags)?; - let watch_paths = members_with_bench_options - .iter() - .filter_map(|(_, bench_options)| { - bench_options - .files - .include - .as_ref() - .map(|set| set.base_paths()) - }) - .flatten() - .collect::>(); - let _ = watcher_communicator.watch_paths(watch_paths); - let collected_bench_modules = members_with_bench_options - .iter() - .map(|(_, bench_options)| { - collect_specifiers( - bench_options.files.clone(), - cli_options.vendor_dir_path().map(ToOwned::to_owned), - is_supported_bench_path, - ) - }) - .collect::, _>>()? - .into_iter() - .flatten() - .collect::>(); - - // Various bench files should not share the same permissions in terms of - // `PermissionsContainer` - otherwise granting/revoking permissions in one - // file would have impact on other files, which is undesirable. - let permission_desc_parser = factory.permission_desc_parser()?.clone(); + let permission_desc_parser = factory.permission_desc_parser()?; let permissions = Permissions::from_options( permission_desc_parser.as_ref(), &cli_options.permissions_options(), )?; + let log_level = cli_options.log_level(); - let graph = module_graph_creator - .create_graph(graph_kind, collected_bench_modules.clone()) - .await?; - module_graph_creator.graph_valid(&graph)?; - let bench_modules = &graph.roots; - - let bench_modules_to_reload = if let Some(changed_paths) = changed_paths - { - let changed_paths = changed_paths.into_iter().collect::>(); - let mut result = IndexSet::with_capacity(bench_modules.len()); - for bench_module_specifier in bench_modules { - if has_graph_root_local_dependent_changed( - &graph, - bench_module_specifier, - &changed_paths, - ) { - result.insert(bench_module_specifier.clone()); - } - } - result + let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); + let workspace_dirs_with_files = if cli_options.is_discovered_config() { + cli_options + .resolve_bench_options_for_members(&bench_flags)? + .into_iter() + .map(|(d, o)| (Arc::new(d), o.files)) + .collect() } else { - bench_modules.clone() + let patterns = bench_flags + .files + .as_file_patterns(cli_options.initial_cwd())?; + let config = cli_options.start_dir.to_bench_config(patterns)?; + let options = BenchOptions::resolve(config, &bench_flags); + vec![(cli_options.start_dir.clone(), options.files)] }; - - let worker_factory = - Arc::new(factory.create_cli_main_worker_factory().await?); - - let specifiers = collected_bench_modules - .into_iter() - .filter(|specifier| bench_modules_to_reload.contains(specifier)) - .collect::>(); - - factory - .main_module_graph_container() - .await? - .check_specifiers(&specifiers, cli_options.ext_flag().as_ref()) + let watch_paths = workspace_dirs_with_files + .iter() + .filter_map(|(_, files)| { + files.include.as_ref().map(|set| set.base_paths()) + }) + .flatten() + .collect::>(); + let _ = watcher_communicator.watch_paths(watch_paths); + let file_container = + WorkspaceFileContainer::from_workspace_dirs_with_files( + workspace_dirs_with_files, + &factory, + None, + |patterns, cli_options, _, _| { + async move { + let info = SpecifierInfo { + check: true, + check_doc: false, + }; + collect_specifiers( + patterns, + cli_options.vendor_dir_path().map(ToOwned::to_owned), + is_supported_bench_path, + ) + .map(|s| s.into_iter().map(|s| (s, info)).collect()) + } + .boxed_local() + }, + (), + ) .await?; + file_container.check().await?; + if workspace_bench_options.no_run { return Ok(()); } - let log_level = cli_options.log_level(); + let factories_with_specifiers = + if let Some(changed_paths) = changed_paths { + file_container + .worker_factories_with_dependent_checked_specifiers( + &changed_paths.into_iter().collect(), + ) + .await? + } else { + file_container.worker_factories_with_checked_specifiers() + }; + bench_specifiers( - worker_factory, + factories_with_specifiers, &permissions, &permission_desc_parser, - specifiers, BenchSpecifierOptions { filter: TestFilter::from_flag(&workspace_bench_options.filter), json: workspace_bench_options.json, From 83ee4c9a73c4d6e6e010793dde6557a07fefc9d2 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Tue, 10 Dec 2024 10:10:31 +0000 Subject: [PATCH 22/33] lint --- cli/tools/bench/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/tools/bench/mod.rs b/cli/tools/bench/mod.rs index 7070d077f5..e5ac626b22 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -426,7 +426,7 @@ pub async fn run_benchmarks( // Various bench files should not share the same permissions in terms of // `PermissionsContainer` - otherwise granting/revoking permissions in one // file would have impact on other files, which is undesirable. - let permission_desc_parser = factory.permission_desc_parser()?.clone(); + let permission_desc_parser = factory.permission_desc_parser()?; let permissions = Permissions::from_options( permission_desc_parser.as_ref(), &cli_options.permissions_options(), @@ -482,7 +482,7 @@ pub async fn run_benchmarks( bench_specifiers( file_container.worker_factories_with_checked_specifiers(), &permissions, - &permission_desc_parser, + permission_desc_parser, BenchSpecifierOptions { filter: TestFilter::from_flag(&workspace_bench_options.filter), json: workspace_bench_options.json, @@ -593,7 +593,7 @@ pub async fn run_benchmarks_with_watch( bench_specifiers( factories_with_specifiers, &permissions, - &permission_desc_parser, + permission_desc_parser, BenchSpecifierOptions { filter: TestFilter::from_flag(&workspace_bench_options.filter), json: workspace_bench_options.json, From 8f5d50c787d824a548dbbba90264b40674c576dd Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 11 Dec 2024 13:48:22 +0000 Subject: [PATCH 23/33] private CliOptions::flags again --- cli/args/mod.rs | 20 +++++++++++++++++++- cli/factory.rs | 18 ++++-------------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index c42dfe07a0..fce65dc6f4 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -811,7 +811,7 @@ pub struct ScopeOptions { pub struct CliOptions { // the source of the options is a detail the rest of the // application need not concern itself with, so keep these private - pub flags: Arc, + flags: Arc, initial_cwd: PathBuf, main_module_cell: std::sync::OnceLock>, maybe_node_modules_folder: Option, @@ -960,6 +960,24 @@ impl CliOptions { ) } + pub fn with_new_start_dir_and_scope_options( + &self, + start_dir: Arc, + scope_options: Option, + ) -> Result { + let (npmrc, _) = discover_npmrc_from_workspace(&start_dir.workspace)?; + let lockfile = CliLockfile::discover(&self.flags, &start_dir.workspace)?; + Self::new( + self.flags.clone(), + self.initial_cwd().to_path_buf(), + lockfile.map(Arc::new), + npmrc, + start_dir, + false, + scope_options.map(Arc::new), + ) + } + /// This method is purposefully verbose to disourage its use. Do not use it /// except in the factory structs. Instead, prefer specific methods on `CliOptions` /// that can take all sources of information into account (ex. config files or env vars). diff --git a/cli/factory.rs b/cli/factory.rs index 49b1af0f6a..ede7c0206a 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1,10 +1,8 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use crate::args::check_warn_tsconfig; -use crate::args::discover_npmrc_from_workspace; use crate::args::get_root_cert_store; use crate::args::CaData; -use crate::args::CliLockfile; use crate::args::CliOptions; use crate::args::DenoSubcommand; use crate::args::Flags; @@ -1131,24 +1129,16 @@ impl WorkspaceFileContainer { let dir_count = workspace_dirs_with_files.len(); let mut entries = Vec::with_capacity(dir_count); for (workspace_dir, files) in workspace_dirs_with_files { - let (npmrc, _) = discover_npmrc_from_workspace(&workspace_dir.workspace)?; - let lockfile = - CliLockfile::discover(&cli_options.flags, &workspace_dir.workspace)?; let scope_options = (dir_count > 1).then(|| ScopeOptions { scope: workspace_dir .has_deno_or_pkg_json() .then(|| workspace_dir.dir_url().clone()), all_scopes: all_scopes.clone(), }); - let cli_options = Arc::new(CliOptions::new( - cli_options.flags.clone(), - cli_options.initial_cwd().to_path_buf(), - lockfile.map(Arc::new), - npmrc, - workspace_dir, - false, - scope_options.map(Arc::new), - )?); + let cli_options = Arc::new( + cli_options + .with_new_start_dir_and_scope_options(workspace_dir, scope_options)?, + ); let mut factory = CliFactory::from_cli_options(cli_options.clone()); factory.watcher_communicator = watcher_communicator.clone(); let file_fetcher = factory.file_fetcher()?; From f45d39b20bf05b58c320c9ae16c7bbd9c2b9678e Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 11 Dec 2024 14:34:15 +0000 Subject: [PATCH 24/33] move -c handling to CliOptions methods --- cli/args/mod.rs | 114 ++++++++++++++++++++++++++++------------- cli/tools/bench/mod.rs | 39 ++++---------- cli/tools/check.rs | 19 ++----- cli/tools/lint/mod.rs | 4 +- cli/tools/test/mod.rs | 39 ++++---------- 5 files changed, 103 insertions(+), 112 deletions(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index fce65dc6f4..4e07485dbf 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1027,10 +1027,6 @@ impl CliOptions { } } - pub fn is_discovered_config(&self) -> bool { - self.flags.is_discovered_config() - } - pub fn npm_system_info(&self) -> NpmSystemInfo { match self.sub_command() { DenoSubcommand::Compile(CompileFlags { @@ -1342,12 +1338,22 @@ impl CliOptions { pub fn resolve_fmt_options_for_members( &self, fmt_flags: &FmtFlags, - ) -> Result, AnyError> { + ) -> Result, FmtOptions)>, AnyError> { let cli_arg_patterns = fmt_flags.files.as_file_patterns(self.initial_cwd())?; - let member_configs = self - .workspace() - .resolve_fmt_config_for_members(&cli_arg_patterns)?; + let member_configs = if self.flags.is_discovered_config() { + self + .workspace() + .resolve_fmt_config_for_members(&cli_arg_patterns)? + .into_iter() + .map(|(d, c)| (Arc::new(d), c)) + .collect::>() + } else { + vec![( + self.start_dir.clone(), + self.start_dir.to_fmt_config(cli_arg_patterns)?, + )] + }; let unstable = self.resolve_config_unstable_fmt_options(); let mut result = Vec::with_capacity(member_configs.len()); for (ctx, config) in member_configs { @@ -1376,23 +1382,43 @@ impl CliOptions { pub fn resolve_file_flags_for_members( &self, file_flags: &FileFlags, - ) -> Result, AnyError> { + ) -> Result, FilePatterns)>, AnyError> { let cli_arg_patterns = file_flags.as_file_patterns(self.initial_cwd())?; - let member_patterns = self - .workspace() - .resolve_file_patterns_for_members(&cli_arg_patterns)?; + let member_patterns = if self.flags.is_discovered_config() { + self + .workspace() + .resolve_file_patterns_for_members(&cli_arg_patterns)? + .into_iter() + .map(|(d, p)| (Arc::new(d), p)) + .collect::>() + } else { + vec![( + self.start_dir.clone(), + self.start_dir.to_resolved_file_patterns(cli_arg_patterns)?, + )] + }; Ok(member_patterns) } pub fn resolve_lint_options_for_members( &self, lint_flags: &LintFlags, - ) -> Result, AnyError> { + ) -> Result, LintOptions)>, AnyError> { let cli_arg_patterns = lint_flags.files.as_file_patterns(self.initial_cwd())?; - let member_configs = self - .workspace() - .resolve_lint_config_for_members(&cli_arg_patterns)?; + let member_configs = if self.flags.is_discovered_config() { + self + .workspace() + .resolve_lint_config_for_members(&cli_arg_patterns)? + .into_iter() + .map(|(d, c)| (Arc::new(d), c)) + .collect::>() + } else { + vec![( + self.start_dir.clone(), + self.start_dir.to_lint_config(cli_arg_patterns)?, + )] + }; let mut result = Vec::with_capacity(member_configs.len()); for (ctx, config) in member_configs { let options = LintOptions::resolve(config, lint_flags); @@ -1430,18 +1456,26 @@ impl CliOptions { pub fn resolve_test_options_for_members( &self, test_flags: &TestFlags, - ) -> Result, AnyError> { + ) -> Result, TestOptions)>, AnyError> { let cli_arg_patterns = test_flags.files.as_file_patterns(self.initial_cwd())?; - let workspace_dir_configs = self - .workspace() - .resolve_test_config_for_members(&cli_arg_patterns)?; - let mut result = Vec::with_capacity(workspace_dir_configs.len()); - for (member_dir, config) in workspace_dir_configs { - let options = TestOptions::resolve(config, test_flags); - result.push((member_dir, options)); - } - Ok(result) + let member_options = if self.flags.is_discovered_config() { + self + .workspace() + .resolve_test_config_for_members(&cli_arg_patterns)? + .into_iter() + .map(|(d, c)| (Arc::new(d), TestOptions::resolve(c, test_flags))) + .collect::>() + } else { + vec![( + self.start_dir.clone(), + TestOptions::resolve( + self.start_dir.to_test_config(cli_arg_patterns)?, + test_flags, + ), + )] + }; + Ok(member_options) } pub fn resolve_workspace_bench_options( @@ -1454,18 +1488,26 @@ impl CliOptions { pub fn resolve_bench_options_for_members( &self, bench_flags: &BenchFlags, - ) -> Result, AnyError> { + ) -> Result, BenchOptions)>, AnyError> { let cli_arg_patterns = bench_flags.files.as_file_patterns(self.initial_cwd())?; - let workspace_dir_configs = self - .workspace() - .resolve_bench_config_for_members(&cli_arg_patterns)?; - let mut result = Vec::with_capacity(workspace_dir_configs.len()); - for (member_dir, config) in workspace_dir_configs { - let options = BenchOptions::resolve(config, bench_flags); - result.push((member_dir, options)); - } - Ok(result) + let member_options = if self.flags.is_discovered_config() { + self + .workspace() + .resolve_bench_config_for_members(&cli_arg_patterns)? + .into_iter() + .map(|(d, c)| (Arc::new(d), BenchOptions::resolve(c, bench_flags))) + .collect::>() + } else { + vec![( + self.start_dir.clone(), + BenchOptions::resolve( + self.start_dir.to_bench_config(cli_arg_patterns)?, + bench_flags, + ), + )] + }; + Ok(member_options) } /// Vector of user script CLI arguments. diff --git a/cli/tools/bench/mod.rs b/cli/tools/bench/mod.rs index e5ac626b22..875a06ed20 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -1,7 +1,6 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use crate::args::BenchFlags; -use crate::args::BenchOptions; use crate::args::Flags; use crate::colors; use crate::display::write_json_to_stdout; @@ -433,20 +432,11 @@ pub async fn run_benchmarks( )?; let log_level = cli_options.log_level(); - let workspace_dirs_with_files = if cli_options.is_discovered_config() { - cli_options - .resolve_bench_options_for_members(&bench_flags)? - .into_iter() - .map(|(d, o)| (Arc::new(d), o.files)) - .collect() - } else { - let patterns = bench_flags - .files - .as_file_patterns(cli_options.initial_cwd())?; - let config = cli_options.start_dir.to_bench_config(patterns)?; - let options = BenchOptions::resolve(config, &bench_flags); - vec![(cli_options.start_dir.clone(), options.files)] - }; + let workspace_dirs_with_files = cli_options + .resolve_bench_options_for_members(&bench_flags)? + .into_iter() + .map(|(d, o)| (d, o.files)) + .collect(); let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, &factory, @@ -527,20 +517,11 @@ pub async fn run_benchmarks_with_watch( let log_level = cli_options.log_level(); let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); - let workspace_dirs_with_files = if cli_options.is_discovered_config() { - cli_options - .resolve_bench_options_for_members(&bench_flags)? - .into_iter() - .map(|(d, o)| (Arc::new(d), o.files)) - .collect() - } else { - let patterns = bench_flags - .files - .as_file_patterns(cli_options.initial_cwd())?; - let config = cli_options.start_dir.to_bench_config(patterns)?; - let options = BenchOptions::resolve(config, &bench_flags); - vec![(cli_options.start_dir.clone(), options.files)] - }; + let workspace_dirs_with_files = cli_options + .resolve_bench_options_for_members(&bench_flags)? + .into_iter() + .map(|(d, o)| (d, o.files)) + .collect::>(); let watch_paths = workspace_dirs_with_files .iter() .filter_map(|(_, files)| { diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 52bd844ccc..142231fd7b 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -50,24 +50,11 @@ pub async fn check( ) -> Result<(), AnyError> { let factory = CliFactory::from_flags(flags); let cli_options = factory.cli_options()?; - let workspace_dirs_with_files = if cli_options.is_discovered_config() { - cli_options - .resolve_file_flags_for_members(&FileFlags { - ignore: Default::default(), - include: check_flags.files, - })? - .into_iter() - .map(|(d, p)| (Arc::new(d), p)) - .collect() - } else { - let file_flags = FileFlags { + let workspace_dirs_with_files = + cli_options.resolve_file_flags_for_members(&FileFlags { ignore: Default::default(), include: check_flags.files, - }; - let patterns = file_flags.as_file_patterns(cli_options.initial_cwd())?; - let patterns = cli_options.start_dir.to_resolved_file_patterns(patterns)?; - vec![(cli_options.start_dir.clone(), patterns)] - }; + })?; let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, &factory, diff --git a/cli/tools/lint/mod.rs b/cli/tools/lint/mod.rs index 596359bdc0..0839b7750c 100644 --- a/cli/tools/lint/mod.rs +++ b/cli/tools/lint/mod.rs @@ -200,7 +200,7 @@ pub async fn lint( } struct PathsWithOptions { - dir: WorkspaceDirectory, + dir: Arc, paths: Vec, options: LintOptions, } @@ -270,7 +270,7 @@ impl WorkspaceLinter { cli_options: &Arc, lint_options: LintOptions, lint_config: LintConfig, - member_dir: WorkspaceDirectory, + member_dir: Arc, paths: Vec, ) -> Result<(), AnyError> { self.file_count += paths.len(); diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 4899a89063..95862a5186 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -3,7 +3,6 @@ use crate::args::CliOptions; use crate::args::Flags; use crate::args::TestFlags; -use crate::args::TestOptions; use crate::args::TestReporterConfig; use crate::colors; use crate::display; @@ -1500,20 +1499,11 @@ pub async fn run_tests( )?; let log_level = cli_options.log_level(); - let workspace_dirs_with_files = if cli_options.is_discovered_config() { - cli_options - .resolve_test_options_for_members(&test_flags)? - .into_iter() - .map(|(d, o)| (Arc::new(d), o.files)) - .collect() - } else { - let patterns = test_flags - .files - .as_file_patterns(cli_options.initial_cwd())?; - let config = cli_options.start_dir.to_test_config(patterns)?; - let options = TestOptions::resolve(config, &test_flags); - vec![(cli_options.start_dir.clone(), options.files)] - }; + let workspace_dirs_with_files = cli_options + .resolve_test_options_for_members(&test_flags)? + .into_iter() + .map(|(d, o)| (d, o.files)) + .collect(); let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( workspace_dirs_with_files, &factory, @@ -1616,20 +1606,11 @@ pub async fn run_tests_with_watch( let log_level = cli_options.log_level(); let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); - let workspace_dirs_with_files = if cli_options.is_discovered_config() { - cli_options - .resolve_test_options_for_members(&test_flags)? - .into_iter() - .map(|(d, o)| (Arc::new(d), o.files)) - .collect() - } else { - let patterns = test_flags - .files - .as_file_patterns(cli_options.initial_cwd())?; - let config = cli_options.start_dir.to_test_config(patterns)?; - let options = TestOptions::resolve(config, &test_flags); - vec![(cli_options.start_dir.clone(), options.files)] - }; + let workspace_dirs_with_files = cli_options + .resolve_test_options_for_members(&test_flags)? + .into_iter() + .map(|(d, o)| (d, o.files)) + .collect::>(); let watch_paths = workspace_dirs_with_files .iter() .filter_map(|(_, files)| { From 5d76fc9d3edcb1ca92f89ed5bc7e95a41ef239df Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 16 Dec 2024 11:13:23 +0000 Subject: [PATCH 25/33] remove need for outer factory --- cli/factory.rs | 197 +++++++++++++++++++++-------------------- cli/tools/bench/mod.rs | 154 +++++++++++++++----------------- cli/tools/check.rs | 56 ++++++------ cli/tools/test/mod.rs | 133 +++++++++++++--------------- 4 files changed, 258 insertions(+), 282 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index ede7c0206a..d31d39ac1a 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -93,6 +93,7 @@ use deno_runtime::deno_node::NodeResolver; use deno_runtime::deno_node::PackageJsonResolver; use deno_runtime::deno_permissions::Permissions; use deno_runtime::deno_permissions::PermissionsContainer; +use deno_runtime::deno_permissions::PermissionsOptions; use deno_runtime::deno_tls::rustls::RootCertStore; use deno_runtime::deno_tls::RootCertStoreProvider; use deno_runtime::deno_web::BlobStore; @@ -1075,33 +1076,99 @@ pub struct SpecifierInfo { pub check_doc: bool, } -struct WorkspaceFileContainerEntry { +pub struct WorkspaceDirFilesFactory { specifiers: Vec<(ModuleSpecifier, SpecifierInfo)>, doc_snippet_specifiers: Vec, - factory: CliFactory, - worker_factory: Arc, + cli_options: Arc, + cli_factory: CliFactory, + permissions_options: Deferred, } -impl WorkspaceFileContainerEntry { - fn checked_specifiers(&self) -> impl Iterator { +impl WorkspaceDirFilesFactory { + pub fn checked_specifiers(&self) -> impl Iterator { self .specifiers .iter() .filter_map(|(s, i)| i.check.then_some(s)) .chain(self.doc_snippet_specifiers.iter()) } + + pub async fn dependent_checked_specifiers( + &self, + canonicalized_dep_paths: &HashSet, + ) -> Result, AnyError> { + let graph_kind = self + .cli_factory + .cli_options()? + .type_check_mode() + .as_graph_kind(); + let module_graph_creator = self.cli_factory.module_graph_creator().await?; + let specifiers = self.checked_specifiers().cloned().collect::>(); + let graph = module_graph_creator + .create_graph( + graph_kind, + specifiers.clone(), + crate::graph_util::NpmCachingStrategy::Eager, + ) + .await?; + module_graph_creator.graph_valid(&graph)?; + let dependent_specifiers = self + .checked_specifiers() + .filter(|s| { + let mut dependency_specifiers = graph.walk( + std::iter::once(*s), + deno_graph::WalkOptions { + follow_dynamic: true, + kind: deno_graph::GraphKind::All, + prefer_fast_check_graph: true, + check_js: true, + }, + ); + while let Some((s, _)) = dependency_specifiers.next() { + if let Ok(path) = url_to_file_path(s) { + if let Ok(path) = canonicalize_path(&path) { + if canonicalized_dep_paths.contains(&path) { + return true; + } + } + } else { + // skip walking this remote module's dependencies + dependency_specifiers.skip_previous_dependencies(); + } + } + false + }) + .collect(); + Ok(dependent_specifiers) + } + + pub fn permissions_options(&self) -> &PermissionsOptions { + self + .permissions_options + .get_or_init(|| self.cli_options.permissions_options()) + } + + pub fn permission_desc_parser( + &self, + ) -> Result<&Arc, AnyError> { + self.cli_factory.permission_desc_parser() + } + + pub async fn create_cli_main_worker_factory( + &self, + ) -> Result { + self.cli_factory.create_cli_main_worker_factory().await + } } -pub struct WorkspaceFileContainer { - entries: Vec, +pub struct WorkspaceFilesFactory { + dirs: Vec, } -impl WorkspaceFileContainer { +impl WorkspaceFilesFactory { #[allow(clippy::type_complexity)] pub async fn from_workspace_dirs_with_files( mut workspace_dirs_with_files: Vec<(Arc, FilePatterns)>, - factory: &CliFactory, - extract_doc_files: Option Result, AnyError>>, collect_specifiers: fn( FilePatterns, Arc, @@ -1115,10 +1182,11 @@ impl WorkspaceFileContainer { >, >, args: T, + extract_doc_files: Option Result, AnyError>>, + cli_options: &CliOptions, + watcher_communicator: Option<&Arc>, ) -> Result { workspace_dirs_with_files.sort_by_cached_key(|(d, _)| d.dir_url().clone()); - let cli_options = factory.cli_options()?; - let watcher_communicator = &factory.watcher_communicator; let all_scopes = Arc::new( workspace_dirs_with_files .iter() @@ -1127,7 +1195,7 @@ impl WorkspaceFileContainer { .collect::>(), ); let dir_count = workspace_dirs_with_files.len(); - let mut entries = Vec::with_capacity(dir_count); + let mut dirs = Vec::with_capacity(dir_count); for (workspace_dir, files) in workspace_dirs_with_files { let scope_options = (dir_count > 1).then(|| ScopeOptions { scope: workspace_dir @@ -1140,11 +1208,11 @@ impl WorkspaceFileContainer { .with_new_start_dir_and_scope_options(workspace_dir, scope_options)?, ); let mut factory = CliFactory::from_cli_options(cli_options.clone()); - factory.watcher_communicator = watcher_communicator.clone(); + factory.watcher_communicator = watcher_communicator.cloned(); let file_fetcher = factory.file_fetcher()?; let specifiers = collect_specifiers( files, - cli_options, + cli_options.clone(), file_fetcher.clone(), args.clone(), ) @@ -1161,30 +1229,32 @@ impl WorkspaceFileContainer { } } } - let worker_factory = - Arc::new(factory.create_cli_main_worker_factory().await?); - entries.push(WorkspaceFileContainerEntry { + dirs.push(WorkspaceDirFilesFactory { specifiers, doc_snippet_specifiers, - factory, - worker_factory, + cli_options, + cli_factory: factory, + permissions_options: Default::default(), }); } - Ok(Self { entries }) + Ok(Self { dirs }) } pub async fn check(&self) -> Result<(), AnyError> { let mut diagnostics = vec![]; let mut all_errors = vec![]; - for entry in &self.entries { - let main_graph_container = - entry.factory.main_module_graph_container().await?.clone(); + for entry in &self.dirs { + let main_graph_container = entry + .cli_factory + .main_module_graph_container() + .await? + .clone(); let specifiers_for_typecheck = entry.checked_specifiers().cloned().collect::>(); if specifiers_for_typecheck.is_empty() { continue; } - let ext_flag = entry.factory.cli_options()?.ext_flag().as_ref(); + let ext_flag = entry.cli_factory.cli_options()?.ext_flag().as_ref(); if let Err(err) = main_graph_container .check_specifiers(&specifiers_for_typecheck, ext_flag) .await @@ -1213,78 +1283,11 @@ impl WorkspaceFileContainer { Ok(()) } + pub fn dirs(&self) -> &Vec { + &self.dirs + } + pub fn found_specifiers(&self) -> bool { - self.entries.iter().any(|e| !e.specifiers.is_empty()) - } - - pub fn worker_factories_with_checked_specifiers( - &self, - ) -> Vec<(Arc, Vec)> { - self - .entries - .iter() - .map(|e| { - ( - e.worker_factory.clone(), - e.checked_specifiers().cloned().collect(), - ) - }) - .collect() - } - - /// Per workspace directory, return a list of included checked specifiers - /// which depend on the modules at the passed paths. - pub async fn worker_factories_with_dependent_checked_specifiers( - &self, - canonicalized_dep_paths: &HashSet, - ) -> Result, Vec)>, AnyError> - { - let mut result = Vec::with_capacity(self.entries.len()); - for entry in &self.entries { - let graph_kind = entry - .factory - .cli_options()? - .type_check_mode() - .as_graph_kind(); - let module_graph_creator = entry.factory.module_graph_creator().await?; - let specifiers = entry.checked_specifiers().cloned().collect::>(); - let graph = module_graph_creator - .create_graph( - graph_kind, - specifiers.clone(), - crate::graph_util::NpmCachingStrategy::Eager, - ) - .await?; - module_graph_creator.graph_valid(&graph)?; - let dependent_specifiers = specifiers - .into_iter() - .filter(|s| { - let mut dependency_specifiers = graph.walk( - std::iter::once(s), - deno_graph::WalkOptions { - follow_dynamic: true, - kind: deno_graph::GraphKind::All, - prefer_fast_check_graph: true, - check_js: true, - }, - ); - while let Some((s, _)) = dependency_specifiers.next() { - if let Ok(path) = url_to_file_path(s) { - if let Ok(path) = canonicalize_path(&path) { - if canonicalized_dep_paths.contains(&path) { - return true; - } - } - } else { - // skip walking this remote module's dependencies - dependency_specifiers.skip_previous_dependencies(); - } - } - false - }) - .collect::>(); - result.push((entry.worker_factory.clone(), dependent_specifiers)); - } - Ok(result) + self.dirs.iter().any(|e| !e.specifiers.is_empty()) } } diff --git a/cli/tools/bench/mod.rs b/cli/tools/bench/mod.rs index 875a06ed20..a9dba2b1f2 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -1,12 +1,12 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use crate::args::BenchFlags; +use crate::args::CliOptions; use crate::args::Flags; use crate::colors; use crate::display::write_json_to_stdout; -use crate::factory::CliFactory; use crate::factory::SpecifierInfo; -use crate::factory::WorkspaceFileContainer; +use crate::factory::WorkspaceFilesFactory; use crate::ops; use crate::tools::test::format_test_error; use crate::tools::test::TestFilter; @@ -32,7 +32,6 @@ use deno_core::ModuleSpecifier; use deno_core::PollEventLoopOptions; use deno_runtime::deno_permissions::Permissions; use deno_runtime::deno_permissions::PermissionsContainer; -use deno_runtime::permissions::RuntimePermissionDescriptorParser; use deno_runtime::tokio_util::create_and_run_current_thread; use deno_runtime::WorkerExecutionMode; use indexmap::IndexMap; @@ -40,7 +39,9 @@ use indexmap::IndexSet; use log::Level; use serde::Deserialize; use serde::Serialize; +use std::collections::HashSet; use std::path::Path; +use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; use tokio::sync::mpsc::unbounded_channel; @@ -264,29 +265,42 @@ async fn bench_specifier_inner( /// Test a collection of specifiers with test modes concurrently. async fn bench_specifiers( - factories_with_specifiers: Vec<( - Arc, - Vec, - )>, - permissions: &Permissions, - permissions_desc_parser: &Arc, + workspace_files_factory: &WorkspaceFilesFactory, + changed_paths: Option<&HashSet>, options: BenchSpecifierOptions, ) -> Result<(), AnyError> { - let specifiers_with_factory = factories_with_specifiers - .into_iter() - .flat_map(|(f, s)| { - s.into_iter().map(|s| (s, f.clone())).collect::>() - }) - .collect::>(); + let mut specifiers_with_services = vec![]; + for factory in workspace_files_factory.dirs() { + let worker_factory = + Arc::new(factory.create_cli_main_worker_factory().await?); + let permission_desc_parser = factory.permission_desc_parser()?; + let permissions = Arc::new(Permissions::from_options( + permission_desc_parser.as_ref(), + factory.permissions_options(), + )?); + let specifiers = if let Some(changed_paths) = changed_paths { + factory.dependent_checked_specifiers(changed_paths).await? + } else { + factory.checked_specifiers().collect() + }; + specifiers_with_services.extend(specifiers.into_iter().map(|s| { + ( + s.clone(), + worker_factory.clone(), + permission_desc_parser.clone(), + permissions.clone(), + ) + })); + } let (sender, mut receiver) = unbounded_channel::(); let log_level = options.log_level; let option_for_handles = options.clone(); - let join_handles = specifiers_with_factory.into_iter().map( - move |(specifier, worker_factory)| { + let join_handles = specifiers_with_services.into_iter().map( + move |(specifier, worker_factory, permissions_desc_parser, permissions)| { let permissions_container = PermissionsContainer::new( permissions_desc_parser.clone(), - permissions.clone(), + permissions.as_ref().clone(), ); let sender = sender.clone(); let options = option_for_handles.clone(); @@ -418,18 +432,9 @@ pub async fn run_benchmarks( flags: Arc, bench_flags: BenchFlags, ) -> Result<(), AnyError> { - let factory = CliFactory::from_flags(flags); - let cli_options = factory.cli_options()?; + let cli_options = CliOptions::from_flags(flags)?; let workspace_bench_options = cli_options.resolve_workspace_bench_options(&bench_flags); - // Various bench files should not share the same permissions in terms of - // `PermissionsContainer` - otherwise granting/revoking permissions in one - // file would have impact on other files, which is undesirable. - let permission_desc_parser = factory.permission_desc_parser()?; - let permissions = Permissions::from_options( - permission_desc_parser.as_ref(), - &cli_options.permissions_options(), - )?; let log_level = cli_options.log_level(); let workspace_dirs_with_files = cli_options @@ -437,42 +442,43 @@ pub async fn run_benchmarks( .into_iter() .map(|(d, o)| (d, o.files)) .collect(); - let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( - workspace_dirs_with_files, - &factory, - None, - |patterns, cli_options, _, _| { - async move { - let info = SpecifierInfo { - check: true, - check_doc: false, - }; - collect_specifiers( - patterns, - cli_options.vendor_dir_path().map(ToOwned::to_owned), - is_supported_bench_path, - ) - .map(|s| s.into_iter().map(|s| (s, info)).collect()) - } - .boxed_local() - }, - (), - ) - .await?; - if !file_container.found_specifiers() { + let workspace_files_factory = + WorkspaceFilesFactory::from_workspace_dirs_with_files( + workspace_dirs_with_files, + |patterns, cli_options, _, _| { + async move { + let info = SpecifierInfo { + check: true, + check_doc: false, + }; + collect_specifiers( + patterns, + cli_options.vendor_dir_path().map(ToOwned::to_owned), + is_supported_bench_path, + ) + .map(|s| s.into_iter().map(|s| (s, info)).collect()) + } + .boxed_local() + }, + (), + None, + &cli_options, + None, + ) + .await?; + if !workspace_files_factory.found_specifiers() { return Err(generic_error("No test modules found")); } - file_container.check().await?; + workspace_files_factory.check().await?; if workspace_bench_options.no_run { return Ok(()); } bench_specifiers( - file_container.worker_factories_with_checked_specifiers(), - &permissions, - permission_desc_parser, + &workspace_files_factory, + None, BenchSpecifierOptions { filter: TestFilter::from_flag(&workspace_bench_options.filter), json: workspace_bench_options.json, @@ -502,18 +508,9 @@ pub async fn run_benchmarks_with_watch( let bench_flags = bench_flags.clone(); watcher_communicator.show_path_changed(changed_paths.clone()); Ok(async move { - let factory = CliFactory::from_flags_for_watcher( - flags, - watcher_communicator.clone(), - ); - let cli_options = factory.cli_options()?; + let cli_options = CliOptions::from_flags(flags)?; let workspace_bench_options = cli_options.resolve_workspace_bench_options(&bench_flags); - let permission_desc_parser = factory.permission_desc_parser()?; - let permissions = Permissions::from_options( - permission_desc_parser.as_ref(), - &cli_options.permissions_options(), - )?; let log_level = cli_options.log_level(); let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); @@ -530,11 +527,9 @@ pub async fn run_benchmarks_with_watch( .flatten() .collect::>(); let _ = watcher_communicator.watch_paths(watch_paths); - let file_container = - WorkspaceFileContainer::from_workspace_dirs_with_files( + let workspace_files_factory = + WorkspaceFilesFactory::from_workspace_dirs_with_files( workspace_dirs_with_files, - &factory, - None, |patterns, cli_options, _, _| { async move { let info = SpecifierInfo { @@ -551,30 +546,21 @@ pub async fn run_benchmarks_with_watch( .boxed_local() }, (), + None, + &cli_options, + Some(&watcher_communicator), ) .await?; - file_container.check().await?; + workspace_files_factory.check().await?; if workspace_bench_options.no_run { return Ok(()); } - let factories_with_specifiers = - if let Some(changed_paths) = changed_paths { - file_container - .worker_factories_with_dependent_checked_specifiers( - &changed_paths.into_iter().collect(), - ) - .await? - } else { - file_container.worker_factories_with_checked_specifiers() - }; - bench_specifiers( - factories_with_specifiers, - &permissions, - permission_desc_parser, + &workspace_files_factory, + changed_paths.map(|p| p.into_iter().collect()).as_ref(), BenchSpecifierOptions { filter: TestFilter::from_flag(&workspace_bench_options.filter), json: workspace_bench_options.json, diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 142231fd7b..c05b5f0d01 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -30,9 +30,8 @@ use crate::cache::CacheDBHash; use crate::cache::Caches; use crate::cache::FastInsecureHasher; use crate::cache::TypeCheckCache; -use crate::factory::CliFactory; use crate::factory::SpecifierInfo; -use crate::factory::WorkspaceFileContainer; +use crate::factory::WorkspaceFilesFactory; use crate::graph_util::BuildFastCheckGraphOptions; use crate::graph_util::ModuleGraphBuilder; use crate::npm::CliNpmResolver; @@ -48,39 +47,40 @@ pub async fn check( flags: Arc, check_flags: CheckFlags, ) -> Result<(), AnyError> { - let factory = CliFactory::from_flags(flags); - let cli_options = factory.cli_options()?; + let cli_options = CliOptions::from_flags(flags)?; let workspace_dirs_with_files = cli_options.resolve_file_flags_for_members(&FileFlags { ignore: Default::default(), include: check_flags.files, })?; - let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( - workspace_dirs_with_files, - &factory, - Some(extract_snippet_files), - |patterns, cli_options, _, (doc, doc_only)| { - async move { - let info = SpecifierInfo { - check: !doc_only, - check_doc: doc || doc_only, - }; - collect_specifiers( - patterns, - cli_options.vendor_dir_path().map(ToOwned::to_owned), - |e| is_script_ext(e.path), - ) - .map(|s| s.into_iter().map(|s| (s, info)).collect()) - } - .boxed_local() - }, - (check_flags.doc, check_flags.doc_only), - ) - .await?; - if !file_container.found_specifiers() { + let workspace_files_factory = + WorkspaceFilesFactory::from_workspace_dirs_with_files( + workspace_dirs_with_files, + |patterns, cli_options, _, (doc, doc_only)| { + async move { + let info = SpecifierInfo { + check: !doc_only, + check_doc: doc || doc_only, + }; + collect_specifiers( + patterns, + cli_options.vendor_dir_path().map(ToOwned::to_owned), + |e| is_script_ext(e.path), + ) + .map(|s| s.into_iter().map(|s| (s, info)).collect()) + } + .boxed_local() + }, + (check_flags.doc, check_flags.doc_only), + Some(extract_snippet_files), + &cli_options, + None, + ) + .await?; + if !workspace_files_factory.found_specifiers() { log::warn!("{} No matching files found.", colors::yellow("Warning")); } - file_container.check().await + workspace_files_factory.check().await } /// Options for performing a check of a module graph. Note that the decision to diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 95862a5186..a5438ad8e7 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -6,9 +6,8 @@ use crate::args::TestFlags; use crate::args::TestReporterConfig; use crate::colors; use crate::display; -use crate::factory::CliFactory; use crate::factory::SpecifierInfo; -use crate::factory::WorkspaceFileContainer; +use crate::factory::WorkspaceFilesFactory; use crate::file_fetcher::FileFetcher; use crate::ops; use crate::util::extract::extract_doc_tests; @@ -53,7 +52,6 @@ use deno_runtime::deno_io::StdioPipe; use deno_runtime::deno_permissions::Permissions; use deno_runtime::deno_permissions::PermissionsContainer; use deno_runtime::fmt_errors::format_js_error; -use deno_runtime::permissions::RuntimePermissionDescriptorParser; use deno_runtime::tokio_util::create_and_run_current_thread; use deno_runtime::worker::MainWorker; use deno_runtime::WorkerExecutionMode; @@ -77,6 +75,7 @@ use std::future::poll_fn; use std::io::Write; use std::num::NonZeroUsize; use std::path::Path; +use std::path::PathBuf; use std::sync::atomic::AtomicBool; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; @@ -1163,24 +1162,37 @@ static HAS_TEST_RUN_SIGINT_HANDLER: AtomicBool = AtomicBool::new(false); /// Test a collection of specifiers with test modes concurrently. async fn test_specifiers( - factories_with_specifiers: Vec<( - Arc, - Vec, - )>, - permissions: &Permissions, - permission_desc_parser: &Arc, + workspace_files_factory: &WorkspaceFilesFactory, + changed_paths: Option<&HashSet>, options: TestSpecifiersOptions, ) -> Result<(), AnyError> { - let mut specifiers_with_factory = factories_with_specifiers - .into_iter() - .flat_map(|(f, s)| { - s.into_iter().map(|s| (s, f.clone())).collect::>() - }) - .collect::>(); + let mut specifiers_with_services = vec![]; + for factory in workspace_files_factory.dirs() { + let worker_factory = + Arc::new(factory.create_cli_main_worker_factory().await?); + let permission_desc_parser = factory.permission_desc_parser()?; + let permissions = Arc::new(Permissions::from_options( + permission_desc_parser.as_ref(), + factory.permissions_options(), + )?); + let specifiers = if let Some(changed_paths) = changed_paths { + factory.dependent_checked_specifiers(changed_paths).await? + } else { + factory.checked_specifiers().collect() + }; + specifiers_with_services.extend(specifiers.into_iter().map(|s| { + ( + s.clone(), + worker_factory.clone(), + permission_desc_parser.clone(), + permissions.clone(), + ) + })); + } if let Some(seed) = options.specifier.shuffle { let mut rng = SmallRng::seed_from_u64(seed); - specifiers_with_factory.sort_by_cached_key(|(s, _)| s.to_string()); - specifiers_with_factory.shuffle(&mut rng); + specifiers_with_services.sort_by_cached_key(|(s, ..)| s.to_string()); + specifiers_with_services.shuffle(&mut rng); } let (test_event_sender_factory, receiver) = create_test_event_channel(); @@ -1195,11 +1207,11 @@ async fn test_specifiers( let reporter = get_test_reporter(&options); let fail_fast_tracker = FailFastTracker::new(options.fail_fast); - let join_handles = specifiers_with_factory.into_iter().map( - move |(specifier, worker_factory)| { + let join_handles = specifiers_with_services.into_iter().map( + move |(specifier, worker_factory, permission_desc_parser, permissions)| { let permissions_container = PermissionsContainer::new( - permission_desc_parser.clone(), - permissions.clone(), + permission_desc_parser, + permissions.as_ref().clone(), ); let worker_sender = test_event_sender_factory.worker(); let fail_fast_tracker = fail_fast_tracker.clone(); @@ -1485,18 +1497,12 @@ pub async fn run_tests( flags: Arc, test_flags: TestFlags, ) -> Result<(), AnyError> { - let factory = CliFactory::from_flags(flags); - let cli_options = factory.cli_options()?; + let cli_options = CliOptions::from_flags(flags)?; let workspace_test_options = cli_options.resolve_workspace_test_options(&test_flags); // Various test files should not share the same permissions in terms of // `PermissionsContainer` - otherwise granting/revoking permissions in one // file would have impact on other files, which is undesirable. - let permission_desc_parser = factory.permission_desc_parser()?; - let permissions = Permissions::from_options( - permission_desc_parser.as_ref(), - &cli_options.permissions_options(), - )?; let log_level = cli_options.log_level(); let workspace_dirs_with_files = cli_options @@ -1504,33 +1510,34 @@ pub async fn run_tests( .into_iter() .map(|(d, o)| (d, o.files)) .collect(); - let file_container = WorkspaceFileContainer::from_workspace_dirs_with_files( - workspace_dirs_with_files, - &factory, - Some(extract_doc_tests), - |patterns, cli_options, file_fetcher, doc| { - collect_specifiers_for_tests(patterns, cli_options, file_fetcher, doc) - .boxed_local() - }, - workspace_test_options.doc, - ) - .await?; + let workspace_files_factory = + WorkspaceFilesFactory::from_workspace_dirs_with_files( + workspace_dirs_with_files, + |patterns, cli_options, file_fetcher, doc| { + collect_specifiers_for_tests(patterns, cli_options, file_fetcher, doc) + .boxed_local() + }, + workspace_test_options.doc, + Some(extract_doc_tests), + &cli_options, + None, + ) + .await?; if !workspace_test_options.permit_no_files - && !file_container.found_specifiers() + && !workspace_files_factory.found_specifiers() { return Err(generic_error("No test modules found")); } - file_container.check().await?; + workspace_files_factory.check().await?; if workspace_test_options.no_run { return Ok(()); } test_specifiers( - file_container.worker_factories_with_checked_specifiers(), - &permissions, - permission_desc_parser, + &workspace_files_factory, + None, TestSpecifiersOptions { cwd: Url::from_directory_path(cli_options.initial_cwd()).map_err( |_| { @@ -1591,18 +1598,9 @@ pub async fn run_tests_with_watch( let test_flags = test_flags.clone(); watcher_communicator.show_path_changed(changed_paths.clone()); Ok(async move { - let factory = CliFactory::from_flags_for_watcher( - flags, - watcher_communicator.clone(), - ); - let cli_options = factory.cli_options()?; + let cli_options = CliOptions::from_flags(flags)?; let workspace_test_options = cli_options.resolve_workspace_test_options(&test_flags); - let permission_desc_parser = factory.permission_desc_parser()?; - let permissions = Permissions::from_options( - permission_desc_parser.as_ref(), - &cli_options.permissions_options(), - )?; let log_level = cli_options.log_level(); let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); @@ -1619,11 +1617,9 @@ pub async fn run_tests_with_watch( .flatten() .collect::>(); let _ = watcher_communicator.watch_paths(watch_paths); - let file_container = - WorkspaceFileContainer::from_workspace_dirs_with_files( + let workspace_files_factory = + WorkspaceFilesFactory::from_workspace_dirs_with_files( workspace_dirs_with_files, - &factory, - Some(extract_doc_tests), |patterns, cli_options, file_fetcher, doc| { collect_specifiers_for_tests( patterns, @@ -1634,30 +1630,21 @@ pub async fn run_tests_with_watch( .boxed_local() }, workspace_test_options.doc, + Some(extract_doc_tests), + &cli_options, + Some(&watcher_communicator), ) .await?; - file_container.check().await?; + workspace_files_factory.check().await?; if workspace_test_options.no_run { return Ok(()); } - let factories_with_specifiers = - if let Some(changed_paths) = changed_paths { - file_container - .worker_factories_with_dependent_checked_specifiers( - &changed_paths.into_iter().collect(), - ) - .await? - } else { - file_container.worker_factories_with_checked_specifiers() - }; - test_specifiers( - factories_with_specifiers, - &permissions, - permission_desc_parser, + &workspace_files_factory, + changed_paths.map(|p| p.into_iter().collect()).as_ref(), TestSpecifiersOptions { cwd: Url::from_directory_path(cli_options.initial_cwd()).map_err( |_| { From 5807b53dae8b1bfb4d91d04f0ac5579961463af0 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 16 Dec 2024 11:57:08 +0000 Subject: [PATCH 26/33] reduce dependency on outer CliOptions --- cli/args/mod.rs | 66 -------------------------- cli/factory.rs | 36 ++++++++++---- cli/tools/bench/mod.rs | 31 ++++--------- cli/tools/test/mod.rs | 103 +++++++++++++++++------------------------ 4 files changed, 78 insertions(+), 158 deletions(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 5ad86cb19c..8eb739857e 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -78,7 +78,6 @@ use std::io::Cursor; use std::io::Read; use std::io::Seek; use std::net::SocketAddr; -use std::num::NonZeroUsize; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -264,22 +263,6 @@ impl CacheSetting { } } -pub struct WorkspaceBenchOptions { - pub filter: Option, - pub json: bool, - pub no_run: bool, -} - -impl WorkspaceBenchOptions { - pub fn resolve(bench_flags: &BenchFlags) -> Self { - Self { - filter: bench_flags.filter.clone(), - json: bench_flags.json, - no_run: bench_flags.no_run, - } - } -} - #[derive(Clone, Debug, Eq, PartialEq)] pub struct BenchOptions { pub files: FilePatterns, @@ -375,41 +358,6 @@ fn resolve_fmt_options( options } -#[derive(Clone, Debug)] -pub struct WorkspaceTestOptions { - pub doc: bool, - pub no_run: bool, - pub fail_fast: Option, - pub permit_no_files: bool, - pub filter: Option, - pub shuffle: Option, - pub concurrent_jobs: NonZeroUsize, - pub trace_leaks: bool, - pub reporter: TestReporterConfig, - pub junit_path: Option, - pub hide_stacktraces: bool, -} - -impl WorkspaceTestOptions { - pub fn resolve(test_flags: &TestFlags) -> Self { - Self { - permit_no_files: test_flags.permit_no_files, - concurrent_jobs: test_flags - .concurrent_jobs - .unwrap_or_else(|| NonZeroUsize::new(1).unwrap()), - doc: test_flags.doc, - fail_fast: test_flags.fail_fast, - filter: test_flags.filter.clone(), - no_run: test_flags.no_run, - shuffle: test_flags.shuffle, - trace_leaks: test_flags.trace_leaks, - reporter: test_flags.reporter, - junit_path: test_flags.junit_path.clone(), - hide_stacktraces: test_flags.hide_stacktraces, - } - } -} - #[derive(Debug, Clone)] pub struct TestOptions { pub files: FilePatterns, @@ -1501,13 +1449,6 @@ impl CliOptions { }) } - pub fn resolve_workspace_test_options( - &self, - test_flags: &TestFlags, - ) -> WorkspaceTestOptions { - WorkspaceTestOptions::resolve(test_flags) - } - pub fn resolve_test_options_for_members( &self, test_flags: &TestFlags, @@ -1533,13 +1474,6 @@ impl CliOptions { Ok(member_options) } - pub fn resolve_workspace_bench_options( - &self, - bench_flags: &BenchFlags, - ) -> WorkspaceBenchOptions { - WorkspaceBenchOptions::resolve(bench_flags) - } - pub fn resolve_bench_options_for_members( &self, bench_flags: &BenchFlags, diff --git a/cli/factory.rs b/cli/factory.rs index d31d39ac1a..432182c71e 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1163,6 +1163,7 @@ impl WorkspaceDirFilesFactory { pub struct WorkspaceFilesFactory { dirs: Vec, + initial_cwd: PathBuf, } impl WorkspaceFilesFactory { @@ -1186,6 +1187,10 @@ impl WorkspaceFilesFactory { cli_options: &CliOptions, watcher_communicator: Option<&Arc>, ) -> Result { + let initial_cwd = cli_options.initial_cwd().to_path_buf(); + if let Some(watcher_communicator) = watcher_communicator { + let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); + } workspace_dirs_with_files.sort_by_cached_key(|(d, _)| d.dir_url().clone()); let all_scopes = Arc::new( workspace_dirs_with_files @@ -1197,6 +1202,15 @@ impl WorkspaceFilesFactory { let dir_count = workspace_dirs_with_files.len(); let mut dirs = Vec::with_capacity(dir_count); for (workspace_dir, files) in workspace_dirs_with_files { + if let Some(watcher_communicator) = watcher_communicator { + let _ = watcher_communicator.watch_paths( + files + .include + .iter() + .flat_map(|set| set.base_paths()) + .collect(), + ); + } let scope_options = (dir_count > 1).then(|| ScopeOptions { scope: workspace_dir .has_deno_or_pkg_json() @@ -1237,7 +1251,19 @@ impl WorkspaceFilesFactory { permissions_options: Default::default(), }); } - Ok(Self { dirs }) + Ok(Self { dirs, initial_cwd }) + } + + pub fn dirs(&self) -> &Vec { + &self.dirs + } + + pub fn initial_cwd(&self) -> &PathBuf { + &self.initial_cwd + } + + pub fn found_specifiers(&self) -> bool { + self.dirs.iter().any(|e| !e.specifiers.is_empty()) } pub async fn check(&self) -> Result<(), AnyError> { @@ -1282,12 +1308,4 @@ impl WorkspaceFilesFactory { } Ok(()) } - - pub fn dirs(&self) -> &Vec { - &self.dirs - } - - pub fn found_specifiers(&self) -> bool { - self.dirs.iter().any(|e| !e.specifiers.is_empty()) - } } diff --git a/cli/tools/bench/mod.rs b/cli/tools/bench/mod.rs index a9dba2b1f2..314f9675ac 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -432,11 +432,8 @@ pub async fn run_benchmarks( flags: Arc, bench_flags: BenchFlags, ) -> Result<(), AnyError> { + let log_level = flags.log_level; let cli_options = CliOptions::from_flags(flags)?; - let workspace_bench_options = - cli_options.resolve_workspace_bench_options(&bench_flags); - let log_level = cli_options.log_level(); - let workspace_dirs_with_files = cli_options .resolve_bench_options_for_members(&bench_flags)? .into_iter() @@ -472,7 +469,7 @@ pub async fn run_benchmarks( workspace_files_factory.check().await?; - if workspace_bench_options.no_run { + if bench_flags.no_run { return Ok(()); } @@ -480,8 +477,8 @@ pub async fn run_benchmarks( &workspace_files_factory, None, BenchSpecifierOptions { - filter: TestFilter::from_flag(&workspace_bench_options.filter), - json: workspace_bench_options.json, + filter: TestFilter::from_flag(&bench_flags.filter), + json: bench_flags.json, log_level, }, ) @@ -508,25 +505,13 @@ pub async fn run_benchmarks_with_watch( let bench_flags = bench_flags.clone(); watcher_communicator.show_path_changed(changed_paths.clone()); Ok(async move { + let log_level: Option = flags.log_level; let cli_options = CliOptions::from_flags(flags)?; - let workspace_bench_options = - cli_options.resolve_workspace_bench_options(&bench_flags); - let log_level = cli_options.log_level(); - - let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); let workspace_dirs_with_files = cli_options .resolve_bench_options_for_members(&bench_flags)? .into_iter() .map(|(d, o)| (d, o.files)) .collect::>(); - let watch_paths = workspace_dirs_with_files - .iter() - .filter_map(|(_, files)| { - files.include.as_ref().map(|set| set.base_paths()) - }) - .flatten() - .collect::>(); - let _ = watcher_communicator.watch_paths(watch_paths); let workspace_files_factory = WorkspaceFilesFactory::from_workspace_dirs_with_files( workspace_dirs_with_files, @@ -554,7 +539,7 @@ pub async fn run_benchmarks_with_watch( workspace_files_factory.check().await?; - if workspace_bench_options.no_run { + if bench_flags.no_run { return Ok(()); } @@ -562,8 +547,8 @@ pub async fn run_benchmarks_with_watch( &workspace_files_factory, changed_paths.map(|p| p.into_iter().collect()).as_ref(), BenchSpecifierOptions { - filter: TestFilter::from_flag(&workspace_bench_options.filter), - json: workspace_bench_options.json, + filter: TestFilter::from_flag(&bench_flags.filter), + json: bench_flags.json, log_level, }, ) diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index a5438ad8e7..a4c1592bf2 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -1497,14 +1497,8 @@ pub async fn run_tests( flags: Arc, test_flags: TestFlags, ) -> Result<(), AnyError> { + let log_level = flags.log_level; let cli_options = CliOptions::from_flags(flags)?; - let workspace_test_options = - cli_options.resolve_workspace_test_options(&test_flags); - // Various test files should not share the same permissions in terms of - // `PermissionsContainer` - otherwise granting/revoking permissions in one - // file would have impact on other files, which is undesirable. - let log_level = cli_options.log_level(); - let workspace_dirs_with_files = cli_options .resolve_test_options_for_members(&test_flags)? .into_iter() @@ -1517,47 +1511,47 @@ pub async fn run_tests( collect_specifiers_for_tests(patterns, cli_options, file_fetcher, doc) .boxed_local() }, - workspace_test_options.doc, + test_flags.doc, Some(extract_doc_tests), &cli_options, None, ) .await?; - if !workspace_test_options.permit_no_files - && !workspace_files_factory.found_specifiers() + if !test_flags.permit_no_files && !workspace_files_factory.found_specifiers() { return Err(generic_error("No test modules found")); } workspace_files_factory.check().await?; - if workspace_test_options.no_run { + if test_flags.no_run { return Ok(()); } + let initial_cwd = workspace_files_factory.initial_cwd(); test_specifiers( &workspace_files_factory, None, TestSpecifiersOptions { - cwd: Url::from_directory_path(cli_options.initial_cwd()).map_err( - |_| { - generic_error(format!( - "Unable to construct URL from the path of cwd: {}", - cli_options.initial_cwd().to_string_lossy(), - )) - }, - )?, - concurrent_jobs: workspace_test_options.concurrent_jobs, - fail_fast: workspace_test_options.fail_fast, + cwd: Url::from_directory_path(initial_cwd).map_err(|_| { + generic_error(format!( + "Unable to construct URL from the path of cwd: {}", + initial_cwd.to_string_lossy(), + )) + })?, + concurrent_jobs: test_flags + .concurrent_jobs + .unwrap_or_else(|| NonZeroUsize::new(1).unwrap()), + fail_fast: test_flags.fail_fast, log_level, - filter: workspace_test_options.filter.is_some(), - reporter: workspace_test_options.reporter, - junit_path: workspace_test_options.junit_path, - hide_stacktraces: workspace_test_options.hide_stacktraces, + filter: test_flags.filter.is_some(), + reporter: test_flags.reporter, + junit_path: test_flags.junit_path, + hide_stacktraces: test_flags.hide_stacktraces, specifier: TestSpecifierOptions { - filter: TestFilter::from_flag(&workspace_test_options.filter), - shuffle: workspace_test_options.shuffle, - trace_leaks: workspace_test_options.trace_leaks, + filter: TestFilter::from_flag(&test_flags.filter), + shuffle: test_flags.shuffle, + trace_leaks: test_flags.trace_leaks, }, }, ) @@ -1598,25 +1592,13 @@ pub async fn run_tests_with_watch( let test_flags = test_flags.clone(); watcher_communicator.show_path_changed(changed_paths.clone()); Ok(async move { + let log_level = flags.log_level; let cli_options = CliOptions::from_flags(flags)?; - let workspace_test_options = - cli_options.resolve_workspace_test_options(&test_flags); - let log_level = cli_options.log_level(); - - let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); let workspace_dirs_with_files = cli_options .resolve_test_options_for_members(&test_flags)? .into_iter() .map(|(d, o)| (d, o.files)) .collect::>(); - let watch_paths = workspace_dirs_with_files - .iter() - .filter_map(|(_, files)| { - files.include.as_ref().map(|set| set.base_paths()) - }) - .flatten() - .collect::>(); - let _ = watcher_communicator.watch_paths(watch_paths); let workspace_files_factory = WorkspaceFilesFactory::from_workspace_dirs_with_files( workspace_dirs_with_files, @@ -1629,7 +1611,7 @@ pub async fn run_tests_with_watch( ) .boxed_local() }, - workspace_test_options.doc, + test_flags.doc, Some(extract_doc_tests), &cli_options, Some(&watcher_communicator), @@ -1638,33 +1620,34 @@ pub async fn run_tests_with_watch( workspace_files_factory.check().await?; - if workspace_test_options.no_run { + if test_flags.no_run { return Ok(()); } + let initial_cwd = workspace_files_factory.initial_cwd(); test_specifiers( &workspace_files_factory, changed_paths.map(|p| p.into_iter().collect()).as_ref(), TestSpecifiersOptions { - cwd: Url::from_directory_path(cli_options.initial_cwd()).map_err( - |_| { - generic_error(format!( - "Unable to construct URL from the path of cwd: {}", - cli_options.initial_cwd().to_string_lossy(), - )) - }, - )?, - concurrent_jobs: workspace_test_options.concurrent_jobs, - fail_fast: workspace_test_options.fail_fast, + cwd: Url::from_directory_path(initial_cwd).map_err(|_| { + generic_error(format!( + "Unable to construct URL from the path of cwd: {}", + initial_cwd.to_string_lossy(), + )) + })?, + concurrent_jobs: test_flags + .concurrent_jobs + .unwrap_or_else(|| NonZeroUsize::new(1).unwrap()), + fail_fast: test_flags.fail_fast, log_level, - filter: workspace_test_options.filter.is_some(), - reporter: workspace_test_options.reporter, - junit_path: workspace_test_options.junit_path, - hide_stacktraces: workspace_test_options.hide_stacktraces, + filter: test_flags.filter.is_some(), + reporter: test_flags.reporter, + junit_path: test_flags.junit_path, + hide_stacktraces: test_flags.hide_stacktraces, specifier: TestSpecifierOptions { - filter: TestFilter::from_flag(&workspace_test_options.filter), - shuffle: workspace_test_options.shuffle, - trace_leaks: workspace_test_options.trace_leaks, + filter: TestFilter::from_flag(&test_flags.filter), + shuffle: test_flags.shuffle, + trace_leaks: test_flags.trace_leaks, }, }, ) From e91b0efd174af677d52f49eb3170e5bbb27ee4fb Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Wed, 1 Jan 2025 10:01:21 +0000 Subject: [PATCH 27/33] fmt --- cli/tools/bench/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/tools/bench/mod.rs b/cli/tools/bench/mod.rs index 20eee1c729..67f7ee8655 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -1,6 +1,5 @@ // Copyright 2018-2025 the Deno authors. MIT license. -use crate::worker::CliMainWorkerFactory; use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; @@ -48,6 +47,7 @@ use crate::util::file_watcher; use crate::util::fs::collect_specifiers; use crate::util::path::is_script_ext; use crate::util::path::matches_pattern_or_exact_path; +use crate::worker::CliMainWorkerFactory; mod mitata; mod reporters; From 0ae3090fa04a208f530366d0ffa4ff05bd4ebb2e Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Sat, 18 Jan 2025 06:08:17 +0000 Subject: [PATCH 28/33] fix merge --- cli/args/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 4c81752c90..c0176dbf3d 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -596,7 +596,7 @@ pub struct CliOptions { sys: CliSys, overrides: CliOptionOverrides, pub start_dir: Arc, - pub deno_dir_provider: Arc>, + pub deno_dir_provider: Arc, pub scope_options: Option>, } From 766452fca43826ec92d85738dce3062fc6f19944 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 20 Jan 2025 08:15:23 +0000 Subject: [PATCH 29/33] single factory --- cli/args/mod.rs | 60 +-- cli/factory.rs | 289 +++++---------- cli/graph_container.rs | 5 +- cli/lsp/language_server.rs | 1 - cli/tools/bench/mod.rs | 142 ++++--- cli/tools/check.rs | 349 ++++++++++-------- cli/tools/test/mod.rs | 136 +++---- .../module_not_found/missing_local_root.out | 1 - .../module_not_found/missing_remote_root.out | 1 - 9 files changed, 445 insertions(+), 539 deletions(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index c0176dbf3d..c5a0639849 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -8,7 +8,7 @@ mod lockfile; mod package_json; use std::borrow::Cow; -use std::collections::BTreeSet; +use std::collections::BTreeMap; use std::collections::HashMap; use std::env; use std::net::SocketAddr; @@ -557,15 +557,6 @@ struct CliOptionOverrides { import_map_specifier: Option>, } -/// Overrides for the options below that when set will -/// use these values over the values derived from the -/// CLI flags or config file. -#[derive(Debug, Clone)] -pub struct ScopeOptions { - pub scope: Option>, - pub all_scopes: Arc>>, -} - fn load_external_import_map( deno_json: &ConfigFile, ) -> Result, AnyError> { @@ -593,11 +584,10 @@ pub struct CliOptions { npmrc: Arc, maybe_lockfile: Option>, maybe_external_import_map: Option<(PathBuf, serde_json::Value)>, - sys: CliSys, overrides: CliOptionOverrides, pub start_dir: Arc, + pub all_dirs: BTreeMap, Arc>, pub deno_dir_provider: Arc, - pub scope_options: Option>, } impl CliOptions { @@ -611,7 +601,6 @@ impl CliOptions { start_dir: Arc, force_global_cache: bool, maybe_external_import_map: Option<(PathBuf, serde_json::Value)>, - scope_options: Option>, ) -> Result { if let Some(insecure_allowlist) = flags.unsafely_ignore_certificate_errors.as_ref() @@ -643,6 +632,9 @@ impl CliOptions { load_env_variables_from_env_file(flags.env_file.as_ref()); + let all_dirs = [(start_dir.dir_url().clone(), start_dir.clone())] + .into_iter() + .collect(); Ok(Self { flags, initial_cwd, @@ -653,9 +645,8 @@ impl CliOptions { main_module_cell: std::sync::OnceLock::new(), maybe_external_import_map, start_dir, + all_dirs, deno_dir_provider, - sys: sys.clone(), - scope_options, }) } @@ -752,39 +743,18 @@ impl CliOptions { Arc::new(start_dir), false, external_import_map, - None, ) } - pub fn with_new_start_dir_and_scope_options( - &self, - start_dir: Arc, - scope_options: Option, - ) -> Result { - let (npmrc, _) = discover_npmrc_from_workspace(&start_dir.workspace)?; - let external_import_map = - if let Some(deno_json) = start_dir.workspace.root_deno_json() { - load_external_import_map(deno_json)? - } else { - None - }; - let lockfile = CliLockfile::discover( - &self.sys, - &self.flags, - &start_dir.workspace, - external_import_map.as_ref().map(|(_, v)| v), - )?; - Self::new( - &self.sys, - self.flags.clone(), - self.initial_cwd().to_path_buf(), - lockfile.map(Arc::new), - npmrc, - start_dir, - false, - external_import_map, - scope_options.map(Arc::new), - ) + pub fn with_all_dirs( + self, + all_dirs: impl IntoIterator>, + ) -> Self { + let all_dirs = all_dirs + .into_iter() + .map(|d| (d.dir_url().clone(), d)) + .collect(); + Self { all_dirs, ..self } } /// This method is purposefully verbose to disourage its use. Do not use it diff --git a/cli/factory.rs b/cli/factory.rs index 4970929456..8dce24f97b 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1,6 +1,5 @@ // Copyright 2018-2025 the Deno authors. MIT license. -use std::collections::BTreeSet; use std::collections::HashSet; use std::future::Future; use std::path::PathBuf; @@ -13,7 +12,6 @@ use deno_config::glob::FilePatterns; use deno_config::workspace::PackageJsonDepResolution; use deno_config::workspace::WorkspaceDirectory; use deno_config::workspace::WorkspaceResolver; -use deno_core::anyhow::anyhow; use deno_core::error::AnyError; use deno_core::futures::FutureExt; use deno_core::FeatureChecker; @@ -42,7 +40,6 @@ use deno_runtime::deno_fs::RealFs; use deno_runtime::deno_node::RealIsBuiltInNodeModuleChecker; use deno_runtime::deno_permissions::Permissions; use deno_runtime::deno_permissions::PermissionsContainer; -use deno_runtime::deno_permissions::PermissionsOptions; use deno_runtime::deno_tls::rustls::RootCertStore; use deno_runtime::deno_tls::RootCertStoreProvider; use deno_runtime::deno_web::BlobStore; @@ -57,7 +54,6 @@ use crate::args::CliOptions; use crate::args::DenoSubcommand; use crate::args::Flags; use crate::args::NpmInstallDepsProvider; -use crate::args::ScopeOptions; use crate::args::TsConfigType; use crate::cache::Caches; use crate::cache::CodeCache; @@ -79,7 +75,6 @@ use crate::graph_util::ModuleGraphCreator; use crate::http_util::HttpClientProvider; use crate::module_loader::CliModuleLoaderFactory; use crate::module_loader::ModuleLoadPreparer; -use crate::module_loader::PrepareModuleLoadError; use crate::node::CliCjsCodeAnalyzer; use crate::node::CliNodeCodeTranslator; use crate::node::CliNodeResolver; @@ -105,12 +100,10 @@ use crate::resolver::CliSloppyImportsResolver; use crate::resolver::FoundPackageJsonDepFlag; use crate::standalone::binary::DenoCompileBinaryWriter; use crate::sys::CliSys; -use crate::tools::check::CheckError; use crate::tools::check::TypeChecker; use crate::tools::coverage::CoverageCollector; use crate::tools::lint::LintRuleProvider; use crate::tools::run::hmr::HmrRunner; -use crate::tsc::Diagnostics; use crate::tsc::TypeCheckingCjsTracker; use crate::util::file_watcher::WatcherCommunicator; use crate::util::fs::canonicalize_path; @@ -1247,15 +1240,96 @@ pub struct SpecifierInfo { pub check_doc: bool, } -pub struct WorkspaceDirFilesFactory { +pub struct CliFactoryWithWorkspaceFiles { + pub inner: CliFactory, + pub cli_options: Arc, specifiers: Vec<(ModuleSpecifier, SpecifierInfo)>, doc_snippet_specifiers: Vec, - cli_options: Arc, - cli_factory: CliFactory, - permissions_options: Deferred, + initial_cwd: PathBuf, } -impl WorkspaceDirFilesFactory { +impl CliFactoryWithWorkspaceFiles { + #[allow(clippy::type_complexity)] + pub async fn from_workspace_dirs_with_files( + mut workspace_dirs_with_files: Vec<(Arc, FilePatterns)>, + collect_specifiers: fn( + FilePatterns, + Arc, + Arc, + T, + ) -> std::pin::Pin< + Box< + dyn Future< + Output = Result, AnyError>, + >, + >, + >, + args: T, + extract_doc_files: Option Result, AnyError>>, + cli_options: CliOptions, + watcher_communicator: Option<&Arc>, + ) -> Result { + let cli_options = + Arc::new(cli_options.with_all_dirs( + workspace_dirs_with_files.iter().map(|(d, _)| d.clone()), + )); + let mut factory = CliFactory::from_cli_options(cli_options.clone()); + factory.watcher_communicator = watcher_communicator.cloned(); + let initial_cwd = cli_options.initial_cwd().to_path_buf(); + if let Some(watcher_communicator) = watcher_communicator { + let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); + } + workspace_dirs_with_files.sort_by_cached_key(|(d, _)| d.dir_url().clone()); + let mut specifiers = Vec::new(); + let mut doc_snippet_specifiers = Vec::new(); + for (_, files) in workspace_dirs_with_files { + if let Some(watcher_communicator) = watcher_communicator { + let _ = watcher_communicator.watch_paths( + files + .include + .iter() + .flat_map(|set| set.base_paths()) + .collect(), + ); + } + let file_fetcher = factory.file_fetcher()?; + let dir_specifiers = collect_specifiers( + files, + cli_options.clone(), + file_fetcher.clone(), + args.clone(), + ) + .await?; + if let Some(extract_doc_files) = extract_doc_files { + let root_permissions = factory.root_permissions_container()?; + for (s, _) in dir_specifiers.iter().filter(|(_, i)| i.check_doc) { + let file = file_fetcher.fetch(s, root_permissions).await?; + let snippet_files = extract_doc_files(file)?; + for snippet_file in snippet_files { + doc_snippet_specifiers.push(snippet_file.url.clone()); + file_fetcher.insert_memory_files(snippet_file); + } + } + } + specifiers.extend(dir_specifiers); + } + Ok(Self { + inner: factory, + cli_options, + specifiers, + doc_snippet_specifiers, + initial_cwd, + }) + } + + pub fn initial_cwd(&self) -> &PathBuf { + &self.initial_cwd + } + + pub fn found_specifiers(&self) -> bool { + !self.specifiers.is_empty() + } + pub fn checked_specifiers(&self) -> impl Iterator { self .specifiers @@ -1268,23 +1342,20 @@ impl WorkspaceDirFilesFactory { &self, canonicalized_dep_paths: &HashSet, ) -> Result, AnyError> { - let graph_kind = self - .cli_factory - .cli_options()? - .type_check_mode() - .as_graph_kind(); - let module_graph_creator = self.cli_factory.module_graph_creator().await?; - let specifiers = self.checked_specifiers().cloned().collect::>(); + let graph_kind = + self.inner.cli_options()?.type_check_mode().as_graph_kind(); + let module_graph_creator = self.inner.module_graph_creator().await?; + let specifiers = self.checked_specifiers().collect::>(); let graph = module_graph_creator .create_graph( graph_kind, - specifiers.clone(), + specifiers.iter().map(|&s| s.clone()).collect(), crate::graph_util::NpmCachingStrategy::Eager, ) .await?; module_graph_creator.graph_valid(&graph)?; - let dependent_specifiers = self - .checked_specifiers() + let dependent_specifiers = specifiers + .into_iter() .filter(|s| { let mut dependency_specifiers = graph.walk( std::iter::once(*s), @@ -1313,172 +1384,16 @@ impl WorkspaceDirFilesFactory { Ok(dependent_specifiers) } - pub fn permissions_options(&self) -> &PermissionsOptions { - self - .permissions_options - .get_or_init(|| self.cli_options.permissions_options()) - } - - pub fn permission_desc_parser( - &self, - ) -> Result<&Arc>, AnyError> { - self.cli_factory.permission_desc_parser() - } - - pub async fn create_cli_main_worker_factory( - &self, - ) -> Result { - self.cli_factory.create_cli_main_worker_factory().await - } -} - -pub struct WorkspaceFilesFactory { - dirs: Vec, - initial_cwd: PathBuf, -} - -impl WorkspaceFilesFactory { - #[allow(clippy::type_complexity)] - pub async fn from_workspace_dirs_with_files( - mut workspace_dirs_with_files: Vec<(Arc, FilePatterns)>, - collect_specifiers: fn( - FilePatterns, - Arc, - Arc, - T, - ) -> std::pin::Pin< - Box< - dyn Future< - Output = Result, AnyError>, - >, - >, - >, - args: T, - extract_doc_files: Option Result, AnyError>>, - cli_options: &CliOptions, - watcher_communicator: Option<&Arc>, - ) -> Result { - let initial_cwd = cli_options.initial_cwd().to_path_buf(); - if let Some(watcher_communicator) = watcher_communicator { - let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); - } - workspace_dirs_with_files.sort_by_cached_key(|(d, _)| d.dir_url().clone()); - let all_scopes = Arc::new( - workspace_dirs_with_files - .iter() - .filter(|(d, _)| d.has_deno_or_pkg_json()) - .map(|(d, _)| d.dir_url().clone()) - .collect::>(), - ); - let dir_count = workspace_dirs_with_files.len(); - let mut dirs = Vec::with_capacity(dir_count); - for (workspace_dir, files) in workspace_dirs_with_files { - if let Some(watcher_communicator) = watcher_communicator { - let _ = watcher_communicator.watch_paths( - files - .include - .iter() - .flat_map(|set| set.base_paths()) - .collect(), - ); - } - let scope_options = (dir_count > 1).then(|| ScopeOptions { - scope: workspace_dir - .has_deno_or_pkg_json() - .then(|| workspace_dir.dir_url().clone()), - all_scopes: all_scopes.clone(), - }); - let cli_options = Arc::new( - cli_options - .with_new_start_dir_and_scope_options(workspace_dir, scope_options)?, - ); - let mut factory = CliFactory::from_cli_options(cli_options.clone()); - factory.watcher_communicator = watcher_communicator.cloned(); - let file_fetcher = factory.file_fetcher()?; - let specifiers = collect_specifiers( - files, - cli_options.clone(), - file_fetcher.clone(), - args.clone(), - ) - .await?; - let mut doc_snippet_specifiers = vec![]; - if let Some(extract_doc_files) = extract_doc_files { - let root_permissions = factory.root_permissions_container()?; - for (s, _) in specifiers.iter().filter(|(_, i)| i.check_doc) { - let file = file_fetcher.fetch(s, root_permissions).await?; - let snippet_files = extract_doc_files(file)?; - for snippet_file in snippet_files { - doc_snippet_specifiers.push(snippet_file.url.clone()); - file_fetcher.insert_memory_files(snippet_file); - } - } - } - dirs.push(WorkspaceDirFilesFactory { - specifiers, - doc_snippet_specifiers, - cli_options, - cli_factory: factory, - permissions_options: Default::default(), - }); - } - Ok(Self { dirs, initial_cwd }) - } - - pub fn dirs(&self) -> &Vec { - &self.dirs - } - - pub fn initial_cwd(&self) -> &PathBuf { - &self.initial_cwd - } - - pub fn found_specifiers(&self) -> bool { - self.dirs.iter().any(|e| !e.specifiers.is_empty()) - } - pub async fn check(&self) -> Result<(), AnyError> { - let mut diagnostics = vec![]; - let mut all_errors = vec![]; - for entry in &self.dirs { - let main_graph_container = entry - .cli_factory - .main_module_graph_container() - .await? - .clone(); - let specifiers_for_typecheck = - entry.checked_specifiers().cloned().collect::>(); - if specifiers_for_typecheck.is_empty() { - continue; - } - let ext_flag = entry.cli_factory.cli_options()?.ext_flag().as_ref(); - if let Err(err) = main_graph_container - .check_specifiers(&specifiers_for_typecheck, ext_flag) - .await - { - match err { - PrepareModuleLoadError::Check(CheckError::Diagnostics( - Diagnostics(d), - )) => diagnostics.extend(d), - err => all_errors.push(err), - } - } + let main_graph_container = + self.inner.main_module_graph_container().await?.clone(); + let specifiers = self.checked_specifiers().cloned().collect::>(); + if specifiers.is_empty() { + return Ok(()); } - if !diagnostics.is_empty() { - all_errors.push(PrepareModuleLoadError::Check(CheckError::Diagnostics( - Diagnostics(diagnostics), - ))); - } - if !all_errors.is_empty() { - return Err(anyhow!( - "{}", - all_errors - .into_iter() - .map(|e| e.to_string()) - .collect::>() - .join("\n\n"), - )); - } - Ok(()) + let ext_flag = self.cli_options.ext_flag().as_ref(); + main_graph_container + .check_specifiers(&specifiers, ext_flag) + .await } } diff --git a/cli/graph_container.rs b/cli/graph_container.rs index b23fe61170..1fe30b47ab 100644 --- a/cli/graph_container.rs +++ b/cli/graph_container.rs @@ -13,7 +13,6 @@ use deno_runtime::deno_permissions::PermissionsContainer; use crate::args::CliOptions; use crate::module_loader::ModuleLoadPreparer; -use crate::module_loader::PrepareModuleLoadError; use crate::util::fs::collect_specifiers; use crate::util::path::is_script_ext; @@ -70,7 +69,7 @@ impl MainModuleGraphContainer { &self, specifiers: &[ModuleSpecifier], ext_overwrite: Option<&String>, - ) -> Result<(), PrepareModuleLoadError> { + ) -> Result<(), AnyError> { let mut graph_permit = self.acquire_update_permit().await; let graph = graph_permit.graph_mut(); self @@ -100,7 +99,7 @@ impl MainModuleGraphContainer { log::warn!("{} No matching files found.", colors::yellow("Warning")); } - Ok(self.check_specifiers(&specifiers, None).await?) + self.check_specifiers(&specifiers, None).await } pub fn collect_specifiers( diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 3662451f01..d14f0e9246 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -3666,7 +3666,6 @@ impl Inner { workspace, force_global_cache, None, - None, )?; let open_docs = self.documents.documents(DocumentsFilter::OpenDiagnosable); diff --git a/cli/tools/bench/mod.rs b/cli/tools/bench/mod.rs index a85ca90709..b245fa360f 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -39,8 +39,8 @@ use crate::args::CliOptions; use crate::args::Flags; use crate::colors; use crate::display::write_json_to_stdout; +use crate::factory::CliFactoryWithWorkspaceFiles; use crate::factory::SpecifierInfo; -use crate::factory::WorkspaceFilesFactory; use crate::ops; use crate::sys::CliSys; use crate::tools::test::format_test_error; @@ -284,57 +284,46 @@ async fn bench_specifier_inner( /// Test a collection of specifiers with test modes concurrently. async fn bench_specifiers( - workspace_files_factory: &WorkspaceFilesFactory, + factory: &CliFactoryWithWorkspaceFiles, changed_paths: Option<&HashSet>, options: BenchSpecifierOptions, ) -> Result<(), AnyError> { - let mut specifiers_with_services = vec![]; - for factory in workspace_files_factory.dirs() { - let worker_factory = - Arc::new(factory.create_cli_main_worker_factory().await?); - let permission_desc_parser = factory.permission_desc_parser()?; - let permissions = Arc::new(Permissions::from_options( - permission_desc_parser.as_ref(), - factory.permissions_options(), - )?); - let specifiers = if let Some(changed_paths) = changed_paths { - factory.dependent_checked_specifiers(changed_paths).await? - } else { - factory.checked_specifiers().collect() - }; - specifiers_with_services.extend(specifiers.into_iter().map(|s| { - ( - s.clone(), - worker_factory.clone(), - permission_desc_parser.clone(), - permissions.clone(), - ) - })); - } + let specifiers = if let Some(changed_paths) = changed_paths { + factory.dependent_checked_specifiers(changed_paths).await? + } else { + factory.checked_specifiers().collect() + }; + let worker_factory = + Arc::new(factory.inner.create_cli_main_worker_factory().await?); + let permission_desc_parser = factory.inner.permission_desc_parser()?; + let permissions = Permissions::from_options( + permission_desc_parser.as_ref(), + &factory.cli_options.permissions_options(), + )?; + let (sender, mut receiver) = unbounded_channel::(); let log_level = options.log_level; let option_for_handles = options.clone(); - let join_handles = specifiers_with_services.into_iter().map( - move |(specifier, worker_factory, permissions_desc_parser, permissions)| { - let permissions_container = PermissionsContainer::new( - permissions_desc_parser.clone(), - permissions.as_ref().clone(), + let join_handles = specifiers.into_iter().cloned().map(move |specifier| { + let worker_factory = worker_factory.clone(); + let permissions_container = PermissionsContainer::new( + permission_desc_parser.clone(), + permissions.clone(), + ); + let sender = sender.clone(); + let options = option_for_handles.clone(); + spawn_blocking(move || { + let future = bench_specifier( + worker_factory, + permissions_container, + specifier, + sender, + options.filter, ); - let sender = sender.clone(); - let options = option_for_handles.clone(); - spawn_blocking(move || { - let future = bench_specifier( - worker_factory, - permissions_container, - specifier, - sender, - options.filter, - ); - create_and_run_current_thread(future) - }) - }, - ); + create_and_run_current_thread(future) + }) + }); let join_stream = stream::iter(join_handles) .buffer_unordered(1) @@ -457,43 +446,42 @@ pub async fn run_benchmarks( .resolve_bench_options_for_members(&bench_flags)? .into_iter() .map(|(d, o)| (d, o.files)) - .collect(); - let workspace_files_factory = - WorkspaceFilesFactory::from_workspace_dirs_with_files( - workspace_dirs_with_files, - |patterns, cli_options, _, _| { - async move { - let info = SpecifierInfo { - check: true, - check_doc: false, - }; - collect_specifiers( - patterns, - cli_options.vendor_dir_path().map(ToOwned::to_owned), - is_supported_bench_path, - ) - .map(|s| s.into_iter().map(|s| (s, info)).collect()) - } - .boxed_local() - }, - (), - None, - &cli_options, - None, - ) - .await?; - if !workspace_files_factory.found_specifiers() { + .collect::>(); + let factory = CliFactoryWithWorkspaceFiles::from_workspace_dirs_with_files( + workspace_dirs_with_files, + |patterns, cli_options, _, _| { + async move { + let info = SpecifierInfo { + check: true, + check_doc: false, + }; + collect_specifiers( + patterns, + cli_options.vendor_dir_path().map(ToOwned::to_owned), + is_supported_bench_path, + ) + .map(|s| s.into_iter().map(|s| (s, info)).collect()) + } + .boxed_local() + }, + (), + None, + cli_options, + None, + ) + .await?; + if !factory.found_specifiers() { return Err(anyhow!("No test modules found")); } - workspace_files_factory.check().await?; + factory.check().await?; if bench_flags.no_run { return Ok(()); } bench_specifiers( - &workspace_files_factory, + &factory, None, BenchSpecifierOptions { filter: TestFilter::from_flag(&bench_flags.filter), @@ -531,8 +519,8 @@ pub async fn run_benchmarks_with_watch( .into_iter() .map(|(d, o)| (d, o.files)) .collect::>(); - let workspace_files_factory = - WorkspaceFilesFactory::from_workspace_dirs_with_files( + let factory = + CliFactoryWithWorkspaceFiles::from_workspace_dirs_with_files( workspace_dirs_with_files, |patterns, cli_options, _, _| { async move { @@ -551,19 +539,19 @@ pub async fn run_benchmarks_with_watch( }, (), None, - &cli_options, + cli_options, Some(&watcher_communicator), ) .await?; - workspace_files_factory.check().await?; + factory.check().await?; if bench_flags.no_run { return Ok(()); } bench_specifiers( - &workspace_files_factory, + &factory, changed_paths.map(|p| p.into_iter().collect()).as_ref(), BenchSpecifierOptions { filter: TestFilter::from_flag(&bench_flags.filter), diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 596cd886b6..aa29a9448d 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -1,5 +1,6 @@ // Copyright 2018-2025 the Deno authors. MIT license. +use std::collections::BTreeMap; use std::collections::HashSet; use std::collections::VecDeque; use std::sync::Arc; @@ -7,6 +8,7 @@ use std::sync::Arc; use deno_ast::MediaType; use deno_ast::ModuleSpecifier; use deno_config::deno_json; +use deno_config::workspace::WorkspaceDirectory; use deno_core::error::AnyError; use deno_core::futures::FutureExt; use deno_error::JsErrorBox; @@ -15,6 +17,7 @@ use deno_graph::ModuleError; use deno_graph::ModuleGraph; use deno_graph::ModuleLoadError; use deno_lib::util::hash::FastInsecureHasher; +use deno_path_util::url_from_directory_path; use deno_semver::npm::NpmPackageNvReference; use deno_terminal::colors; use once_cell::sync::Lazy; @@ -32,8 +35,8 @@ use crate::args::TypeCheckMode; use crate::cache::CacheDBHash; use crate::cache::Caches; use crate::cache::TypeCheckCache; +use crate::factory::CliFactoryWithWorkspaceFiles; use crate::factory::SpecifierInfo; -use crate::factory::WorkspaceFilesFactory; use crate::graph_util::maybe_additional_sloppy_imports_message; use crate::graph_util::BuildFastCheckGraphOptions; use crate::graph_util::ModuleGraphBuilder; @@ -59,34 +62,33 @@ pub async fn check( ignore: Default::default(), include: check_flags.files, })?; - let workspace_files_factory = - WorkspaceFilesFactory::from_workspace_dirs_with_files( - workspace_dirs_with_files, - |patterns, cli_options, _, (doc, doc_only)| { - async move { - let info = SpecifierInfo { - check: !doc_only, - check_doc: doc || doc_only, - }; - collect_specifiers( - patterns, - cli_options.vendor_dir_path().map(ToOwned::to_owned), - |e| is_script_ext(e.path), - ) - .map(|s| s.into_iter().map(|s| (s, info)).collect()) - } - .boxed_local() - }, - (check_flags.doc, check_flags.doc_only), - Some(extract_snippet_files), - &cli_options, - None, - ) - .await?; - if !workspace_files_factory.found_specifiers() { + let factory = CliFactoryWithWorkspaceFiles::from_workspace_dirs_with_files( + workspace_dirs_with_files, + |patterns, cli_options, _, (doc, doc_only)| { + async move { + let info = SpecifierInfo { + check: !doc_only, + check_doc: doc || doc_only, + }; + collect_specifiers( + patterns, + cli_options.vendor_dir_path().map(ToOwned::to_owned), + |e| is_script_ext(e.path), + ) + .map(|s| s.into_iter().map(|s| (s, info)).collect()) + } + .boxed_local() + }, + (check_flags.doc, check_flags.doc_only), + Some(extract_snippet_files), + cli_options, + None, + ) + .await?; + if !factory.found_specifiers() { log::warn!("{} No matching files found.", colors::yellow("Warning")); } - workspace_files_factory.check().await + factory.check().await } /// Options for performing a check of a module graph. Note that the decision to @@ -229,17 +231,6 @@ impl TypeChecker { } log::debug!("Type checking."); - let ts_config_result = self - .cli_options - .resolve_ts_config_for_emit(TsConfigType::Check { lib: options.lib })?; - if options.log_ignored_options { - check_warn_tsconfig(&ts_config_result); - } - - let type_check_mode = options.type_check_mode; - let ts_config = ts_config_result.ts_config; - let cache = TypeCheckCache::new(self.caches.type_checking_cache_db()); - let check_js = ts_config.get_check_js(); // add fast check to the graph before getting the roots if options.build_fast_check_graph { @@ -251,125 +242,159 @@ impl TypeChecker { )?; } - let is_visible_diagnostic = |d: &tsc::Diagnostic| { - if self.is_remote_diagnostic(d) { - return type_check_mode == TypeCheckMode::All - && d.include_when_remote() - && self - .cli_options - .scope_options - .as_ref() - .map(|o| o.scope.is_none()) - .unwrap_or(true); - } - let Some(scope_options) = &self.cli_options.scope_options else { - return true; - }; - let Some(specifier) = d - .file_name - .as_ref() - .and_then(|s| ModuleSpecifier::parse(s).ok()) - else { - return true; - }; - if specifier.scheme() != "file" { - return true; - } - let scope = scope_options - .all_scopes - .iter() - .rfind(|s| specifier.as_str().starts_with(s.as_str())); - scope == scope_options.scope.as_ref() - }; - let TscRoots { - roots: root_names, - missing_diagnostics, - maybe_check_hash, - } = get_tsc_roots( - &self.sys, - &self.npm_resolver, - &self.node_resolver, - &graph, - check_js, - check_state_hash(&self.npm_resolver), - type_check_mode, - &ts_config, - ); + let graph = Arc::new(graph); - let missing_diagnostics = missing_diagnostics.filter(is_visible_diagnostic); + let mut all_dirs = self.cli_options.all_dirs.clone(); + let initial_cwd_url = + url_from_directory_path(self.cli_options.initial_cwd()) + .map_err(JsErrorBox::from_err)?; + let initial_workspace_dir_url = all_dirs + .keys() + .rfind(|s| initial_cwd_url.as_str().starts_with(s.as_str())) + .cloned() + .unwrap_or_else(|| { + all_dirs.insert( + self.cli_options.start_dir.dir_url().clone(), + self.cli_options.start_dir.clone(), + ); + self.cli_options.start_dir.dir_url().clone() + }); + let is_scoped = all_dirs.len() > 1; - if root_names.is_empty() && missing_diagnostics.is_empty() { - return Ok((graph.into(), Default::default())); - } - if !options.reload { - // do not type check if we know this is type checked - if let Some(check_hash) = maybe_check_hash { - if cache.has_check_hash(check_hash) { - log::debug!("Already type checked."); - return Ok((graph.into(), Default::default())); + let mut diagnostics = Diagnostics::default(); + + for (dir_url, workspace_dir) in &all_dirs { + let is_initial_workspace_dir = *dir_url == initial_workspace_dir_url; + let ts_config_result = workspace_dir + .to_ts_config_for_emit(TsConfigType::Check { lib: options.lib })?; + if options.log_ignored_options { + check_warn_tsconfig(&ts_config_result); + } + let type_check_mode = options.type_check_mode; + let ts_config = ts_config_result.ts_config; + let cache = TypeCheckCache::new(self.caches.type_checking_cache_db()); + let check_js = ts_config.get_check_js(); + + let is_visible_diagnostic = |d: &tsc::Diagnostic| { + if self.is_remote_diagnostic(d) { + return type_check_mode == TypeCheckMode::All + && d.include_when_remote() + && !is_scoped; + } + let Some(specifier) = d + .file_name + .as_ref() + .and_then(|s| ModuleSpecifier::parse(s).ok()) + else { + return true; + }; + if specifier.scheme() != "file" { + return true; + } + let scope = all_dirs + .keys() + .rfind(|s| specifier.as_str().starts_with(s.as_str())); + scope + .map(|s| s == dir_url) + .unwrap_or(is_initial_workspace_dir) + }; + let TscRoots { + roots: root_names, + display_roots, + missing_diagnostics, + maybe_check_hash, + } = get_tsc_roots( + &self.sys, + &self.npm_resolver, + &self.node_resolver, + &graph, + check_js, + check_state_hash(&self.npm_resolver), + type_check_mode, + &ts_config, + &all_dirs, + &initial_workspace_dir_url, + is_scoped.then_some(dir_url.as_ref()), + ); + + let missing_diagnostics = + missing_diagnostics.filter(is_visible_diagnostic); + let has_missing_diagnostics = !missing_diagnostics.is_empty(); + diagnostics.extend(missing_diagnostics); + + if root_names.is_empty() { + continue; + } + + if !options.reload { + // do not type check if we know this is type checked + if let Some(check_hash) = maybe_check_hash { + if cache.has_check_hash(check_hash) { + log::debug!("Already type checked."); + continue; + } } } + + for root in &display_roots { + let root_str = root.as_str(); + log::info!( + "{} {}", + colors::green("Check"), + to_percent_decoded_str(root_str) + ); + } + + // while there might be multiple roots, we can't "merge" the build info, so we + // try to retrieve the build info for first root, which is the most common use + // case. + let first_root = root_names[0].0.clone(); + let maybe_tsbuildinfo = if options.reload { + None + } else { + cache.get_tsbuildinfo(&first_root) + }; + // to make tsc build info work, we need to consistently hash modules, so that + // tsc can better determine if an emit is still valid or not, so we provide + // that data here. + let tsconfig_hash_data = FastInsecureHasher::new_deno_versioned() + .write(&ts_config.as_bytes()) + .finish(); + let response = tsc::exec(tsc::Request { + config: ts_config, + debug: self.cli_options.log_level() == Some(log::Level::Debug), + graph: graph.clone(), + hash_data: tsconfig_hash_data, + maybe_npm: Some(tsc::RequestNpmState { + cjs_tracker: self.cjs_tracker.clone(), + node_resolver: self.node_resolver.clone(), + npm_resolver: self.npm_resolver.clone(), + }), + maybe_tsbuildinfo, + root_names, + check_mode: type_check_mode, + })?; + + let response_diagnostics = + response.diagnostics.filter(is_visible_diagnostic); + + if let Some(tsbuildinfo) = response.maybe_tsbuildinfo { + cache.set_tsbuildinfo(&first_root, &tsbuildinfo); + } + + if !has_missing_diagnostics && response_diagnostics.is_empty() { + if let Some(check_hash) = maybe_check_hash { + cache.add_check_hash(check_hash); + } + } + + diagnostics.extend(response_diagnostics); + + log::debug!("{}", response.stats); } - for root in &graph.roots { - let root_str = root.as_str(); - log::info!( - "{} {}", - colors::green("Check"), - to_percent_decoded_str(root_str) - ); - } - - // while there might be multiple roots, we can't "merge" the build info, so we - // try to retrieve the build info for first root, which is the most common use - // case. - let maybe_tsbuildinfo = if options.reload { - None - } else { - cache.get_tsbuildinfo(&graph.roots[0]) - }; - // to make tsc build info work, we need to consistently hash modules, so that - // tsc can better determine if an emit is still valid or not, so we provide - // that data here. - let tsconfig_hash_data = FastInsecureHasher::new_deno_versioned() - .write(&ts_config.as_bytes()) - .finish(); - let graph = Arc::new(graph); - let response = tsc::exec(tsc::Request { - config: ts_config, - debug: self.cli_options.log_level() == Some(log::Level::Debug), - graph: graph.clone(), - hash_data: tsconfig_hash_data, - maybe_npm: Some(tsc::RequestNpmState { - cjs_tracker: self.cjs_tracker.clone(), - node_resolver: self.node_resolver.clone(), - npm_resolver: self.npm_resolver.clone(), - }), - maybe_tsbuildinfo, - root_names, - check_mode: type_check_mode, - })?; - - let response_diagnostics = - response.diagnostics.filter(is_visible_diagnostic); - - let mut diagnostics = missing_diagnostics; - diagnostics.extend(response_diagnostics); - diagnostics.apply_fast_check_source_maps(&graph); - if let Some(tsbuildinfo) = response.maybe_tsbuildinfo { - cache.set_tsbuildinfo(&graph.roots[0], &tsbuildinfo); - } - - if diagnostics.is_empty() { - if let Some(check_hash) = maybe_check_hash { - cache.add_check_hash(check_hash); - } - } - - log::debug!("{}", response.stats); - Ok((graph, diagnostics)) } @@ -390,6 +415,7 @@ impl TypeChecker { struct TscRoots { roots: Vec<(ModuleSpecifier, MediaType)>, + display_roots: Vec, missing_diagnostics: tsc::Diagnostics, maybe_check_hash: Option, } @@ -410,6 +436,9 @@ fn get_tsc_roots( npm_cache_state_hash: Option, type_check_mode: TypeCheckMode, ts_config: &TsConfig, + all_dirs: &BTreeMap, Arc>, + initial_workspace_dir_url: &ModuleSpecifier, + current_workspace_dir_url: Option<&ModuleSpecifier>, ) -> TscRoots { fn maybe_get_check_entry( module: &deno_graph::Module, @@ -495,6 +524,7 @@ fn get_tsc_roots( let mut result = TscRoots { roots: Vec::with_capacity(graph.specifiers_count()), + display_roots: Vec::with_capacity(graph.roots.len()), missing_diagnostics: Default::default(), maybe_check_hash: None, }; @@ -525,6 +555,16 @@ fn get_tsc_roots( // put in the global types first so that they're resolved before anything else for (referrer, import) in graph.imports.iter() { + if let Some(current_workspace_dir_url) = current_workspace_dir_url { + let scope = all_dirs + .keys() + .rfind(|s| referrer.as_str().starts_with(s.as_str())) + .map(|s| s.as_ref()) + .unwrap_or(initial_workspace_dir_url); + if scope != current_workspace_dir_url { + continue; + } + } for specifier in import .dependencies .values() @@ -556,6 +596,17 @@ fn get_tsc_roots( // then the roots for root in &graph.roots { + if let Some(current_workspace_dir_url) = current_workspace_dir_url { + let scope = all_dirs + .keys() + .rfind(|s| root.as_str().starts_with(s.as_str())) + .map(|s| s.as_ref()) + .unwrap_or(initial_workspace_dir_url); + if scope != current_workspace_dir_url { + continue; + } + } + result.display_roots.push(root.clone()); let specifier = graph.resolve(root); if seen.insert(specifier) { pending.push_back((specifier, false)); diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 7b598f09f1..7573efb709 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -73,8 +73,8 @@ use crate::args::TestFlags; use crate::args::TestReporterConfig; use crate::colors; use crate::display; +use crate::factory::CliFactoryWithWorkspaceFiles; use crate::factory::SpecifierInfo; -use crate::factory::WorkspaceFilesFactory; use crate::file_fetcher::CliFileFetcher; use crate::ops; use crate::sys::CliSys; @@ -1180,38 +1180,27 @@ static HAS_TEST_RUN_SIGINT_HANDLER: AtomicBool = AtomicBool::new(false); /// Test a collection of specifiers with test modes concurrently. async fn test_specifiers( - workspace_files_factory: &WorkspaceFilesFactory, + factory: &CliFactoryWithWorkspaceFiles, changed_paths: Option<&HashSet>, options: TestSpecifiersOptions, ) -> Result<(), AnyError> { - let mut specifiers_with_services = vec![]; - for factory in workspace_files_factory.dirs() { - let worker_factory = - Arc::new(factory.create_cli_main_worker_factory().await?); - let permission_desc_parser = factory.permission_desc_parser()?; - let permissions = Arc::new(Permissions::from_options( - permission_desc_parser.as_ref(), - factory.permissions_options(), - )?); - let specifiers = if let Some(changed_paths) = changed_paths { - factory.dependent_checked_specifiers(changed_paths).await? - } else { - factory.checked_specifiers().collect() - }; - specifiers_with_services.extend(specifiers.into_iter().map(|s| { - ( - s.clone(), - worker_factory.clone(), - permission_desc_parser.clone(), - permissions.clone(), - ) - })); - } + let mut specifiers = if let Some(changed_paths) = changed_paths { + factory.dependent_checked_specifiers(changed_paths).await? + } else { + factory.checked_specifiers().collect() + }; if let Some(seed) = options.specifier.shuffle { let mut rng = SmallRng::seed_from_u64(seed); - specifiers_with_services.sort_by_cached_key(|(s, ..)| s.to_string()); - specifiers_with_services.shuffle(&mut rng); + specifiers.sort(); + specifiers.shuffle(&mut rng); } + let worker_factory = + Arc::new(factory.inner.create_cli_main_worker_factory().await?); + let permission_desc_parser = factory.inner.permission_desc_parser()?; + let permissions = Permissions::from_options( + permission_desc_parser.as_ref(), + &factory.cli_options.permissions_options(), + )?; let (test_event_sender_factory, receiver) = create_test_event_channel(); let concurrent_jobs = options.concurrent_jobs; @@ -1225,27 +1214,26 @@ async fn test_specifiers( let reporter = get_test_reporter(&options); let fail_fast_tracker = FailFastTracker::new(options.fail_fast); - let join_handles = specifiers_with_services.into_iter().map( - move |(specifier, worker_factory, permission_desc_parser, permissions)| { - let permissions_container = PermissionsContainer::new( - permission_desc_parser, - permissions.as_ref().clone(), - ); - let worker_sender = test_event_sender_factory.worker(); - let fail_fast_tracker = fail_fast_tracker.clone(); - let specifier_options = options.specifier.clone(); - spawn_blocking(move || { - create_and_run_current_thread(test_specifier( - worker_factory, - permissions_container, - specifier, - worker_sender, - fail_fast_tracker, - specifier_options, - )) - }) - }, - ); + let join_handles = specifiers.into_iter().cloned().map(move |specifier| { + let worker_factory = worker_factory.clone(); + let permissions_container = PermissionsContainer::new( + permission_desc_parser.clone(), + permissions.clone(), + ); + let worker_sender = test_event_sender_factory.worker(); + let fail_fast_tracker = fail_fast_tracker.clone(); + let specifier_options = options.specifier.clone(); + spawn_blocking(move || { + create_and_run_current_thread(test_specifier( + worker_factory, + permissions_container, + specifier, + worker_sender, + fail_fast_tracker, + specifier_options, + )) + }) + }); let join_stream = stream::iter(join_handles) .buffer_unordered(concurrent_jobs.get()) @@ -1516,40 +1504,38 @@ pub async fn run_tests( .resolve_test_options_for_members(&test_flags)? .into_iter() .map(|(d, o)| (d, o.files)) - .collect(); - let workspace_files_factory = - WorkspaceFilesFactory::from_workspace_dirs_with_files( - workspace_dirs_with_files, - |patterns, cli_options, file_fetcher, doc| { - collect_specifiers_for_tests(patterns, cli_options, file_fetcher, doc) - .boxed_local() - }, - test_flags.doc, - Some(extract_doc_tests), - &cli_options, - None, - ) - .await?; - if !test_flags.permit_no_files && !workspace_files_factory.found_specifiers() - { + .collect::>(); + let factory = CliFactoryWithWorkspaceFiles::from_workspace_dirs_with_files( + workspace_dirs_with_files, + |patterns, cli_options, file_fetcher, doc| { + collect_specifiers_for_tests(patterns, cli_options, file_fetcher, doc) + .boxed_local() + }, + test_flags.doc, + Some(extract_doc_tests), + cli_options, + None, + ) + .await?; + if !test_flags.permit_no_files && !factory.found_specifiers() { return Err(anyhow!("No test modules found")); } - workspace_files_factory.check().await?; + factory.check().await?; if test_flags.no_run { return Ok(()); } - let initial_cwd = workspace_files_factory.initial_cwd(); + let initial_cwd = factory.initial_cwd(); test_specifiers( - &workspace_files_factory, + &factory, None, TestSpecifiersOptions { cwd: Url::from_directory_path(initial_cwd).map_err(|_| { anyhow!( "Unable to construct URL from the path of cwd: {}", - cli_options.initial_cwd().to_string_lossy(), + factory.cli_options.initial_cwd().to_string_lossy(), ) })?, concurrent_jobs: test_flags @@ -1612,8 +1598,8 @@ pub async fn run_tests_with_watch( .into_iter() .map(|(d, o)| (d, o.files)) .collect::>(); - let workspace_files_factory = - WorkspaceFilesFactory::from_workspace_dirs_with_files( + let factory = + CliFactoryWithWorkspaceFiles::from_workspace_dirs_with_files( workspace_dirs_with_files, |patterns, cli_options, file_fetcher, doc| { collect_specifiers_for_tests( @@ -1626,26 +1612,26 @@ pub async fn run_tests_with_watch( }, test_flags.doc, Some(extract_doc_tests), - &cli_options, + cli_options, Some(&watcher_communicator), ) .await?; - workspace_files_factory.check().await?; + factory.check().await?; if test_flags.no_run { return Ok(()); } - let initial_cwd = workspace_files_factory.initial_cwd(); + let initial_cwd = factory.initial_cwd(); test_specifiers( - &workspace_files_factory, + &factory, changed_paths.map(|p| p.into_iter().collect()).as_ref(), TestSpecifiersOptions { cwd: Url::from_directory_path(initial_cwd).map_err(|_| { anyhow!( "Unable to construct URL from the path of cwd: {}", - cli_options.initial_cwd().to_string_lossy(), + factory.cli_options.initial_cwd().to_string_lossy(), ) })?, concurrent_jobs: test_flags diff --git a/tests/specs/check/module_not_found/missing_local_root.out b/tests/specs/check/module_not_found/missing_local_root.out index 34b150c9a3..01667f3f78 100644 --- a/tests/specs/check/module_not_found/missing_local_root.out +++ b/tests/specs/check/module_not_found/missing_local_root.out @@ -1,2 +1 @@ -Check file:///[WILDLINE]/non_existent.ts error: TS2307 [ERROR]: Cannot find module 'file:///[WILDLINE]/non_existent.ts'. diff --git a/tests/specs/check/module_not_found/missing_remote_root.out b/tests/specs/check/module_not_found/missing_remote_root.out index e408938e41..99b4cca155 100644 --- a/tests/specs/check/module_not_found/missing_remote_root.out +++ b/tests/specs/check/module_not_found/missing_remote_root.out @@ -1,3 +1,2 @@ Download http://localhost:4545/missing_non_existent.ts -Check http://localhost:4545/missing_non_existent.ts error: TS2307 [ERROR]: Cannot find module 'http://localhost:4545/missing_non_existent.ts'. From 1e073ee1d67479e425317fbf0c8cb06b8e008178 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 20 Jan 2025 09:26:08 +0000 Subject: [PATCH 30/33] don't store specifier info --- cli/factory.rs | 35 +++++++++++++++++++---------------- cli/tools/bench/mod.rs | 12 ++++++------ cli/tools/check.rs | 4 ++-- cli/tools/test/mod.rs | 14 +++++++------- 4 files changed, 34 insertions(+), 31 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index 8dce24f97b..200388d0f9 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1233,17 +1233,17 @@ impl CliFactory { #[derive(Debug, Copy, Clone)] pub struct SpecifierInfo { - /// Type check as an ES module. - pub check: bool, + /// Include as an ES module. + pub include: bool, /// Type check virtual modules from doc snippets. If this is set but `check` /// is not, this may be a markdown file for example. - pub check_doc: bool, + pub include_doc: bool, } pub struct CliFactoryWithWorkspaceFiles { pub inner: CliFactory, pub cli_options: Arc, - specifiers: Vec<(ModuleSpecifier, SpecifierInfo)>, + file_specifiers: Vec, doc_snippet_specifiers: Vec, initial_cwd: PathBuf, } @@ -1280,7 +1280,7 @@ impl CliFactoryWithWorkspaceFiles { let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); } workspace_dirs_with_files.sort_by_cached_key(|(d, _)| d.dir_url().clone()); - let mut specifiers = Vec::new(); + let mut file_specifiers = Vec::new(); let mut doc_snippet_specifiers = Vec::new(); for (_, files) in workspace_dirs_with_files { if let Some(watcher_communicator) = watcher_communicator { @@ -1293,7 +1293,7 @@ impl CliFactoryWithWorkspaceFiles { ); } let file_fetcher = factory.file_fetcher()?; - let dir_specifiers = collect_specifiers( + let specifiers = collect_specifiers( files, cli_options.clone(), file_fetcher.clone(), @@ -1302,7 +1302,7 @@ impl CliFactoryWithWorkspaceFiles { .await?; if let Some(extract_doc_files) = extract_doc_files { let root_permissions = factory.root_permissions_container()?; - for (s, _) in dir_specifiers.iter().filter(|(_, i)| i.check_doc) { + for (s, _) in specifiers.iter().filter(|(_, i)| i.include_doc) { let file = file_fetcher.fetch(s, root_permissions).await?; let snippet_files = extract_doc_files(file)?; for snippet_file in snippet_files { @@ -1311,12 +1311,16 @@ impl CliFactoryWithWorkspaceFiles { } } } - specifiers.extend(dir_specifiers); + file_specifiers.extend( + specifiers + .into_iter() + .filter_map(|(s, i)| i.include.then_some(s)), + ); } Ok(Self { inner: factory, cli_options, - specifiers, + file_specifiers, doc_snippet_specifiers, initial_cwd, }) @@ -1327,25 +1331,24 @@ impl CliFactoryWithWorkspaceFiles { } pub fn found_specifiers(&self) -> bool { - !self.specifiers.is_empty() + !self.file_specifiers.is_empty() || !self.doc_snippet_specifiers.is_empty() } - pub fn checked_specifiers(&self) -> impl Iterator { + pub fn specifiers(&self) -> impl Iterator { self - .specifiers + .file_specifiers .iter() - .filter_map(|(s, i)| i.check.then_some(s)) .chain(self.doc_snippet_specifiers.iter()) } - pub async fn dependent_checked_specifiers( + pub async fn dependent_specifiers( &self, canonicalized_dep_paths: &HashSet, ) -> Result, AnyError> { let graph_kind = self.inner.cli_options()?.type_check_mode().as_graph_kind(); let module_graph_creator = self.inner.module_graph_creator().await?; - let specifiers = self.checked_specifiers().collect::>(); + let specifiers = self.specifiers().collect::>(); let graph = module_graph_creator .create_graph( graph_kind, @@ -1387,7 +1390,7 @@ impl CliFactoryWithWorkspaceFiles { pub async fn check(&self) -> Result<(), AnyError> { let main_graph_container = self.inner.main_module_graph_container().await?.clone(); - let specifiers = self.checked_specifiers().cloned().collect::>(); + let specifiers = self.specifiers().cloned().collect::>(); if specifiers.is_empty() { return Ok(()); } diff --git a/cli/tools/bench/mod.rs b/cli/tools/bench/mod.rs index b245fa360f..e18cb13183 100644 --- a/cli/tools/bench/mod.rs +++ b/cli/tools/bench/mod.rs @@ -289,9 +289,9 @@ async fn bench_specifiers( options: BenchSpecifierOptions, ) -> Result<(), AnyError> { let specifiers = if let Some(changed_paths) = changed_paths { - factory.dependent_checked_specifiers(changed_paths).await? + factory.dependent_specifiers(changed_paths).await? } else { - factory.checked_specifiers().collect() + factory.specifiers().collect() }; let worker_factory = Arc::new(factory.inner.create_cli_main_worker_factory().await?); @@ -452,8 +452,8 @@ pub async fn run_benchmarks( |patterns, cli_options, _, _| { async move { let info = SpecifierInfo { - check: true, - check_doc: false, + include: true, + include_doc: false, }; collect_specifiers( patterns, @@ -525,8 +525,8 @@ pub async fn run_benchmarks_with_watch( |patterns, cli_options, _, _| { async move { let info = SpecifierInfo { - check: true, - check_doc: false, + include: true, + include_doc: false, }; collect_specifiers( patterns, diff --git a/cli/tools/check.rs b/cli/tools/check.rs index aa29a9448d..a5fa1abae3 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -67,8 +67,8 @@ pub async fn check( |patterns, cli_options, _, (doc, doc_only)| { async move { let info = SpecifierInfo { - check: !doc_only, - check_doc: doc || doc_only, + include: !doc_only, + include_doc: doc || doc_only, }; collect_specifiers( patterns, diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 7573efb709..8fe81f405f 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -1185,9 +1185,9 @@ async fn test_specifiers( options: TestSpecifiersOptions, ) -> Result<(), AnyError> { let mut specifiers = if let Some(changed_paths) = changed_paths { - factory.dependent_checked_specifiers(changed_paths).await? + factory.dependent_specifiers(changed_paths).await? } else { - factory.checked_specifiers().collect() + factory.specifiers().collect() }; if let Some(seed) = options.specifier.shuffle { let mut rng = SmallRng::seed_from_u64(seed); @@ -1467,8 +1467,8 @@ pub async fn collect_specifiers_for_tests( .into_iter() .map(|specifier| { let info = SpecifierInfo { - check: module_specifiers.contains(&specifier), - check_doc: true, + include: module_specifiers.contains(&specifier), + include_doc: true, }; (specifier, info) }) @@ -1476,8 +1476,8 @@ pub async fn collect_specifiers_for_tests( })? } else { let info = SpecifierInfo { - check: true, - check_doc: false, + include: true, + include_doc: false, }; module_specifiers .into_iter() @@ -1488,7 +1488,7 @@ pub async fn collect_specifiers_for_tests( let file = file_fetcher.fetch_bypass_permissions(specifier).await?; let (media_type, _) = file.resolve_media_type_and_charset(); if matches!(media_type, MediaType::Unknown | MediaType::Dts) { - info.check = false; + info.include = false; } } Ok(specifiers) From 1dd361492d49c772b537b4312fa50ce63e1980a6 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 20 Jan 2025 09:35:28 +0000 Subject: [PATCH 31/33] remote CliFactoryWithWorkspaceFiles::initial_cwd() --- cli/factory.rs | 7 ------- cli/tools/test/mod.rs | 8 ++++---- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index 200388d0f9..51e2f9233a 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1245,7 +1245,6 @@ pub struct CliFactoryWithWorkspaceFiles { pub cli_options: Arc, file_specifiers: Vec, doc_snippet_specifiers: Vec, - initial_cwd: PathBuf, } impl CliFactoryWithWorkspaceFiles { @@ -1275,7 +1274,6 @@ impl CliFactoryWithWorkspaceFiles { )); let mut factory = CliFactory::from_cli_options(cli_options.clone()); factory.watcher_communicator = watcher_communicator.cloned(); - let initial_cwd = cli_options.initial_cwd().to_path_buf(); if let Some(watcher_communicator) = watcher_communicator { let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); } @@ -1322,14 +1320,9 @@ impl CliFactoryWithWorkspaceFiles { cli_options, file_specifiers, doc_snippet_specifiers, - initial_cwd, }) } - pub fn initial_cwd(&self) -> &PathBuf { - &self.initial_cwd - } - pub fn found_specifiers(&self) -> bool { !self.file_specifiers.is_empty() || !self.doc_snippet_specifiers.is_empty() } diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 8fe81f405f..266a129016 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -1527,7 +1527,7 @@ pub async fn run_tests( return Ok(()); } - let initial_cwd = factory.initial_cwd(); + let initial_cwd = factory.cli_options.initial_cwd(); test_specifiers( &factory, None, @@ -1535,7 +1535,7 @@ pub async fn run_tests( cwd: Url::from_directory_path(initial_cwd).map_err(|_| { anyhow!( "Unable to construct URL from the path of cwd: {}", - factory.cli_options.initial_cwd().to_string_lossy(), + initial_cwd.to_string_lossy(), ) })?, concurrent_jobs: test_flags @@ -1623,7 +1623,7 @@ pub async fn run_tests_with_watch( return Ok(()); } - let initial_cwd = factory.initial_cwd(); + let initial_cwd = factory.cli_options.initial_cwd(); test_specifiers( &factory, changed_paths.map(|p| p.into_iter().collect()).as_ref(), @@ -1631,7 +1631,7 @@ pub async fn run_tests_with_watch( cwd: Url::from_directory_path(initial_cwd).map_err(|_| { anyhow!( "Unable to construct URL from the path of cwd: {}", - factory.cli_options.initial_cwd().to_string_lossy(), + initial_cwd.to_string_lossy(), ) })?, concurrent_jobs: test_flags From 798f733c09b323b54249e0980da425f3b82f2fc5 Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 20 Jan 2025 10:13:42 +0000 Subject: [PATCH 32/33] workspace_files --- cli/factory.rs | 77 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index 51e2f9233a..292548f037 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -1240,11 +1240,16 @@ pub struct SpecifierInfo { pub include_doc: bool, } +struct WorkspaceFile { + specifier: ModuleSpecifier, + doc_snippet_specifiers: Vec, + doc_only: bool, +} + pub struct CliFactoryWithWorkspaceFiles { pub inner: CliFactory, pub cli_options: Arc, - file_specifiers: Vec, - doc_snippet_specifiers: Vec, + workspace_files: Vec, } impl CliFactoryWithWorkspaceFiles { @@ -1278,8 +1283,7 @@ impl CliFactoryWithWorkspaceFiles { let _ = watcher_communicator.watch_paths(cli_options.watch_paths()); } workspace_dirs_with_files.sort_by_cached_key(|(d, _)| d.dir_url().clone()); - let mut file_specifiers = Vec::new(); - let mut doc_snippet_specifiers = Vec::new(); + let mut workspace_files = Vec::new(); for (_, files) in workspace_dirs_with_files { if let Some(watcher_communicator) = watcher_communicator { let _ = watcher_communicator.watch_paths( @@ -1298,40 +1302,44 @@ impl CliFactoryWithWorkspaceFiles { args.clone(), ) .await?; - if let Some(extract_doc_files) = extract_doc_files { - let root_permissions = factory.root_permissions_container()?; - for (s, _) in specifiers.iter().filter(|(_, i)| i.include_doc) { - let file = file_fetcher.fetch(s, root_permissions).await?; - let snippet_files = extract_doc_files(file)?; - for snippet_file in snippet_files { - doc_snippet_specifiers.push(snippet_file.url.clone()); - file_fetcher.insert_memory_files(snippet_file); + workspace_files.reserve_exact(specifiers.len()); + for (specifier, info) in &specifiers { + let mut doc_snippet_specifiers = Vec::new(); + if info.include_doc { + if let Some(extract_doc_files) = extract_doc_files { + let root_permissions = factory.root_permissions_container()?; + let file = file_fetcher.fetch(specifier, root_permissions).await?; + let snippet_files = extract_doc_files(file)?; + for snippet_file in snippet_files { + doc_snippet_specifiers.push(snippet_file.url.clone()); + file_fetcher.insert_memory_files(snippet_file); + } } } + workspace_files.push(WorkspaceFile { + specifier: specifier.clone(), + doc_snippet_specifiers, + doc_only: !info.include, + }) } - file_specifiers.extend( - specifiers - .into_iter() - .filter_map(|(s, i)| i.include.then_some(s)), - ); } Ok(Self { inner: factory, cli_options, - file_specifiers, - doc_snippet_specifiers, + workspace_files, }) } pub fn found_specifiers(&self) -> bool { - !self.file_specifiers.is_empty() || !self.doc_snippet_specifiers.is_empty() + !self.workspace_files.is_empty() } pub fn specifiers(&self) -> impl Iterator { - self - .file_specifiers - .iter() - .chain(self.doc_snippet_specifiers.iter()) + self.workspace_files.iter().flat_map(|f| { + std::iter::once(&f.specifier) + .filter(|_| !f.doc_only) + .chain(f.doc_snippet_specifiers.iter()) + }) } pub async fn dependent_specifiers( @@ -1341,20 +1349,24 @@ impl CliFactoryWithWorkspaceFiles { let graph_kind = self.inner.cli_options()?.type_check_mode().as_graph_kind(); let module_graph_creator = self.inner.module_graph_creator().await?; - let specifiers = self.specifiers().collect::>(); let graph = module_graph_creator .create_graph( graph_kind, - specifiers.iter().map(|&s| s.clone()).collect(), + self + .workspace_files + .iter() + .map(|f| f.specifier.clone()) + .collect(), crate::graph_util::NpmCachingStrategy::Eager, ) .await?; module_graph_creator.graph_valid(&graph)?; - let dependent_specifiers = specifiers - .into_iter() - .filter(|s| { + let dependent_specifiers = self + .workspace_files + .iter() + .filter(|f| { let mut dependency_specifiers = graph.walk( - std::iter::once(*s), + std::iter::once(&f.specifier), deno_graph::WalkOptions { follow_dynamic: true, kind: deno_graph::GraphKind::All, @@ -1376,6 +1388,11 @@ impl CliFactoryWithWorkspaceFiles { } false }) + .flat_map(|f| { + std::iter::once(&f.specifier) + .filter(|_| !f.doc_only) + .chain(f.doc_snippet_specifiers.iter()) + }) .collect(); Ok(dependent_specifiers) } From 36fcff8670d919ebbb13480e1275cef3332cc2fb Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 20 Jan 2025 16:56:57 +0000 Subject: [PATCH 33/33] coverage fixture --- .../coverage/filter_doc_testing_urls/test_coverage.out | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/specs/coverage/filter_doc_testing_urls/test_coverage.out b/tests/specs/coverage/filter_doc_testing_urls/test_coverage.out index 65548061a1..c553b804ba 100644 --- a/tests/specs/coverage/filter_doc_testing_urls/test_coverage.out +++ b/tests/specs/coverage/filter_doc_testing_urls/test_coverage.out @@ -1,9 +1,9 @@ -Check [WILDCARD]/test.ts Check [WILDCARD]/source.ts$[WILDCARD].ts -running 1 test from ./test.ts -add() ... ok ([WILDCARD]) +Check [WILDCARD]/test.ts running 1 test from ./source.ts$[WILDCARD].ts file:///[WILDCARD]/source.ts$[WILDCARD].ts ... ok ([WILDCARD]) +running 1 test from ./test.ts +add() ... ok ([WILDCARD]) ok | 2 passed | 0 failed ([WILDCARD])