From d93ed293eb57f1dd72349a6b0b7a04416e077dea Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Mon, 3 Mar 2025 09:29:58 -0500 Subject: [PATCH] Escape template filenames in glob patterns (#16407) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes #9381. This PR fixes errors like ``` Cause: error parsing glob '/Users/me/project/{{cookiecutter.project_dirname}}/__pycache__': nested alternate groups are not allowed ``` caused by glob special characters in filenames like `{{cookiecutter.project_dirname}}`. When the user is matching that directory exactly, they can use the workaround given by https://github.com/astral-sh/ruff/issues/7959#issuecomment-1764751734, but that doesn't work for a nested config file with relative paths. For example, the directory tree in the reproduction repo linked [here](https://github.com/astral-sh/ruff/issues/9381#issuecomment-2677696408): ``` . ├── README.md ├── hello.py ├── pyproject.toml ├── uv.lock └── {{cookiecutter.repo_name}} ├── main.py ├── pyproject.toml └── tests └── maintest.py ``` where the inner `pyproject.toml` contains a relative glob: ```toml [tool.ruff.lint.per-file-ignores] "tests/*" = ["F811"] ``` ## Test Plan A new CLI test in both the linter and formatter. The formatter test may not be necessary because I didn't have to modify any additional code to pass it, but the original report mentioned both `check` and `format`, so I wanted to be sure both were fixed. --- crates/ruff/tests/format.rs | 34 ++++++++ crates/ruff/tests/lint.rs | 82 +++++++++++++++++++ crates/ruff_linter/src/fs.rs | 19 +++-- crates/ruff_linter/src/settings/mod.rs | 7 +- crates/ruff_linter/src/settings/types.rs | 51 +++++++++--- .../src/session/index/ruff_settings.rs | 7 +- crates/ruff_workspace/src/configuration.rs | 16 ++-- crates/ruff_workspace/src/resolver.rs | 49 +++-------- 8 files changed, 195 insertions(+), 70 deletions(-) diff --git a/crates/ruff/tests/format.rs b/crates/ruff/tests/format.rs index 122980d5ed..01839ebc8c 100644 --- a/crates/ruff/tests/format.rs +++ b/crates/ruff/tests/format.rs @@ -2133,3 +2133,37 @@ with open("a_really_long_foo") as foo, open("a_really_long_bar") as bar, open("a ----- stderr ----- "#); } + +/// Regression test for with very helpful +/// reproduction repo here: +#[test] +fn cookiecutter_globbing() -> Result<()> { + // This is a simplified directory structure from the repo linked above. The essence of the + // problem is this `{{cookiecutter.repo_name}}` directory containing a config file with a glob. + // The absolute path of the glob contains the glob metacharacters `{{` and `}}` even though the + // user's glob does not. + let tempdir = TempDir::new()?; + let cookiecutter = tempdir.path().join("{{cookiecutter.repo_name}}"); + let cookiecutter_toml = cookiecutter.join("pyproject.toml"); + let tests = cookiecutter.join("tests"); + fs::create_dir_all(&tests)?; + fs::write( + cookiecutter_toml, + r#"tool.ruff.lint.per-file-ignores = { "tests/*" = ["F811"] }"#, + )?; + let maintest = tests.join("maintest.py"); + fs::write(maintest, "import foo\nimport bar\nimport foo\n")?; + + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(["format", "--no-cache", "--diff"]) + .current_dir(tempdir.path()), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + 1 file already formatted + "); + + Ok(()) +} diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index 0ec437e502..c826cd861c 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -2782,3 +2782,85 @@ fn cache_syntax_errors() -> Result<()> { Ok(()) } + +/// Regression test for with very helpful +/// reproduction repo here: +#[test] +fn cookiecutter_globbing() -> Result<()> { + // This is a simplified directory structure from the repo linked above. The essence of the + // problem is this `{{cookiecutter.repo_name}}` directory containing a config file with a glob. + // The absolute path of the glob contains the glob metacharacters `{{` and `}}` even though the + // user's glob does not. + let tempdir = TempDir::new()?; + let cookiecutter = tempdir.path().join("{{cookiecutter.repo_name}}"); + let cookiecutter_toml = cookiecutter.join("pyproject.toml"); + let tests = cookiecutter.join("tests"); + fs::create_dir_all(&tests)?; + fs::write( + &cookiecutter_toml, + r#"tool.ruff.lint.per-file-ignores = { "tests/*" = ["F811"] }"#, + )?; + // F811 example from the docs to ensure the glob still works + let maintest = tests.join("maintest.py"); + fs::write(maintest, "import foo\nimport bar\nimport foo")?; + + insta::with_settings!({filters => vec![(r"\\", "/")]}, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .arg("--select=F811") + .current_dir(tempdir.path()), @r" + success: true + exit_code: 0 + ----- stdout ----- + All checks passed! + + ----- stderr ----- + "); + }); + + // after removing the config file with the ignore, F811 applies, so the glob worked above + fs::remove_file(cookiecutter_toml)?; + + insta::with_settings!({filters => vec![(r"\\", "/")]}, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .arg("--select=F811") + .current_dir(tempdir.path()), @r" + success: false + exit_code: 1 + ----- stdout ----- + {{cookiecutter.repo_name}}/tests/maintest.py:3:8: F811 [*] Redefinition of unused `foo` from line 1 + Found 1 error. + [*] 1 fixable with the `--fix` option. + + ----- stderr ----- + "); + }); + + Ok(()) +} + +/// Like the test above but exercises the non-absolute path case in `PerFile::new` +#[test] +fn cookiecutter_globbing_no_project_root() -> Result<()> { + let tempdir = TempDir::new()?; + let tempdir = tempdir.path().join("{{cookiecutter.repo_name}}"); + fs::create_dir(&tempdir)?; + + insta::with_settings!({filters => vec![(r"\\", "/")]}, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .current_dir(&tempdir) + .args(STDIN_BASE_OPTIONS) + .args(["--extend-per-file-ignores", "generated.py:Q"]), @r" + success: true + exit_code: 0 + ----- stdout ----- + All checks passed! + + ----- stderr ----- + warning: No Python files found under the given path(s) + "); + }); + + Ok(()) +} diff --git a/crates/ruff_linter/src/fs.rs b/crates/ruff_linter/src/fs.rs index 985f2e599a..983651121a 100644 --- a/crates/ruff_linter/src/fs.rs +++ b/crates/ruff_linter/src/fs.rs @@ -5,6 +5,19 @@ use path_absolutize::Absolutize; use crate::registry::RuleSet; use crate::settings::types::CompiledPerFileIgnoreList; +/// Return the current working directory. +/// +/// On WASM this just returns `.`. Otherwise, defer to [`path_absolutize::path_dedot::CWD`]. +pub fn get_cwd() -> &'static Path { + #[cfg(target_arch = "wasm32")] + { + static CWD: std::sync::LazyLock = std::sync::LazyLock::new(|| PathBuf::from(".")); + &CWD + } + #[cfg(not(target_arch = "wasm32"))] + path_absolutize::path_dedot::CWD.as_path() +} + /// Create a set with codes matching the pattern/code pairs. pub(crate) fn ignores_from_path(path: &Path, ignore_list: &CompiledPerFileIgnoreList) -> RuleSet { ignore_list @@ -36,11 +49,7 @@ pub fn normalize_path_to, R: AsRef>(path: P, project_root: pub fn relativize_path>(path: P) -> String { let path = path.as_ref(); - #[cfg(target_arch = "wasm32")] - let cwd = Path::new("."); - #[cfg(not(target_arch = "wasm32"))] - let cwd = path_absolutize::path_dedot::CWD.as_path(); - + let cwd = get_cwd(); if let Ok(path) = path.strip_prefix(cwd) { return format!("{}", path.display()); } diff --git a/crates/ruff_linter/src/settings/mod.rs b/crates/ruff_linter/src/settings/mod.rs index cbaaa4608a..e4aff6e749 100644 --- a/crates/ruff_linter/src/settings/mod.rs +++ b/crates/ruff_linter/src/settings/mod.rs @@ -2,7 +2,6 @@ //! command-line options. Structure is optimized for internal usage, as opposed //! to external visibility or parsing. -use path_absolutize::path_dedot; use regex::Regex; use rustc_hash::FxHashSet; use std::fmt::{Display, Formatter}; @@ -24,7 +23,7 @@ use crate::rules::{ pep8_naming, pycodestyle, pydoclint, pydocstyle, pyflakes, pylint, pyupgrade, ruff, }; use crate::settings::types::{CompiledPerFileIgnoreList, ExtensionMapping, FilePatternSet}; -use crate::{codes, RuleSelector}; +use crate::{codes, fs, RuleSelector}; use super::line_width::IndentWidth; @@ -414,7 +413,7 @@ impl LinterSettings { per_file_ignores: CompiledPerFileIgnoreList::default(), fix_safety: FixSafetyTable::default(), - src: vec![path_dedot::CWD.clone(), path_dedot::CWD.join("src")], + src: vec![fs::get_cwd().to_path_buf(), fs::get_cwd().join("src")], // Needs duplicating tab_size: IndentWidth::default(), line_length: LineLength::default(), @@ -474,6 +473,6 @@ impl LinterSettings { impl Default for LinterSettings { fn default() -> Self { - Self::new(path_dedot::CWD.as_path()) + Self::new(fs::get_cwd()) } } diff --git a/crates/ruff_linter/src/settings/types.rs b/crates/ruff_linter/src/settings/types.rs index 5aa3bca5ab..7087d9856d 100644 --- a/crates/ruff_linter/src/settings/types.rs +++ b/crates/ruff_linter/src/settings/types.rs @@ -159,10 +159,41 @@ impl UnsafeFixes { } } +/// Represents a path to be passed to [`Glob::new`]. +#[derive(Debug, Clone, CacheKey, PartialEq, PartialOrd, Eq, Ord)] +pub struct GlobPath { + path: PathBuf, +} + +impl GlobPath { + /// Constructs a [`GlobPath`] by escaping any glob metacharacters in `root` and normalizing + /// `path` to the escaped `root`. + /// + /// See [`fs::normalize_path_to`] for details of the normalization. + pub fn normalize(path: impl AsRef, root: impl AsRef) -> Self { + let root = root.as_ref().to_string_lossy(); + let escaped = globset::escape(&root); + let absolute = fs::normalize_path_to(path, escaped); + Self { path: absolute } + } + + pub fn into_inner(self) -> PathBuf { + self.path + } +} + +impl Deref for GlobPath { + type Target = PathBuf; + + fn deref(&self) -> &Self::Target { + &self.path + } +} + #[derive(Debug, Clone, CacheKey, PartialEq, PartialOrd, Eq, Ord)] pub enum FilePattern { Builtin(&'static str), - User(String, PathBuf), + User(String, GlobPath), } impl FilePattern { @@ -202,9 +233,10 @@ impl FromStr for FilePattern { type Err = anyhow::Error; fn from_str(s: &str) -> Result { - let pattern = s.to_string(); - let absolute = fs::normalize_path(&pattern); - Ok(Self::User(pattern, absolute)) + Ok(Self::User( + s.to_string(), + GlobPath::normalize(s, fs::get_cwd()), + )) } } @@ -281,7 +313,7 @@ pub struct PerFile { /// The glob pattern used to construct the [`PerFile`]. basename: String, /// The same pattern as `basename` but normalized to the project root directory. - absolute: PathBuf, + absolute: GlobPath, /// Whether the glob pattern should be negated (e.g. `!*.ipynb`) negated: bool, /// The per-file data associated with these glob patterns. @@ -298,15 +330,12 @@ impl PerFile { if negated { pattern.drain(..1); } - let path = Path::new(&pattern); - let absolute = match project_root { - Some(project_root) => fs::normalize_path_to(path, project_root), - None => fs::normalize_path(path), - }; + + let project_root = project_root.unwrap_or(fs::get_cwd()); Self { + absolute: GlobPath::normalize(&pattern, project_root), basename: pattern, - absolute, negated, data, } diff --git a/crates/ruff_server/src/session/index/ruff_settings.rs b/crates/ruff_server/src/session/index/ruff_settings.rs index 650f5c5b69..b7c93dde07 100644 --- a/crates/ruff_server/src/session/index/ruff_settings.rs +++ b/crates/ruff_server/src/session/index/ruff_settings.rs @@ -7,9 +7,8 @@ use std::sync::Arc; use anyhow::Context; use ignore::{WalkBuilder, WalkState}; -use ruff_linter::{ - fs::normalize_path_to, settings::types::FilePattern, settings::types::PreviewMode, -}; +use ruff_linter::settings::types::GlobPath; +use ruff_linter::{settings::types::FilePattern, settings::types::PreviewMode}; use ruff_workspace::resolver::match_exclusion; use ruff_workspace::Settings; use ruff_workspace::{ @@ -375,7 +374,7 @@ impl ConfigurationTransformer for EditorConfigurationTransformer<'_> { exclude .into_iter() .map(|pattern| { - let absolute = normalize_path_to(&pattern, project_root); + let absolute = GlobPath::normalize(&pattern, project_root); FilePattern::User(pattern, absolute) }) .collect() diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index dc7e463fa5..49d63b40c6 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -30,7 +30,7 @@ use ruff_linter::settings::fix_safety_table::FixSafetyTable; use ruff_linter::settings::rule_table::RuleTable; use ruff_linter::settings::types::{ CompiledPerFileIgnoreList, CompiledPerFileTargetVersionList, ExtensionMapping, FilePattern, - FilePatternSet, OutputFormat, PerFileIgnore, PerFileTargetVersion, PreviewMode, + FilePatternSet, GlobPath, OutputFormat, PerFileIgnore, PerFileTargetVersion, PreviewMode, RequiredVersion, UnsafeFixes, }; use ruff_linter::settings::{LinterSettings, DEFAULT_SELECTORS, DUMMY_VARIABLE_RGX, TASK_TAGS}; @@ -476,7 +476,7 @@ impl Configuration { paths .into_iter() .map(|pattern| { - let absolute = fs::normalize_path_to(&pattern, project_root); + let absolute = GlobPath::normalize(&pattern, project_root); FilePattern::User(pattern, absolute) }) .collect() @@ -495,7 +495,7 @@ impl Configuration { paths .into_iter() .map(|pattern| { - let absolute = fs::normalize_path_to(&pattern, project_root); + let absolute = GlobPath::normalize(&pattern, project_root); FilePattern::User(pattern, absolute) }) .collect() @@ -507,7 +507,7 @@ impl Configuration { paths .into_iter() .map(|pattern| { - let absolute = fs::normalize_path_to(&pattern, project_root); + let absolute = GlobPath::normalize(&pattern, project_root); FilePattern::User(pattern, absolute) }) .collect() @@ -517,7 +517,7 @@ impl Configuration { paths .into_iter() .map(|pattern| { - let absolute = fs::normalize_path_to(&pattern, project_root); + let absolute = GlobPath::normalize(&pattern, project_root); FilePattern::User(pattern, absolute) }) .collect() @@ -700,7 +700,7 @@ impl LintConfiguration { paths .into_iter() .map(|pattern| { - let absolute = fs::normalize_path_to(&pattern, project_root); + let absolute = GlobPath::normalize(&pattern, project_root); FilePattern::User(pattern, absolute) }) .collect() @@ -1203,7 +1203,7 @@ impl FormatConfiguration { paths .into_iter() .map(|pattern| { - let absolute = fs::normalize_path_to(&pattern, project_root); + let absolute = GlobPath::normalize(&pattern, project_root); FilePattern::User(pattern, absolute) }) .collect() @@ -1267,7 +1267,7 @@ impl AnalyzeConfiguration { paths .into_iter() .map(|pattern| { - let absolute = fs::normalize_path_to(&pattern, project_root); + let absolute = GlobPath::normalize(&pattern, project_root); FilePattern::User(pattern, absolute) }) .collect() diff --git a/crates/ruff_workspace/src/resolver.rs b/crates/ruff_workspace/src/resolver.rs index 7c51eb9326..4c556a3fbc 100644 --- a/crates/ruff_workspace/src/resolver.rs +++ b/crates/ruff_workspace/src/resolver.rs @@ -877,7 +877,7 @@ mod tests { use path_absolutize::Absolutize; use tempfile::TempDir; - use ruff_linter::settings::types::FilePattern; + use ruff_linter::settings::types::{FilePattern, GlobPath}; use crate::configuration::Configuration; use crate::pyproject::find_settings_toml; @@ -972,13 +972,8 @@ mod tests { let project_root = Path::new("/tmp/"); let path = Path::new("foo").absolutize_from(project_root).unwrap(); - let exclude = FilePattern::User( - "foo".to_string(), - Path::new("foo") - .absolutize_from(project_root) - .unwrap() - .to_path_buf(), - ); + let exclude = + FilePattern::User("foo".to_string(), GlobPath::normalize("foo", project_root)); let file_path = &path; let file_basename = path.file_name().unwrap(); assert!(match_exclusion( @@ -988,13 +983,8 @@ mod tests { )); let path = Path::new("foo/bar").absolutize_from(project_root).unwrap(); - let exclude = FilePattern::User( - "bar".to_string(), - Path::new("bar") - .absolutize_from(project_root) - .unwrap() - .to_path_buf(), - ); + let exclude = + FilePattern::User("bar".to_string(), GlobPath::normalize("bar", project_root)); let file_path = &path; let file_basename = path.file_name().unwrap(); assert!(match_exclusion( @@ -1008,10 +998,7 @@ mod tests { .unwrap(); let exclude = FilePattern::User( "baz.py".to_string(), - Path::new("baz.py") - .absolutize_from(project_root) - .unwrap() - .to_path_buf(), + GlobPath::normalize("baz.py", project_root), ); let file_path = &path; let file_basename = path.file_name().unwrap(); @@ -1024,10 +1011,7 @@ mod tests { let path = Path::new("foo/bar").absolutize_from(project_root).unwrap(); let exclude = FilePattern::User( "foo/bar".to_string(), - Path::new("foo/bar") - .absolutize_from(project_root) - .unwrap() - .to_path_buf(), + GlobPath::normalize("foo/bar", project_root), ); let file_path = &path; let file_basename = path.file_name().unwrap(); @@ -1042,10 +1026,7 @@ mod tests { .unwrap(); let exclude = FilePattern::User( "foo/bar/baz.py".to_string(), - Path::new("foo/bar/baz.py") - .absolutize_from(project_root) - .unwrap() - .to_path_buf(), + GlobPath::normalize("foo/bar/baz.py", project_root), ); let file_path = &path; let file_basename = path.file_name().unwrap(); @@ -1060,10 +1041,7 @@ mod tests { .unwrap(); let exclude = FilePattern::User( "foo/bar/*.py".to_string(), - Path::new("foo/bar/*.py") - .absolutize_from(project_root) - .unwrap() - .to_path_buf(), + GlobPath::normalize("foo/bar/*.py", project_root), ); let file_path = &path; let file_basename = path.file_name().unwrap(); @@ -1076,13 +1054,8 @@ mod tests { let path = Path::new("foo/bar/baz.py") .absolutize_from(project_root) .unwrap(); - let exclude = FilePattern::User( - "baz".to_string(), - Path::new("baz") - .absolutize_from(project_root) - .unwrap() - .to_path_buf(), - ); + let exclude = + FilePattern::User("baz".to_string(), GlobPath::normalize("baz", project_root)); let file_path = &path; let file_basename = path.file_name().unwrap(); assert!(!match_exclusion(