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 all 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
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.