Skip to content

Commit 3220242

Browse files
chirizxcntBre
andauthored
[flake8-use-pathlib] Add autofix for PTH202 (#18763)
<!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? --> ## Summary /closes #2331 <!-- What's the purpose of the change? What does it do, and why? --> ## Test Plan update snapshots <!-- How was it tested? --> --------- Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com>
1 parent 6abafcb commit 3220242

11 files changed

Lines changed: 1306 additions & 61 deletions

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

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,81 @@
22
from pathlib import Path
33
from os.path import getsize
44

5+
filename = "filename"
6+
filename1 = __file__
7+
filename2 = Path("filename")
8+
59

610
os.path.getsize("filename")
711
os.path.getsize(b"filename")
812
os.path.getsize(Path("filename"))
913
os.path.getsize(__file__)
1014

15+
os.path.getsize(filename)
16+
os.path.getsize(filename1)
17+
os.path.getsize(filename2)
18+
19+
os.path.getsize(filename="filename")
20+
os.path.getsize(filename=b"filename")
21+
os.path.getsize(filename=Path("filename"))
22+
os.path.getsize(filename=__file__)
23+
1124
getsize("filename")
1225
getsize(b"filename")
1326
getsize(Path("filename"))
1427
getsize(__file__)
28+
29+
getsize(filename="filename")
30+
getsize(filename=b"filename")
31+
getsize(filename=Path("filename"))
32+
getsize(filename=__file__)
33+
34+
getsize(filename)
35+
getsize(filename1)
36+
getsize(filename2)
37+
38+
39+
os.path.getsize(
40+
"filename", # comment
41+
)
42+
43+
os.path.getsize(
44+
# comment
45+
"filename"
46+
,
47+
# comment
48+
)
49+
50+
os.path.getsize(
51+
# comment
52+
b"filename"
53+
# comment
54+
)
55+
56+
os.path.getsize( # comment
57+
Path(__file__)
58+
# comment
59+
) # comment
60+
61+
getsize( # comment
62+
"filename")
63+
64+
getsize( # comment
65+
b"filename",
66+
#comment
67+
)
68+
69+
os.path.getsize("file" + "name")
70+
71+
getsize \
72+
\
73+
\
74+
( # comment
75+
"filename",
76+
)
77+
78+
getsize(Path("filename").resolve())
79+
80+
import pathlib
81+
82+
os.path.getsize(pathlib.Path("filename"))
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
import os
2+
3+
os.path.getsize(filename="filename")
4+
os.path.getsize(filename=b"filename")
5+
os.path.getsize(filename=__file__)

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1056,7 +1056,6 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
10561056
Rule::OsPathSplitext,
10571057
Rule::BuiltinOpen,
10581058
Rule::PyPath,
1059-
Rule::OsPathGetsize,
10601059
Rule::OsPathGetatime,
10611060
Rule::OsPathGetmtime,
10621061
Rule::OsPathGetctime,
@@ -1066,6 +1065,9 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
10661065
]) {
10671066
flake8_use_pathlib::rules::replaceable_by_pathlib(checker, call);
10681067
}
1068+
if checker.is_rule_enabled(Rule::OsPathGetsize) {
1069+
flake8_use_pathlib::rules::os_path_getsize(checker, call);
1070+
}
10691071
if checker.is_rule_enabled(Rule::PathConstructorCurrentDirectory) {
10701072
flake8_use_pathlib::rules::path_constructor_current_directory(checker, call);
10711073
}

crates/ruff_linter/src/preview.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,11 @@ pub(crate) const fn is_fix_manual_list_comprehension_enabled(settings: &LinterSe
5050
settings.preview.is_enabled()
5151
}
5252

53+
// https://github.com/astral-sh/ruff/pull/18763
54+
pub(crate) const fn is_fix_os_path_getsize_enabled(settings: &LinterSettings) -> bool {
55+
settings.preview.is_enabled()
56+
}
57+
5358
// https://github.com/astral-sh/ruff/pull/11436
5459
// https://github.com/astral-sh/ruff/pull/11168
5560
pub(crate) const fn is_dunder_init_fix_unused_import_enabled(settings: &LinterSettings) -> bool {

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ mod tests {
1212
use crate::assert_diagnostics;
1313
use crate::registry::Rule;
1414
use crate::settings;
15+
use crate::settings::types::PreviewMode;
1516
use crate::test::test_path;
1617

1718
#[test_case(Path::new("full_name.py"))]
@@ -58,6 +59,7 @@ mod tests {
5859
#[test_case(Rule::PyPath, Path::new("py_path_2.py"))]
5960
#[test_case(Rule::PathConstructorCurrentDirectory, Path::new("PTH201.py"))]
6061
#[test_case(Rule::OsPathGetsize, Path::new("PTH202.py"))]
62+
#[test_case(Rule::OsPathGetsize, Path::new("PTH202_2.py"))]
6163
#[test_case(Rule::OsPathGetatime, Path::new("PTH203.py"))]
6264
#[test_case(Rule::OsPathGetmtime, Path::new("PTH204.py"))]
6365
#[test_case(Rule::OsPathGetctime, Path::new("PTH205.py"))]
@@ -76,4 +78,23 @@ mod tests {
7678
assert_diagnostics!(snapshot, diagnostics);
7779
Ok(())
7880
}
81+
82+
#[test_case(Rule::OsPathGetsize, Path::new("PTH202.py"))]
83+
#[test_case(Rule::OsPathGetsize, Path::new("PTH202_2.py"))]
84+
fn preview_flake8_use_pathlib(rule_code: Rule, path: &Path) -> Result<()> {
85+
let snapshot = format!(
86+
"preview__{}_{}",
87+
rule_code.noqa_code(),
88+
path.to_string_lossy()
89+
);
90+
let diagnostics = test_path(
91+
Path::new("flake8_use_pathlib").join(path).as_path(),
92+
&settings::LinterSettings {
93+
preview: PreviewMode::Enabled,
94+
..settings::LinterSettings::for_rule(rule_code)
95+
},
96+
)?;
97+
assert_diagnostics!(snapshot, diagnostics);
98+
Ok(())
99+
}
79100
}

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

Lines changed: 79 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
1+
use crate::checkers::ast::Checker;
2+
use crate::importer::ImportRequest;
3+
use crate::preview::is_fix_os_path_getsize_enabled;
4+
use crate::{Applicability, Edit, Fix, FixAvailability, Violation};
15
use ruff_macros::{ViolationMetadata, derive_message_formats};
2-
3-
use crate::Violation;
6+
use ruff_python_ast::name::QualifiedName;
7+
use ruff_python_ast::{Expr, ExprCall};
8+
use ruff_text_size::Ranged;
49

510
/// ## What it does
611
/// Checks for uses of `os.path.getsize`.
@@ -32,6 +37,9 @@ use crate::Violation;
3237
/// it can be less performant than the lower-level alternatives that work directly with strings,
3338
/// especially on older versions of Python.
3439
///
40+
/// ## Fix Safety
41+
/// This rule's fix is marked as unsafe if the replacement would remove comments attached to the original expression.
42+
///
3543
/// ## References
3644
/// - [Python documentation: `Path.stat`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.stat)
3745
/// - [Python documentation: `os.path.getsize`](https://docs.python.org/3/library/os.path.html#os.path.getsize)
@@ -43,8 +51,77 @@ use crate::Violation;
4351
pub(crate) struct OsPathGetsize;
4452

4553
impl Violation for OsPathGetsize {
54+
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
55+
4656
#[derive_message_formats]
4757
fn message(&self) -> String {
4858
"`os.path.getsize` should be replaced by `Path.stat().st_size`".to_string()
4959
}
60+
61+
fn fix_title(&self) -> Option<String> {
62+
Some("Replace with `Path(...).stat().st_size`".to_string())
63+
}
64+
}
65+
66+
/// PTH202
67+
pub(crate) fn os_path_getsize(checker: &Checker, call: &ExprCall) {
68+
if !matches!(
69+
checker
70+
.semantic()
71+
.resolve_qualified_name(&call.func)
72+
.as_ref()
73+
.map(QualifiedName::segments),
74+
Some(["os", "path", "getsize"])
75+
) {
76+
return;
77+
}
78+
79+
if call.arguments.len() != 1 {
80+
return;
81+
}
82+
83+
let Some(arg) = call.arguments.find_argument_value("filename", 0) else {
84+
return;
85+
};
86+
87+
let arg_code = checker.locator().slice(arg.range());
88+
let range = call.range();
89+
90+
let applicability = if checker.comment_ranges().intersects(range) {
91+
Applicability::Unsafe
92+
} else {
93+
Applicability::Safe
94+
};
95+
96+
let mut diagnostic = checker.report_diagnostic(OsPathGetsize, range);
97+
98+
if is_fix_os_path_getsize_enabled(checker.settings()) {
99+
diagnostic.try_set_fix(|| {
100+
let (import_edit, binding) = checker.importer().get_or_import_symbol(
101+
&ImportRequest::import("pathlib", "Path"),
102+
call.start(),
103+
checker.semantic(),
104+
)?;
105+
106+
let replacement = if is_path_call(checker, arg) {
107+
format!("{arg_code}.stat().st_size")
108+
} else {
109+
format!("{binding}({arg_code}).stat().st_size")
110+
};
111+
112+
Ok(
113+
Fix::safe_edits(Edit::range_replacement(replacement, range), [import_edit])
114+
.with_applicability(applicability),
115+
)
116+
});
117+
}
118+
}
119+
120+
fn is_path_call(checker: &Checker, expr: &Expr) -> bool {
121+
expr.as_call_expr().is_some_and(|expr_call| {
122+
checker
123+
.semantic()
124+
.resolve_qualified_name(&expr_call.func)
125+
.is_some_and(|name| matches!(name.segments(), ["pathlib", "Path"]))
126+
})
50127
}

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use ruff_text_size::Ranged;
55

66
use crate::checkers::ast::Checker;
77
use crate::rules::flake8_use_pathlib::rules::{
8-
Glob, OsPathGetatime, OsPathGetctime, OsPathGetmtime, OsPathGetsize,
8+
Glob, OsPathGetatime, OsPathGetctime, OsPathGetmtime,
99
};
1010
use crate::rules::flake8_use_pathlib::violations::{
1111
BuiltinOpen, Joiner, OsChmod, OsGetcwd, OsListdir, OsMakedirs, OsMkdir, OsPathAbspath,
@@ -194,8 +194,6 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) {
194194
["os", "path", "samefile"] => checker.report_diagnostic_if_enabled(OsPathSamefile, range),
195195
// PTH122
196196
["os", "path", "splitext"] => checker.report_diagnostic_if_enabled(OsPathSplitext, range),
197-
// PTH202
198-
["os", "path", "getsize"] => checker.report_diagnostic_if_enabled(OsPathGetsize, range),
199197
// PTH203
200198
["os", "path", "getatime"] => checker.report_diagnostic_if_enabled(OsPathGetatime, range),
201199
// PTH204

0 commit comments

Comments
 (0)