Skip to content

Commit

Permalink
[refurb] Implement implicit-cwd (FURB177) (#7704)
Browse files Browse the repository at this point in the history
## Summary

Implement
[`no-implicit-cwd`](https://github.com/dosisod/refurb/blob/master/docs/checks.md#furb177-no-implicit-cwd)
as `implicit-cwd`

Related to #1348.

## Test Plan

`cargo test`
  • Loading branch information
danparizher committed Sep 29, 2023
1 parent 246d93e commit 78b8741
Show file tree
Hide file tree
Showing 10 changed files with 244 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ info:
- "-"
- "--isolated"
- "--no-cache"
- "--format"
- "--output-format"
- json
- "--stdin-filename"
- F401.py
Expand Down
31 changes: 31 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB177.py
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/registry/rule_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use ruff_macros::CacheKey;
use std::fmt::{Debug, Formatter};
use std::iter::FusedIterator;

const RULESET_SIZE: usize = 11;
const RULESET_SIZE: usize = 12;

/// A set of [`Rule`]s.
///
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/refurb/mod.rs
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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.

0 comments on commit 78b8741

Please sign in to comment.