Error instead of `panic!` when running Ruff from a deleted directory (#16903) #17054

maxmynter · 2025-03-28T23:07:33Z

Closes #16903

Summary

Check if the current working directory exist. If not, provide an error instead of panicking.

Fixed a stale comment in resolve_default_files.

Test Plan

I added a test to the resolve_files.rs.

Manual testing follows steps of #16903 :

Terminal 1

mkdir tmp
cd tmp

Terminal 2

rm -rf tmp

Terminal 1

ruff check

Open Issues / Questions to Reviewer

All tests pass when executed with cargo nextest run.

However, with cargo test the parallelization makes the other tests fail as we change the pwd.

Serial execution with cargo test seems to require another dependency or some workarounds.

Do you think an additional dependency or test complexity is worth testing this small edge case, do you have another implementation idea, or should i rather remove the test?

P.S.: I'm currently participating in a batch at the Recurse Center and would love to contribute more for the next six weeks to improve my Rust. Let me know if you're open to mentoring/reviewing and/or if you have specific areas where help would be most valued.

to reflect function behavior.

otherwise ruff panics if the working directory does not exist.

This test works when executed with nexttest but has interactions with other `resolve_files` tests when executed.

ntBre

Thanks for working on this! I took a look at this earlier in the week, and I was wondering how to go about testing it too.

I think if you move the new integration test to its own file, it will be built and run separately by cargo test and avoid the parallelization issues you ran into. Then you can also skip saving the original_dir and restoring it since the test should be run on its own.

Alternatively, we could possibly skip testing this to avoid changing the cwd at all, which feels a little scary. What do you think @MichaReiser?

ntBre · 2025-03-29T16:46:24Z

crates/ruff/src/resolve.rs

 ) -> Result<PyprojectConfig> {
+    // Ensure directory exists before proceeding
+    if std::env::current_dir().is_err() {
+        return Err(anyhow!("Working directory does not exist"));


I think we could use bail! here instead of Err(anyhow!(...)).

I also think we might as well do something like this:

let Ok(cwd) = std::env::current_dir() else { bail!("Working directory does not exist"); };

instead of using path_dedot::CWD below, which is just a LazyLock around std::env::current_dir. That makes it a bit more clear what this check is for, I think.

We may also want to move this check into the if config_arguments.isolated check if that's the only place CWD is used.

Agree on your proposal for rewriting. It makes the intentions of the check clear and we can get rid of the comment.

path_dedot::CWD is used a couple of times throughout the function so I will keep it outside of the if clause.

ntBre · 2025-03-29T16:59:01Z

crates/ruff/tests/resolve_files.rs

+    let temp_path = temp_dir.path().to_path_buf();
+    let original_dir = std::env::current_dir().unwrap();
+    std::env::set_current_dir(&temp_path).unwrap();
+    std::mem::drop(temp_dir);


nit: I would just use drop directly without qualifying it.

Agree that it reads nicer.
I will also just import from std::env so we can use directory operations without qualifiers.

ntBre · 2025-03-29T17:00:01Z

crates/ruff/tests/resolve_files.rs

+    insta::with_settings!({
+        filters => TEST_FILTERS.to_vec()
+    },


I don't think you actually need these filters and could leave them out. Paths only cause problems when they appear in the snapshot output.

github-actions · 2025-03-29T17:07:05Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

to have the test binary is built and run seperately. This is necessary as changing the working directory within the tests likely breaks other tests executed in parallel by changing the working directory. Also trim down the test logic a bit. "--no-cache" is redundant. Use imports instead of qualifiers for readability.

This is already achieved by having the test in a dedicated file.

Use bail! and let else since it communicates the intent more clearly.

maxmynter · 2025-03-30T03:02:15Z

Having the test in a dedicated file now works for both cargo test and cargo nextest.

MichaReiser · 2025-03-30T12:33:24Z

crates/ruff/src/resolve.rs

    config_arguments: &ConfigArguments,
    stdin_filename: Option<&Path>,
 ) -> Result<PyprojectConfig> {
+    let Ok(_cwd) = std::env::current_dir() else {


It feels off that the path isn't used here. I think we should change the code to call cwd.parse_dot() and use it instead of &path_dedot::CWD below

Edit: This is the same as what @ntBre was proposing

MichaReiser · 2025-03-30T12:35:11Z

crates/ruff/tests/direxist_guard.rs

+    set_current_dir(&temp_path).unwrap();
+    drop(temp_dir);
+
+    assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)).arg("check"), @r###"


I agree with @ntBre that changing the working directory is problematic. Let's only override the current working directory for the spanned CLI command.

Suggested change

assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)).arg("check"), @r###"

assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)).arg("check").current_dir(&temp_path), @r###"

I think I tried current_dir when I looked at this, and it caused a different panic in the Command invocation, separate from the one we're trying to test. But it would be good to double check, and maybe I missed something.

Yes, the assert_cmd_snapshot or current_dir calls an unwrap() somewhere. The test fails with:

[...] called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory"

Hmm I see. Let's add some module level documentation (or at least test level) explaining why this test has to be its own module.

Instead of path_dedot::CWD where appropriate

MichaReiser · 2025-03-31T08:14:06Z

crates/ruff/src/resolve.rs

-            .unwrap_or(&path_dedot::CWD.as_path()),
-    )? {
+    if let Some(pyproject) =
+        pyproject::find_settings_toml(stdin_filename.as_ref().unwrap_or(&cwd.as_path()))?


Is it intentional that you're only using parse_dot in some paths?

I think I'd change the method to

let Ok(cwd) = std::env::current_dir() else { bail!("Working directory does not exist") }; let cwd = cwd.parse_dot()?;

It was born out of me wrestling with the typechecker, using the previous implementation as reference.
I agree that a more consistent, less verbose usage is preferable.

Is the change in 27a8964 what you had in mind? I'm a bit out of my depth with Cow and the path handling.

That looks right to me. I think we could skip calling parse_dot at all since path_dedot::CWD doesn't:

https://github.com/magiclen/path-dedot/blob/36c197f922d4c8779594ce25b8ffc6cb0315c6a8/src/lib.rs#L329-L330

but I think Micha's point was that we should be consistent one way or the other.

Interesting. I used it because the first line on the path_dedot crate docs say it's purpose is to handle filepaths that contain dots.

But if we only use(d) it as a global variable to access the cwd, I think we can drop it.

Addressed in 98c7711

ntBre

Thanks for following up on this, it's looking good. Just one more nit to avoid as_ref calls and an idea for fixing Windows CI.

I'll defer to Micha for the final approval, but this looks good to me once CI passes.

ntBre · 2025-03-31T20:24:49Z

crates/ruff/src/resolve.rs

-            .unwrap_or(&path_dedot::CWD.as_path()),
-    )? {
+    if let Some(pyproject) =
+        pyproject::find_settings_toml(stdin_filename.as_ref().unwrap_or(&cwd.as_ref()))?


nit: You do need as_ref currently, but stdin_filename is already an Option<&Path>, so you can actually simplify both of these cases (here and below on line 137) to this:

Suggested change

pyproject::find_settings_toml(stdin_filename.as_ref().unwrap_or(&cwd.as_ref()))?

pyproject::find_settings_toml(stdin_filename.unwrap_or(&cwd))?

ntBre · 2025-03-31T20:27:34Z

crates/ruff/tests/direxist_guard.rs

+//! Tests in the same module become flaky under `cargo test`s parallel execution
+//! due to in-test working directory manipulation.
+
+#![cfg(not(target_family = "wasm"))]


I think the Windows CI failure is real. I think Windows will keep the directory around as long as a process is using it. Maybe we need to exclude both wasm and windows targets? I always forget the syntax for this, but I think it's something like this:

Suggested change

#![cfg(not(target_family = "wasm"))]

#![cfg(not(any(target_family = "wasm", target_family = "windows")))]

I'll leave the Windows CI failures to you, if that's fine... I don't have access to a Windows machine.

* main: [red-knot] Add property tests for callable types (#17006) [red-knot] Disjointness for callable types (#17094) [red-knot] Flatten `Type::Callable` into four `Type` variants (#17126) mdtest.py: do a full mdtest run immediately when the script is executed (#17128) [red-knot] Fix callable subtyping for standard parameters (#17125) [red-knot] Fix more `redundant-cast` false positives (#17119) Sync vendored typeshed stubs (#17106) [red-knot] support Any as a class in typeshed (#17107) Visit `Identifier` node as part of the `SourceOrderVisitor` (#17110) [red-knot] Don't infer Todo for quite so many tuple type expressions (#17116) CI: Run pre-commit on depot machine (#17120) Error instead of `panic!` when running Ruff from a deleted directory (#16903) (#17054) Control flow graph: setup (#17064) [red-knot] Playground improvements (#17109) [red-knot] IDE crate (#17045) Update dependency vite to v6.2.4 (#17104) [red-knot] Add redundant-cast error (#17100) [red-knot] Narrowing on `in tuple[...]` and `in str` (#17059)

maxmynter added 3 commits March 28, 2025 18:29

Update stale comment

533b39e

to reflect function behavior.

Test if working directory exists in check

142149f

otherwise ruff panics if the working directory does not exist.

(tests) Handle execution in non-existend pwd

31577db

This test works when executed with nexttest but has interactions with other `resolve_files` tests when executed.

ntBre added the bug Something isn't working label Mar 29, 2025

ntBre reviewed Mar 29, 2025

View reviewed changes

maxmynter added 3 commits March 29, 2025 22:11

(tests, chore) Remove unnecessary directory restoration

eb2f27b

This is already achieved by having the test in a dedicated file.

(refactor) Simplify cwd existence check

8d66d6e

Use bail! and let else since it communicates the intent more clearly.

maxmynter requested a review from ntBre March 30, 2025 03:03

MichaReiser requested changes Mar 30, 2025

View reviewed changes

Use validated CWD

f2a94d4

Instead of path_dedot::CWD where appropriate

MichaReiser reviewed Mar 31, 2025

View reviewed changes

maxmynter added 2 commits March 31, 2025 15:36

Refactor to consistently use resolved cwd

27a8964

Add module level comment to explain need for isolation

fe78408

maxmynter requested a review from MichaReiser March 31, 2025 20:16

ntBre reviewed Mar 31, 2025

View reviewed changes

maxmynter and others added 3 commits March 31, 2025 23:22

Remove parse_dot ivocation

98c7711

Refactor to avoid .as_ref calls

fd7d259

Limit tests to unix

f7cb344

MichaReiser enabled auto-merge (squash) April 1, 2025 07:12

MichaReiser added the cli Related to the command-line interface label Apr 1, 2025

MichaReiser approved these changes Apr 1, 2025

View reviewed changes

MichaReiser merged commit d0c8eaa into astral-sh:main Apr 1, 2025
22 checks passed

BrewTestBot mentioned this pull request Apr 3, 2025

ruff 0.11.3 Homebrew/homebrew-core#218052

Merged

	assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)).arg("check"), @r###"
	assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)).arg("check").current_dir(&temp_path), @r###"

	pyproject::find_settings_toml(stdin_filename.as_ref().unwrap_or(&cwd.as_ref()))?
	pyproject::find_settings_toml(stdin_filename.unwrap_or(&cwd))?

	#![cfg(not(target_family = "wasm"))]
	#![cfg(not(any(target_family = "wasm", target_family = "windows")))]

Error instead of panic! when running Ruff from a deleted directory (#16903) #17054

Error instead of panic! when running Ruff from a deleted directory (#16903) #17054

Uh oh!

Conversation

maxmynter commented Mar 28, 2025

Summary

Test Plan

Open Issues / Questions to Reviewer

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

maxmynter commented Mar 30, 2025

Uh oh!

MichaReiser Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxmynter Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Error instead of `panic!` when running Ruff from a deleted directory (#16903) #17054

Error instead of `panic!` when running Ruff from a deleted directory (#16903) #17054

github-actions bot commented Mar 29, 2025 •

edited

Loading

`ruff-ecosystem` results

MichaReiser Mar 30, 2025 •

edited

Loading

maxmynter Apr 1, 2025 •

edited

Loading