Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[refurb] Implement implicit-cwd (FURB177) #7704

Merged
merged 18 commits into from Sep 29, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view

This file was deleted.

53 changes: 0 additions & 53 deletions crates/ruff_cli/tests/snapshots/integration_test__stdin_json.snap

This file was deleted.

31 changes: 31 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB177.py
@@ -0,0 +1,31 @@
import pathlib
from pathlib import Path

# Errors
_ = Path().resolve()
_ = pathlib.Path().resolve()

_ = Path("").resolve()
_ = pathlib.Path("").resolve()

_ = Path(".").resolve()
_ = pathlib.Path(".").resolve()

_ = Path("", **kwargs).resolve()
_ = pathlib.Path("", **kwargs).resolve()

_ = Path(".", **kwargs).resolve()
_ = pathlib.Path(".", **kwargs).resolve()

# OK
_ = Path.cwd()
_ = pathlib.Path.cwd()

_ = Path("foo").resolve()
_ = pathlib.Path("foo").resolve()

_ = Path(".", "foo").resolve()
_ = pathlib.Path(".", "foo").resolve()

_ = Path(*args).resolve()
_ = pathlib.Path(*args).resolve()
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Expand Up @@ -913,6 +913,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::ExceptionWithoutExcInfo) {
flake8_logging::rules::exception_without_exc_info(checker, call);
}
if checker.enabled(Rule::ImplicitCwd) {
refurb::rules::no_implicit_cwd(checker, call);
}
}
Expr::Dict(
dict @ ast::ExprDict {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Expand Up @@ -925,6 +925,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Refurb, "140") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedStarmap),
(Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy),
(Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate),
(Refurb, "177") => (RuleGroup::Preview, rules::refurb::rules::ImplicitCwd),

// flake8-logging
(Flake8Logging, "001") => (RuleGroup::Preview, rules::flake8_logging::rules::DirectLoggerInstantiation),
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/refurb/mod.rs
Expand Up @@ -21,6 +21,7 @@ mod tests {
#[test_case(Rule::SliceCopy, Path::new("FURB145.py"))]
#[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.py"))]
#[test_case(Rule::PrintEmptyString, Path::new("FURB105.py"))]
#[test_case(Rule::ImplicitCwd, Path::new("FURB177.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
108 changes: 108 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/implicit_cwd.rs
@@ -0,0 +1,108 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Constant, Expr, ExprAttribute, ExprCall};
use ruff_text_size::Ranged;

use crate::{checkers::ast::Checker, importer::ImportRequest, registry::AsRule};

/// ## What it does
/// Checks for current-directory lookups using `Path().resolve()`.
///
/// ## Why is this bad?
/// When looking up the current directory, prefer `Path.cwd()` over
/// `Path().resolve()`, as `Path.cwd()` is more explicit in its intent.
///
/// ## Example
/// ```python
/// cwd = Path.resolve()
/// ```
///
/// Use instead:
/// ```python
/// cwd = Path.cwd()
/// ```
///
/// ## References
/// - [Python documentation: `Path.cwd`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.cwd)

#[violation]
pub struct ImplicitCwd;

impl Violation for ImplicitCwd {
#[derive_message_formats]
fn message(&self) -> String {
format!("Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups")
}
}

/// FURB177
pub(crate) fn no_implicit_cwd(checker: &mut Checker, call: &ExprCall) {
if !call.arguments.is_empty() {
return;
}

let Expr::Attribute(ExprAttribute { attr, value, .. }) = call.func.as_ref() else {
return;
};

if attr != "resolve" {
return;
}

let Expr::Call(ExprCall {
func, arguments, ..
}) = value.as_ref()
else {
return;
};

// Match on arguments, but ignore keyword arguments. `Path()` accepts keyword arguments, but
// ignores them. See: https://github.com/python/cpython/issues/98094.
match arguments.args.as_slice() {
// Ex) `Path().resolve()`
[] => {}
// Ex) `Path(".").resolve()`
[arg] => {
let Expr::Constant(ast::ExprConstant {
value: Constant::Str(str),
..
}) = arg
else {
return;
};
if !matches!(str.value.as_str(), "" | ".") {
return;
}
}
// Ex) `Path("foo", "bar").resolve()`
_ => return,
}

if !checker
.semantic()
.resolve_call_path(func)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["pathlib", "Path"]))
{
return;
}

let mut diagnostic = Diagnostic::new(ImplicitCwd, call.range());

if checker.patch(diagnostic.kind.rule()) {
diagnostic.try_set_fix(|| {
let (import_edit, binding) = checker.importer().get_or_import_symbol(
&ImportRequest::import("pathlib", "Path"),
call.start(),
checker.semantic(),
)?;
Ok(Fix::suggested_edits(
Edit::range_replacement(format!("{binding}.cwd()"), call.range()),
[import_edit],
))
});
}

checker
.diagnostics
.push(Diagnostic::new(ImplicitCwd, call.range()));
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/mod.rs
@@ -1,5 +1,6 @@
pub(crate) use check_and_remove_from_set::*;
pub(crate) use delete_full_slice::*;
pub(crate) use implicit_cwd::*;
pub(crate) use print_empty_string::*;
pub(crate) use reimplemented_starmap::*;
pub(crate) use repeated_append::*;
Expand All @@ -8,6 +9,7 @@ pub(crate) use unnecessary_enumerate::*;

mod check_and_remove_from_set;
mod delete_full_slice;
mod implicit_cwd;
mod print_empty_string;
mod reimplemented_starmap;
mod repeated_append;
Expand Down