Skip to content

Commit

Permalink
Fix corner case involving terminal backslash after fixing W293 (#5172)
Browse files Browse the repository at this point in the history
## Summary

Fixes #4404. 

Consider this file:
```python
if True:
    x = 1; \
<space><space><space>
```

The current implementation of W293 removes the 3 spaces on line 2. This
fix changes the file to:
```python
if True:
    x = 1; \
```
A file can't end in a `\`, according to Python's [lexical
analysis](https://docs.python.org/3/reference/lexical_analysis.html), so
subsequent iterations of the autofixer fail (the AST-based ones
specifically, since they depend on a valid syntax tree and get
re-parsed).

This patch examines the line before the line checked in `W293`. If its
first non-whitespace character is a `\`, the patch will extend the
diagnostic's fix range to all whitespace up until the previous line's
*second* non-whitespace character; that is, it deletes all spaces and
potential `\`s up until the next non-whitespace character on the
previous line.

## Test Plan
Ran `cargo run -p ruff_cli -- ~/Downloads/aa.py --fix --select W293,D100
--no-cache` against the above file. This resulted in:
```
/Users/evan/Downloads/aa.py:1:1: D100 Missing docstring in public module
Found 2 errors (1 fixed, 1 remaining).
```
The file's contents, after the fix:
```python
if True:
    x = 1;<space>
```
The `\` was removed, leaving the terminal space. The space should be
handled by `Rule::TrailingWhitespace`, not `BlankLineWithWhitespace`.
  • Loading branch information
evanrittenhouse committed Jun 20, 2023
1 parent 64bd955 commit 62aa77d
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 12 deletions.
7 changes: 4 additions & 3 deletions crates/ruff/src/checkers/physical_lines.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Lint rules based on checking physical lines.
use std::path::Path;

use ruff_text_size::TextSize;
use std::path::Path;

use ruff_diagnostics::Diagnostic;
use ruff_python_ast::source_code::{Indexer, Locator, Stylist};
Expand Down Expand Up @@ -146,7 +146,7 @@ pub(crate) fn check_physical_lines(
}

if enforce_trailing_whitespace || enforce_blank_line_contains_whitespace {
if let Some(diagnostic) = trailing_whitespace(&line, settings) {
if let Some(diagnostic) = trailing_whitespace(&line, locator, indexer, settings) {
diagnostics.push(diagnostic);
}
}
Expand Down Expand Up @@ -185,9 +185,10 @@ pub(crate) fn check_physical_lines(

#[cfg(test)]
mod tests {
use std::path::Path;

use rustpython_parser::lexer::lex;
use rustpython_parser::Mode;
use std::path::Path;

use ruff_python_ast::source_code::{Indexer, Locator, Stylist};

Expand Down
21 changes: 16 additions & 5 deletions crates/ruff/src/rules/pycodestyle/rules/trailing_whitespace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ use ruff_text_size::{TextLen, TextRange, TextSize};

use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers;
use ruff_python_ast::source_code::{Indexer, Locator};
use ruff_python_whitespace::Line;

use crate::registry::Rule;
Expand Down Expand Up @@ -72,7 +74,12 @@ impl AlwaysAutofixableViolation for BlankLineWithWhitespace {
}

/// W291, W293
pub(crate) fn trailing_whitespace(line: &Line, settings: &Settings) -> Option<Diagnostic> {
pub(crate) fn trailing_whitespace(
line: &Line,
locator: &Locator,
indexer: &Indexer,
settings: &Settings,
) -> Option<Diagnostic> {
let whitespace_len: TextSize = line
.chars()
.rev()
Expand All @@ -86,16 +93,20 @@ pub(crate) fn trailing_whitespace(line: &Line, settings: &Settings) -> Option<Di
if settings.rules.enabled(Rule::BlankLineWithWhitespace) {
let mut diagnostic = Diagnostic::new(BlankLineWithWhitespace, range);
if settings.rules.should_fix(Rule::BlankLineWithWhitespace) {
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_deletion(range)));
// Remove any preceding continuations, to avoid introducing a potential
// syntax error.
diagnostic.set_fix(Fix::automatic(Edit::range_deletion(TextRange::new(
helpers::preceded_by_continuations(line.start(), locator, indexer)
.unwrap_or(range.start()),
range.end(),
))));
}
return Some(diagnostic);
}
} else if settings.rules.enabled(Rule::TrailingWhitespace) {
let mut diagnostic = Diagnostic::new(TrailingWhitespace, range);
if settings.rules.should_fix(Rule::TrailingWhitespace) {
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_deletion(range)));
diagnostic.set_fix(Fix::automatic(Edit::range_deletion(range)));
}
return Some(diagnostic);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ W29.py:4:6: W291 [*] Trailing whitespace
|
= help: Remove trailing whitespace

Suggested fix
Fix
1 1 | #: Okay
2 2 | # 情
3 3 | #: W291:1:6
Expand All @@ -33,7 +33,7 @@ W29.py:11:35: W291 [*] Trailing whitespace
|
= help: Remove trailing whitespace

Suggested fix
Fix
8 8 | bang = 12
9 9 | #: W291:2:35
10 10 | '''multiline
Expand All @@ -54,7 +54,7 @@ W29.py:13:6: W291 [*] Trailing whitespace
|
= help: Remove trailing whitespace

Suggested fix
Fix
10 10 | '''multiline
11 11 | string with trailing whitespace'''
12 12 | #: W291 W292 noeol
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ W29.py:7:1: W293 [*] Blank line contains whitespace
|
= help: Remove whitespace from blank line

Suggested fix
Fix
4 4 | print
5 5 | #: W293:2:1
6 6 | class Foo(object):
Expand Down

0 comments on commit 62aa77d

Please sign in to comment.