diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH211.py b/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH211.py new file mode 100644 index 0000000000..5acf2febe2 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_use_pathlib/PTH211.py @@ -0,0 +1,15 @@ +import os +from pathlib import Path + + +os.symlink("usr/bin/python", "tmp/python") +os.symlink(b"usr/bin/python", b"tmp/python") +Path("tmp/python").symlink_to("usr/bin/python") # Ok + +os.symlink("usr/bin/python", "tmp/python", target_is_directory=True) +os.symlink(b"usr/bin/python", b"tmp/python", target_is_directory=True) +Path("tmp/python").symlink_to("usr/bin/python", target_is_directory=True) # Ok + +fd = os.open(".", os.O_RDONLY) +os.symlink("source.txt", "link.txt", dir_fd=fd) # Ok: dir_fd is not supported by pathlib +os.close(fd) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 6cfa73f876..de027ff99e 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1041,6 +1041,7 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) { Rule::OsPathGetctime, Rule::Glob, Rule::OsListdir, + Rule::OsSymlink, ]) { flake8_use_pathlib::rules::replaceable_by_pathlib(checker, call); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index c96881fbaf..2ab35f2f16 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -934,6 +934,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8UsePathlib, "207") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::Glob), (Flake8UsePathlib, "208") => (RuleGroup::Stable, rules::flake8_use_pathlib::violations::OsListdir), (Flake8UsePathlib, "210") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::InvalidPathlibWithSuffix), + (Flake8UsePathlib, "211") => (RuleGroup::Preview, rules::flake8_use_pathlib::violations::OsSymlink), // flake8-logging-format (Flake8LoggingFormat, "001") => (RuleGroup::Stable, rules::flake8_logging_format::violations::LoggingStringFormat), diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs index fcb05b3f09..7d02e85e9e 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs @@ -66,6 +66,7 @@ mod tests { #[test_case(Rule::OsListdir, Path::new("PTH208.py"))] #[test_case(Rule::InvalidPathlibWithSuffix, Path::new("PTH210.py"))] #[test_case(Rule::InvalidPathlibWithSuffix, Path::new("PTH210_1.py"))] + #[test_case(Rule::OsSymlink, Path::new("PTH211.py"))] fn rules_pypath(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs index 22c62c0690..243729c70f 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/replaceable_by_pathlib.rs @@ -13,7 +13,7 @@ use crate::rules::flake8_use_pathlib::violations::{ BuiltinOpen, Joiner, OsChmod, OsGetcwd, OsListdir, OsMakedirs, OsMkdir, OsPathAbspath, OsPathBasename, OsPathDirname, OsPathExists, OsPathExpanduser, OsPathIsabs, OsPathIsdir, OsPathIsfile, OsPathIslink, OsPathJoin, OsPathSamefile, OsPathSplitext, OsReadlink, OsRemove, - OsRename, OsReplace, OsRmdir, OsStat, OsUnlink, PyPath, + OsRename, OsReplace, OsRmdir, OsStat, OsSymlink, OsUnlink, PyPath, }; use ruff_python_ast::PythonVersion; @@ -38,7 +38,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { .arguments .find_argument_value("path", 0) .is_some_and(|expr| is_file_descriptor(expr, checker.semantic())) - || is_argument_non_default(&call.arguments, "dir_fd", 2) + || is_keyword_only_argument_non_default(&call.arguments, "dir_fd") { return; } @@ -54,7 +54,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { // 0 1 2 // os.mkdir(path, mode=0o777, *, dir_fd=None) // ``` - if is_argument_non_default(&call.arguments, "dir_fd", 2) { + if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") { return; } Diagnostic::new(OsMkdir, range) @@ -68,8 +68,8 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { // 0 1 2 3 // os.rename(src, dst, *, src_dir_fd=None, dst_dir_fd=None) // ``` - if is_argument_non_default(&call.arguments, "src_dir_fd", 2) - || is_argument_non_default(&call.arguments, "dst_dir_fd", 3) + if is_keyword_only_argument_non_default(&call.arguments, "src_dir_fd") + || is_keyword_only_argument_non_default(&call.arguments, "dst_dir_fd") { return; } @@ -84,8 +84,8 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { // 0 1 2 3 // os.replace(src, dst, *, src_dir_fd=None, dst_dir_fd=None) // ``` - if is_argument_non_default(&call.arguments, "src_dir_fd", 2) - || is_argument_non_default(&call.arguments, "dst_dir_fd", 3) + if is_keyword_only_argument_non_default(&call.arguments, "src_dir_fd") + || is_keyword_only_argument_non_default(&call.arguments, "dst_dir_fd") { return; } @@ -99,7 +99,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { // 0 1 // os.rmdir(path, *, dir_fd=None) // ``` - if is_argument_non_default(&call.arguments, "dir_fd", 1) { + if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") { return; } Diagnostic::new(OsRmdir, range) @@ -112,7 +112,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { // 0 1 // os.remove(path, *, dir_fd=None) // ``` - if is_argument_non_default(&call.arguments, "dir_fd", 1) { + if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") { return; } Diagnostic::new(OsRemove, range) @@ -125,7 +125,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { // 0 1 // os.unlink(path, *, dir_fd=None) // ``` - if is_argument_non_default(&call.arguments, "dir_fd", 1) { + if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") { return; } Diagnostic::new(OsUnlink, range) @@ -155,7 +155,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { .arguments .find_argument_value("path", 0) .is_some_and(|expr| is_file_descriptor(expr, checker.semantic())) - || is_argument_non_default(&call.arguments, "dir_fd", 1) + || is_keyword_only_argument_non_default(&call.arguments, "dir_fd") { return; } @@ -202,6 +202,20 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { ["os", "path", "getmtime"] => Diagnostic::new(OsPathGetmtime, range), // PTH205 ["os", "path", "getctime"] => Diagnostic::new(OsPathGetctime, range), + // PTH211 + ["os", "symlink"] => { + // `dir_fd` is not supported by pathlib, so check if there are non-default values. + // Signature as of Python 3.13 (https://docs.python.org/3/library/os.html#os.symlink) + // ```text + // 0 1 2 3 + // os.symlink(src, dst, target_is_directory=False, *, dir_fd=None) + // ``` + if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") { + return; + } + Diagnostic::new(OsSymlink, range) + } + // PTH123 ["" | "builtins", "open"] => { // `closefd` and `opener` are not supported by pathlib, so check if they are @@ -248,7 +262,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { // 0 1 2 3 4 // glob.glob(pathname, *, root_dir=None, dir_fd=None, recursive=False, include_hidden=False) // ``` - if is_argument_non_default(&call.arguments, "dir_fd", 2) { + if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") { return; } @@ -267,7 +281,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { // 0 1 2 3 4 // glob.iglob(pathname, *, root_dir=None, dir_fd=None, recursive=False, include_hidden=False) // ``` - if is_argument_non_default(&call.arguments, "dir_fd", 2) { + if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") { return; } @@ -287,7 +301,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { // 0 1 // os.readlink(path, *, dir_fd=None) // ``` - if is_argument_non_default(&call.arguments, "dir_fd", 1) { + if is_keyword_only_argument_non_default(&call.arguments, "dir_fd") { return; } Diagnostic::new(OsReadlink, range) @@ -303,6 +317,7 @@ pub(crate) fn replaceable_by_pathlib(checker: &Checker, call: &ExprCall) { } Diagnostic::new(OsListdir, range) } + _ => return, }; @@ -348,3 +363,9 @@ fn is_argument_non_default(arguments: &ast::Arguments, name: &str, position: usi .find_argument_value(name, position) .is_some_and(|expr| !expr.is_none_literal_expr()) } + +fn is_keyword_only_argument_non_default(arguments: &ast::Arguments, name: &str) -> bool { + arguments + .find_keyword(name) + .is_some_and(|keyword| !keyword.value.is_none_literal_expr()) +} diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__PTH211_PTH211.py.snap b/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__PTH211_PTH211.py.snap new file mode 100644 index 0000000000..76ac48db2c --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/snapshots/ruff_linter__rules__flake8_use_pathlib__tests__PTH211_PTH211.py.snap @@ -0,0 +1,36 @@ +--- +source: crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs +--- +PTH211.py:5:1: PTH211 `os.symlink` should be replaced by `Path.symlink_to` + | +5 | os.symlink("usr/bin/python", "tmp/python") + | ^^^^^^^^^^ PTH211 +6 | os.symlink(b"usr/bin/python", b"tmp/python") +7 | Path("tmp/python").symlink_to("usr/bin/python") # Ok + | + +PTH211.py:6:1: PTH211 `os.symlink` should be replaced by `Path.symlink_to` + | +5 | os.symlink("usr/bin/python", "tmp/python") +6 | os.symlink(b"usr/bin/python", b"tmp/python") + | ^^^^^^^^^^ PTH211 +7 | Path("tmp/python").symlink_to("usr/bin/python") # Ok + | + +PTH211.py:9:1: PTH211 `os.symlink` should be replaced by `Path.symlink_to` + | + 7 | Path("tmp/python").symlink_to("usr/bin/python") # Ok + 8 | + 9 | os.symlink("usr/bin/python", "tmp/python", target_is_directory=True) + | ^^^^^^^^^^ PTH211 +10 | os.symlink(b"usr/bin/python", b"tmp/python", target_is_directory=True) +11 | Path("tmp/python").symlink_to("usr/bin/python", target_is_directory=True) # Ok + | + +PTH211.py:10:1: PTH211 `os.symlink` should be replaced by `Path.symlink_to` + | + 9 | os.symlink("usr/bin/python", "tmp/python", target_is_directory=True) +10 | os.symlink(b"usr/bin/python", b"tmp/python", target_is_directory=True) + | ^^^^^^^^^^ PTH211 +11 | Path("tmp/python").symlink_to("usr/bin/python", target_is_directory=True) # Ok + | diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs index 22935ea28d..1bed2ba4bf 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs @@ -1215,3 +1215,45 @@ impl Violation for OsListdir { "Use `pathlib.Path.iterdir()` instead.".to_string() } } + +/// ## What it does +/// Checks for uses of `os.symlink`. +/// +/// ## Why is this bad? +/// `pathlib` offers a high-level API for path manipulation, as compared to +/// the lower-level API offered by `os.symlink`. +/// +/// ## Example +/// ```python +/// import os +/// +/// os.symlink("usr/bin/python", "tmp/python", target_is_directory=False) +/// ``` +/// +/// Use instead: +/// ```python +/// from pathlib import Path +/// +/// Path("tmp/python").symlink_to("usr/bin/python") +/// ``` +/// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// +/// ## References +/// - [Python documentation: `Path.symlink_to`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.symlink_to) +/// - [PEP 428 – The pathlib module – object-oriented filesystem paths](https://peps.python.org/pep-0428/) +/// - [Correspondence between `os` and `pathlib`](https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module) +/// - [Why you should be using pathlib](https://treyhunner.com/2018/12/why-you-should-be-using-pathlib/) +/// - [No really, pathlib is great](https://treyhunner.com/2019/01/no-really-pathlib-is-great/) +#[derive(ViolationMetadata)] +pub(crate) struct OsSymlink; + +impl Violation for OsSymlink { + #[derive_message_formats] + fn message(&self) -> String { + "`os.symlink` should be replaced by `Path.symlink_to`".to_string() + } +} diff --git a/ruff.schema.json b/ruff.schema.json index e0f8ce55fb..37fc8102c7 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3889,6 +3889,7 @@ "PTH208", "PTH21", "PTH210", + "PTH211", "PYI", "PYI0", "PYI00",