From 86010bec092d074b161800da06149cfb79fb9f4b Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 8 Jul 2024 10:12:10 -0400 Subject: [PATCH] fix(workspace): better cli file argument handling (#24447) Closes https://github.com/denoland/deno/issues/24422 --- Cargo.lock | 4 +- Cargo.toml | 2 +- cli/args/flags.rs | 22 +-- cli/args/mod.rs | 283 +++++++------------------------------ cli/lsp/config.rs | 4 +- cli/lsp/language_server.rs | 2 +- cli/tools/fmt.rs | 7 +- cli/tools/lint/mod.rs | 17 +-- cli/util/collections.rs | 38 ----- cli/util/mod.rs | 1 - 10 files changed, 84 insertions(+), 296 deletions(-) delete mode 100644 cli/util/collections.rs diff --git a/Cargo.lock b/Cargo.lock index 8daaa4551c..04be7a90fc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1307,9 +1307,9 @@ dependencies = [ [[package]] name = "deno_config" -version = "0.20.0" +version = "0.20.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "64772162a8e8c1b3a9c48b4a0924e29f5b8f0ae23ea2027361937e96d04d493d" +checksum = "ff203375858a92c7afa82324e89ff1f84c04fb456613135c4abccc6b8f31e8e5" dependencies = [ "anyhow", "deno_semver", diff --git a/Cargo.toml b/Cargo.toml index 2e5464718f..439ec744aa 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -101,7 +101,7 @@ console_static_text = "=0.8.1" data-encoding = "2.3.3" data-url = "=0.3.0" deno_cache_dir = "=0.10.0" -deno_config = { version = "=0.20.0", default-features = false } +deno_config = { version = "=0.20.1", default-features = false } dlopen2 = "0.6.1" ecb = "=0.1.2" elliptic-curve = { version = "0.13.4", features = ["alloc", "arithmetic", "ecdh", "std", "pem"] } diff --git a/cli/args/flags.rs b/cli/args/flags.rs index b68b4aadb7..89f53d5937 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -25,6 +25,7 @@ use log::debug; use log::Level; use serde::Deserialize; use serde::Serialize; +use std::collections::HashSet; use std::env; use std::ffi::OsString; use std::net::SocketAddr; @@ -36,7 +37,6 @@ use std::path::PathBuf; use std::str::FromStr; use crate::args::resolve_no_prompt; -use crate::util::collections::CheckedSet; use crate::util::fs::canonicalize_path; use super::flags_net; @@ -865,20 +865,20 @@ impl Flags { args } - /// Extract the directory paths the config file should be discovered from. + /// Extract the paths the config file should be discovered from. /// /// Returns `None` if the config file should not be auto-discovered. pub fn config_path_args(&self, current_dir: &Path) -> Option> { fn resolve_multiple_files( - files: &[String], + files_or_dirs: &[String], current_dir: &Path, ) -> Vec { - let mut seen = CheckedSet::with_capacity(files.len()); - let result = files + let mut seen = HashSet::with_capacity(files_or_dirs.len()); + let result = files_or_dirs .iter() .filter_map(|p| { - let path = normalize_path(current_dir.join(p).parent()?); - if seen.insert(&path) { + let path = normalize_path(current_dir.join(p)); + if seen.insert(path.clone()) { Some(path) } else { None @@ -9298,7 +9298,7 @@ mod tests { .unwrap(); assert_eq!( flags.config_path_args(&cwd), - Some(vec![cwd.join("dir/a/"), cwd.join("dir/b/")]) + Some(vec![cwd.join("dir/a/a.js"), cwd.join("dir/b/b.js")]) ); let flags = flags_from_vec(svec!["deno", "lint"]).unwrap(); @@ -9314,7 +9314,11 @@ mod tests { .unwrap(); assert_eq!( flags.config_path_args(&cwd), - Some(vec![cwd.join("dir/a/"), cwd.join("dir/")]) + Some(vec![ + cwd.join("dir/a/a.js"), + cwd.join("dir/a/a2.js"), + cwd.join("dir/b.js") + ]) ); } diff --git a/cli/args/mod.rs b/cli/args/mod.rs index a54003277a..950ba72e25 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -83,7 +83,6 @@ use crate::file_fetcher::FileFetcher; use crate::util::fs::canonicalize_path_maybe_not_exists; use crate::version; -use deno_config::glob::PathOrPatternSet; use deno_config::FmtConfig; use deno_config::LintConfig; use deno_config::TestConfig; @@ -274,18 +273,11 @@ pub struct BenchOptions { } impl BenchOptions { - pub fn resolve( - bench_config: BenchConfig, - bench_flags: &BenchFlags, - maybe_flags_base: Option<&Path>, - ) -> Result { - Ok(Self { - files: resolve_files( - bench_config.files, - &bench_flags.files, - maybe_flags_base, - )?, - }) + pub fn resolve(bench_config: BenchConfig, _bench_flags: &BenchFlags) -> Self { + // this is the same, but keeping the same pattern as everywhere else for the future + Self { + files: bench_config.files, + } } } @@ -309,19 +301,11 @@ impl FmtOptions { } } - pub fn resolve( - fmt_config: FmtConfig, - fmt_flags: &FmtFlags, - maybe_flags_base: Option<&Path>, - ) -> Result { - Ok(Self { + pub fn resolve(fmt_config: FmtConfig, fmt_flags: &FmtFlags) -> Self { + Self { options: resolve_fmt_options(fmt_flags, fmt_config.options), - files: resolve_files( - fmt_config.files, - &fmt_flags.files, - maybe_flags_base, - )?, - }) + files: fmt_config.files, + } } } @@ -401,18 +385,11 @@ pub struct TestOptions { } impl TestOptions { - pub fn resolve( - test_config: TestConfig, - test_flags: TestFlags, - maybe_flags_base: Option<&Path>, - ) -> Result { - Ok(Self { - files: resolve_files( - test_config.files, - &test_flags.files, - maybe_flags_base, - )?, - }) + pub fn resolve(test_config: TestConfig, _test_flags: &TestFlags) -> Self { + // this is the same, but keeping the same pattern as everywhere else for the future + Self { + files: test_config.files, + } } } @@ -482,25 +459,17 @@ impl LintOptions { } } - pub fn resolve( - lint_config: LintConfig, - lint_flags: LintFlags, - maybe_flags_base: Option<&Path>, - ) -> Result { - Ok(Self { - files: resolve_files( - lint_config.files, - &lint_flags.files, - maybe_flags_base, - )?, + pub fn resolve(lint_config: LintConfig, lint_flags: &LintFlags) -> Self { + Self { + files: lint_config.files, rules: resolve_lint_rules_options( - lint_config.rules, - lint_flags.maybe_rules_tags, - lint_flags.maybe_rules_include, - lint_flags.maybe_rules_exclude, + lint_config.options.rules, + lint_flags.maybe_rules_tags.clone(), + lint_flags.maybe_rules_include.clone(), + lint_flags.maybe_rules_exclude.clone(), ), fix: lint_flags.fix, - }) + } } } @@ -907,9 +876,9 @@ impl CliOptions { let workspace = match &flags.config_flag { deno_config::ConfigFlag::Discover => { - if let Some(start_dirs) = flags.config_path_args(&initial_cwd) { + if let Some(start_paths) = flags.config_path_args(&initial_cwd) { Workspace::discover( - WorkspaceDiscoverStart::Dirs(&start_dirs), + WorkspaceDiscoverStart::Paths(&start_paths), &resolve_workspace_discover_options(), )? } else { @@ -1355,28 +1324,20 @@ impl CliOptions { pub fn resolve_fmt_options_for_members( &self, fmt_flags: &FmtFlags, - ) -> Result, AnyError> { + ) -> Result, AnyError> { let cli_arg_patterns = fmt_flags.files.as_file_patterns(self.initial_cwd())?; - let member_ctxs = - self.workspace.resolve_ctxs_from_patterns(&cli_arg_patterns); - let mut result = Vec::with_capacity(member_ctxs.len()); - for member_ctx in &member_ctxs { - let options = self.resolve_fmt_options(fmt_flags, member_ctx)?; - result.push(options); + let member_configs = self + .workspace + .resolve_fmt_config_for_members(&cli_arg_patterns)?; + let mut result = Vec::with_capacity(member_configs.len()); + for (ctx, config) in member_configs { + let options = FmtOptions::resolve(config, fmt_flags); + result.push((ctx, options)); } Ok(result) } - pub fn resolve_fmt_options( - &self, - fmt_flags: &FmtFlags, - ctx: &WorkspaceMemberContext, - ) -> Result { - let fmt_config = ctx.to_fmt_config()?; - FmtOptions::resolve(fmt_config, fmt_flags, Some(&self.initial_cwd)) - } - pub fn resolve_workspace_lint_options( &self, lint_flags: &LintFlags, @@ -1391,27 +1352,18 @@ impl CliOptions { ) -> Result, AnyError> { let cli_arg_patterns = lint_flags.files.as_file_patterns(self.initial_cwd())?; - let member_ctxs = - self.workspace.resolve_ctxs_from_patterns(&cli_arg_patterns); - let mut result = Vec::with_capacity(member_ctxs.len()); - for member_ctx in member_ctxs { - let options = - self.resolve_lint_options(lint_flags.clone(), &member_ctx)?; - result.push((member_ctx, options)); + let member_configs = self + .workspace + .resolve_lint_config_for_members(&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); + result.push((ctx, options)); } Ok(result) } - pub fn resolve_lint_options( - &self, - lint_flags: LintFlags, - ctx: &WorkspaceMemberContext, - ) -> Result { - let lint_config = ctx.to_lint_config()?; - LintOptions::resolve(lint_config, lint_flags, Some(&self.initial_cwd)) - } - - pub fn resolve_lint_config( + pub fn resolve_deno_lint_config( &self, ) -> Result { let ts_config_result = @@ -1445,12 +1397,12 @@ impl CliOptions { ) -> Result, AnyError> { let cli_arg_patterns = test_flags.files.as_file_patterns(self.initial_cwd())?; - let member_ctxs = - self.workspace.resolve_ctxs_from_patterns(&cli_arg_patterns); + let member_ctxs = self + .workspace + .resolve_test_config_for_members(&cli_arg_patterns)?; let mut result = Vec::with_capacity(member_ctxs.len()); - for member_ctx in member_ctxs { - let options = - self.resolve_test_options(test_flags.clone(), &member_ctx)?; + for (member_ctx, config) in member_ctxs { + let options = TestOptions::resolve(config, test_flags); result.push((member_ctx, options)); } Ok(result) @@ -1463,40 +1415,23 @@ impl CliOptions { WorkspaceBenchOptions::resolve(bench_flags) } - pub fn resolve_test_options( - &self, - test_flags: TestFlags, - ctx: &WorkspaceMemberContext, - ) -> Result { - let test_config = ctx.to_test_config()?; - TestOptions::resolve(test_config, test_flags, Some(&self.initial_cwd)) - } - pub fn resolve_bench_options_for_members( &self, bench_flags: &BenchFlags, ) -> Result, AnyError> { let cli_arg_patterns = bench_flags.files.as_file_patterns(self.initial_cwd())?; - let member_ctxs = - self.workspace.resolve_ctxs_from_patterns(&cli_arg_patterns); + let member_ctxs = self + .workspace + .resolve_bench_config_for_members(&cli_arg_patterns)?; let mut result = Vec::with_capacity(member_ctxs.len()); - for member_ctx in member_ctxs { - let options = self.resolve_bench_options(bench_flags, &member_ctx)?; + for (member_ctx, config) in member_ctxs { + let options = BenchOptions::resolve(config, bench_flags); result.push((member_ctx, options)); } Ok(result) } - pub fn resolve_bench_options( - &self, - bench_flags: &BenchFlags, - ctx: &WorkspaceMemberContext, - ) -> Result { - let bench_config = ctx.to_bench_config()?; - BenchOptions::resolve(bench_config, bench_flags, Some(&self.initial_cwd)) - } - pub fn resolve_deno_graph_workspace_members( &self, ) -> Result, AnyError> { @@ -1873,31 +1808,6 @@ impl StorageKeyResolver { } } -/// Collect included and ignored files. CLI flags take precedence -/// over config file, i.e. if there's `files.ignore` in config file -/// and `--ignore` CLI flag, only the flag value is taken into account. -fn resolve_files( - mut files_config: FilePatterns, - file_flags: &FileFlags, - maybe_flags_base: Option<&Path>, -) -> Result { - if !file_flags.include.is_empty() { - files_config.include = - Some(PathOrPatternSet::from_include_relative_path_or_patterns( - maybe_flags_base.unwrap_or(&files_config.base), - &file_flags.include, - )?); - } - if !file_flags.ignore.is_empty() { - files_config.exclude = - PathOrPatternSet::from_exclude_relative_path_or_patterns( - maybe_flags_base.unwrap_or(&files_config.base), - &file_flags.ignore, - )?; - } - Ok(files_config) -} - /// Resolves the no_prompt value based on the cli flags and environment. pub fn resolve_no_prompt(flags: &PermissionFlags) -> bool { flags.no_prompt || has_flag_env_var("DENO_NO_PROMPT") @@ -1937,10 +1847,10 @@ pub fn config_to_deno_graph_workspace_member( #[cfg(test)] mod test { - use super::*; - use deno_config::glob::FileCollector; use pretty_assertions::assert_eq; + use super::*; + #[test] fn resolve_import_map_flags_take_precedence() { let config_text = r#"{ @@ -2018,95 +1928,6 @@ mod test { assert_eq!(resolver.resolve_storage_key(&specifier), None); } - #[test] - fn resolve_files_test() { - use test_util::TempDir; - let temp_dir = TempDir::new(); - - temp_dir.create_dir_all("data"); - temp_dir.create_dir_all("nested"); - temp_dir.create_dir_all("nested/foo"); - temp_dir.create_dir_all("nested/fizz"); - temp_dir.create_dir_all("pages"); - - temp_dir.write("data/tes.ts", ""); - temp_dir.write("data/test1.js", ""); - temp_dir.write("data/test1.ts", ""); - temp_dir.write("data/test12.ts", ""); - - temp_dir.write("nested/foo/foo.ts", ""); - temp_dir.write("nested/foo/bar.ts", ""); - temp_dir.write("nested/foo/fizz.ts", ""); - temp_dir.write("nested/foo/bazz.ts", ""); - - temp_dir.write("nested/fizz/foo.ts", ""); - temp_dir.write("nested/fizz/bar.ts", ""); - temp_dir.write("nested/fizz/fizz.ts", ""); - temp_dir.write("nested/fizz/bazz.ts", ""); - - temp_dir.write("pages/[id].ts", ""); - - let temp_dir_path = temp_dir.path().as_path(); - let error = PathOrPatternSet::from_include_relative_path_or_patterns( - temp_dir_path, - &["data/**********.ts".to_string()], - ) - .unwrap_err(); - assert!(error.to_string().starts_with("Failed to expand glob")); - - let resolved_files = resolve_files( - FilePatterns { - base: temp_dir_path.to_path_buf(), - include: Some( - PathOrPatternSet::from_include_relative_path_or_patterns( - temp_dir_path, - &[ - "data/test1.?s".to_string(), - "nested/foo/*.ts".to_string(), - "nested/fizz/*.ts".to_string(), - "pages/[id].ts".to_string(), - ], - ) - .unwrap(), - ), - exclude: PathOrPatternSet::from_exclude_relative_path_or_patterns( - temp_dir_path, - &["nested/**/*bazz.ts".to_string()], - ) - .unwrap(), - }, - &Default::default(), - Some(temp_dir_path), - ) - .unwrap(); - - let mut files = FileCollector::new(|_| true) - .ignore_git_folder() - .ignore_node_modules() - .collect_file_patterns(&deno_config::fs::RealDenoConfigFs, resolved_files) - .unwrap(); - - files.sort(); - - assert_eq!( - files, - vec![ - "data/test1.js", - "data/test1.ts", - "nested/fizz/bar.ts", - "nested/fizz/fizz.ts", - "nested/fizz/foo.ts", - "nested/foo/bar.ts", - "nested/foo/fizz.ts", - "nested/foo/foo.ts", - "pages/[id].ts", - ] - .into_iter() - .map(|p| deno_core::normalize_path(temp_dir_path.join(p))) - .collect::>() - ); - } - #[test] fn jsr_urls() { let reg_url = jsr_url(); diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index 3c360b683b..3fded336b6 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -1232,7 +1232,7 @@ impl ConfigData { .and_then(|config_file| { config_file .to_fmt_config() - .and_then(|o| FmtOptions::resolve(o, &Default::default(), None)) + .map(|o| FmtOptions::resolve(o, &Default::default())) .inspect_err(|err| { lsp_warn!(" Couldn't read formatter configuration: {}", err) }) @@ -1260,7 +1260,7 @@ impl ConfigData { .and_then(|config_file| { config_file .to_lint_config() - .and_then(|o| LintOptions::resolve(o, Default::default(), None)) + .map(|o| LintOptions::resolve(o, &Default::default())) .inspect_err(|err| { lsp_warn!(" Couldn't read lint configuration: {}", err) }) diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index b3deef35bb..093ea1dab2 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -3557,7 +3557,7 @@ impl Inner { .unwrap_or_else(|| self.initial_cwd.clone()); // todo: we need a way to convert config data to a Workspace let workspace = Arc::new(Workspace::discover( - deno_config::workspace::WorkspaceDiscoverStart::Dirs(&[ + deno_config::workspace::WorkspaceDiscoverStart::Paths(&[ initial_cwd.clone() ]), &WorkspaceDiscoverOptions { diff --git a/cli/tools/fmt.rs b/cli/tools/fmt.rs index 9a21b4c105..040991e7c2 100644 --- a/cli/tools/fmt.rs +++ b/cli/tools/fmt.rs @@ -53,8 +53,9 @@ pub async fn format(flags: Flags, fmt_flags: FmtFlags) -> Result<(), AnyError> { if fmt_flags.is_stdin() { let cli_options = CliOptions::from_flags(flags)?; let start_ctx = cli_options.workspace.resolve_start_ctx(); - let fmt_options = - cli_options.resolve_fmt_options(&fmt_flags, &start_ctx)?; + let fmt_config = start_ctx + .to_fmt_config(FilePatterns::new_with_base(start_ctx.dir_path()))?; + let fmt_options = FmtOptions::resolve(fmt_config, &fmt_flags); return format_stdin( &fmt_flags, fmt_options, @@ -143,7 +144,7 @@ fn resolve_paths_with_options_batches( cli_options.resolve_fmt_options_for_members(fmt_flags)?; let mut paths_with_options_batches = Vec::with_capacity(members_fmt_options.len()); - for member_fmt_options in members_fmt_options { + for (_ctx, member_fmt_options) in members_fmt_options { let files = collect_fmt_files(cli_options, member_fmt_options.files.clone())?; if !files.is_empty() { diff --git a/cli/tools/lint/mod.rs b/cli/tools/lint/mod.rs index 606d5835cb..2754262657 100644 --- a/cli/tools/lint/mod.rs +++ b/cli/tools/lint/mod.rs @@ -91,7 +91,7 @@ pub async fn lint(flags: Flags, lint_flags: LintFlags) -> Result<(), AnyError> { Ok(async move { let factory = CliFactory::from_flags(flags)?; let cli_options = factory.cli_options(); - let lint_config = cli_options.resolve_lint_config()?; + let lint_config = cli_options.resolve_deno_lint_config()?; let mut paths_with_options_batches = resolve_paths_with_options_batches(cli_options, &lint_flags)?; for paths_with_options in &mut paths_with_options_batches { @@ -143,7 +143,7 @@ pub async fn lint(flags: Flags, lint_flags: LintFlags) -> Result<(), AnyError> { let factory = CliFactory::from_flags(flags)?; let cli_options = factory.cli_options(); let is_stdin = lint_flags.is_stdin(); - let lint_config = cli_options.resolve_lint_config()?; + let deno_lint_config = cli_options.resolve_deno_lint_config()?; let workspace_lint_options = cli_options.resolve_workspace_lint_options(&lint_flags)?; let success = if is_stdin { @@ -151,14 +151,15 @@ pub async fn lint(flags: Flags, lint_flags: LintFlags) -> Result<(), AnyError> { let reporter_lock = Arc::new(Mutex::new(create_reporter( workspace_lint_options.reporter_kind, ))); - let lint_options = - cli_options.resolve_lint_options(lint_flags, &start_ctx)?; + let lint_config = start_ctx + .to_lint_config(FilePatterns::new_with_base(start_ctx.dir_path()))?; + let lint_options = LintOptions::resolve(lint_config, &lint_flags); let lint_rules = get_config_rules_err_empty( lint_options.rules, start_ctx.maybe_deno_json().map(|c| c.as_ref()), )?; let file_path = cli_options.initial_cwd().join(STDIN_FILE_NAME); - let r = lint_stdin(&file_path, lint_rules.rules, lint_config); + let r = lint_stdin(&file_path, lint_rules.rules, deno_lint_config); let success = handle_lint_result( &file_path.to_string_lossy(), r, @@ -179,7 +180,7 @@ pub async fn lint(flags: Flags, lint_flags: LintFlags) -> Result<(), AnyError> { linter .lint_files( paths_with_options.options, - lint_config.clone(), + deno_lint_config.clone(), paths_with_options.ctx, paths_with_options.paths, ) @@ -618,7 +619,7 @@ fn apply_lint_fixes( fn lint_stdin( file_path: &Path, lint_rules: Vec<&'static dyn LintRule>, - config: LintConfig, + deno_lint_config: LintConfig, ) -> Result<(ParsedSource, Vec), AnyError> { let mut source_code = String::new(); if stdin().read_to_string(&mut source_code).is_err() { @@ -632,7 +633,7 @@ fn lint_stdin( specifier: specifier_from_file_path(file_path)?, source_code: deno_ast::strip_bom(source_code), media_type: MediaType::TypeScript, - config, + config: deno_lint_config, }) .map_err(AnyError::from) } diff --git a/cli/util/collections.rs b/cli/util/collections.rs deleted file mode 100644 index 21f73024b1..0000000000 --- a/cli/util/collections.rs +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. - -use std::marker::PhantomData; - -pub struct CheckedSet { - _kind: PhantomData, - checked: std::collections::HashSet, -} - -impl Default for CheckedSet { - fn default() -> Self { - Self { - _kind: Default::default(), - checked: Default::default(), - } - } -} - -impl CheckedSet { - pub fn with_capacity(capacity: usize) -> Self { - Self { - _kind: PhantomData, - checked: std::collections::HashSet::with_capacity(capacity), - } - } - - pub fn insert(&mut self, value: &T) -> bool { - self.checked.insert(self.get_hash(value)) - } - - fn get_hash(&self, value: &T) -> u64 { - use std::collections::hash_map::DefaultHasher; - use std::hash::Hasher; - let mut hasher = DefaultHasher::new(); - value.hash(&mut hasher); - hasher.finish() - } -} diff --git a/cli/util/mod.rs b/cli/util/mod.rs index 2b6583fbc5..b7eef95d3a 100644 --- a/cli/util/mod.rs +++ b/cli/util/mod.rs @@ -2,7 +2,6 @@ // Note: Only add code in this folder that has no application specific logic pub mod checksum; -pub mod collections; pub mod console; pub mod diff; pub mod display;