Skip to content

Commit

Permalink
flake8-use-pathlib autofix and new rules
Browse files Browse the repository at this point in the history
PTH200-204:
- Simplify path constructor `Path(".")` to `Path()`
- Replace `os.path.getsize` with `Path.stat()`

Autofix for:
- PTH200: `Path(".")` to `Path()`
- PTH109: `os.path.getcwd(x)` to `Path(x).cwd()`

Promote `get_member_import_name_alias` from `pylint` to `ast.helpers`
  • Loading branch information
sbrugman committed Feb 11, 2023
1 parent 5b54325 commit 69884b3
Show file tree
Hide file tree
Showing 30 changed files with 1,225 additions and 274 deletions.
9 changes: 7 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1290,7 +1290,7 @@ For more, see [flake8-use-pathlib](https://pypi.org/project/flake8-use-pathlib/)
| PTH106 | pathlib-rmdir | `os.rmdir` should be replaced by `.rmdir()` | |
| PTH107 | pathlib-remove | `os.remove` should be replaced by `.unlink()` | |
| PTH108 | pathlib-unlink | `os.unlink` should be replaced by `.unlink()` | |
| PTH109 | pathlib-getcwd | `os.getcwd` should be replaced by `Path.cwd()` | |
| PTH109 | [pathlib-getcwd](https://github.com/charliermarsh/ruff/blob/main/docs/rules/pathlib-getcwd.md) | `os.getcwd` should be replaced by `Path.cwd()` | 🛠 |
| PTH110 | pathlib-exists | `os.path.exists` should be replaced by `.exists()` | |
| PTH111 | pathlib-expanduser | `os.path.expanduser` should be replaced by `.expanduser()` | |
| PTH112 | pathlib-is-dir | `os.path.isdir` should be replaced by `.is_dir()` | |
Expand All @@ -1299,13 +1299,18 @@ For more, see [flake8-use-pathlib](https://pypi.org/project/flake8-use-pathlib/)
| PTH115 | pathlib-readlink | `os.readlink` should be replaced by `.readlink()` | |
| PTH116 | pathlib-stat | `os.stat` should be replaced by `.stat()` or `.owner()` or `.group()` | |
| PTH117 | pathlib-is-abs | `os.path.isabs` should be replaced by `.is_absolute()` | |
| PTH118 | pathlib-join | `os.path.join` should be replaced by foo_path / "bar" | |
| PTH118 | pathlib-join | `os.path.join` should be replaced by `foo_path / "bar"` | |
| PTH119 | pathlib-basename | `os.path.basename` should be replaced by `.name` | |
| PTH120 | pathlib-dirname | `os.path.dirname` should be replaced by `.parent` | |
| PTH121 | pathlib-samefile | `os.path.samefile` should be replaced by `.samefile()` | |
| PTH122 | pathlib-splitext | `os.path.splitext` should be replaced by `.suffix` | |
| PTH123 | pathlib-open | `open("foo")` should be replaced by `Path("foo").open()` | |
| PTH124 | pathlib-py-path | `py.path` is in maintenance mode, use `pathlib` instead | |
| PTH200 | [path-constructor-current-directory](https://github.com/charliermarsh/ruff/blob/main/docs/rules/path-constructor-current-directory.md) | Do not pass the current directory explicitly to `Path` | 🛠 |
| PTH201 | pathlib-getsize | `os.path.getsize` should be replaced by `stat().st_size` | |
| PTH202 | pathlib-getatime | `os.path.getatime` should be replaced by `stat().st_atime` | |
| PTH203 | pathlib-getmtime | `os.path.getmtime` should be replaced by `stat().st_mtime` | |
| PTH204 | pathlib-getctime | `os.path.getctime` should be replaced by `stat().st_ctime` | |

### eradicate (ERA)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os
import os.path
import pathlib

p = "/foo"

Expand Down
34 changes: 34 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_use_pathlib/guarded.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# ensure that no fixes are applied when`pathlib` is not imported
# (needed until this item is resolved: https://github.com/charliermarsh/ruff/issues/835)
import os
import os.path

p = "/foo"

a = os.path.abspath(p)
aa = os.chmod(p)
aaa = os.mkdir(p)
os.makedirs(p)
os.rename(p)
os.replace(p)
os.rmdir(p)
os.remove(p)
os.unlink(p)
os.getcwd(p)
b = os.path.exists(p)
bb = os.path.expanduser(p)
bbb = os.path.isdir(p)
bbbb = os.path.isfile(p)
bbbbb = os.path.islink(p)
os.readlink(p)
os.stat(p)
os.path.isabs(p)
os.path.join(p)
os.path.basename(p)
os.path.dirname(p)
os.path.samefile(p)
os.path.splitext(p)
with open(p) as fp:
fp.read()
open(p).close()
os.getcwdb(p)
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os as foo
import os.path as foo_p
import pathlib as pth

p = "/foo"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from os import remove, unlink, getcwd, readlink, stat
from os.path import abspath, exists, expanduser, isdir, isfile, islink
from os.path import isabs, join, basename, dirname, samefile, splitext
from pathlib import Path

p = "/foo"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from os.path import isfile as xisfile, islink as xislink, isabs as xisabs
from os.path import join as xjoin, basename as xbasename, dirname as xdirname
from os.path import samefile as xsamefile, splitext as xsplitext
from pathlib import Path as pth

p = "/foo"

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
from pathlib import Path
from pathlib import Path as pth

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

# no match
_ = Path()
print(".")
Path("file.txt")
Path(".", "folder")
19 changes: 19 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_use_pathlib/stat.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import os
from pathlib import Path

os.path.getatime("filename")
os.path.getatime(b"filename")
os.path.getatime(Path("filename"))

os.path.getmtime("filename")
os.path.getmtime(b"filename")
os.path.getmtime(Path("filename"))

os.path.getctime("filename")
os.path.getctime(b"filename")
os.path.getctime(Path("filename"))

os.path.getsize("filename")
os.path.getsize(b"filename")
os.path.getsize(Path("filename"))
os.path.getsize(__file__)
Original file line number Diff line number Diff line change
@@ -1,3 +1,18 @@
import os
from pathlib import Path

(Path("") / "").open()

_ = Path(os.getcwd())

_ = Path(
os.\
getcwd()
)

_ = Path(
os.getcwdb(),
)

# should not be unwrapped
_ = Path(os.getcwd(), hello='world')
60 changes: 60 additions & 0 deletions crates/ruff/src/ast/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1146,6 +1146,66 @@ pub fn is_logger_candidate(func: &Expr) -> bool {
false
}

/// Return the appropriate `sys.exit` reference based on the current set of
/// imports, or `None` is `sys.exit` hasn't been imported.
pub fn get_member_import_name_alias(
checker: &Checker,
module: &str,
member: &str,
) -> Option<String> {
checker.current_scopes().find_map(|scope| {
scope
.bindings
.values()
.find_map(|index| match &checker.bindings[*index].kind {
// e.g. module=sys object=exit
// `import sys` -> `sys.exit`
// `import sys as sys2` -> `sys2.exit`
BindingKind::Importation(name, full_name) => {
if full_name == &module {
Some(format!("{name}.{member}"))
} else {
None
}
}
// e.g. module=os.path object=join
// `from os.path import join` -> `join`
// `from os.path import join as join2` -> `join2`
BindingKind::FromImportation(name, full_name) => {
let mut parts = full_name.split('.');
if parts.next() == Some(module)
&& parts.next() == Some(member)
&& parts.next().is_none()
{
Some((*name).to_string())
} else {
None
}
}
// e.g. module=os.path object=join
// `from os.path import *` -> `join`
BindingKind::StarImportation(_, name) => {
if name.as_ref().map(|name| name == module).unwrap_or_default() {
Some(member.to_string())
} else {
None
}
}
// e.g. module=os.path object=join
// `import os.path ` -> `os.path.join`
BindingKind::SubmoduleImportation(_, full_name) => {
if full_name == &module {
Some(format!("{full_name}.{member}"))
} else {
None
}
}
// Non-imports.
_ => None,
})
})
}

#[cfg(test)]
mod tests {
use anyhow::Result;
Expand Down
14 changes: 13 additions & 1 deletion crates/ruff/src/checkers/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2756,7 +2756,19 @@ where
|| self.settings.rules.enabled(&Rule::PathlibOpen)
|| self.settings.rules.enabled(&Rule::PathlibPyPath)
{
flake8_use_pathlib::helpers::replaceable_by_pathlib(self, func);
flake8_use_pathlib::helpers::replaceable_by_pathlib(
self,
func,
self.current_expr_parent().map(Into::into),
);
}

if self
.settings
.rules
.enabled(&Rule::PathConstructorCurrentDirectory)
{
flake8_use_pathlib::rules::simplify_path_constructor(self, expr, func);
}

// flake8-logging-format
Expand Down
5 changes: 5 additions & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,11 @@ ruff_macros::define_rule_mapping!(
PTH122 => rules::flake8_use_pathlib::violations::PathlibSplitext,
PTH123 => rules::flake8_use_pathlib::violations::PathlibOpen,
PTH124 => rules::flake8_use_pathlib::violations::PathlibPyPath,
PTH200 => rules::flake8_use_pathlib::rules::PathConstructorCurrentDirectory,
PTH201 => rules::flake8_use_pathlib::violations::PathlibGetsize,
PTH202 => rules::flake8_use_pathlib::violations::PathlibGetatime,
PTH203 => rules::flake8_use_pathlib::violations::PathlibGetmtime,
PTH204 => rules::flake8_use_pathlib::violations::PathlibGetctime,
// flake8-logging-format
G001 => rules::flake8_logging_format::violations::LoggingStringFormat,
G002 => rules::flake8_logging_format::violations::LoggingPercentFormat,
Expand Down
67 changes: 67 additions & 0 deletions crates/ruff/src/rules/flake8_use_pathlib/fixes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
use rustpython_parser::ast::{Expr, ExprKind};

use crate::ast::helpers;
use crate::ast::types::Range;
use crate::autofix::apply_fix;
use crate::checkers::ast::Checker;
use crate::fix::Fix;
use crate::registry::DiagnosticKind;
use crate::source_code::Locator;

pub fn pathlib_fix(
checker: &mut Checker,
diagnostic: &DiagnosticKind,
func: &Expr,
parent: Option<&Expr>,
) -> Option<Fix> {
// Guard that Path is imported, `content` contains the name or aliaas
if let Some(content) = helpers::get_member_import_name_alias(checker, "pathlib", "Path") {
let mut fix = match diagnostic {
DiagnosticKind::PathlibGetcwd(_) => Some(Fix::replacement(
format!("{content}.cwd"),
func.location,
func.end_location.unwrap(),
)),
_ => None,
};

// Wrapped in a `Path()` call
if let Some(fixme) = fix.clone() {
if let Some(parent) = parent {
if checker
.resolve_call_path(parent)
.map_or(false, |call_path| {
call_path.as_slice() == ["pathlib", "Path"]
})
{
if let ExprKind::Call { args, keywords, .. } = &parent.node {
if args.len() == 1 && keywords.is_empty() {
// Reset the line index
let fixme = Fix::replacement(
fixme.content.to_string(),
helpers::to_relative(fixme.location, func.location),
helpers::to_relative(fixme.end_location, func.location),
);

// Apply the fix
let arg = args.first().unwrap();
let contents = checker.locator.slice_source_code_range(&Range::new(
arg.location,
arg.end_location.unwrap(),
));

fix = Some(Fix::replacement(
apply_fix(&fixme, &Locator::new(contents)),
parent.location,
parent.end_location.unwrap(),
));
}
}
}
}
}
fix
} else {
None
}
}
24 changes: 17 additions & 7 deletions crates/ruff/src/rules/flake8_use_pathlib/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
use rustpython_parser::ast::Expr;

use super::fixes::pathlib_fix;
use crate::ast::types::Range;
use crate::checkers::ast::Checker;
use crate::registry::{Diagnostic, DiagnosticKind};
use crate::rules::flake8_use_pathlib::violations::{
PathlibAbspath, PathlibBasename, PathlibChmod, PathlibDirname, PathlibExists,
PathlibExpanduser, PathlibGetcwd, PathlibIsAbs, PathlibIsDir, PathlibIsFile, PathlibIsLink,
PathlibJoin, PathlibMakedirs, PathlibMkdir, PathlibOpen, PathlibPyPath, PathlibReadlink,
PathlibRemove, PathlibRename, PathlibReplace, PathlibRmdir, PathlibSamefile, PathlibSplitext,
PathlibStat, PathlibUnlink,
PathlibExpanduser, PathlibGetatime, PathlibGetctime, PathlibGetcwd, PathlibGetmtime,
PathlibGetsize, PathlibIsAbs, PathlibIsDir, PathlibIsFile, PathlibIsLink, PathlibJoin,
PathlibMakedirs, PathlibMkdir, PathlibOpen, PathlibPyPath, PathlibReadlink, PathlibRemove,
PathlibRename, PathlibReplace, PathlibRmdir, PathlibSamefile, PathlibSplitext, PathlibStat,
PathlibUnlink,
};
use crate::settings::types::PythonVersion;

pub fn replaceable_by_pathlib(checker: &mut Checker, expr: &Expr) {
pub fn replaceable_by_pathlib(checker: &mut Checker, expr: &Expr, parent: Option<&Expr>) {
if let Some(diagnostic_kind) =
checker
.resolve_call_path(expr)
Expand Down Expand Up @@ -46,12 +48,20 @@ pub fn replaceable_by_pathlib(checker: &mut Checker, expr: &Expr) {
["os", "readlink"] if checker.settings.target_version >= PythonVersion::Py39 => {
Some(PathlibReadlink.into())
}
["os", "path", "getsize"] => Some(PathlibGetsize.into()),
["os", "path", "getatime"] => Some(PathlibGetatime.into()),
["os", "path", "getmtime"] => Some(PathlibGetmtime.into()),
["os", "path", "getctime"] => Some(PathlibGetctime.into()),
_ => None,
})
{
let diagnostic =
let mut diagnostic =
Diagnostic::new::<DiagnosticKind>(diagnostic_kind, Range::from_located(expr));

if checker.patch(diagnostic.kind.rule()) {
if let Some(fix) = pathlib_fix(checker, &diagnostic.kind, expr, parent) {
diagnostic.amend(fix);
}
}
if checker.settings.rules.enabled(diagnostic.kind.rule()) {
checker.diagnostics.push(diagnostic);
}
Expand Down
10 changes: 10 additions & 0 deletions crates/ruff/src/rules/flake8_use_pathlib/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! Rules from [flake8-use-pathlib](https://pypi.org/project/flake8-use-pathlib/).
pub(crate) mod fixes;
pub(crate) mod helpers;
pub(crate) mod rules;
pub(crate) mod violations;

#[cfg(test)]
Expand All @@ -18,6 +20,9 @@ mod tests {
#[test_case(Path::new("import_from_as.py"); "PTH1_3")]
#[test_case(Path::new("import_from.py"); "PTH1_4")]
#[test_case(Path::new("use_pathlib.py"); "PTH1_5")]
#[test_case(Path::new("stat.py"); "PTH1_6")]
#[test_case(Path::new("simplify_pathlib_constructor.py"); "PTH1_7")]
#[test_case(Path::new("guarded.py"); "PTH1_8")]
fn rules(path: &Path) -> Result<()> {
let snapshot = format!("{}", path.to_string_lossy());
let diagnostics = test_path(
Expand Down Expand Up @@ -47,6 +52,11 @@ mod tests {
Rule::PathlibSamefile,
Rule::PathlibSplitext,
Rule::PathlibOpen,
Rule::PathlibGetsize,
Rule::PathlibGetatime,
Rule::PathlibGetmtime,
Rule::PathlibGetctime,
Rule::PathConstructorCurrentDirectory,
]),
)?;
insta::assert_yaml_snapshot!(snapshot, diagnostics);
Expand Down
3 changes: 3 additions & 0 deletions crates/ruff/src/rules/flake8_use_pathlib/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub use simplify_path_constructor::{simplify_path_constructor, PathConstructorCurrentDirectory};

mod simplify_path_constructor;
Loading

0 comments on commit 69884b3

Please sign in to comment.