Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions crates/ruff_linter/src/rules/flake8_use_pathlib/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,22 @@ pub(crate) fn check_os_pathlib_single_arg_calls(
let fix = match applicability {
Some(Applicability::Unsafe) => Fix::unsafe_edits(edit, [import_edit]),
_ => {
let applicability = if checker.comment_ranges().intersects(range) {
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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
// applicability is Some(Applicability::Safe), use it
Applicability::Safe
};
Fix::applicable_edits(edit, [import_edit], applicability)
Fix::applicable_edits(edit, [import_edit], determined_applicability)
}
};

Expand Down Expand Up @@ -176,8 +186,12 @@ pub(crate) fn check_os_pathlib_two_arg_calls(

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 (None -> Path)
Applicability::Unsafe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

Ok(Fix::applicable_edits(
Expand Down Expand Up @@ -209,3 +223,9 @@ pub(crate) fn is_argument_non_default(arguments: &Arguments, name: &str, positio
.find_argument_value(name, position)
.is_some_and(|expr| !expr.is_none_literal_expr())
}

/// Returns `true` if the given call is a top-level expression in its statement.
/// This means the call's return value is not used, so return type changes don't matter.
pub(crate) fn is_top_level_expression_call(checker: &Checker, _call: &ExprCall) -> bool {
checker.semantic().current_expression_parent().is_none()
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::checkers::ast::Checker;
use crate::importer::ImportRequest;
use crate::preview::is_fix_os_getcwd_enabled;
use crate::rules::flake8_use_pathlib::helpers::is_top_level_expression_call;
use crate::{FixAvailability, Violation};
use ruff_diagnostics::{Applicability, Edit, Fix};
use ruff_macros::{ViolationMetadata, derive_message_formats};
Expand Down Expand Up @@ -37,6 +38,8 @@ use ruff_text_size::Ranged;
///
/// ## Fix Safety
/// This rule's fix is marked as unsafe if the replacement would remove comments attached to the original expression.
/// 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.
///
/// ## References
/// - [Python documentation: `Path.cwd`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.cwd)
Expand Down Expand Up @@ -83,7 +86,10 @@ pub(crate) fn os_getcwd(checker: &Checker, call: &ExprCall, segments: &[&str]) {
checker.semantic(),
)?;

let applicability = if checker.comment_ranges().intersects(range) {
// 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ use crate::{FixAvailability, Violation};
/// behaviors is required, there's no existing `pathlib` alternative. See CPython issue
/// [#69200](https://github.com/python/cpython/issues/69200).
///
/// Additionally, the fix is marked as unsafe because `os.path.abspath()` returns a `str`, while
/// `Path.resolve()` returns a `Path` object. This change in return type can break code that uses
/// the return value.
///
/// ## References
/// - [Python documentation: `Path.resolve`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.resolve)
/// - [Python documentation: `os.path.abspath`](https://docs.python.org/3/library/os.path.html#os.path.abspath)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ use crate::{FixAvailability, Violation};
/// As a result, code relying on the exact string returned by `os.path.dirname`
/// may behave differently after the fix.
///
/// Additionally, the fix is marked as unsafe because `os.path.dirname()` returns a `str`, while
/// `Path.parent` returns a `Path` object. This change in return type can break code that uses
/// the return value.
///
/// ## 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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::checkers::ast::Checker;
use crate::preview::is_fix_os_path_exists_enabled;
use crate::rules::flake8_use_pathlib::helpers::check_os_pathlib_single_arg_calls;
use crate::{FixAvailability, Violation};
use ruff_diagnostics::Applicability;

/// ## What it does
/// Checks for uses of `os.path.exists`.
Expand Down Expand Up @@ -72,6 +73,6 @@ pub(crate) fn os_path_exists(checker: &Checker, call: &ExprCall, segments: &[&st
"path",
is_fix_os_path_exists_enabled(checker.settings()),
OsPathExists,
None,
Some(Applicability::Safe),
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ use crate::{FixAvailability, Violation};
/// directory can't be resolved: `os.path.expanduser` returns the
/// input unchanged, while `Path.expanduser` raises `RuntimeError`.
///
/// Additionally, the fix is marked as unsafe because `os.path.expanduser()` returns a `str`, while
/// `Path.expanduser()` returns a `Path` object. This change in return type can break code that uses
/// the return value.
///
/// ## References
/// - [Python documentation: `Path.expanduser`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.expanduser)
/// - [Python documentation: `os.path.expanduser`](https://docs.python.org/3/library/os.path.html#os.path.expanduser)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::checkers::ast::Checker;
use crate::preview::is_fix_os_path_isfile_enabled;
use crate::rules::flake8_use_pathlib::helpers::check_os_pathlib_single_arg_calls;
use crate::{FixAvailability, Violation};
use ruff_diagnostics::Applicability;

/// ## What it does
/// Checks for uses of `os.path.isfile`.
Expand Down Expand Up @@ -73,6 +74,6 @@ pub(crate) fn os_path_isfile(checker: &Checker, call: &ExprCall, segments: &[&st
"path",
is_fix_os_path_isfile_enabled(checker.settings()),
OsPathIsfile,
None,
Some(Applicability::Safe),
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::checkers::ast::Checker;
use crate::preview::is_fix_os_path_islink_enabled;
use crate::rules::flake8_use_pathlib::helpers::check_os_pathlib_single_arg_calls;
use crate::{FixAvailability, Violation};
use ruff_diagnostics::Applicability;

/// ## What it does
/// Checks for uses of `os.path.islink`.
Expand Down Expand Up @@ -73,6 +74,6 @@ pub(crate) fn os_path_islink(checker: &Checker, call: &ExprCall, segments: &[&st
"path",
is_fix_os_path_islink_enabled(checker.settings()),
OsPathIslink,
None,
Some(Applicability::Safe),
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ use crate::{FixAvailability, Violation};
///
/// ## Fix Safety
/// This rule's fix is marked as unsafe if the replacement would remove comments attached to the original expression.
/// Additionally, the fix is marked as unsafe when the return value is used because the type changes
/// from `str` to a `Path` object.
///
/// ## References
/// - [Python documentation: `Path.readlink`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.readline)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ use ruff_python_ast::ExprCall;
///
/// ## Fix Safety
/// This rule's fix is marked as unsafe if the replacement would remove comments attached to the original expression.
/// Additionally, the fix is marked as unsafe when the return value is used because the type changes
/// from `None` to a `Path` object.
///
/// ## References
/// - [Python documentation: `Path.rename`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.rename)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ use ruff_python_ast::ExprCall;
///
/// ## Fix Safety
/// This rule's fix is marked as unsafe if the replacement would remove comments attached to the original expression.
/// Additionally, the fix is marked as unsafe when the return value is used because the type changes
/// from `None` to a `Path` object.
///
/// ## References
/// - [Python documentation: `Path.replace`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.replace)
Expand Down