Skip to content

Commit 89258f1

Browse files
authored
[flake8-use-pathlib] Add autofix for PTH101, PTH104, PTH105, PTH121 (#19404)
## Summary Part of #2331 ## Test Plan `cargo nextest run flake8_use_pathlib`
1 parent 1dcef1a commit 89258f1

20 files changed

+666
-283
lines changed

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,3 +47,19 @@ def _():
4747
from builtin import open
4848

4949
with open(p) as _: ... # No error
50+
51+
file = "file_1.py"
52+
53+
rename(file, "file_2.py")
54+
55+
rename(
56+
# commment 1
57+
file, # comment 2
58+
"file_2.py"
59+
,
60+
# comment 3
61+
)
62+
63+
rename(file, "file_2.py", src_dir_fd=None, dst_dir_fd=None)
64+
65+
rename(file, "file_2.py", src_dir_fd=1)

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1039,14 +1039,10 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
10391039
flake8_simplify::rules::zip_dict_keys_and_values(checker, call);
10401040
}
10411041
if checker.any_rule_enabled(&[
1042-
Rule::OsChmod,
10431042
Rule::OsMkdir,
10441043
Rule::OsMakedirs,
1045-
Rule::OsRename,
1046-
Rule::OsReplace,
10471044
Rule::OsStat,
10481045
Rule::OsPathJoin,
1049-
Rule::OsPathSamefile,
10501046
Rule::OsPathSplitext,
10511047
Rule::BuiltinOpen,
10521048
Rule::PyPath,
@@ -1112,6 +1108,18 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
11121108
if checker.is_rule_enabled(Rule::OsGetcwd) {
11131109
flake8_use_pathlib::rules::os_getcwd(checker, call, segments);
11141110
}
1111+
if checker.is_rule_enabled(Rule::OsChmod) {
1112+
flake8_use_pathlib::rules::os_chmod(checker, call, segments);
1113+
}
1114+
if checker.is_rule_enabled(Rule::OsRename) {
1115+
flake8_use_pathlib::rules::os_rename(checker, call, segments);
1116+
}
1117+
if checker.is_rule_enabled(Rule::OsReplace) {
1118+
flake8_use_pathlib::rules::os_replace(checker, call, segments);
1119+
}
1120+
if checker.is_rule_enabled(Rule::OsPathSamefile) {
1121+
flake8_use_pathlib::rules::os_path_samefile(checker, call, segments);
1122+
}
11151123
if checker.is_rule_enabled(Rule::PathConstructorCurrentDirectory) {
11161124
flake8_use_pathlib::rules::path_constructor_current_directory(
11171125
checker, call, segments,

crates/ruff_linter/src/codes.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -920,11 +920,11 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
920920

921921
// flake8-use-pathlib
922922
(Flake8UsePathlib, "100") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsPathAbspath),
923-
(Flake8UsePathlib, "101") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::OsChmod),
923+
(Flake8UsePathlib, "101") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsChmod),
924924
(Flake8UsePathlib, "102") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::OsMkdir),
925925
(Flake8UsePathlib, "103") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::OsMakedirs),
926-
(Flake8UsePathlib, "104") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::OsRename),
927-
(Flake8UsePathlib, "105") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::OsReplace),
926+
(Flake8UsePathlib, "104") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsRename),
927+
(Flake8UsePathlib, "105") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsReplace),
928928
(Flake8UsePathlib, "106") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsRmdir),
929929
(Flake8UsePathlib, "107") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsRemove),
930930
(Flake8UsePathlib, "108") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsUnlink),
@@ -940,7 +940,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
940940
(Flake8UsePathlib, "118") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::OsPathJoin),
941941
(Flake8UsePathlib, "119") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsPathBasename),
942942
(Flake8UsePathlib, "120") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsPathDirname),
943-
(Flake8UsePathlib, "121") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::OsPathSamefile),
943+
(Flake8UsePathlib, "121") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsPathSamefile),
944944
(Flake8UsePathlib, "122") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::OsPathSplitext),
945945
(Flake8UsePathlib, "123") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::BuiltinOpen),
946946
(Flake8UsePathlib, "124") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::PyPath),

crates/ruff_linter/src/preview.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,26 @@ pub(crate) const fn is_fix_os_path_dirname_enabled(settings: &LinterSettings) ->
134134
settings.preview.is_enabled()
135135
}
136136

137+
// https://github.com/astral-sh/ruff/pull/19404
138+
pub(crate) const fn is_fix_os_chmod_enabled(settings: &LinterSettings) -> bool {
139+
settings.preview.is_enabled()
140+
}
141+
142+
// https://github.com/astral-sh/ruff/pull/19404
143+
pub(crate) const fn is_fix_os_rename_enabled(settings: &LinterSettings) -> bool {
144+
settings.preview.is_enabled()
145+
}
146+
147+
// https://github.com/astral-sh/ruff/pull/19404
148+
pub(crate) const fn is_fix_os_replace_enabled(settings: &LinterSettings) -> bool {
149+
settings.preview.is_enabled()
150+
}
151+
152+
// https://github.com/astral-sh/ruff/pull/19404
153+
pub(crate) const fn is_fix_os_path_samefile_enabled(settings: &LinterSettings) -> bool {
154+
settings.preview.is_enabled()
155+
}
156+
137157
// https://github.com/astral-sh/ruff/pull/19245
138158
pub(crate) const fn is_fix_os_getcwd_enabled(settings: &LinterSettings) -> bool {
139159
settings.preview.is_enabled()

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

Lines changed: 84 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
use crate::checkers::ast::Checker;
22
use crate::importer::ImportRequest;
33
use crate::{Applicability, Edit, Fix, Violation};
4-
use ruff_python_ast::{self as ast};
5-
use ruff_python_ast::{Expr, ExprCall};
4+
use ruff_python_ast::{self as ast, Expr, ExprCall};
5+
use ruff_python_semantic::{SemanticModel, analyze::typing};
66
use ruff_text_size::Ranged;
77

88
pub(crate) fn is_keyword_only_argument_non_default(arguments: &ast::Arguments, name: &str) -> bool {
@@ -72,3 +72,85 @@ pub(crate) fn check_os_pathlib_single_arg_calls(
7272
});
7373
}
7474
}
75+
76+
pub(crate) fn get_name_expr(expr: &Expr) -> Option<&ast::ExprName> {
77+
match expr {
78+
Expr::Name(name) => Some(name),
79+
Expr::Call(ExprCall { func, .. }) => get_name_expr(func),
80+
_ => None,
81+
}
82+
}
83+
84+
/// Returns `true` if the given expression looks like a file descriptor, i.e., if it is an integer.
85+
pub(crate) fn is_file_descriptor(expr: &Expr, semantic: &SemanticModel) -> bool {
86+
if matches!(
87+
expr,
88+
Expr::NumberLiteral(ast::ExprNumberLiteral {
89+
value: ast::Number::Int(_),
90+
..
91+
})
92+
) {
93+
return true;
94+
}
95+
96+
let Some(name) = get_name_expr(expr) else {
97+
return false;
98+
};
99+
100+
let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else {
101+
return false;
102+
};
103+
104+
typing::is_int(binding, semantic)
105+
}
106+
107+
pub(crate) fn check_os_pathlib_two_arg_calls(
108+
checker: &Checker,
109+
call: &ExprCall,
110+
attr: &str,
111+
path_arg: &str,
112+
second_arg: &str,
113+
fix_enabled: bool,
114+
violation: impl Violation,
115+
) {
116+
let range = call.range();
117+
let mut diagnostic = checker.report_diagnostic(violation, call.func.range());
118+
119+
let (Some(path_expr), Some(second_expr)) = (
120+
call.arguments.find_argument_value(path_arg, 0),
121+
call.arguments.find_argument_value(second_arg, 1),
122+
) else {
123+
return;
124+
};
125+
126+
let path_code = checker.locator().slice(path_expr.range());
127+
let second_code = checker.locator().slice(second_expr.range());
128+
129+
if fix_enabled {
130+
diagnostic.try_set_fix(|| {
131+
let (import_edit, binding) = checker.importer().get_or_import_symbol(
132+
&ImportRequest::import("pathlib", "Path"),
133+
call.start(),
134+
checker.semantic(),
135+
)?;
136+
137+
let replacement = if is_pathlib_path_call(checker, path_expr) {
138+
format!("{path_code}.{attr}({second_code})")
139+
} else {
140+
format!("{binding}({path_code}).{attr}({second_code})")
141+
};
142+
143+
let applicability = if checker.comment_ranges().intersects(range) {
144+
Applicability::Unsafe
145+
} else {
146+
Applicability::Safe
147+
};
148+
149+
Ok(Fix::applicable_edits(
150+
Edit::range_replacement(replacement, range),
151+
[import_edit],
152+
applicability,
153+
))
154+
});
155+
}
156+
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
pub(crate) use glob_rule::*;
22
pub(crate) use invalid_pathlib_with_suffix::*;
3+
pub(crate) use os_chmod::*;
34
pub(crate) use os_getcwd::*;
45
pub(crate) use os_path_abspath::*;
56
pub(crate) use os_path_basename::*;
@@ -14,8 +15,11 @@ pub(crate) use os_path_isabs::*;
1415
pub(crate) use os_path_isdir::*;
1516
pub(crate) use os_path_isfile::*;
1617
pub(crate) use os_path_islink::*;
18+
pub(crate) use os_path_samefile::*;
1719
pub(crate) use os_readlink::*;
1820
pub(crate) use os_remove::*;
21+
pub(crate) use os_rename::*;
22+
pub(crate) use os_replace::*;
1923
pub(crate) use os_rmdir::*;
2024
pub(crate) use os_sep_split::*;
2125
pub(crate) use os_unlink::*;
@@ -24,6 +28,7 @@ pub(crate) use replaceable_by_pathlib::*;
2428

2529
mod glob_rule;
2630
mod invalid_pathlib_with_suffix;
31+
mod os_chmod;
2732
mod os_getcwd;
2833
mod os_path_abspath;
2934
mod os_path_basename;
@@ -38,8 +43,11 @@ mod os_path_isabs;
3843
mod os_path_isdir;
3944
mod os_path_isfile;
4045
mod os_path_islink;
46+
mod os_path_samefile;
4147
mod os_readlink;
4248
mod os_remove;
49+
mod os_rename;
50+
mod os_replace;
4351
mod os_rmdir;
4452
mod os_sep_split;
4553
mod os_unlink;
Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
use crate::checkers::ast::Checker;
2+
use crate::preview::is_fix_os_chmod_enabled;
3+
use crate::rules::flake8_use_pathlib::helpers::{
4+
check_os_pathlib_two_arg_calls, is_file_descriptor, is_keyword_only_argument_non_default,
5+
};
6+
use crate::{FixAvailability, Violation};
7+
use ruff_macros::{ViolationMetadata, derive_message_formats};
8+
use ruff_python_ast::ExprCall;
9+
10+
/// ## What it does
11+
/// Checks for uses of `os.chmod`.
12+
///
13+
/// ## Why is this bad?
14+
/// `pathlib` offers a high-level API for path manipulation, as compared to
15+
/// the lower-level API offered by `os`. When possible, using `Path` object
16+
/// methods such as `Path.chmod()` can improve readability over the `os`
17+
/// module's counterparts (e.g., `os.chmod()`).
18+
///
19+
/// ## Examples
20+
/// ```python
21+
/// import os
22+
///
23+
/// os.chmod("file.py", 0o444)
24+
/// ```
25+
///
26+
/// Use instead:
27+
/// ```python
28+
/// from pathlib import Path
29+
///
30+
/// Path("file.py").chmod(0o444)
31+
/// ```
32+
///
33+
/// ## Known issues
34+
/// While using `pathlib` can improve the readability and type safety of your code,
35+
/// it can be less performant than the lower-level alternatives that work directly with strings,
36+
/// especially on older versions of Python.
37+
///
38+
/// ## Fix Safety
39+
/// This rule's fix is marked as unsafe if the replacement would remove comments attached to the original expression.
40+
///
41+
/// ## References
42+
/// - [Python documentation: `Path.chmod`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.chmod)
43+
/// - [Python documentation: `os.chmod`](https://docs.python.org/3/library/os.html#os.chmod)
44+
/// - [PEP 428 – The pathlib module – object-oriented filesystem paths](https://peps.python.org/pep-0428/)
45+
/// - [Correspondence between `os` and `pathlib`](https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module)
46+
/// - [Why you should be using pathlib](https://treyhunner.com/2018/12/why-you-should-be-using-pathlib/)
47+
/// - [No really, pathlib is great](https://treyhunner.com/2019/01/no-really-pathlib-is-great/)
48+
#[derive(ViolationMetadata)]
49+
pub(crate) struct OsChmod;
50+
51+
impl Violation for OsChmod {
52+
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
53+
54+
#[derive_message_formats]
55+
fn message(&self) -> String {
56+
"`os.chmod()` should be replaced by `Path.chmod()`".to_string()
57+
}
58+
59+
fn fix_title(&self) -> Option<String> {
60+
Some("Replace with `Path(...).chmod(...)`".to_string())
61+
}
62+
}
63+
64+
/// PTH101
65+
pub(crate) fn os_chmod(checker: &Checker, call: &ExprCall, segments: &[&str]) {
66+
if segments != ["os", "chmod"] {
67+
return;
68+
}
69+
70+
// `dir_fd` is not supported by pathlib, so check if it's set to non-default values.
71+
// Signature as of Python 3.13 (https://docs.python.org/3/library/os.html#os.chmod)
72+
// ```text
73+
// 0 1 2 3
74+
// os.chmod(path, mode, *, dir_fd=None, follow_symlinks=True)
75+
// ```
76+
if call
77+
.arguments
78+
.find_argument_value("path", 0)
79+
.is_some_and(|expr| is_file_descriptor(expr, checker.semantic()))
80+
|| is_keyword_only_argument_non_default(&call.arguments, "dir_fd")
81+
{
82+
return;
83+
}
84+
85+
check_os_pathlib_two_arg_calls(
86+
checker,
87+
call,
88+
"chmod",
89+
"path",
90+
"mode",
91+
is_fix_os_chmod_enabled(checker.settings()),
92+
OsChmod,
93+
);
94+
}

0 commit comments

Comments
 (0)