[`flake8-use-pathlib`] Mark fixes unsafe for return type changes (`PTH104`, `PTH105`, `PTH109`, `PTH115`) by danparizher · Pull Request #21440 · astral-sh/ruff

danparizher · 2025-11-13T22:20:16Z

Summary

Marks fixes as unsafe when they change return types (None → Path, str/bytes → Path, str → Path), except when the call is a top-level expression.

Fixes #21431.

Problem

Fixes for os.rename, os.replace, os.getcwd/os.getcwdb, and os.readlink were marked safe despite changing return types, which can break code that uses the return value.

Approach

Added is_top_level_expression_call helper to detect when a call is a top-level expression (return value unused). Updated check_os_pathlib_two_arg_calls and check_os_pathlib_single_arg_calls to mark fixes as unsafe unless the call is a top-level expression. Updated PTH109 to use the helper for applicability determination.

Test Plan

Updated snapshots for preview_full_name.py, preview_import_as.py, preview_import_from.py, and preview_import_from_as.py to reflect unsafe markers.

astral-sh-bot · 2025-11-13T22:31:09Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -0 violations, +0 -38 fixes in 3 projects; 52 projects unchanged)

apache/airflow (+0 -0 violations, +0 -22 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL

- airflow-core/src/airflow/cli/cli_config.py:179:40: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ airflow-core/src/airflow/cli/cli_config.py:179:40: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- airflow-core/tests/unit/utils/log/test_file_processor_handler.py:44:33: PTH115 [*] `os.readlink()` should be replaced by `Path.readlink()`
+ airflow-core/tests/unit/utils/log/test_file_processor_handler.py:44:33: PTH115 `os.readlink()` should be replaced by `Path.readlink()`
- airflow-core/tests/unit/utils/log/test_file_processor_handler.py:58:33: PTH115 [*] `os.readlink()` should be replaced by `Path.readlink()`
+ airflow-core/tests/unit/utils/log/test_file_processor_handler.py:58:33: PTH115 `os.readlink()` should be replaced by `Path.readlink()`
- airflow-core/tests/unit/utils/test_cli_util.py:189:38: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ airflow-core/tests/unit/utils/test_cli_util.py:189:38: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- airflow-core/tests/unit/utils/test_cli_util.py:194:37: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ airflow-core/tests/unit/utils/test_cli_util.py:194:37: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- airflow-ctl-tests/tests/airflowctl_tests/conftest.py:114:47: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ airflow-ctl-tests/tests/airflowctl_tests/conftest.py:114:47: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- dev/breeze/src/airflow_breeze/utils/docs_publisher.py:87:61: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ dev/breeze/src/airflow_breeze/utils/docs_publisher.py:87:61: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- dev/breeze/src/airflow_breeze/utils/run_utils.py:132:41: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ dev/breeze/src/airflow_breeze/utils/run_utils.py:132:41: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- providers/elasticsearch/tests/unit/elasticsearch/log/test_es_task_handler.py:893:48: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ providers/elasticsearch/tests/unit/elasticsearch/log/test_es_task_handler.py:893:48: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- scripts/ci/prek/common_prek_utils.py:85:29: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
... 4 additional changes omitted for rule PTH109

bokeh/bokeh (+0 -0 violations, +0 -14 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL

- release/system.py:57:50: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ release/system.py:57:50: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- release/system.py:61:34: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ release/system.py:61:34: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- src/bokeh/io/export.py:527:76: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ src/bokeh/io/export.py:527:76: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- src/bokeh/io/util.py:78:36: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ src/bokeh/io/util.py:78:36: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- src/bokeh/sphinxext/util.py:48:22: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ src/bokeh/sphinxext/util.py:48:22: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- tests/unit/bokeh/application/handlers/test_code_runner.py:159:16: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ tests/unit/bokeh/application/handlers/test_code_runner.py:159:16: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`
- tests/unit/bokeh/util/test_browser.py:98:63: PTH109 [*] `os.getcwd()` should be replaced by `Path.cwd()`
+ tests/unit/bokeh/util/test_browser.py:98:63: PTH109 `os.getcwd()` should be replaced by `Path.cwd()`

zulip/zulip (+0 -0 violations, +0 -2 fixes)

ruff check --no-cache --exit-zero --no-fix --output-format concise --preview --select ALL

- scripts/lib/clean_emoji_cache.py:37:43: PTH115 [*] `os.readlink()` should be replaced by `Path.readlink()`
+ scripts/lib/clean_emoji_cache.py:37:43: PTH115 `os.readlink()` should be replaced by `Path.readlink()`

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PTH109	32	0	0	0	32
PTH115	6	0	0	0	6

ntBre

I made a couple of suggestions for simplification, but we also need to verify that this change is only applied to the correct rules. I flagged a few cases inline where rules not mentioned in #21431 were changed unnecessarily, but I didn't check them exhaustively.

ntBre · 2025-11-14T19:02:00Z

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_getcwd.rs

            let applicability = if checker.comment_ranges().intersects(range) {
                Applicability::Unsafe
-            } else {
+            } else if is_top_level_expression_call(checker, call) {
+                // Safe when the call is a top-level expression (return value not used)
                Applicability::Safe
+            } else {
+                // Unsafe because the return type changes (str/bytes -> Path)
+                Applicability::Unsafe
            };


I think I would slightly prefer something like:

Suggested change

let applicability = if checker.comment_ranges().intersects(range) {

Applicability::Unsafe

} else {

} else if is_top_level_expression_call(checker, call) {

// Safe when the call is a top-level expression (return value not used)

Applicability::Safe

} else {

// Unsafe because the return type changes (str/bytes -> Path)

Applicability::Unsafe

};

// Unsafe when the fix would delete comments or change a used return value

let applicability = if checker.comment_ranges().intersects(range)

|| !is_top_level_expression_call(checker, call) {

Applicability::Unsafe

} else {

Applicability::Safe

};

ntBre · 2025-11-14T19:04:35Z

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_getcwd.rs

+/// Additionally, the fix is marked as unsafe because `os.getcwd()` and `os.getcwdb()` return `str` or `bytes`,
+/// while `Path.cwd()` returns a `Path` object. This change in return type can break code that uses the return value.
+/// The fix is safe when the function call is a top-level expression in its statement (i.e., the return value is not used).


I think we could simplify this slightly to something like:

Suggested change

/// Additionally, the fix is marked as unsafe because `os.getcwd()` and `os.getcwdb()` return `str` or `bytes`,

/// while `Path.cwd()` returns a `Path` object. This change in return type can break code that uses the return value.

/// The fix is safe when the function call is a top-level expression in its statement (i.e., the return value is not used).

/// Additionally, the fix is marked as unsafe when the return value is used because the type changes

/// from `str` or `bytes` to a `Path` object.

I think something similar would work for the other documentation updates too.

ntBre · 2025-11-14T19:07:37Z

...e_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview_full_name.py.snap

@@ -315,6 +317,7 @@ help: Replace with `Path(...).is_file()`
 22 | bbbbb = os.path.islink(p)
 23 | os.readlink(p)
 24 | os.stat(p)
+note: This is an unsafe fix and may change runtime behavior


This doesn't look right. Should isfile be affected by these changes? I think both versions return a bool.

ntBre · 2025-11-14T19:08:07Z

...e_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview_full_name.py.snap

 23 | os.readlink(p)
 24 | os.stat(p)
 25 | os.path.isabs(p)
+note: This is an unsafe fix and may change runtime behavior


I think this one also shouldn't change.

ntBre · 2025-11-14T19:08:44Z

...e_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__preview_import_as.py.snap

 19 | bb = foo_p.expanduser(p)
 20 | bbb = foo_p.isdir(p)
 21 | bbbb = foo_p.isfile(p)
+note: This is an unsafe fix and may change runtime behavior


Another one that shouldn't change, from what I can tell.

ntBre · 2025-11-14T19:11:59Z

crates/ruff_linter/src/rules/flake8_use_pathlib/helpers.rs

+    if let Stmt::Expr(ast::StmtExpr {
+        value: child,
+        range: _,
+        node_index: _,
+    }) = checker.semantic().current_statement()
+    {
+        // Check if the call is the same expression as the statement's value
+        // We compare by checking if the call's range is contained within the child's range
+        // and if they're the same expression node
+        if let Expr::Call(call_expr) = child.as_ref() {
+            return call_expr.range() == call.range();
+        }
+    }
+    false


Assuming that call is the SemanticModel::current_expression, I think this function could just be:

Suggested change

if let Stmt::Expr(ast::StmtExpr {

value: child,

range: _,

node_index: _,

}) = checker.semantic().current_statement()

{

// Check if the call is the same expression as the statement's value

// We compare by checking if the call's range is contained within the child's range

// and if they're the same expression node

if let Expr::Call(call_expr) = child.as_ref() {

return call_expr.range() == call.range();

}

}

false

checker.semantic().current_expression_parent().is_none()

danparizher · 2025-11-14T20:59:45Z

Thanks for the feedback! I had thought that the same concepts applied to a few other rules, but it appears not after taking a closer look.

ntBre

Thanks! This is looking better, but I had a few more suggestions.

I tried to flag all of the places the docs should say "str or bytes," but I started using typing.AnyStr as a shorthand since that's what ty shows. It's technically a type variable that depends on the input, but I think it's fine just to say "str or bytes" in each of those cases.

ntBre · 2025-11-18T21:51:11Z

crates/ruff_linter/src/rules/flake8_use_pathlib/helpers.rs

+                let determined_applicability = if checker.comment_ranges().intersects(range) {
                    Applicability::Unsafe
+                } else if applicability.is_none() {
+                    // When applicability is None, we need to determine it based on return type changes
+                    if is_top_level_expression_call(checker, call) {
+                        // Safe when the call is a top-level expression (return value not used)
+                        Applicability::Safe
+                    } else {
+                        // Unsafe because the return type changes (str/bytes -> Path or None -> Path)
+                        Applicability::Unsafe
+                    }


I don't think we should do this check here. I checked all of the references for check_os_pathlib_single_arg_calls, and it's only used by PTH115, of the rules we're trying to fix in this PR.

I think we should just do the is_top_level_expression_call check in PTH115 itself.

ntBre · 2025-11-18T21:52:29Z

crates/ruff_linter/src/rules/flake8_use_pathlib/helpers.rs

+            } else if is_top_level_expression_call(checker, call) {
+                // Safe when the call is a top-level expression (return value not used)
                Applicability::Safe
+            } else {
+                // Unsafe because the return type changes (None -> Path)
+                Applicability::Unsafe


As above, this helper also affects rules outside of PTH{104,105,109,115}. We should narrow the check to the affected rules and pass that applicability into the helper.

Just a note on this - to have applicability passed through the helper, I had to add #[allow(clippy::too_many_arguments)].

Let me know if that's fine or if you'd rather have it refactored.

crates/ruff_linter/src/rules/flake8_use_pathlib/helpers.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_readlink.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_dirname.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_abspath.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_getcwd.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_expanduser.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_isfile.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_islink.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/helpers.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_exists.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_readlink.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_replace.rs

crates/ruff_linter/src/rules/flake8_use_pathlib/helpers.rs

ntBre

Thanks! I just fixed one more round of tiny nits. Once the ecosystem check runs, I'll merge.

* origin/main: (67 commits) Move `Token`, `TokenKind` and `Tokens` to `ruff-python-ast` (#21760) [ty] Don't confuse multiple occurrences of `typing.Self` when binding bound methods (#21754) Use our org-wide Renovate preset (#21759) Delete `my-script.py` (#21751) [ty] Move `all_members`, and related types/routines, out of `ide_support.rs` (#21695) [ty] Fix find-references for import aliases (#21736) [ty] add tests for workspaces (#21741) [ty] Stop testing the (brittle) constraint set display implementation (#21743) [ty] Use generator over list comprehension to avoid cast (#21748) [ty] Add a diagnostic for prohibited `NamedTuple` attribute overrides (#21717) [ty] Fix subtyping with `type[T]` and unions (#21740) Use `npm ci --ignore-scripts` everywhere (#21742) [`flake8-simplify`] Fix truthiness assumption for non-iterable arguments in tuple/list/set calls (`SIM222`, `SIM223`) (#21479) [`flake8-use-pathlib`] Mark fixes unsafe for return type changes (`PTH104`, `PTH105`, `PTH109`, `PTH115`) (#21440) [ty] Fix auto-import code action to handle pre-existing import Enable PEP 740 attestations when publishing to PyPI (#21735) [ty] Fix find references for type defined in stub (#21732) Use OIDC instead of codspeed token (#21719) [ty] Exclude `typing_extensions` from completions unless it's really available [ty] Fix false positives for `class F(Generic[*Ts]): ...` (#21723) ...

fix-21431

bd82aa2

ntBre added fixes Related to suggested fixes for violations preview Related to preview mode features labels Nov 14, 2025

ntBre requested changes Nov 14, 2025

View reviewed changes

Refactor, clarify documentation

4e31a60

danparizher requested a review from ntBre November 14, 2025 20:59

ntBre requested changes Nov 18, 2025

View reviewed changes

Refactor applicability handling

d258167

danparizher requested a review from ntBre November 20, 2025 00:24

ntBre requested changes Nov 21, 2025

View reviewed changes

Apply feedback

28aa109

danparizher requested a review from ntBre November 29, 2025 01:18

nits

6ca60ab

ntBre approved these changes Dec 1, 2025

View reviewed changes

ntBre merged commit bc44dc2 into astral-sh:main Dec 1, 2025
61 of 62 checks passed

danparizher deleted the fix-21431 branch December 1, 2025 22:36

BrewTestBot mentioned this pull request Dec 4, 2025

ruff 0.14.8 Homebrew/homebrew-core#257115

Merged

Conversation

danparizher commented Nov 13, 2025

Summary

Problem

Approach

Test Plan

Uh oh!

astral-sh-bot bot commented Nov 13, 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!

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!

Choose a reason for hiding this comment

Uh oh!

danparizher commented Nov 14, 2025

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

astral-sh-bot bot commented Nov 13, 2025 •

edited

Loading

`ruff-ecosystem` results