[flake8-use-pathlib] Fix PTH101, PTH104, PTH105, PTH121 fixes (#20143)

## Summary
Fixes #20134

## Test Plan
`cargo nextest run flake8_use_pathlib`

---------

Co-authored-by: Dan Parizher <danparizher@users.noreply.github.com>
Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>

Author: 3 people

crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/full_name.py

@@ -125,3 +125,15 @@ def bar(x: int):
 os.makedirs(name="name", mode=0o777, exist_ok=False)
 
 os.makedirs("name", unknown_kwarg=True)
+
+# https://github.com/astral-sh/ruff/issues/20134
+os.chmod("pth1_link", mode=0o600, follow_symlinks=      False    )
+os.chmod("pth1_link", mode=0o600, follow_symlinks=True)
+
+# Only diagnostic
+os.chmod("pth1_file", 0o700, None, True, 1, *[1], **{"x": 1}, foo=1)
+
+os.rename("pth1_file", "pth1_file1", None, None, 1, *[1], **{"x": 1}, foo=1)
+os.replace("pth1_file1", "pth1_file", None, None, 1, *[1], **{"x": 1}, foo=1)
+
+os.path.samefile("pth1_file", "pth1_link", 1, *[1], **{"x": 1}, foo=1)

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

@@ -1,12 +1,12 @@
-use ruff_python_ast::{self as ast, Expr, ExprCall};
+use ruff_python_ast::{self as ast, Arguments, Expr, ExprCall};
 use ruff_python_semantic::{SemanticModel, analyze::typing};
 use ruff_text_size::Ranged;
 
 use crate::checkers::ast::Checker;
 use crate::importer::ImportRequest;
 use crate::{Applicability, Edit, Fix, Violation};
 
-pub(crate) fn is_keyword_only_argument_non_default(arguments: &ast::Arguments, name: &str) -> bool {
+pub(crate) fn is_keyword_only_argument_non_default(arguments: &Arguments, name: &str) -> bool {
     arguments
         .find_keyword(name)
         .is_some_and(|keyword| !keyword.value.is_none_literal_expr())
@@ -24,10 +24,7 @@ pub(crate) fn is_pathlib_path_call(checker: &Checker, expr: &Expr) -> bool {
 /// Check if the given segments represent a pathlib Path subclass or `PackagePath` with preview mode support.
 /// In stable mode, only checks for `Path` and `PurePath`. In preview mode, also checks for
 /// `PosixPath`, `PurePosixPath`, `WindowsPath`, `PureWindowsPath`, and `PackagePath`.
-pub(crate) fn is_pure_path_subclass_with_preview(
-    checker: &crate::checkers::ast::Checker,
-    segments: &[&str],
-) -> bool {
+pub(crate) fn is_pure_path_subclass_with_preview(checker: &Checker, segments: &[&str]) -> bool {
     let is_core_pathlib = matches!(segments, ["pathlib", "Path" | "PurePath"]);
 
     if is_core_pathlib {
@@ -193,7 +190,7 @@ pub(crate) fn check_os_pathlib_two_arg_calls(
 }
 
 pub(crate) fn has_unknown_keywords_or_starred_expr(
-    arguments: &ast::Arguments,
+    arguments: &Arguments,
     allowed: &[&str],
 ) -> bool {
     if arguments.args.iter().any(Expr::is_starred_expr) {
@@ -207,11 +204,7 @@ pub(crate) fn has_unknown_keywords_or_starred_expr(
 }
 
 /// Returns `true` if argument `name` is set to a non-default `None` value.
-pub(crate) fn is_argument_non_default(
-    arguments: &ast::Arguments,
-    name: &str,
-    position: usize,
-) -> bool {
+pub(crate) fn is_argument_non_default(arguments: &Arguments, name: &str, position: usize) -> bool {
     arguments
         .find_argument_value(name, position)
         .is_some_and(|expr| !expr.is_none_literal_expr())

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

@@ -1,11 +1,16 @@
+use ruff_diagnostics::{Applicability, Edit, Fix};
+use ruff_macros::{ViolationMetadata, derive_message_formats};
+use ruff_python_ast::{ArgOrKeyword, ExprCall};
+use ruff_text_size::Ranged;
+
 use crate::checkers::ast::Checker;
+use crate::importer::ImportRequest;
 use crate::preview::is_fix_os_chmod_enabled;
 use crate::rules::flake8_use_pathlib::helpers::{
-    check_os_pathlib_two_arg_calls, is_file_descriptor, is_keyword_only_argument_non_default,
+    has_unknown_keywords_or_starred_expr, is_file_descriptor, is_keyword_only_argument_non_default,
+    is_pathlib_path_call,
 };
 use crate::{FixAvailability, Violation};
-use ruff_macros::{ViolationMetadata, derive_message_formats};
-use ruff_python_ast::ExprCall;
 
 /// ## What it does
 /// Checks for uses of `os.chmod`.
@@ -73,22 +78,80 @@ pub(crate) fn os_chmod(checker: &Checker, call: &ExprCall, segments: &[&str]) {
     //           0     1          2               3
     // os.chmod(path, mode, *, dir_fd=None, follow_symlinks=True)
     // ```
-    if call
-        .arguments
-        .find_argument_value("path", 0)
-        .is_some_and(|expr| is_file_descriptor(expr, checker.semantic()))
+    let path_arg = call.arguments.find_argument_value("path", 0);
+
+    if path_arg.is_some_and(|expr| is_file_descriptor(expr, checker.semantic()))
         || is_keyword_only_argument_non_default(&call.arguments, "dir_fd")
     {
         return;
     }
 
-    check_os_pathlib_two_arg_calls(
-        checker,
-        call,
-        "chmod",
-        "path",
-        "mode",
-        is_fix_os_chmod_enabled(checker.settings()),
-        OsChmod,
-    );
+    let range = call.range();
+    let mut diagnostic = checker.report_diagnostic(OsChmod, call.func.range());
+
+    if !is_fix_os_chmod_enabled(checker.settings()) {
+        return;
+    }
+
+    if call.arguments.len() < 2 {
+        return;
+    }
+
+    if has_unknown_keywords_or_starred_expr(
+        &call.arguments,
+        &["path", "mode", "dir_fd", "follow_symlinks"],
+    ) {
+        return;
+    }
+
+    let (Some(path_arg), Some(_)) = (path_arg, call.arguments.find_argument_value("mode", 1))
+    else {
+        return;
+    };
+
+    diagnostic.try_set_fix(|| {
+        let (import_edit, binding) = checker.importer().get_or_import_symbol(
+            &ImportRequest::import("pathlib", "Path"),
+            call.start(),
+            checker.semantic(),
+        )?;
+
+        let locator = checker.locator();
+        let path_code = locator.slice(path_arg.range());
+
+        let args = |arg: ArgOrKeyword| match arg {
+            ArgOrKeyword::Arg(expr) if expr.range() != path_arg.range() => {
+                Some(locator.slice(expr.range()))
+            }
+            ArgOrKeyword::Keyword(kw)
+                if matches!(kw.arg.as_deref(), Some("mode" | "follow_symlinks")) =>
+            {
+                Some(locator.slice(kw.range()))
+            }
+            _ => None,
+        };
+
+        let chmod_args = itertools::join(
+            call.arguments.arguments_source_order().filter_map(args),
+            ", ",
+        );
+
+        let replacement = if is_pathlib_path_call(checker, path_arg) {
+            format!("{path_code}.chmod({chmod_args})")
+        } else {
+            format!("{binding}({path_code}).chmod({chmod_args})")
+        };
+
+        let applicability = if checker.comment_ranges().intersects(range) {
+            Applicability::Unsafe
+        } else {
+            Applicability::Safe
+        };
+
+        Ok(Fix::applicable_edits(
+            Edit::range_replacement(replacement, range),
+            [import_edit],
+            applicability,
+        ))
+    });
 }

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

@@ -1,6 +1,8 @@
 use crate::checkers::ast::Checker;
 use crate::preview::is_fix_os_path_samefile_enabled;
-use crate::rules::flake8_use_pathlib::helpers::check_os_pathlib_two_arg_calls;
+use crate::rules::flake8_use_pathlib::helpers::{
+    check_os_pathlib_two_arg_calls, has_unknown_keywords_or_starred_expr,
+};
 use crate::{FixAvailability, Violation};
 use ruff_macros::{ViolationMetadata, derive_message_formats};
 use ruff_python_ast::ExprCall;
@@ -65,13 +67,16 @@ pub(crate) fn os_path_samefile(checker: &Checker, call: &ExprCall, segments: &[&
         return;
     }
 
+    let fix_enabled = is_fix_os_path_samefile_enabled(checker.settings())
+        && !has_unknown_keywords_or_starred_expr(&call.arguments, &["f1", "f2"]);
+
     check_os_pathlib_two_arg_calls(
         checker,
         call,
         "samefile",
         "f1",
         "f2",
-        is_fix_os_path_samefile_enabled(checker.settings()),
+        fix_enabled,
         OsPathSamefile,
     );
 }

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

@@ -1,7 +1,8 @@
 use crate::checkers::ast::Checker;
 use crate::preview::is_fix_os_rename_enabled;
 use crate::rules::flake8_use_pathlib::helpers::{
-    check_os_pathlib_two_arg_calls, is_keyword_only_argument_non_default,
+    check_os_pathlib_two_arg_calls, has_unknown_keywords_or_starred_expr,
+    is_keyword_only_argument_non_default,
 };
 use crate::{FixAvailability, Violation};
 use ruff_macros::{ViolationMetadata, derive_message_formats};
@@ -79,13 +80,11 @@ pub(crate) fn os_rename(checker: &Checker, call: &ExprCall, segments: &[&str]) {
         return;
     }
 
-    check_os_pathlib_two_arg_calls(
-        checker,
-        call,
-        "rename",
-        "src",
-        "dst",
-        is_fix_os_rename_enabled(checker.settings()),
-        OsRename,
-    );
+    let fix_enabled = is_fix_os_rename_enabled(checker.settings())
+        && !has_unknown_keywords_or_starred_expr(
+            &call.arguments,
+            &["src", "dst", "src_dir_fd", "dst_dir_fd"],
+        );
+
+    check_os_pathlib_two_arg_calls(checker, call, "rename", "src", "dst", fix_enabled, OsRename);
 }

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

@@ -1,7 +1,8 @@
 use crate::checkers::ast::Checker;
 use crate::preview::is_fix_os_replace_enabled;
 use crate::rules::flake8_use_pathlib::helpers::{
-    check_os_pathlib_two_arg_calls, is_keyword_only_argument_non_default,
+    check_os_pathlib_two_arg_calls, has_unknown_keywords_or_starred_expr,
+    is_keyword_only_argument_non_default,
 };
 use crate::{FixAvailability, Violation};
 use ruff_macros::{ViolationMetadata, derive_message_formats};
@@ -82,13 +83,19 @@ pub(crate) fn os_replace(checker: &Checker, call: &ExprCall, segments: &[&str])
         return;
     }
 
+    let fix_enabled = is_fix_os_replace_enabled(checker.settings())
+        && !has_unknown_keywords_or_starred_expr(
+            &call.arguments,
+            &["src", "dst", "src_dir_fd", "dst_dir_fd"],
+        );
+
     check_os_pathlib_two_arg_calls(
         checker,
         call,
         "replace",
         "src",
         "dst",
-        is_fix_os_replace_enabled(checker.settings()),
+        fix_enabled,
         OsReplace,
     );
 }

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

@@ -104,14 +104,6 @@ pub(crate) fn os_symlink(checker: &Checker, call: &ExprCall, segments: &[&str])
         return;
     };
 
-    let target_is_directory_arg = call.arguments.find_argument_value("target_is_directory", 2);
-
-    if let Some(expr) = &target_is_directory_arg {
-        if expr.as_boolean_literal_expr().is_none() {
-            return;
-        }
-    }
-
     diagnostic.try_set_fix(|| {
         let (import_edit, binding) = checker.importer().get_or_import_symbol(
             &ImportRequest::import("pathlib", "Path"),
@@ -129,7 +121,9 @@ pub(crate) fn os_symlink(checker: &Checker, call: &ExprCall, segments: &[&str])
         let src_code = locator.slice(src.range());
         let dst_code = locator.slice(dst.range());
 
-        let target_is_directory = target_is_directory_arg
+        let target_is_directory = call
+            .arguments
+            .find_argument_value("target_is_directory", 2)
             .and_then(|expr| {
                 let code = locator.slice(expr.range());
                 expr.as_boolean_literal_expr()

crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__full_name.py.snap

@@ -500,5 +500,72 @@ PTH103 `os.makedirs()` should be replaced by `Path.mkdir(parents=True)`
 126 |
 127 | os.makedirs("name", unknown_kwarg=True)
     | ^^^^^^^^^^^
+128 |
+129 | # https://github.com/astral-sh/ruff/issues/20134
     |
 help: Replace with `Path(...).mkdir(parents=True)`
+
+PTH101 `os.chmod()` should be replaced by `Path.chmod()`
+   --> full_name.py:130:1
+    |
+129 | # https://github.com/astral-sh/ruff/issues/20134
+130 | os.chmod("pth1_link", mode=0o600, follow_symlinks=      False    )
+    | ^^^^^^^^
+131 | os.chmod("pth1_link", mode=0o600, follow_symlinks=True)
+    |
+help: Replace with `Path(...).chmod(...)`
+
+PTH101 `os.chmod()` should be replaced by `Path.chmod()`
+   --> full_name.py:131:1
+    |
+129 | # https://github.com/astral-sh/ruff/issues/20134
+130 | os.chmod("pth1_link", mode=0o600, follow_symlinks=      False    )
+131 | os.chmod("pth1_link", mode=0o600, follow_symlinks=True)
+    | ^^^^^^^^
+132 |
+133 | # Only diagnostic
+    |
+help: Replace with `Path(...).chmod(...)`
+
+PTH101 `os.chmod()` should be replaced by `Path.chmod()`
+   --> full_name.py:134:1
+    |
+133 | # Only diagnostic
+134 | os.chmod("pth1_file", 0o700, None, True, 1, *[1], **{"x": 1}, foo=1)
+    | ^^^^^^^^
+135 |
+136 | os.rename("pth1_file", "pth1_file1", None, None, 1, *[1], **{"x": 1}, foo=1)
+    |
+help: Replace with `Path(...).chmod(...)`
+
+PTH104 `os.rename()` should be replaced by `Path.rename()`
+   --> full_name.py:136:1
+    |
+134 | os.chmod("pth1_file", 0o700, None, True, 1, *[1], **{"x": 1}, foo=1)
+135 |
+136 | os.rename("pth1_file", "pth1_file1", None, None, 1, *[1], **{"x": 1}, foo=1)
+    | ^^^^^^^^^
+137 | os.replace("pth1_file1", "pth1_file", None, None, 1, *[1], **{"x": 1}, foo=1)
+    |
+help: Replace with `Path(...).rename(...)`
+
+PTH105 `os.replace()` should be replaced by `Path.replace()`
+   --> full_name.py:137:1
+    |
+136 | os.rename("pth1_file", "pth1_file1", None, None, 1, *[1], **{"x": 1}, foo=1)
+137 | os.replace("pth1_file1", "pth1_file", None, None, 1, *[1], **{"x": 1}, foo=1)
+    | ^^^^^^^^^^
+138 |
+139 | os.path.samefile("pth1_file", "pth1_link", 1, *[1], **{"x": 1}, foo=1)
+    |
+help: Replace with `Path(...).replace(...)`
+
+PTH121 `os.path.samefile()` should be replaced by `Path.samefile()`
+   --> full_name.py:139:1
+    |
+137 | os.replace("pth1_file1", "pth1_file", None, None, 1, *[1], **{"x": 1}, foo=1)
+138 |
+139 | os.path.samefile("pth1_file", "pth1_link", 1, *[1], **{"x": 1}, foo=1)
+    | ^^^^^^^^^^^^^^^^
+    |
+help: Replace with `Path(...).samefile()`