Skip to content

Commit

Permalink
Refactor whitespace around operator (#4223)
Browse files Browse the repository at this point in the history
  • Loading branch information
@MichaReiser
MichaReiser authored May 5, 2023
1 parent 2124feb commit e93f378
Show file tree
Hide file tree
Showing 5 changed files with 269 additions and 170 deletions.
9 changes: 5 additions & 4 deletions crates/ruff/src/rules/pycodestyle/rules/logical_lines/missing_whitespace_after_keyword.rs
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
use itertools::Itertools;
use ruff_text_size::TextRange;

use crate::checkers::logical_lines::LogicalLinesContext;
use crate::rules::pycodestyle::rules::logical_lines::LogicalLine;
use ruff_diagnostics::Violation;
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::token_kind::TokenKind;
use ruff_text_size::TextRange;

#[violation]
pub struct MissingWhitespaceAfterKeyword;
Expand All @@ -22,7 +20,10 @@ pub(crate) fn missing_whitespace_after_keyword(
line: &LogicalLine,
context: &mut LogicalLinesContext,
) {
for (tok0, tok1) in line.tokens().iter().tuple_windows() {
for window in line.tokens().windows(2) {
let tok0 = &window[0];
let tok1 = &window[1];

let tok0_kind = tok0.kind();
let tok1_kind = tok1.kind();

Expand Down
259 changes: 153 additions & 106 deletions crates/ruff/src/rules/pycodestyle/rules/logical_lines/missing_whitespace_around_operator.rs
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use crate::checkers::logical_lines::LogicalLinesContext;
use ruff_diagnostics::Violation;
use crate::rules::pycodestyle::rules::logical_lines::{LogicalLine, LogicalLineToken};
use ruff_diagnostics::{DiagnosticKind, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::token_kind::TokenKind;
use ruff_text_size::{TextRange, TextSize};

use crate::rules::pycodestyle::rules::logical_lines::LogicalLine;
use ruff_text_size::TextRange;

// E225
#[violation]
Expand Down Expand Up @@ -56,131 +55,179 @@ pub(crate) fn missing_whitespace_around_operator(
line: &LogicalLine,
context: &mut LogicalLinesContext,
) {
#[derive(Copy, Clone, Eq, PartialEq)]
enum NeedsSpace {
Yes,
No,
Unset,
}

let mut needs_space_main = NeedsSpace::No;
let mut needs_space_aux = NeedsSpace::Unset;
let mut prev_end_aux = TextSize::default();
let mut parens = 0u32;
let mut prev_type: TokenKind = TokenKind::EndOfFile;
let mut prev_end = TextSize::default();
let mut prev_token: Option<&LogicalLineToken> = None;
let mut tokens = line.tokens().iter().peekable();

for token in line.tokens() {
while let Some(token) = tokens.next() {
let kind = token.kind();

if kind.is_skip_comment() {
if kind.is_trivia() {
continue;
}

match kind {
TokenKind::Lpar | TokenKind::Lambda => parens += 1,
TokenKind::Rpar => parens -= 1,
TokenKind::Rpar => parens = parens.saturating_sub(1),
_ => {}
};

let needs_space = needs_space_main == NeedsSpace::Yes
|| needs_space_aux != NeedsSpace::Unset
|| prev_end_aux != TextSize::new(0);
if needs_space {
if token.start() > prev_end {
if needs_space_main != NeedsSpace::Yes && needs_space_aux != NeedsSpace::Yes {
let needs_space = if kind == TokenKind::Equal && parens > 0 {
// Allow keyword args or defaults: foo(bar=None).
NeedsSpace::No
} else if kind == TokenKind::Slash {
// Tolerate the "/" operator in function definition
// For more info see PEP570

// `def f(a, /, b):` or `def f(a, b, /):` or `f = lambda a, /:`
// ^ ^ ^
let slash_in_func = matches!(
tokens.peek().map(|t| t.kind()),
Some(TokenKind::Comma | TokenKind::Rpar | TokenKind::Colon)
);

NeedsSpace::from(!slash_in_func)
} else if kind.is_unary() || kind == TokenKind::DoubleStar {
let is_binary = prev_token.map_or(false, |prev_token| {
let prev_kind = prev_token.kind();

// Check if the operator is used as a binary operator.
// Allow unary operators: -123, -x, +1.
// Allow argument unpacking: foo(*args, **kwargs)
matches!(
prev_kind,
TokenKind::Rpar | TokenKind::Rsqb | TokenKind::Rbrace
) || !(prev_kind.is_operator()
|| prev_kind.is_keyword()
|| prev_kind.is_soft_keyword())
});

if is_binary {
if kind == TokenKind::DoubleStar {
// Enforce consistent spacing, but don't enforce whitespaces.
NeedsSpace::Optional
} else {
NeedsSpace::Yes
}
} else {
NeedsSpace::No
}
} else if is_whitespace_needed(kind) {
NeedsSpace::Yes
} else {
NeedsSpace::No
};

if needs_space != NeedsSpace::No {
let has_leading_trivia = prev_token.map_or(true, |prev| {
prev.end() < token.start() || prev.kind().is_trivia()
});

let has_trailing_trivia = tokens.peek().map_or(true, |next| {
token.end() < next.start() || next.kind().is_trivia()
});

match (has_leading_trivia, has_trailing_trivia) {
// Operator with trailing but no leading space, enforce consistent spacing
(false, true) => {
context.push(
MissingWhitespaceAroundOperator,
TextRange::empty(prev_end_aux),
TextRange::empty(token.start()),
);
}
needs_space_main = NeedsSpace::No;
needs_space_aux = NeedsSpace::Unset;
prev_end_aux = TextSize::new(0);
} else if kind == TokenKind::Greater
&& matches!(prev_type, TokenKind::Less | TokenKind::Minus)
{
// Tolerate the "<>" operator, even if running Python 3
// Deal with Python 3's annotated return value "->"
} else if prev_type == TokenKind::Slash
&& matches!(kind, TokenKind::Comma | TokenKind::Rpar | TokenKind::Colon)
|| (prev_type == TokenKind::Rpar && kind == TokenKind::Colon)
{
// Tolerate the "/" operator in function definition
// For more info see PEP570
} else {
if needs_space_main == NeedsSpace::Yes || needs_space_aux == NeedsSpace::Yes {
context.push(MissingWhitespaceAroundOperator, TextRange::empty(prev_end));
} else if prev_type != TokenKind::DoubleStar {
if prev_type == TokenKind::Percent {
context.push(
MissingWhitespaceAroundModuloOperator,
TextRange::empty(prev_end_aux),
);
} else if !prev_type.is_arithmetic() {
context.push(
MissingWhitespaceAroundBitwiseOrShiftOperator,
TextRange::empty(prev_end_aux),
);
} else {
// Operator with leading but no trailing space, enforce consistent spacing.
(true, false) => {
context.push(
MissingWhitespaceAroundOperator,
TextRange::empty(token.end()),
);
}
// Operator with no space, require spaces if it is required by the operator.
(false, false) => {
if needs_space == NeedsSpace::Yes {
context.push(
MissingWhitespaceAroundArithmeticOperator,
TextRange::empty(prev_end_aux),
diagnostic_kind_for_operator(kind),
TextRange::empty(token.start()),
);
}
}
needs_space_main = NeedsSpace::No;
needs_space_aux = NeedsSpace::Unset;
prev_end_aux = TextSize::new(0);
}
} else if (kind.is_operator() || matches!(kind, TokenKind::Name))
&& prev_end != TextSize::default()
{
if kind == TokenKind::Equal && parens > 0 {
// Allow keyword args or defaults: foo(bar=None).
} else if kind.is_whitespace_needed() {
needs_space_main = NeedsSpace::Yes;
needs_space_aux = NeedsSpace::Unset;
prev_end_aux = TextSize::new(0);
} else if kind.is_unary() {
// Check if the operator is used as a binary operator
// Allow unary operators: -123, -x, +1.
// Allow argument unpacking: foo(*args, **kwargs)
if (matches!(
prev_type,
TokenKind::Rpar | TokenKind::Rsqb | TokenKind::Rbrace
)) || (!prev_type.is_operator()
&& !prev_type.is_keyword()
&& !prev_type.is_soft_keyword())
{
needs_space_main = NeedsSpace::Unset;
needs_space_aux = NeedsSpace::Unset;
prev_end_aux = TextSize::new(0);
(true, true) => {
// Operator has leading and trailing space, all good
}
} else if kind.is_whitespace_optional() {
needs_space_main = NeedsSpace::Unset;
needs_space_aux = NeedsSpace::Unset;
prev_end_aux = TextSize::new(0);
}
}

if needs_space_main == NeedsSpace::Unset {
// Surrounding space is optional, but ensure that
// trailing space matches opening space
prev_end_aux = prev_end;
needs_space_aux = if token.start() == prev_end {
NeedsSpace::No
} else {
NeedsSpace::Yes
};
} else if needs_space_main == NeedsSpace::Yes && token.start() == prev_end_aux {
// A needed opening space was not found
context.push(MissingWhitespaceAroundOperator, TextRange::empty(prev_end));
needs_space_main = NeedsSpace::No;
needs_space_aux = NeedsSpace::Unset;
prev_end_aux = TextSize::new(0);
}
prev_token = Some(token);
}
}

#[derive(Copy, Clone, Eq, PartialEq, Debug)]
enum NeedsSpace {
/// Needs a leading and trailing space.
Yes,

/// Doesn't need a leading or trailing space. Or in other words, we don't care how many
/// leading or trailing spaces that token has.
No,

/// Needs consistent leading and trailing spacing. The operator needs spacing if
/// * it has a leading space
/// * it has a trailing space
Optional,
}

impl From<bool> for NeedsSpace {
fn from(value: bool) -> Self {
if value {
NeedsSpace::Yes
} else {
NeedsSpace::No
}
prev_type = kind;
prev_end = token.end();
}
}

fn diagnostic_kind_for_operator(operator: TokenKind) -> DiagnosticKind {
if operator == TokenKind::Percent {
DiagnosticKind::from(MissingWhitespaceAroundModuloOperator)
} else if operator.is_bitwise_or_shift() {
DiagnosticKind::from(MissingWhitespaceAroundBitwiseOrShiftOperator)
} else if operator.is_arithmetic() {
DiagnosticKind::from(MissingWhitespaceAroundArithmeticOperator)
} else {
DiagnosticKind::from(MissingWhitespaceAroundOperator)
}
}

fn is_whitespace_needed(kind: TokenKind) -> bool {
matches!(
kind,
TokenKind::DoubleStarEqual
| TokenKind::StarEqual
| TokenKind::SlashEqual
| TokenKind::DoubleSlashEqual
| TokenKind::PlusEqual
| TokenKind::MinusEqual
| TokenKind::NotEqual
| TokenKind::Less
| TokenKind::Greater
| TokenKind::PercentEqual
| TokenKind::CircumflexEqual
| TokenKind::AmperEqual
| TokenKind::VbarEqual
| TokenKind::EqEqual
| TokenKind::LessEqual
| TokenKind::GreaterEqual
| TokenKind::LeftShiftEqual
| TokenKind::RightShiftEqual
| TokenKind::Equal
| TokenKind::And
| TokenKind::Or
| TokenKind::In
| TokenKind::Is
| TokenKind::Rarrow
| TokenKind::ColonEqual
| TokenKind::Slash
| TokenKind::Percent
) || kind.is_arithmetic()
|| kind.is_bitwise_or_shift()
}
Loading

0 comments on commit e93f378

Please sign in to comment.