Skip to content

Commit

Permalink
[pycodestyle] Fix whitespace-related false positives and false nega…
Browse files Browse the repository at this point in the history
…tives inside type-parameter lists
  • Loading branch information
AlexWaygood committed Oct 10, 2024
1 parent 5b4afd3 commit 1f39a4b
Show file tree
Hide file tree
Showing 7 changed files with 830 additions and 32 deletions.
34 changes: 34 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pycodestyle/E23.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,3 +104,37 @@ def main() -> None:
]
}
]

# Should be E231 errors on all of these type parameters and function parameters, but not on their (strange) defaults
def pep_696_bad[A:object="foo"[::-1], B:object =[[["foo", "bar"]]], C:object= bytes](
x:A = "foo"[::-1],
y:B = [[["foo", "bar"]]],
z:object = "fooo",
):
pass

class PEP696Bad[A:object="foo"[::-1], B:object =[[["foo", "bar"]]], C:object= bytes]:
pass

class PEP696BadWithEmptyBases[A:object="foo"[::-1], B:object =[[["foo", "bar"]]], C:object= bytes]():
pass

class PEP696BadWithNonEmptyBases[A:object="foo"[::-1], B:object =[[["foo", "bar"]]], C:object= bytes](object, something_dynamic[x::-1]):
pass

# Should be no E231 errors on any of these:
def pep_696_good[A: object="foo"[::-1], B: object =[[["foo", "bar"]]], C: object= bytes](
x: A = "foo"[::-1],
y: B = [[["foo", "bar"]]],
z: object = "fooo",
):
pass

class PEP696Good[A: object="foo"[::-1], B: object =[[["foo", "bar"]]], C: object= bytes]:
pass

class PEP696GoodWithEmptyBases[A: object="foo"[::-1], B: object =[[["foo", "bar"]]], C: object= bytes]():
pass

class PEP696GoodWithNonEmptyBases[A: object="foo"[::-1], B: object =[[["foo", "bar"]]], C: object= bytes](object, something_dynamic[x::-1]):
pass
15 changes: 15 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pycodestyle/E25.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,18 @@ def add(a: int = _default(name='f')):
print(f"{foo = }")
# ...but then it creates false negatives for now
print(f"{foo(a = 1)}")

# There should be at least one E251 diagnostic for each type parameter here:
def pep_696_bad[A=int, B =str, C= bool, D:object=int, E: object=str, F: object =bool, G: object= bytes]():
pass

class PEP696Bad[A=int, B =str, C= bool, D:object=int, E: object=str, F: object =bool, G: object= bytes]:
pass

# The last of these should cause us to emit E231,
# but E231 isn't tested by this fixture:
def pep_696_good[A = int, B: object = str, C:object = memoryview]():
pass

class PEP696Good[A = int, B: object = str, C:object = memoryview]:
pass
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use ruff_text_size::Ranged;

use crate::checkers::logical_lines::LogicalLinesContext;

use super::LogicalLine;
use super::{LogicalLine, TypeParamsState};

/// ## What it does
/// Checks for missing whitespace after `,`, `;`, and `:`.
Expand All @@ -28,22 +28,10 @@ pub struct MissingWhitespace {
token: TokenKind,
}

impl MissingWhitespace {
fn token_text(&self) -> char {
match self.token {
TokenKind::Colon => ':',
TokenKind::Semi => ';',
TokenKind::Comma => ',',
_ => unreachable!(),
}
}
}

impl AlwaysFixableViolation for MissingWhitespace {
#[derive_message_formats]
fn message(&self) -> String {
let token = self.token_text();
format!("Missing whitespace after '{token}'")
format!("Missing whitespace after {}", self.token)
}

fn fix_title(&self) -> String {
Expand All @@ -54,11 +42,13 @@ impl AlwaysFixableViolation for MissingWhitespace {
/// E231
pub(crate) fn missing_whitespace(line: &LogicalLine, context: &mut LogicalLinesContext) {
let mut fstrings = 0u32;
let mut type_params_state = TypeParamsState::new();
let mut brackets = Vec::new();
let mut iter = line.tokens().iter().peekable();

while let Some(token) = iter.next() {
let kind = token.kind();
type_params_state.visit_token_kind(kind);
match kind {
TokenKind::FStringStart => fstrings += 1,
TokenKind::FStringEnd => fstrings = fstrings.saturating_sub(1),
Expand Down Expand Up @@ -97,7 +87,9 @@ pub(crate) fn missing_whitespace(line: &LogicalLine, context: &mut LogicalLinesC
if let Some(next_token) = iter.peek() {
match (kind, next_token.kind()) {
(TokenKind::Colon, _)
if matches!(brackets.last(), Some(TokenKind::Lsqb)) =>
if matches!(brackets.last(), Some(TokenKind::Lsqb))
&& !(type_params_state.in_type_params()
&& brackets.len() == 1) =>
{
continue; // Slice syntax, no space required
}
Expand All @@ -111,13 +103,10 @@ pub(crate) fn missing_whitespace(line: &LogicalLine, context: &mut LogicalLinesC
}
}

let mut diagnostic =
let diagnostic =
Diagnostic::new(MissingWhitespace { token: kind }, token.range());
diagnostic.set_fix(Fix::safe_edit(Edit::insertion(
" ".to_string(),
token.end(),
)));
context.push_diagnostic(diagnostic);
let fix = Fix::safe_edit(Edit::insertion(" ".to_string(), token.end()));
context.push_diagnostic(diagnostic.with_fix(fix));
}
}
_ => {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,74 @@ struct Line {
tokens_end: u32,
}

/// Keeps track of whether we are currently visiting the [type parameters]
/// of a class or function definition in a [`LogicalLine`].
///
/// Call [`TypeParamsState::visit_token_kind`] on the [`TokenKind`] of each
/// successive [`LogicalLineToken`] to ensure the state remains up to date.
///
/// [type parameters]: https://docs.python.org/3/reference/compound_stmts.html#type-params
#[derive(Debug, Clone, Copy)]
enum TypeParamsState {
BeforeClassOrDefKeyword,
BeforeTypeParams,
InTypeParams { inner_square_brackets: u32 },
TypeParamsEnded,
}

impl TypeParamsState {
const fn new() -> Self {
Self::BeforeClassOrDefKeyword
}

const fn in_class_or_function_def(self) -> bool {
!matches!(self, Self::BeforeClassOrDefKeyword)
}

const fn before_type_params(self) -> bool {
matches!(self, Self::BeforeTypeParams)
}

const fn in_type_params(self) -> bool {
matches!(self, Self::InTypeParams { .. })
}

fn visit_token_kind(&mut self, token: TokenKind) {
match token {
TokenKind::Class | TokenKind::Def if !self.in_class_or_function_def() => {
*self = TypeParamsState::BeforeTypeParams;
}
TokenKind::Lpar if self.before_type_params() => {
*self = TypeParamsState::TypeParamsEnded;
}
TokenKind::Lsqb => match self {
TypeParamsState::BeforeClassOrDefKeyword | TypeParamsState::TypeParamsEnded => {}
TypeParamsState::BeforeTypeParams => {
*self = TypeParamsState::InTypeParams {
inner_square_brackets: 0,
};
}
TypeParamsState::InTypeParams {
inner_square_brackets,
} => *inner_square_brackets += 1,
},
TokenKind::Rsqb => {
if let TypeParamsState::InTypeParams {
inner_square_brackets,
} = self
{
if *inner_square_brackets == 0 {
*self = TypeParamsState::TypeParamsEnded;
} else {
*inner_square_brackets -= 1;
}
}
}
_ => {}
}
}
}

#[cfg(test)]
mod tests {
use ruff_python_parser::parse_module;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ use ruff_python_parser::TokenKind;
use ruff_text_size::{Ranged, TextRange, TextSize};

use crate::checkers::logical_lines::LogicalLinesContext;
use crate::rules::pycodestyle::rules::logical_lines::{LogicalLine, LogicalLineToken};
use crate::rules::pycodestyle::rules::logical_lines::{
LogicalLine, LogicalLineToken, TypeParamsState,
};

/// ## What it does
/// Checks for missing whitespace around the equals sign in an unannotated
Expand Down Expand Up @@ -106,17 +108,15 @@ pub(crate) fn whitespace_around_named_parameter_equals(
let mut annotated_func_arg = false;
let mut prev_end = TextSize::default();

let mut type_params_state = TypeParamsState::new();
let in_def = is_in_def(line.tokens());
let mut iter = line.tokens().iter().peekable();

while let Some(token) = iter.next() {
let kind = token.kind();

if kind == TokenKind::NonLogicalNewline {
continue;
}

match kind {
let token_kind = token.kind();
type_params_state.visit_token_kind(token_kind);
match token_kind {
TokenKind::NonLogicalNewline => continue,
TokenKind::FStringStart => fstrings += 1,
TokenKind::FStringEnd => fstrings = fstrings.saturating_sub(1),
TokenKind::Lpar | TokenKind::Lsqb => {
Expand All @@ -128,15 +128,16 @@ pub(crate) fn whitespace_around_named_parameter_equals(
annotated_func_arg = false;
}
}

TokenKind::Colon if parens == 1 && in_def => {
annotated_func_arg = true;
}
TokenKind::Comma if parens == 1 => {
annotated_func_arg = false;
}
TokenKind::Equal if parens > 0 && fstrings == 0 => {
if annotated_func_arg && parens == 1 {
TokenKind::Equal
if type_params_state.in_type_params() || (parens > 0 && fstrings == 0) =>
{
if type_params_state.in_type_params() || (annotated_func_arg && parens == 1) {
let start = token.start();
if start == prev_end && prev_end != TextSize::new(0) {
let mut diagnostic =
Expand Down
Loading

0 comments on commit 1f39a4b

Please sign in to comment.