Skip to content

Commit

Permalink
[ruff] Add assert-with-print-expression rule (#11974) (#11981)
Browse files Browse the repository at this point in the history
## Summary

Addresses #11974 to add a `RUF` rule to replace `print` expressions in
`assert` statements with the inner message.

An autofix is available, but is considered unsafe as it changes
behaviour of the execution, notably:
- removal of the printout in `stdout`, and
- `AssertionError` instance containing a different message.

While the detection of the condition is a straightforward matter,
deciding how to resolve the print arguments into a string literal can be
a relatively subjective matter. The implementation of this PR chooses to
be as tolerant as possible, and will attempt to reformat any number of
`print` arguments containing single or concatenated strings or variables
into either a string literal, or a f-string if any variables or
placeholders are detected.

## Test Plan

`cargo test`.

## Examples
For ease of discussion, this is the diff for the tests:

```diff
 # Standard Case
 # Expects:
 # - single StringLiteral
-assert True, print("This print is not intentional.")
+assert True, "This print is not intentional."
 
 # Concatenated string literals
 # Expects:
 # - single StringLiteral
-assert True, print("This print" " is not intentional.")
+assert True, "This print is not intentional."
 
 # Positional arguments, string literals
 # Expects:
 # - single StringLiteral concatenated with " "
-assert True, print("This print", "is not intentional")
+assert True, "This print is not intentional"
 
 # Concatenated string literals combined with Positional arguments
 # Expects:
 # - single stringliteral concatenated with " " only between `print` and `is`
-assert True, print("This " "print", "is not intentional.")
+assert True, "This print is not intentional."
 
 # Positional arguments, string literals with a variable
 # Expects:
 # - single FString concatenated with " "
-assert True, print("This", print.__name__, "is not intentional.")
+assert True, f"This {print.__name__} is not intentional."

 # Mixed brackets string literals
 # Expects:
 # - single StringLiteral concatenated with " "
-assert True, print("This print", 'is not intentional', """and should be removed""")
+assert True, "This print is not intentional and should be removed"
 
 # Mixed brackets with other brackets inside
 # Expects:
 # - single StringLiteral concatenated with " " and escaped brackets
-assert True, print("This print", 'is not "intentional"', """and "should" be 'removed'""")
+assert True, "This print is not \"intentional\" and \"should\" be 'removed'"
 
 # Positional arguments, string literals with a separator
 # Expects:
 # - single StringLiteral concatenated with "|"
-assert True, print("This print", "is not intentional", sep="|")
+assert True, "This print|is not intentional"
 
 # Positional arguments, string literals with None as separator
 # Expects:
 # - single StringLiteral concatenated with " "
-assert True, print("This print", "is not intentional", sep=None)
+assert True, "This print is not intentional"
 
 # Positional arguments, string literals with variable as separator, needs f-string
 # Expects:
 # - single FString concatenated with "{U00A0}"
-assert True, print("This print", "is not intentional", sep=U00A0)
+assert True, f"This print{U00A0}is not intentional"
 
 # Unnecessary f-string
 # Expects:
 # - single StringLiteral
-assert True, print(f"This f-string is just a literal.")
+assert True, "This f-string is just a literal."
 
 # Positional arguments, string literals and f-strings
 # Expects:
 # - single FString concatenated with " "
-assert True, print("This print", f"is not {'intentional':s}")
+assert True, f"This print is not {'intentional':s}"
 
 # Positional arguments, string literals and f-strings with a separator
 # Expects:
 # - single FString concatenated with "|"
-assert True, print("This print", f"is not {'intentional':s}", sep="|")
+assert True, f"This print|is not {'intentional':s}"
 
 # A single f-string
 # Expects:
 # - single FString
-assert True, print(f"This print is not {'intentional':s}")
+assert True, f"This print is not {'intentional':s}"
 
 # A single f-string with a redundant separator
 # Expects:
 # - single FString
-assert True, print(f"This print is not {'intentional':s}", sep="|")
+assert True, f"This print is not {'intentional':s}"
 
 # Complex f-string with variable as separator
 # Expects:
 # - single FString concatenated with "{U00A0}", all placeholders preserved
 condition = "True is True"
 maintainer = "John Doe"
-assert True, print("Unreachable due to", condition, f", ask {maintainer} for advice", sep=U00A0)
+assert True, f"Unreachable due to{U00A0}{condition}{U00A0}, ask {maintainer} for advice"
 
 # Empty print
 # Expects:
 # - `msg` entirely removed from assertion
-assert True, print()
+assert True
 
 # Empty print with separator
 # Expects:
 # - `msg` entirely removed from assertion
-assert True, print(sep=" ")
+assert True
 
 # Custom print function that actually returns a string
 # Expects:
@@ -100,4 +100,4 @@
 # Use of `builtins.print`
 # Expects:
 # - single StringLiteral
-assert True, builtins.print("This print should be removed.")
+assert True, "This print should be removed."
```

## Known Issues

The current implementation resolves all arguments and separators of the
`print` expression into a single string, be it
`StringLiteralValue::single` or a `FStringValue::single`. This:

- potentially joins together strings well beyond the ideal character
limit for each line, and
- does not preserve multi-line strings in their original format, in
favour of a single line `"...\n...\n..."` format.

These are purely formatting issues only occurring in unusual scenarios.

Additionally, the autofix will tolerate `print` calls that were
previously invalid:

```python
assert True, print("this", "should not be allowed", sep=42)
```

This will be transformed into
```python
assert True, f"this{42}should not be allowed"
```
which some could argue is an alteration of behaviour.

---------

Co-authored-by: Charlie Marsh <[email protected]>
  • Loading branch information
denwong47 and charliermarsh authored Jun 23, 2024
1 parent 0c8b5eb commit c3f61a0
Show file tree
Hide file tree
Showing 8 changed files with 810 additions and 5 deletions.
108 changes: 108 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF030.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
U00A0 = "\u00a0"

# Standard Case
# Expects:
# - single StringLiteral
assert True, print("This print is not intentional.")

# Concatenated string literals
# Expects:
# - single StringLiteral
assert True, print("This print" " is not intentional.")

# Positional arguments, string literals
# Expects:
# - single StringLiteral concatenated with " "
assert True, print("This print", "is not intentional")

# Concatenated string literals combined with Positional arguments
# Expects:
# - single stringliteral concatenated with " " only between `print` and `is`
assert True, print("This " "print", "is not intentional.")

# Positional arguments, string literals with a variable
# Expects:
# - single FString concatenated with " "
assert True, print("This", print.__name__, "is not intentional.")

# Mixed brackets string literals
# Expects:
# - single StringLiteral concatenated with " "
assert True, print("This print", 'is not intentional', """and should be removed""")

# Mixed brackets with other brackets inside
# Expects:
# - single StringLiteral concatenated with " " and escaped brackets
assert True, print("This print", 'is not "intentional"', """and "should" be 'removed'""")

# Positional arguments, string literals with a separator
# Expects:
# - single StringLiteral concatenated with "|"
assert True, print("This print", "is not intentional", sep="|")

# Positional arguments, string literals with None as separator
# Expects:
# - single StringLiteral concatenated with " "
assert True, print("This print", "is not intentional", sep=None)

# Positional arguments, string literals with variable as separator, needs f-string
# Expects:
# - single FString concatenated with "{U00A0}"
assert True, print("This print", "is not intentional", sep=U00A0)

# Unnecessary f-string
# Expects:
# - single StringLiteral
assert True, print(f"This f-string is just a literal.")

# Positional arguments, string literals and f-strings
# Expects:
# - single FString concatenated with " "
assert True, print("This print", f"is not {'intentional':s}")

# Positional arguments, string literals and f-strings with a separator
# Expects:
# - single FString concatenated with "|"
assert True, print("This print", f"is not {'intentional':s}", sep="|")

# A single f-string
# Expects:
# - single FString
assert True, print(f"This print is not {'intentional':s}")

# A single f-string with a redundant separator
# Expects:
# - single FString
assert True, print(f"This print is not {'intentional':s}", sep="|")

# Complex f-string with variable as separator
# Expects:
# - single FString concatenated with "{U00A0}", all placeholders preserved
condition = "True is True"
maintainer = "John Doe"
assert True, print("Unreachable due to", condition, f", ask {maintainer} for advice", sep=U00A0)

# Empty print
# Expects:
# - `msg` entirely removed from assertion
assert True, print()

# Empty print with separator
# Expects:
# - `msg` entirely removed from assertion
assert True, print(sep=" ")

# Custom print function that actually returns a string
# Expects:
# - no violation as the function is not a built-in print
def print(s: str):
return "This is my assertion error message: " + s

assert True, print("this print shall not be removed.")

import builtins

# Use of `builtins.print`
# Expects:
# - single StringLiteral
assert True, builtins.print("This print should be removed.")
15 changes: 10 additions & 5 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1232,11 +1232,13 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
}
}
}
Stmt::Assert(ast::StmtAssert {
test,
msg,
range: _,
}) => {
Stmt::Assert(
assert_stmt @ ast::StmtAssert {
test,
msg,
range: _,
},
) => {
if !checker.semantic.in_type_checking_block() {
if checker.enabled(Rule::Assert) {
checker
Expand Down Expand Up @@ -1267,6 +1269,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::InvalidMockAccess) {
pygrep_hooks::rules::non_existent_mock_method(checker, test);
}
if checker.enabled(Rule::AssertWithPrintMessage) {
ruff::rules::assert_with_print_message(checker, assert_stmt);
}
}
Stmt::With(with_stmt @ ast::StmtWith { items, body, .. }) => {
if checker.enabled(Rule::TooManyNestedBlocks) {
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 @@ -977,6 +977,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "027") => (RuleGroup::Preview, rules::ruff::rules::MissingFStringSyntax),
(Ruff, "028") => (RuleGroup::Preview, rules::ruff::rules::InvalidFormatterSuppressionComment),
(Ruff, "029") => (RuleGroup::Preview, rules::ruff::rules::UnusedAsync),
(Ruff, "030") => (RuleGroup::Preview, rules::ruff::rules::AssertWithPrintMessage),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "101") => (RuleGroup::Preview, rules::ruff::rules::RedirectedNOQA),
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ mod tests {
#[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_2.py"))]
#[test_case(Rule::InvalidFormatterSuppressionComment, Path::new("RUF028.py"))]
#[test_case(Rule::UnusedAsync, Path::new("RUF029.py"))]
#[test_case(Rule::AssertWithPrintMessage, Path::new("RUF030.py"))]
#[test_case(Rule::RedirectedNOQA, Path::new("RUF101.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
Expand Down
Loading

0 comments on commit c3f61a0

Please sign in to comment.