From bd5b3e4f6e0a4ce69fd6d74fd8bfb31cfc245a4f Mon Sep 17 00:00:00 2001 From: Takayuki Maeda Date: Sat, 20 Sep 2025 03:40:23 +0900 Subject: [PATCH] Deduplicate input paths (#20105) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes #20035, fixes #19395 This is for deduplicating input paths to avoid processing the same file multiple times. This is my first contribution, so I'm sorry if I miss something. Please tell me if this is needed for this feature. ## Test Plan I just added a test `find_python_files_deduplicated` in https://github.com/TaKO8Ki/ruff/blob/eee1020e322e693bf977d91bf3edd03b45420254/crates/ruff_workspace/src/resolver.rs#L1017 . This pull request adds changes to `WalkPythonFilesState::finish`, which is used in `python_files_in_path`, so they affect some commands such as `analyze`, `format`, `check` and so on. I will add snapshot tests for them if necessary. I’ve already confirmed that the same thing happens with ruff check as well. ``` $ echo "x = 1" > example/foo.py $ uvx ruff check example example/foo.py I002 [*] Missing required import: `from __future__ import annotations` --> /path/to/example/foo.py:1:1 help: Insert required import: `from __future__ import annotations` I002 [*] Missing required import: `from __future__ import annotations` --> /path/to/example/foo.py:1:1 help: Insert required import: `from __future__ import annotations` Found 2 errors. [*] 2 fixable with the `--fix` option. ``` --- crates/ruff/tests/format.rs | 29 ++++++++++++++++ crates/ruff/tests/lint.rs | 44 ++++++++++++++++++++++++ crates/ruff_workspace/src/resolver.rs | 48 +++++++++++++++++++-------- 3 files changed, 107 insertions(+), 14 deletions(-) diff --git a/crates/ruff/tests/format.rs b/crates/ruff/tests/format.rs index 35443daa23..f1369048f7 100644 --- a/crates/ruff/tests/format.rs +++ b/crates/ruff/tests/format.rs @@ -500,6 +500,35 @@ OTHER = "OTHER" Ok(()) } +/// Regression test for +#[test] +fn deduplicate_directory_and_explicit_file() -> Result<()> { + let tempdir = TempDir::new()?; + let root = tempdir.path(); + + let main = root.join("main.py"); + fs::write(&main, "x = 1\n")?; + + assert_cmd_snapshot!( + Command::new(get_cargo_bin(BIN_NAME)) + .current_dir(root) + .args(["format", "--no-cache", "--check"]) + .arg(".") + .arg("main.py"), + @r" + success: false + exit_code: 1 + ----- stdout ----- + Would reformat: main.py + 1 file would be reformatted + + ----- stderr ----- + " + ); + + Ok(()) +} + #[test] fn syntax_error() -> Result<()> { let tempdir = TempDir::new()?; diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index b3a053408b..2869ee157a 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -271,6 +271,50 @@ OTHER = "OTHER" Ok(()) } +/// Regression test for +#[test] +fn deduplicate_directory_and_explicit_file() -> Result<()> { + let tempdir = TempDir::new()?; + let root = tempdir.path(); + let ruff_toml = tempdir.path().join("ruff.toml"); + fs::write( + &ruff_toml, + r#" +[lint] +exclude = ["main.py"] +"#, + )?; + + let main = root.join("main.py"); + fs::write(&main, "import os\n")?; + + insta::with_settings!({ + filters => vec![(tempdir_filter(&tempdir).as_str(), "[TMP]/")] + }, { + assert_cmd_snapshot!( + Command::new(get_cargo_bin(BIN_NAME)) + .current_dir(root) + .args(STDIN_BASE_OPTIONS) + .args(["--config", &ruff_toml.file_name().unwrap().to_string_lossy()]) + .arg(".") + // Explicitly pass main.py, should be linted regardless of it being excluded by lint.exclude + .arg("main.py"), + @r" + success: false + exit_code: 1 + ----- stdout ----- + main.py:1:8: F401 [*] `os` imported but unused + Found 1 error. + [*] 1 fixable with the `--fix` option. + + ----- stderr ----- + " + ); + }); + + Ok(()) +} + #[test] fn exclude_stdin() -> Result<()> { let tempdir = TempDir::new()?; diff --git a/crates/ruff_workspace/src/resolver.rs b/crates/ruff_workspace/src/resolver.rs index 84a31ebbee..d6e642ed48 100644 --- a/crates/ruff_workspace/src/resolver.rs +++ b/crates/ruff_workspace/src/resolver.rs @@ -523,10 +523,42 @@ impl<'config> WalkPythonFilesState<'config> { let (files, error) = self.merged.into_inner().unwrap(); error?; - Ok((files, self.resolver.into_inner().unwrap())) + let deduplicated_files = deduplicate_files(files); + + Ok((deduplicated_files, self.resolver.into_inner().unwrap())) } } +/// Deduplicate files by path, prioritizing `Root` files over `Nested` files. +/// +/// When the same path appears both as a directly specified input (`Root`) +/// and via directory traversal (`Nested`), keep the `Root` entry and drop +/// the `Nested` entry. +/// +/// Dropping the root entry means that the explicitly passed path may be +/// unintentionally ignored, since it is treated as nested and can be excluded +/// despite being requested. +/// +/// Concretely, with `lint.exclude = ["foo.py"]` and `ruff check . foo.py`, +/// we must keep `Root(foo.py)` and drop `Nested(foo.py)` so `foo.py` is +/// linted as the user requested. +fn deduplicate_files(mut files: ResolvedFiles) -> ResolvedFiles { + // Sort by path; for identical paths, prefer Root over Nested; place errors after files + files.sort_by(|a, b| match (a, b) { + (Ok(a_file), Ok(b_file)) => a_file.cmp(b_file), + (Ok(_), Err(_)) => Ordering::Less, + (Err(_), Ok(_)) => Ordering::Greater, + (Err(_), Err(_)) => Ordering::Equal, + }); + + files.dedup_by(|a, b| match (a, b) { + (Ok(a_file), Ok(b_file)) => a_file.path() == b_file.path(), + _ => false, + }); + + files +} + struct PythonFilesVisitorBuilder<'s, 'config> { state: &'s WalkPythonFilesState<'config>, transformer: &'s (dyn ConfigurationTransformer + Sync), @@ -682,7 +714,7 @@ impl Drop for PythonFilesVisitor<'_, '_> { } } -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, Ord, PartialOrd)] pub enum ResolvedFile { /// File explicitly passed to the CLI Root(PathBuf), @@ -715,18 +747,6 @@ impl ResolvedFile { } } -impl PartialOrd for ResolvedFile { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl Ord for ResolvedFile { - fn cmp(&self, other: &Self) -> Ordering { - self.path().cmp(other.path()) - } -} - /// Return `true` if the Python file at [`Path`] is _not_ excluded. pub fn python_file_at_path( path: &Path,