Skip to content

Commit

Permalink
[refurb] Implement print-empty-string (FURB105) (#7617)
Browse files Browse the repository at this point in the history
## Summary

Implement
[`simplify-print`](https://github.com/dosisod/refurb/blob/master/refurb/checks/builtin/print.py)
as `print-empty-string` (`FURB105`).

Extends the original rule in that it also checks for multiple empty
string positional arguments with an empty string separator.

Related to #1348.

## Test Plan

`cargo test`
  • Loading branch information
tjkuson committed Sep 24, 2023
1 parent 865c898 commit 604cf52
Show file tree
Hide file tree
Showing 8 changed files with 535 additions and 0 deletions.
32 changes: 32 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB105.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Errors.

print("")
print("", sep=",")
print("", end="bar")
print("", sep=",", end="bar")
print(sep="")
print("", sep="")
print("", "", sep="")
print("", "", sep="", end="")
print("", "", sep="", end="bar")
print("", sep="", end="bar")
print(sep="", end="bar")
print("", "foo", sep="")
print("foo", "", sep="")
print("foo", "", "bar", sep="")
print("", *args)
print("", *args, sep="")

# OK.

print()
print("foo")
print("", "")
print("", "foo")
print("foo", "")
print("", "", sep=",")
print("", "foo", sep=",")
print("foo", "", sep=",")
print("foo", "", "bar", "", sep=",")
print("", "", **kwargs)
print("", **kwargs)
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 @@ -895,6 +895,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::UnsupportedMethodCallOnAll) {
flake8_pyi::rules::unsupported_method_call_on_all(checker, func);
}
if checker.enabled(Rule::PrintEmptyString) {
refurb::rules::print_empty_string(checker, call);
}
if checker.enabled(Rule::QuadraticListSummation) {
ruff::rules::quadratic_list_summation(checker, call);
}
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 @@ -914,6 +914,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Slots, "002") => (RuleGroup::Unspecified, rules::flake8_slots::rules::NoSlotsInNamedtupleSubclass),

// refurb
(Refurb, "105") => (RuleGroup::Preview, rules::refurb::rules::PrintEmptyString),
#[allow(deprecated)]
(Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend),
#[allow(deprecated)]
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 @@ -20,6 +20,7 @@ mod tests {
#[test_case(Rule::ReimplementedStarmap, Path::new("FURB140.py"))]
#[test_case(Rule::SliceCopy, Path::new("FURB145.py"))]
#[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.py"))]
#[test_case(Rule::PrintEmptyString, Path::new("FURB105.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
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
pub(crate) use check_and_remove_from_set::*;
pub(crate) use delete_full_slice::*;
pub(crate) use print_empty_string::*;
pub(crate) use reimplemented_starmap::*;
pub(crate) use repeated_append::*;
pub(crate) use slice_copy::*;
pub(crate) use unnecessary_enumerate::*;

mod check_and_remove_from_set;
mod delete_full_slice;
mod print_empty_string;
mod reimplemented_starmap;
mod repeated_append;
mod slice_copy;
Expand Down
177 changes: 177 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/print_empty_string.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Constant, Expr};
use ruff_python_codegen::Generator;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::registry::AsRule;

/// ## What it does
/// Checks for `print` calls with an empty string as the only positional
/// argument.
///
/// ## Why is this bad?
/// Prefer calling `print` without any positional arguments, which is
/// equivalent and more concise.
///
/// ## Example
/// ```python
/// print("")
/// ```
///
/// Use instead:
/// ```python
/// print()
/// ```
///
/// ## References
/// - [Python documentation: `print`](https://docs.python.org/3/library/functions.html#print)
#[violation]
pub struct PrintEmptyString {
separator: Option<Separator>,
}

impl Violation for PrintEmptyString {
const AUTOFIX: AutofixKind = AutofixKind::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let PrintEmptyString { separator } = self;
match separator {
None | Some(Separator::Retain) => format!("Unnecessary empty string passed to `print`"),
Some(Separator::Remove) => {
format!("Unnecessary empty string passed to `print` with redundant separator")
}
}
}

fn autofix_title(&self) -> Option<String> {
let PrintEmptyString { separator } = self;
match separator {
None | Some(Separator::Retain) => Some("Remove empty string".to_string()),
Some(Separator::Remove) => Some("Remove empty string and separator".to_string()),
}
}
}

/// FURB105
pub(crate) fn print_empty_string(checker: &mut Checker, call: &ast::ExprCall) {
// Avoid flagging, e.g., `print("", "", **kwargs)`.
if call
.arguments
.keywords
.iter()
.any(|keyword| keyword.arg.is_none())
{
return;
}

if checker
.semantic()
.resolve_call_path(&call.func)
.as_ref()
.is_some_and(|call_path| matches!(call_path.as_slice(), ["", "print"]))
{
// Ex) `print("", sep="")`
let empty_separator = call
.arguments
.find_keyword("sep")
.map_or(false, |keyword| is_empty_string(&keyword.value));

// Avoid flagging, e.g., `print("", "", sep="sep")`
if !empty_separator && call.arguments.args.len() != 1 {
return;
}

// Check if the positional arguments is are all empty strings, or if
// there are any empty strings and the `sep` keyword argument is also
// an empty string.
if call.arguments.args.iter().all(is_empty_string)
|| (empty_separator && call.arguments.args.iter().any(is_empty_string))
{
let separator = call
.arguments
.keywords
.iter()
.any(|keyword| {
keyword
.arg
.as_ref()
.is_some_and(|arg| arg.as_str() == "sep")
})
.then(|| {
let is_starred = call.arguments.args.iter().any(Expr::is_starred_expr);
if is_starred {
return Separator::Retain;
}

let non_empty = call
.arguments
.args
.iter()
.filter(|arg| !is_empty_string(arg))
.count();
if non_empty > 1 {
return Separator::Retain;
}

Separator::Remove
});

let mut diagnostic = Diagnostic::new(PrintEmptyString { separator }, call.range());

if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(Fix::suggested(Edit::replacement(
generate_suggestion(call, separator, checker.generator()),
call.start(),
call.end(),
)));
}

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

/// Check if an expression is a constant empty string.
fn is_empty_string(expr: &Expr) -> bool {
matches!(
expr,
Expr::Constant(ast::ExprConstant {
value: Constant::Str(s),
..
}) if s.is_empty()
)
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum Separator {
Remove,
Retain,
}

/// Generate a suggestion to remove the empty string positional argument and
/// the `sep` keyword argument, if it exists.
fn generate_suggestion(
call: &ast::ExprCall,
separator: Option<Separator>,
generator: Generator,
) -> String {
let mut call = call.clone();

// Remove all empty string positional arguments.
call.arguments.args.retain(|arg| !is_empty_string(arg));

// Remove the `sep` keyword argument if it exists.
if separator == Some(Separator::Remove) {
call.arguments.keywords.retain(|keyword| {
keyword
.arg
.as_ref()
.map_or(true, |arg| arg.as_str() != "sep")
});
}

generator.expr(&call.into())
}

0 comments on commit 604cf52

Please sign in to comment.