Skip to content

Commit

Permalink
Escape template filenames in glob patterns (astral-sh#16407)
Browse files Browse the repository at this point in the history
## Summary

Fixes astral-sh#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
astral-sh#7959 (comment),
but that doesn't work for a nested config file with relative paths. For
example, the directory tree in the reproduction repo linked
[here](astral-sh#9381 (comment)):

```
.
├── 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.
  • Loading branch information
ntBre authored Mar 3, 2025
1 parent 4d92e20 commit d93ed29
Show file tree
Hide file tree
Showing 8 changed files with 195 additions and 70 deletions.
34 changes: 34 additions & 0 deletions crates/ruff/tests/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 <https://github.com/astral-sh/ruff/issues/9381> with very helpful
/// reproduction repo here: <https://github.com/lucasfijen/example_ruff_glob_bug>
#[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(())
}
82 changes: 82 additions & 0 deletions crates/ruff/tests/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2782,3 +2782,85 @@ fn cache_syntax_errors() -> Result<()> {

Ok(())
}

/// Regression test for <https://github.com/astral-sh/ruff/issues/9381> with very helpful
/// reproduction repo here: <https://github.com/lucasfijen/example_ruff_glob_bug>
#[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(())
}
19 changes: 14 additions & 5 deletions crates/ruff_linter/src/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<PathBuf> = 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
Expand Down Expand Up @@ -36,11 +49,7 @@ pub fn normalize_path_to<P: AsRef<Path>, R: AsRef<Path>>(path: P, project_root:
pub fn relativize_path<P: AsRef<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());
}
Expand Down
7 changes: 3 additions & 4 deletions crates/ruff_linter/src/settings/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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;

Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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())
}
}
51 changes: 40 additions & 11 deletions crates/ruff_linter/src/settings/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Path>, root: impl AsRef<Path>) -> 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 {
Expand Down Expand Up @@ -202,9 +233,10 @@ impl FromStr for FilePattern {
type Err = anyhow::Error;

fn from_str(s: &str) -> Result<Self, Self::Err> {
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()),
))
}
}

Expand Down Expand Up @@ -281,7 +313,7 @@ pub struct PerFile<T> {
/// 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.
Expand All @@ -298,15 +330,12 @@ impl<T> PerFile<T> {
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,
}
Expand Down
7 changes: 3 additions & 4 deletions crates/ruff_server/src/session/index/ruff_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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()
Expand Down
16 changes: 8 additions & 8 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
Loading

0 comments on commit d93ed29

Please sign in to comment.