Skip to content

Commit

Permalink
[flake8-use-pathlib] Dotless suffix passed to Path.with_suffix()
Browse files Browse the repository at this point in the history
…(`PTH901`) (#14779)

## Summary

Resolves #14441.

## Test Plan

`cargo nextest run` and `cargo insta test`.
  • Loading branch information
InSyncWithFoo authored Dec 6, 2024
1 parent 1559c73 commit 89368a6
Show file tree
Hide file tree
Showing 11 changed files with 1,371 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
from pathlib import (
Path,
PosixPath,
PurePath,
PurePosixPath,
PureWindowsPath,
WindowsPath,
)
import pathlib


path = Path()
posix_path: pathlib.PosixPath = PosixPath()
pure_path: PurePath = PurePath()
pure_posix_path = pathlib.PurePosixPath()
pure_windows_path: PureWindowsPath = pathlib.PureWindowsPath()
windows_path: pathlib.WindowsPath = pathlib.WindowsPath()


### Errors
path.with_suffix("py")
path.with_suffix(r"s")
path.with_suffix(u'' "json")
path.with_suffix(suffix="js")

posix_path.with_suffix("py")
posix_path.with_suffix(r"s")
posix_path.with_suffix(u'' "json")
posix_path.with_suffix(suffix="js")

pure_path.with_suffix("py")
pure_path.with_suffix(r"s")
pure_path.with_suffix(u'' "json")
pure_path.with_suffix(suffix="js")

pure_posix_path.with_suffix("py")
pure_posix_path.with_suffix(r"s")
pure_posix_path.with_suffix(u'' "json")
pure_posix_path.with_suffix(suffix="js")

pure_windows_path.with_suffix("py")
pure_windows_path.with_suffix(r"s")
pure_windows_path.with_suffix(u'' "json")
pure_windows_path.with_suffix(suffix="js")

windows_path.with_suffix("py")
windows_path.with_suffix(r"s")
windows_path.with_suffix(u'' "json")
windows_path.with_suffix(suffix="js")


### No errors
path.with_suffix()
path.with_suffix('')
path.with_suffix(".py")
path.with_suffix("foo", "bar")
path.with_suffix(suffix)
path.with_suffix(f"oo")
path.with_suffix(b"ar")

posix_path.with_suffix()
posix_path.with_suffix('')
posix_path.with_suffix(".py")
posix_path.with_suffix("foo", "bar")
posix_path.with_suffix(suffix)
posix_path.with_suffix(f"oo")
posix_path.with_suffix(b"ar")

pure_path.with_suffix()
pure_path.with_suffix('')
pure_path.with_suffix(".py")
pure_path.with_suffix("foo", "bar")
pure_path.with_suffix(suffix)
pure_path.with_suffix(f"oo")
pure_path.with_suffix(b"ar")

pure_posix_path.with_suffix()
pure_posix_path.with_suffix('')
pure_posix_path.with_suffix(".py")
pure_posix_path.with_suffix("foo", "bar")
pure_posix_path.with_suffix(suffix)
pure_posix_path.with_suffix(f"oo")
pure_posix_path.with_suffix(b"ar")

pure_windows_path.with_suffix()
pure_windows_path.with_suffix('')
pure_windows_path.with_suffix(".py")
pure_windows_path.with_suffix("foo", "bar")
pure_windows_path.with_suffix(suffix)
pure_windows_path.with_suffix(f"oo")
pure_windows_path.with_suffix(b"ar")

windows_path.with_suffix()
windows_path.with_suffix('')
windows_path.with_suffix(".py")
windows_path.with_suffix("foo", "bar")
windows_path.with_suffix(suffix)
windows_path.with_suffix(f"oo")
windows_path.with_suffix(b"ar")
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
from pathlib import (
Path,
PosixPath,
PurePath,
PurePosixPath,
PureWindowsPath,
WindowsPath,
)


def test_path(p: Path) -> None:
## Errors
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
p.with_suffix(suffix="js")

## No errors
p.with_suffix()
p.with_suffix('')
p.with_suffix(".py")
p.with_suffix("foo", "bar")
p.with_suffix(suffix)
p.with_suffix(f"oo")
p.with_suffix(b"ar")


def test_posix_path(p: PosixPath) -> None:
## Errors
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
p.with_suffix(suffix="js")

## No errors
p.with_suffix()
p.with_suffix('')
p.with_suffix(".py")
p.with_suffix("foo", "bar")
p.with_suffix(suffix)
p.with_suffix(f"oo")
p.with_suffix(b"ar")


def test_pure_path(p: PurePath) -> None:
## Errors
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
p.with_suffix(suffix="js")

## No errors
p.with_suffix()
p.with_suffix('')
p.with_suffix(".py")
p.with_suffix("foo", "bar")
p.with_suffix(suffix)
p.with_suffix(f"oo")
p.with_suffix(b"ar")


def test_pure_posix_path(p: PurePosixPath) -> None:
## Errors
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
p.with_suffix(suffix="js")

## No errors
p.with_suffix()
p.with_suffix('')
p.with_suffix(".py")
p.with_suffix("foo", "bar")
p.with_suffix(suffix)
p.with_suffix(f"oo")
p.with_suffix(b"ar")


def test_pure_windows_path(p: PureWindowsPath) -> None:
## Errors
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
p.with_suffix(suffix="js")

## No errors
p.with_suffix()
p.with_suffix('')
p.with_suffix(".py")
p.with_suffix("foo", "bar")
p.with_suffix(suffix)
p.with_suffix(f"oo")
p.with_suffix(b"ar")


def test_windows_path(p: WindowsPath) -> None:
## Errors
p.with_suffix("py")
p.with_suffix(r"s")
p.with_suffix(u'' "json")
p.with_suffix(suffix="js")

## No errors
p.with_suffix()
p.with_suffix('')
p.with_suffix(".py")
p.with_suffix("foo", "bar")
p.with_suffix(suffix)
p.with_suffix(f"oo")
p.with_suffix(b"ar")
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 @@ -1096,6 +1096,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::UnnecessaryCastToInt) {
ruff::rules::unnecessary_cast_to_int(checker, call);
}
if checker.enabled(Rule::DotlessPathlibWithSuffix) {
flake8_use_pathlib::rules::dotless_pathlib_with_suffix(checker, call);
}
}
Expr::Dict(dict) => {
if checker.any_enabled(&[
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 @@ -909,6 +909,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8UsePathlib, "206") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::OsSepSplit),
(Flake8UsePathlib, "207") => (RuleGroup::Stable, rules::flake8_use_pathlib::rules::Glob),
(Flake8UsePathlib, "208") => (RuleGroup::Preview, rules::flake8_use_pathlib::violations::OsListdir),
(Flake8UsePathlib, "210") => (RuleGroup::Preview, rules::flake8_use_pathlib::rules::DotlessPathlibWithSuffix),

// flake8-logging-format
(Flake8LoggingFormat, "001") => (RuleGroup::Stable, rules::flake8_logging_format::violations::LoggingStringFormat),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_use_pathlib/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ mod tests {
#[test_case(Rule::OsSepSplit, Path::new("PTH206.py"))]
#[test_case(Rule::Glob, Path::new("PTH207.py"))]
#[test_case(Rule::OsListdir, Path::new("PTH208.py"))]
#[test_case(Rule::DotlessPathlibWithSuffix, Path::new("PTH210.py"))]
#[test_case(Rule::DotlessPathlibWithSuffix, Path::new("PTH210_1.py"))]
fn rules_pypath(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
use crate::checkers::ast::Checker;
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::{Expr, ExprAttribute, ExprCall, ExprStringLiteral, StringFlags};
use ruff_python_semantic::analyze::typing;
use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged;

/// ## What it does
/// Checks for `pathlib.Path.with_suffix()` calls where
/// the given suffix does not have a leading dot.
///
/// ## Why is this bad?
/// `Path.with_suffix()` will raise an error at runtime
/// if the given suffix is not prefixed with a dot.
///
/// ## Examples
///
/// ```python
/// path.with_suffix("py")
/// ```
///
/// Use instead:
///
/// ```python
/// path.with_suffix(".py")
/// ```
///
/// ## Known problems
/// This rule is prone to false negatives due to type inference limitations,
/// as it will only detect paths that are either instantiated (`p = Path(...)`)
/// or annotated (`def f(p: Path)`) as such.
///
/// ## Fix safety
/// The fix for this rule adds a leading period to the string passed
/// to the `with_suffix()` call. This fix is marked as unsafe, as it
/// changes runtime behaviour: the call would previously always have
/// raised an exception, but no longer will.
///
/// Moreover, it's impossible to determine if this is the correct fix
/// for a given situation (it's possible that the string was correct
/// but was being passed to the wrong method entirely, for example).
#[derive(ViolationMetadata)]
pub(crate) struct DotlessPathlibWithSuffix;

impl AlwaysFixableViolation for DotlessPathlibWithSuffix {
#[derive_message_formats]
fn message(&self) -> String {
"Dotless suffix passed to `.with_suffix()`".to_string()
}

fn fix_title(&self) -> String {
"Add a leading dot".to_string()
}
}

/// PTH210
pub(crate) fn dotless_pathlib_with_suffix(checker: &mut Checker, call: &ExprCall) {
let (func, arguments) = (&call.func, &call.arguments);

if !is_path_with_suffix_call(checker.semantic(), func) {
return;
}

if arguments.len() > 1 {
return;
}

let Some(Expr::StringLiteral(string)) = arguments.find_argument("suffix", 0) else {
return;
};

let string_value = string.value.to_str();

if string_value.is_empty() || string_value.starts_with('.') {
return;
}

let diagnostic = Diagnostic::new(DotlessPathlibWithSuffix, call.range);
let Some(fix) = add_leading_dot_fix(string) else {
unreachable!("Expected to always be able to fix this rule");
};

checker.diagnostics.push(diagnostic.with_fix(fix));
}

fn is_path_with_suffix_call(semantic: &SemanticModel, func: &Expr) -> bool {
let Expr::Attribute(ExprAttribute { value, attr, .. }) = func else {
return false;
};

if attr != "with_suffix" {
return false;
}

let Expr::Name(name) = value.as_ref() else {
return false;
};
let Some(binding) = semantic.only_binding(name).map(|id| semantic.binding(id)) else {
return false;
};

typing::is_pathlib_path(binding, semantic)
}

fn add_leading_dot_fix(string: &ExprStringLiteral) -> Option<Fix> {
let first_part = string.value.iter().next()?;

let opener_length = first_part.flags.opener_len();
let after_leading_quote = first_part.start().checked_add(opener_length)?;

let edit = Edit::insertion(".".to_string(), after_leading_quote);

Some(Fix::unsafe_edit(edit))
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_use_pathlib/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
pub(crate) use dotless_pathlib_with_suffix::*;
pub(crate) use glob_rule::*;
pub(crate) use os_path_getatime::*;
pub(crate) use os_path_getctime::*;
Expand All @@ -7,6 +8,7 @@ pub(crate) use os_sep_split::*;
pub(crate) use path_constructor_current_directory::*;
pub(crate) use replaceable_by_pathlib::*;

mod dotless_pathlib_with_suffix;
mod glob_rule;
mod os_path_getatime;
mod os_path_getctime;
Expand Down
Loading

0 comments on commit 89368a6

Please sign in to comment.