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 11 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
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 @@ -910,6 +910,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::NoImplicitCwd) {
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 @@ -924,6 +924,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::NoImplicitCwd),

// flake8-logging
(Flake8Logging, "001") => (RuleGroup::Preview, rules::flake8_logging::rules::DirectLoggerInstantiation),
Expand Down
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::NoImplicitCwd, 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
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 no_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 no_implicit_cwd;
mod print_empty_string;
mod reimplemented_starmap;
mod repeated_append;
Expand Down
108 changes: 108 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/no_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 NoImplicitCwd;

impl Violation for NoImplicitCwd {
#[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(NoImplicitCwd, 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(NoImplicitCwd, call.range()))
}
@@ -0,0 +1,94 @@
---
source: crates/ruff_linter/src/rules/refurb/mod.rs
---
FURB177.py:5:5: FURB177 Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups
|
4 | # Errors
5 | _ = Path().resolve()
| ^^^^^^^^^^^^^^^^ FURB177
6 | _ = pathlib.Path().resolve()
|

FURB177.py:6:5: FURB177 Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups
|
4 | # Errors
5 | _ = Path().resolve()
6 | _ = pathlib.Path().resolve()
| ^^^^^^^^^^^^^^^^^^^^^^^^ FURB177
7 |
8 | _ = Path("").resolve()
|

FURB177.py:8:5: FURB177 Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups
|
6 | _ = pathlib.Path().resolve()
7 |
8 | _ = Path("").resolve()
| ^^^^^^^^^^^^^^^^^^ FURB177
9 | _ = pathlib.Path("").resolve()
|

FURB177.py:9:5: FURB177 Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups
|
8 | _ = Path("").resolve()
9 | _ = pathlib.Path("").resolve()
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB177
10 |
11 | _ = Path(".").resolve()
|

FURB177.py:11:5: FURB177 Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups
|
9 | _ = pathlib.Path("").resolve()
10 |
11 | _ = Path(".").resolve()
| ^^^^^^^^^^^^^^^^^^^ FURB177
12 | _ = pathlib.Path(".").resolve()
|

FURB177.py:12:5: FURB177 Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups
|
11 | _ = Path(".").resolve()
12 | _ = pathlib.Path(".").resolve()
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB177
13 |
14 | _ = Path("", **kwargs).resolve()
|

FURB177.py:14:5: FURB177 Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups
|
12 | _ = pathlib.Path(".").resolve()
13 |
14 | _ = Path("", **kwargs).resolve()
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB177
15 | _ = pathlib.Path("", **kwargs).resolve()
|

FURB177.py:15:5: FURB177 Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups
|
14 | _ = Path("", **kwargs).resolve()
15 | _ = pathlib.Path("", **kwargs).resolve()
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB177
16 |
17 | _ = Path(".", **kwargs).resolve()
|

FURB177.py:17:5: FURB177 Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups
|
15 | _ = pathlib.Path("", **kwargs).resolve()
16 |
17 | _ = Path(".", **kwargs).resolve()
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB177
18 | _ = pathlib.Path(".", **kwargs).resolve()
|

FURB177.py:18:5: FURB177 Prefer `Path.cwd()` over `Path().resolve()` for current-directory lookups
|
17 | _ = Path(".", **kwargs).resolve()
18 | _ = pathlib.Path(".", **kwargs).resolve()
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB177
19 |
20 | # OK
|


2 changes: 2 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.