Skip to content

Commit

Permalink
[flake8-use-pathlib] Catch redundant joins in PTH201 and avoid sy…
Browse files Browse the repository at this point in the history
…ntax errors (#15177)

## Summary

Resolves #10453, resolves #15165.

## Test Plan

`cargo nextest run` and `cargo insta test`.
  • Loading branch information
InSyncWithFoo authored Dec 30, 2024
1 parent d349217 commit 901b7dd
Show file tree
Hide file tree
Showing 4 changed files with 440 additions and 69 deletions.
Original file line number Diff line number Diff line change
@@ -1,15 +1,70 @@
from pathlib import Path, PurePath
from pathlib import Path as pth


# match
_ = Path(".")
_ = pth(".")
_ = PurePath(".")
_ = Path("")

Path('', )

Path(
'',
)

Path( # Comment before argument
'',
)

Path(
'', # EOL comment
)

Path(
'' # Comment in the middle of implicitly concatenated string
".",
)

Path(
'' # Comment before comma
,
)

Path(
'',
) / "bare"

Path( # Comment before argument
'',
) / ("parenthesized")

Path(
'', # EOL comment
) / ( ("double parenthesized" ) )

( Path(
'' # Comment in the middle of implicitly concatenated string
".",
) )/ (("parenthesized path call")
# Comment between closing parentheses
)

Path(
'' # Comment before comma
,
) / "multiple" / (
"frag" # Comment
'ment'
)


# no match
_ = Path()
print(".")
Path("file.txt")
Path(".", "folder")
PurePath(".", "folder")

Path()
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -983,7 +983,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
flake8_use_pathlib::rules::replaceable_by_pathlib(checker, call);
}
if checker.enabled(Rule::PathConstructorCurrentDirectory) {
flake8_use_pathlib::rules::path_constructor_current_directory(checker, expr, func);
flake8_use_pathlib::rules::path_constructor_current_directory(checker, call);
}
if checker.enabled(Rule::OsSepSplit) {
flake8_use_pathlib::rules::os_sep_split(checker, call);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use std::ops::Range;

use ruff_diagnostics::{AlwaysFixableViolation, Applicability, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::{self as ast, Expr, ExprCall};
use ruff_python_ast::parenthesize::parenthesized_range;
use ruff_python_ast::{AstNode, Expr, ExprBinOp, ExprCall, Operator};
use ruff_python_semantic::SemanticModel;
use ruff_python_trivia::CommentRanges;
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
use crate::fix::edits::{remove_argument, Parentheses};

/// ## What it does
/// Checks for `pathlib.Path` objects that are initialized with the current
Expand Down Expand Up @@ -43,7 +50,17 @@ impl AlwaysFixableViolation for PathConstructorCurrentDirectory {
}

/// PTH201
pub(crate) fn path_constructor_current_directory(checker: &mut Checker, expr: &Expr, func: &Expr) {
pub(crate) fn path_constructor_current_directory(checker: &mut Checker, call: &ExprCall) {
let applicability = |range| {
if checker.comment_ranges().intersects(range) {
Applicability::Unsafe
} else {
Applicability::Safe
}
};

let (func, arguments) = (&call.func, &call.arguments);

if !checker
.semantic()
.resolve_qualified_name(func)
Expand All @@ -54,21 +71,75 @@ pub(crate) fn path_constructor_current_directory(checker: &mut Checker, expr: &E
return;
}

let Expr::Call(ExprCall { arguments, .. }) = expr else {
if !arguments.keywords.is_empty() {
return;
}

let [Expr::StringLiteral(arg)] = &*arguments.args else {
return;
};

if !arguments.keywords.is_empty() {
if !matches!(arg.value.to_str(), "" | ".") {
return;
}

let [Expr::StringLiteral(ast::ExprStringLiteral { value, range })] = &*arguments.args else {
return;
let mut diagnostic = Diagnostic::new(PathConstructorCurrentDirectory, arg.range());

match parent_and_next_path_fragment_range(
checker.semantic(),
checker.comment_ranges(),
checker.source(),
) {
Some((parent_range, next_fragment_range)) => {
let next_fragment_expr = checker.locator().slice(next_fragment_range);
let call_expr = checker.locator().slice(call.range());

let relative_argument_range: Range<usize> = {
let range = arg.range() - call.start();
range.start().into()..range.end().into()
};

let mut new_call_expr = call_expr.to_string();
new_call_expr.replace_range(relative_argument_range, next_fragment_expr);

let edit = Edit::range_replacement(new_call_expr, parent_range);

diagnostic.set_fix(Fix::applicable_edit(edit, applicability(parent_range)));
}
None => diagnostic.try_set_fix(|| {
let edit = remove_argument(arg, arguments, Parentheses::Preserve, checker.source())?;
Ok(Fix::applicable_edit(edit, applicability(call.range())))
}),
};

if matches!(value.to_str(), "" | ".") {
let mut diagnostic = Diagnostic::new(PathConstructorCurrentDirectory, *range);
diagnostic.set_fix(Fix::safe_edit(Edit::range_deletion(*range)));
checker.diagnostics.push(diagnostic);
checker.diagnostics.push(diagnostic);
}

fn parent_and_next_path_fragment_range(
semantic: &SemanticModel,
comment_ranges: &CommentRanges,
source: &str,
) -> Option<(TextRange, TextRange)> {
let parent = semantic.current_expression_parent()?;

let Expr::BinOp(parent @ ExprBinOp { op, right, .. }) = parent else {
return None;
};

let range = right.range();

if !matches!(op, Operator::Div) {
return None;
}

Some((
parent.range(),
parenthesized_range(
right.into(),
parent.as_any_node_ref(),
comment_ranges,
source,
)
.unwrap_or(range),
))
}
Loading

0 comments on commit 901b7dd

Please sign in to comment.